fix(msi): install into the root dig-updater probes, closing a non-convergent update loop - #194
Conversation
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES-REQUIRED
(Recorded as a COMMENT review: GitHub rejects both --approve and --request-changes from the PR author identity with HTTP 422. The three inline threads below are the blocker - they stay unresolved until addressed, which bars merge under required_conversation_resolution.)
CHANGES-REQUIRED - reviewed at ce65bda
(The dispatch quoted fe1f989; the head has moved. All line numbers below resolve against ce65bda.)
mergeable=MERGEABLE, mergeStateStatus=CLEAN, so unlike #189 the check rows are meaningful. build .msi (windows-x64) genuinely ran and passed - your premise correction is confirmed.
The direction is right and the on-machine evidence is the best kind. Three findings, one substantive.
What I verified as correct
RemoveFilenarrow safety (scrutiny point 2): clean.Name="dig-node.exe"is a literal, the directory isINSTALLFOLDERvia the component, andOn="install"restricts it to the install action. It cannot touchdigstore.exe,dig-updater.exe,dig-dns.exeor dig-app, and it runs inside the transaction, so a rollback restores whatever it deleted.- The upgrade transaction / no-ghost claim (scrutiny point 1): sound as reasoned.
RemoveExistingProductsat 1401 is inside the install transaction and is rolled back on failure; StopServices 1900 precedes RemoveFiles 3500 so the image is not locked;ServiceControl Start="install" Wait="yes"plusServiceInstall Vital="yes"makes a failed start an error rather than a silent skip. I did not executemsiexec- this is MSI semantics plus your machine run, not my own measurement. - Cross-hive precedence (scrutiny point 3): both halves hold. dig-installer writes
HKCU\Environment\Path(src/paths.rs:465-486) and its ownsrc/pathcheck.rs:242-243states the machine-then-user composition. And an MSIEnvironmentrow is deleted on uninstall, so re-pointing really would strip the shared root for digstore and dig-updater. UpgradeCodebyte-identical contract intact:dig-node.wxs:41== dig-installersrc/msi.rs:57.- Version
0.99.10correct against main at0.99.9.--shortstat==--ignore-cr-at-eol(238/18 both) - no CRLF inflation. NoClosesin the body, so nothing auto-closes early. Old-root sweep clean: the three survivingDIG Network\dig-nodestrings are deliberate historical narration plus one unrelated HKLM registry key.
Findings
- (gating)
dig-node.wxs:117- theRemoveFilerationale rests on a sequence that does not happen, and the real consequence of the shared root is unanalysed. - (gating)
scripts/tests/msi-install-root.test.sh:144- the fourth fix has no test. Proven by mutation. - (gating)
scripts/tests/msi-install-root.test.sh:96- the PATH-ownership guard is fail-open to a multi-line<Environment>row. Proven by mutation.
Detail inline.
…vergent update loop
The Windows package installed dig-node.exe to `%ProgramFiles%\DIG Network\dig-node\`,
added THAT directory to the MACHINE PATH, and pointed the `net.dignetwork.dig-node`
service image at it. dig-updater runs this package on every Windows update
(`msiexec /i <pkg> /qn /norestart`; dig-node's Windows InstallMethod is WindowsMsi) and
then reads the installed version from `<install-root>\dig-node.exe` — the canonical
protected root `%ProgramFiles%\DIG�in`.
So the install never touched the file the updater probes. The probed version never
changed, every beacon cycle re-ran the same install, and PATH kept serving the stale
copy: Windows auto-update could not converge. The same mismatch made dig-installer fail
its own audit, since it verifies the service image and the PATH resolution of
`dig-node.exe` against that root — a check against a directory this package's payload
had just created. dig-updater's own tests already assert the layout this change moves
to, so no dig-updater change is needed.
The PATH shadowing is cross-hive and therefore unconditional: dig-installer writes the
shared root to the USER hive, this package wrote its own root to the MACHINE hive, and a
fresh session composes machine PATH before user PATH.
* INSTALLFOLDER is now `%ProgramFiles%\DIG�in`. The UpgradeCode is unchanged — it IS
the migration mechanism, and changing it would leave two products both owning the
service.
* The machine-PATH component is REMOVED, not re-pointed. One entry needs one owner: an
MSI `Environment` row is deleted on uninstall, so a row naming the shared root would
take that root off PATH for every other component living there.
* `RemoveExistingProducts` is pinned EXPLICITLY to `afterInstallValidate`. The old
product's binary, PATH row and service registration are removed, and the service
reinstalled and started, inside one transaction — an interruption rolls back to the
old product with its service intact. The late schedule is forbidden and commented as
such: the old product's `ServiceControl Remove="uninstall"` matches by NAME and would
delete the service the new product just registered. `REINSTALLMODE=amus` is not used;
it turns a repair into a silent downgrade.
* A name-scoped `RemoveFile` clears a foreign `dig-node.exe` from the shared root before
installing. dig-installer drops a raw binary there, and Windows Installer keeps such a
file rather than overwrite it — leaving the package to complete over a binary it did
not install, which is precisely the stale version dig-updater would probe next.
Verified on a real machine put back into the old layout with the released v0.99.4 MSI,
then upgraded: service image under the protected root and RUNNING, both old directories
gone, machine PATH free of DIG entries, one Add/Remove entry, and `dig-node.exe`
resolving only to the protected copy in a FRESH environment block (scheduled task — a
running shell cannot show this). dig-installer then reported "DIG is ready", including
the audit line that used to fail. That procedure is now a checked-in script,
packaging/windows/verify-upgrade-migration.ps1, since CI builds the .msi but never
installs it.
Refs DIG-Network/dig_ecosystem#2251 (close on release + pointer bump, not on merge)
Refs DIG-Network/dig-installer#62
Co-Authored-By: Claude <noreply@anthropic.com>
3ec53a1 to
ede9dc4
Compare
|
Gate round 2 addressed — PR #194 updated, HEAD Reframed around the real defect. The PR title and body now lead with the non-convergent update The three findings, each fixed and each proven:
New: Evidence bridge for the round-2 edits. The on-machine run used a package built before these CI status: a GitHub Actions outage, not this change. Next action: re-run the four outage-failed jobs when Actions recovers, then gate → merge → |
|
Round 2 addressed. The reframe is the important part — this PR fixes a non-convergent auto-update loop, not a path nit. Keep that first in the body; it is what makes the change urgent rather than tidy. Two corrections to your realizations: Realization 4 — the Realization 2 is a real finding and I have hit it too. Realization 1 is the one to internalise. The MSI-database bridge is a genuinely good argument and it belongs in the PR body if it is not there already: the installed-and-verified build and the final build differ in exactly one place, the version in the Guard scoping — the right call. A wildcard removal in the shared root would delete digstore, dig-dns, dig-updater and dig-app binaries, which is worse than the bug being fixed. Asserting the scope, not just the presence, is what makes that guard real. The unproven items stay stated as unproven: the verifier script has not run end to end as one unit, and a from-scratch install of the fixed MSI needs a release. Do not let green checks imply either. |
|
All three gating findings verified fixed at this head. Checked each against the tree rather than taking the round-2 report. 1 — the false rationale. The header now states who runs the package accurately: dig-updater runs it on EVERY Windows update ( 2 — the fail-open PATH guard. Now flattens with 3 — the unguarded Why this PR matters more than its title suggests. It is not path hygiene. The beacon probes All five required checks green; |
What changed, and why it matters more than a path tidy
dig-updaterruns this package on every Windows update (msiexec /i <pkg> /qn /norestart;dig-node's Windows
InstallMethodisWindowsMsi, and the live manifest servesdig-node-0.99.4-windows-x64.msitoday). After running it, the beacon reads the installed versionfrom
<install-root>\dig-node.exe— the canonical protected root%ProgramFiles%\DIG�in.The package installed to
%ProgramFiles%\DIG Network\dig-node\instead. So the install nevertouched the file the updater probes. The probed version never changed, every beacon cycle re-ran
the same install, and PATH kept serving the stale copy: Windows auto-update was non-convergent,
permanently. The package cannot simply be retired — doing so would break Windows auto-update and
redden the signed feed for every channel.
The same mismatch is what made
dig-installerfail its own audit (it verifies the service image andthe PATH resolution of
dig-node.exeagainst that root), so every install failed a safety checkagainst a directory its own payload had just created.
dig-updater's own tests already assert the layout this PR moves to, so dig-updater needs no change
and is the acceptance witness for this one.
The PATH shadowing is cross-hive and therefore unconditional:
dig-installerwrites the shared rootto the user hive (
HKCU\Environment\Path), this package wrote its own root to the machinehive, and a fresh session composes machine
PATHbefore userPATH.Four changes in
packaging/windows/dig-node.wxs(UpgradeCodebyte-identical — it IS the migrationmechanism; changing it would leave two products both owning
net.dignetwork.dig-node):INSTALLFOLDER=%ProgramFiles%\DIG\bin(the canonical root —canonicalskill / SYSTEM.mdinstall-root section; dig-node's own
security.rsalready names this root).PATHcomponent is removed, not re-pointed. One entry needs one owner: an MSIEnvironmentrow is deleted on uninstall, so a row naming the shared root would take that rootoff PATH for digstore, dig-updater and everything else living there. The service is registered
with an absolute image path and needs no PATH; the user-facing CLI is
dign, which dig-installerplaces.
RemoveExistingProductspinned explicitly toafterInstallValidate, with the reasoning inthe file. The late schedule (
afterInstallExecute) is the plausible-but-broken choice: the oldproduct's
ServiceControl Remove="uninstall"matches the service by name and would delete theservice the new product just registered.
RemoveFileclears a foreigndig-node.exefrom the shared root beforeinstalling. dig-installer drops a raw binary there (it does not run this package — the old
header comment claiming otherwise is corrected), and Windows Installer keeps such a file rather
than overwrite it, so the package completes over a binary it did not install — which is exactly
the stale version dig-updater probes next. Scoped by name because the shared root also holds
digstore, dig-dns, dig-updater and dig-app.
REINSTALLMODE=amusis deliberately NOT used: itturns a repair into a silent downgrade.
What the migration does to a box that already has the old layout
All three things the old package owned — binary, machine-
PATHrow, service registration — move inone transaction:
(sequence numbers read out of the compiled MSI, not the source).
net.dignetwork.dig-nodeisabsent only inside the transaction: an interruption rolls back to the old product with its service
intact, and a completed run ends with the service registered against the new image. There is no
reachable resting state with a registered product and no service — which is exactly the state a
hand-repair produced on the user's machine (deleting the directory left a registered ghost, and
repairing that removed the running service).
Blast radius checked
gitnexus is disabled in the loop (§2.0 override), so this was done with ripgrep + a direct read across
dig-nodeanddig-installer:packaging/windows/dig-node.wxs— the only declaration of the install location. No Rust code inthis repo derives a path from it; the service resolves its own binary via
current_exe().SPEC.md— two statements of the old path (§9.1 gate prose, §9.7 package list). Both updated, plusa new normative paragraph on upgrade sequencing and the PATH-ownership rule.
dig-installer—msi::MSI_PACKAGESkeys onUpgradeCode+ DisplayName, both unchanged, soits supersede/uninstall path keeps working against packages built from this source.
docs.dig.net— no occurrence of the old path.That is why the on-machine evidence below exists rather than only unit tests.
Evidence
Proven on a real Windows machine that was deliberately put back into the old layout by installing
the released
v0.99.4MSI, then upgraded with a package built from this branch:ImagePathC:\Program Files\DIG Network\dig-node\dig-node.exeC:\Program Files\DIG\bin\dig-node.exe, RunningC:\Program Files\DIG NetworkPATHC:\Program Files\DIG Network\dig-node\DIG NETWORK: NODE 0.99.4DIG NETWORK: NODE 0.99.9, one entry, no orphanwhere dig-node.exe, fresh environment block…\DIG Network\dig-node\dig-node.exethen…\DIG\bin\dig-node.exe…\DIG\bin\dig-node.exeonlyThe fresh-environment probe runs through the Task Scheduler. A running shell cannot show this: the
stored machine
PATHcontains a literal%PATH%self-reference, so expanding it inside a composedshell splices that shell's own PATH in and inverts the ordering.
dig-installerrun end to end afterwards (elevated,--with-dig-node):✓ DIG is ready.,including the audit line that used to fail —
✓ dig-node runs from the protected install root ("C:\Program Files\DIG\bin\dig-node.exe" run-service)— and
✓ dig-node --version resolved + ran as micha.Compiled-artifact inspection (
WindowsInstaller.Installerover the built MSI):DirectorytableINSTALLFOLDER → DIGROOTFOLDER(DIG) → ProgramFiles64Folder; theEnvironmenttable is absent;RemoveFile dig-node.exe INSTALLFOLDER InstallMode=1;Upgradeattributes513(max-inclusive —AllowSameVersionUpgradesfrom #618 preserved).The verified artifact vs the final artifact. The on-machine run above used a package built
before the round-2 comment/SPEC edits. Dumping both MSI databases, the ONLY difference is the
version in the
Upgradetable (0.99.9→0.99.10):Directory,Component,ServiceInstall,ServiceControl,RemoveFile, the absentEnvironmenttable and the wholeInstallExecuteSequenceare identical, so the migration evidence carries over. A UAC prompt wasdeclined when I tried to re-run the verifier against the final build, so that re-run did not happen;
verify-upgrade-migration.ps1parses clean and its elevation guard fires, but the script has notbeen executed end to end as a single unit — its individual steps were all run by hand.
New test
scripts/tests/msi-install-root.test.sh(runs on the existing ubuntuscriptsCI job,so this packaging change does get automated cover). Each assertion was proven load-bearing by
reverting only its own fix and confirming only that assertion fails:
DIG Network\dig-nodeEnvironmentcomponent (one line)Schedule="afterInstallExecute"RemoveFileelementName="*.exe"On="uninstall"The location check composes the directory chain rather than grepping for
DIG, because thenearest wrong implementations (a different manufacturer folder, a
DIG\<component>leaf) contain theright substring.
What is NOT proven
dig-installerrun that actually executes the FIXED MSI. The installer fetchesfrom GitHub releases, so it still runs the old
v0.99.4package; in the green run above it skippedthe MSI step (
already up to date) and validated the layout this branch produced. The fullfetch→install→ready loop can only be closed once this is released.
build .msi (windows-x64)DOES run on this PR and passed, so thechanged WiX source is proven to COMPILE in CI (correcting my initial assumption that dig-node's
gating jobs are ubuntu-only). Nothing in CI installs or upgrades the MSI, so the migration
behaviour above rests entirely on the local run, not on CI.
Relationship to
DIG-Network/dig-installer#62That PR removes the superseded
DIG Networkroot on the installer side, on the premise that the rootis historical. It was not historical — this package recreated it on every install, which is what
this PR fixes upstream. The two are complementary and neither makes the other unnecessary:
this release and that never take another MSI upgrade. Its refusal conditions must keep holding.
Note that an MSI upgrade from a pre-fix version already removes the directory, the PATH row and the
old registration by itself (measured above), so #62 should treat an absent old root as the normal
outcome, not as evidence it has nothing to do.
Gate round 2 — what changed since the first review
RemoveFilerationale was false, and this PR was promoting it to normative SPEC. It saiddig-installer "places its own copy of the binary there before running this package". dig-installer
never runs the package; it drops a
RawBinary. Both that comment and the pre-existing header line("the dig-installer just fetches + runs this .msi") are rewritten against what actually happens:
dig-updater runs
/i, dig-installer drops a raw binary today, and unifying that is a separateunit of work. SPEC §9.7 carries the same correction.
RemoveFileMUST had no mechanical guard — deleting the line left all assertions green.Now guarded, and proven load-bearing three ways: deleting the element → 1 FAIL; widening it to
Name="*.exe"→ 2 FAILs (the shared root makes a wildcard removal worse than the original bug);changing it to
On="uninstall"→ 1 FAIL. SPEC now states four requirements and the test assertsfour.
<Environment>componentwith its attributes wrapped across two lines passed every assertion. The check now flattens with
tr ' ' ' 'first; re-running that same wrapped injection FAILs, and the file was restored froma copy afterwards.
Stage 2 — not in this PR, and what it needs
dig-installerswitching fromRawBinaryto running this same.msiis a different repo and aseparate unit of work. What it needs from this side: the package now installs to the root
dig-installer already manages, owns no PATH row, and tolerates a foreign binary already present in
that root — so dig-installer can adopt
msiexec /iwithout a layout change here. Until it lands,two writers keep sharing
DIG�in\dig-node.exe, which is why theRemoveFileexists.Version
0.99.9→0.99.10(patch: a packaging fix, no API or wire change). Re-derived fromorigin/mainat rebase time — main moved to
0.99.9during this work.