feat(win): Abort-path installer rollback with backup retention - #3265
feat(win): Abort-path installer rollback with backup retention#3265liugddx wants to merge 7 commits into
Conversation
|
@liugddx — heads up on an overlap worth resolving before either of us spends more review cycles: we are fixing the same CI fault in the same two files, independently. #3241 replaces the reserve-then-release CDP port with the port Chromium actually bound; this PR does the same thing by parsing Your mechanism is better on the axis that bit me: a stderr line is fresh per launch, so it cannot go stale. The So I am happy to drop my port-discovery half in favour of yours. What I would keep, because it is orthogonal to how the port is discovered and neither of us covers it elsewhere:
Two orderings work and it is not my call: either this PR drops its verifier changes and rebases once #3241 lands (it is small and ready today), or #3241 keeps only the two items above and yours carries port discovery. I will follow whatever you and the maintainers prefer — just want to avoid a merge conflict deciding it for us. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the design instincts here are right, and several of the placement decisions are ones I checked line by line against the pinned template and found correct. Reviewed exact head d1aee1ae441218d594528163df711e0e9780cd44 (note the head moved past the 3fc75306d I started from; installer.nsh is unchanged between them, everything below is read at the live head).
What holds: the backup really does happen before anything is destroyed — the customInit insertion point lands after initMultiUser and before the install section — customInstall really is the last step of the section, after registry and shortcuts, and a fresh install is completely inert. The backup is a same-volume sibling, so the rename path never meets EXDEV, and if $INSTDIR is a junction the rename fails into the copy fallback, which is a degradation rather than a hazard. directoryTreeManifest correctly throws on non-regular entries. And one risk in the PR body can be retired: I compiled installer.nsh with electron-builder's own pinned NSIS 3.0.4.1 makensis, invoked the way NsisTarget invokes it, and the NSIS syntax, S==, SHELL_CONTEXT, .onInstFailed and the three customFiles_* macro names all check out — so the first CI run is not the real syntax check. It did surface one encoding problem, filed below.
The two words in the title are where I disagree, and both are P1.
"Transactional" overstates it. The backup is a per-file copy, not a rename, and the commit point is not a single atomic operation — it is a five-stage tearable sequence: copy the backup, destroy the old version, extract the new one, write the registry, delete the backup. That is recoverable-with-a-backup, which is a real and worthwhile property, but it is not a transaction and the difference shows up in the interruption table below.
"Proven" is not earned yet, and the mechanism it would prove is unreachable. Restore hangs off .onInstFailed, which NSIS only calls on Abort — and the pinned install section contains no Abort at all. Every real failure exit is a Quit: the old uninstaller failing, and extraction failing or being cancelled. So on production paths this rollback never fires; the only thing that reaches it is the failpoint this PR injects. Separately, the CI step that would demonstrate it has never executed — I pulled check-runs and logs for every commit on this branch and the package job failed or was cancelled each time, with zero occurrences of the rollback step in any log.
Two P1s, six P2s and two P3s inline. Not approving while P0/P1/P2 findings are open.
Interruption points and where they leave the user — the two rows that matter most are the ones the PR body opens by describing:
| Interruption | End state | Recoverable? |
|---|---|---|
| Backup copy fails (space / lock / MAX_PATH) | Old install intact, upgrade refused, exit 101, nothing visible | Yes, but the upgrade can be pinned at 101 indefinitely and silently |
Old uninstaller fails (Quit) |
Old install usually rescued by un.restoreFiles; backup orphaned forever |
Partly — app works, a full copy leaks |
Extraction cancelled (Quit) |
$INSTDIR already emptied, registry already removed, backup exists and no code will ever use it |
No |
| Silent-mode extraction falls through to the ignore-errors fallback | Possibly incomplete install, exit 0, and customInstall deletes the backup |
No — this PR destroys the recovery source |
Injected Abort failpoint (the only path into rollback) |
Byte-identical restore plus registry, exit 102 | Yes — but never executed in any CI run |
Killed between RMDir /r and Rename |
No $INSTDIR; backup intact but ignored on the next run |
No, automatically |
| Power loss anywhere | Nothing is fsynced; arbitrary torn state | No — correctly declared out of scope |
Review disclosure: this review was prepared with Claude Code, which read the diff at this head, cross-read the pinned app-builder-lib 26.15.3 and NSIS 3.0.4.1 sources for every claim about template behaviour, queried the GitHub check-runs API and job logs for the CI-evidence finding, and compiled the new .nsh locally with the pinned makensis. Evidence grade is stated per finding — the CI-evidence, encoding and LogicLib findings were reproduced by execution; the Win32-semantics ones are labelled inference. The human contributor reviewed this before posting.
| SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK} | ||
| FunctionEnd | ||
|
|
||
| Function .onInstFailed |
There was a problem hiding this comment.
[P1] Cover the failure paths that actually happen, or stop calling this mid-install rollback. .onInstFailed is only invoked on Abort, and the pinned install section has no Abort in it — I grepped the template and every hit is in .onInit, a MUI page callback, or the uninstaller. The real exits are Quit: include/installUtil.nsh:132 when the old uninstaller returns non-zero, and include/extractAppPackage.nsh:131 (and :86 for the zip branch) when extraction exhausts its retries or the user cancels. Quit does not call .onInstFailed. So the concrete case the PR body opens with is exactly the one not covered: the user double-clicks an upgrade installer, customInit backs up successfully, uninstallOldVersion has already done RMDir /r $INSTDIR, extraction then hits a DLL an antivirus is holding, and after five retries the user clicks Cancel on the MB_RETRYCANCEL — the machine now has no Maka, the registry entries are gone, and the backup is sitting in $INSTDIR.pre-upgrade-backup with nothing that will ever look at it and no way for the user to know it exists. NSIS has no Quit hook, so covering this means a sentinel written in customInit and a recovery check on the next launch, plus taking the uninstall-failure branch through customUnInstallCheck. If that is out of scope for this PR, then retitle to "Abort-path rollback with backup retention" and say plainly in the docs that it covers no currently known real failure branch. Regression test: inject a failpoint that Quits rather than Aborts and assert the current behaviour, so the gap is pinned rather than assumed. Confirmed by reading the pinned app-builder-lib 26.15.3 template that apps/desktop/package.json pins.
| "apps/desktop/release/Maka-${version}-win-x64.exe" \ | ||
| apps/desktop/release-autoupdate-next | ||
|
|
||
| - name: Prove deterministic mid-install failure rollback |
There was a problem hiding this comment.
[P1] Get this step green once and put the log in the PR body before the word "proven" ships. I queried check-runs for every commit on this branch — 90f1a275e, 3335af80b, 76de157af, db5450d7c, e84ab2c3a, 3fc75306d — and the package job is failure, failure, cancelled, failure, failure, failure, with the head's still in progress. Pulling the full logs for the four failed jobs and grepping for verify-windows-installer-rollback returns zero hits in every one: the step has never run. The most recent job died in the preceding verify:windows-autoupdate on a CDP attach that took about six minutes against a 90-second budget, which is the very bug the head commit is trying to fix. So the only execution evidence this PR currently has is that a normal 0.1.9→0.1.11 upgrade still passes the lifecycle gate with the hook installed — a happy-path non-regression, not a demonstration of rollback. Until the step reports, "proven" should not appear in the title, in docs/windows-support.md, or in the Chinese docs. When it does run, the body should carry the manifest file count, the exit code 102, the 0 differing files line and the control run. Reproduced by execution against the GitHub API and the job logs.
| ${If} $R9 S== "after-extract" | ||
| StrCpy $makaFailpointArmed "1" | ||
| ${EndIf} | ||
| ${If} ${FileExists} "$INSTDIR\${APP_EXECUTABLE_FILENAME}" |
There was a problem hiding this comment.
[P2] Decide what a leftover backup means, because right now both possible answers are wrong. This condition keys re-entry off "does $INSTDIR\Maka.exe exist", which mishandles the two states a failed attempt actually leaves. If the previous run Quit after extraction had written some new-version files including Maka.exe, this reads as "an old install is here", so line 193 unconditionally RMDir /rs the one correct backup and line 195 snapshots the broken partial install in its place — the only recovery source is silently destroyed. If instead the previous run was killed between the RMDir /r and the Rename inside the rollback, $INSTDIR does not exist, this whole block is skipped, and a full copy of the application including the bundled git and mingw64 trees is left on disk forever, since customInstall only deletes the backup when the same path later succeeds. There is a third edge waiting behind it: makaRestoreFromBackup's guard only asks whether a backup exists, not which install it belongs to, so the day a real Abort is introduced, a failed fresh install would resurrect that stale backup into $INSTDIR with all the $makaPrev* values empty, producing an old version with no uninstall registry entry. Write a manifest into the backup — source version, timestamp, completeness marker — and have customInit decide from it; treat "backup present, $INSTDIR missing" as an unfinished rollback to resume rather than as a fresh install.
| # new-version files behind, merging the backup over them would produce a | ||
| # mixed tree that the success exit code would then misreport — and delete | ||
| # the only recovery source. Keep the backup and report failure instead. | ||
| ${If} ${FileExists} "$INSTDIR\*.*" |
There was a problem hiding this comment.
[P2] Test for a non-empty directory, not for existence — an empty shell turns a fully recoverable rollback into a 103. RMDir /r "$INSTDIR" very commonly empties a directory on Windows without removing it, because a process has it as its current directory or Explorer holds a handle; the subsequent Rename then necessarily fails, since MoveFile refuses an existing destination even when empty. The fallback arrives here and treats existence as residue — and it does so wrongly, because the pinned NSIS 3.0.4.1 LogicLib.nsh defines _FileExists as a bare IfFileExists, and IfFileExists "dir\*.*" is true for an empty directory since . and .. match. I confirmed that by reading the pinned LogicLib source. So the result is SetErrorLevel 103 and an immediate Return, which also skips the registry restore below — the user ends up with an empty install directory, no application, and a maka.pre-upgrade-backup folder, in exactly the situation where the copy fallback would have worked perfectly. Enumerate with FindFirst skipping . and .., or attempt a non-recursive RMDir "$INSTDIR" first to clear an empty shell. Regression test: hold a directory handle open on $INSTDIR, trigger the failpoint, and assert 102. The MoveFile-on-existing-directory behaviour is inference from Win32 semantics; the LogicLib behaviour is not.
| SetOutPath $PLUGINSDIR | ||
| ClearErrors | ||
| RMDir /r "$INSTDIR" | ||
| Rename "$makaBackupDir" "$INSTDIR" |
There was a problem hiding this comment.
[P2] Clear the error flag before the rename, and judge success by the filesystem rather than by the flag. Line 81 clears errors, then line 82 runs RMDir /r and line 83 runs Rename, and the ${If} ${Errors} below cannot tell which of the two set it. If RMDir /r sets the flag — for instance because its target does not exist — while the Rename actually succeeded, the code takes the failure branch, does not reclassify because $INSTDIR now exists, falls into the fallback, sees the directory it just correctly renamed, calls it residue, and reports 103 while skipping the registry restore entirely. The user's application is back, byte for byte, and has vanished from Apps and Features with no UninstallString. That specific RMDir behaviour I could not verify — I have no Windows runtime here, so treat the trigger as inference — but two commands sharing one error flag with no ClearErrors between them is a defect regardless of which one sets it. Clear before the Rename, and decide success by testing for $INSTDIR\${APP_EXECUTABLE_FILENAME}.
| # the backup always mirrors the install being upgraded right now. | ||
| RMDir /r "$makaBackupDir" | ||
| CreateDirectory "$makaBackupDir" | ||
| CopyFiles /SILENT "$INSTDIR\*.*" "$makaBackupDir" |
There was a problem hiding this comment.
[P2] Move the backup after the running app is stopped, check free space first, and make 101 visible. customInit runs inside .onInit, which is before CHECK_APP_RUNNING in the install section — so this full-tree copy happens while the application is still running, which is the state most likely to produce a locked file. Any single copy failure means SetErrorLevel 101 and Quit, and because electron-updater's quitAndInstall spawns the installer detached, a non-zero exit is completely invisible in the UI: the user just sees the upgrade not happen, with no message, and it will not happen next time either. Three ways to land there permanently, all of which upgrade fine today: free space, because the backup doubles peak usage while setInstallSectionSpaceRequired reserves only the install size and nothing pre-checks; a single exclusively locked file from antivirus or the indexer; and path length, since the backup directory is 21 characters longer than $INSTDIR and CopyFiles goes through SHFileOperation under MAX_PATH, against a tree that already contains ~110-character relative paths under node-pty/third_party/conpty plus the bundled mingw64 tree. Do the backup after the process is stopped, add a free-space precheck, and make 101 produce a visible message outside silent mode and a log at a known path inside it. The insertion order is confirmed against the pinned template; the three causes are inference.
| # mixed tree that the success exit code would then misreport — and delete | ||
| # the only recovery source. Keep the backup and report failure instead. | ||
| ${If} ${FileExists} "$INSTDIR\*.*" | ||
| DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery" |
There was a problem hiding this comment.
[P2] DetailPrint cannot reach the user this path is written for. The one-click installer runs silently in the upgrade scenario — especially when electron-updater launched it — so the "keeping the backup for manual recovery" instruction is emitted into a window nobody is looking at, and 103 leaves the user with no application, no explanation, and a directory whose purpose they have no way to learn. Write a plain-text recovery note next to the backup directory on this path, and show a MessageBox when not silent. docs/windows-support.md should also carry the manual recovery steps; today both the English and Chinese docs say only that hard kills and power loss are not covered, which tells the user what will not be rescued without telling them what to do when it is not.
| * install tree must not contain them, and silently hashing a link target would | ||
| * make two different trees compare equal. | ||
| */ | ||
| export async function directoryTreeManifest(rootDirectory) { |
There was a problem hiding this comment.
[P2] Commit the tests the Verification section describes, or change the section. The body says table-driven tests of directoryTreeManifest and diffTreeManifests exist — sorted POSIX paths, sha256 shape, identical-to-empty-diff, change/delete/add detection, and the platform guard — but the PR contains no .test.mjs at all; I checked the file list. So these two new shared exports and the new verifier's platform guard have zero coverage in the repository, and the only thing that would ever execute them is the Windows lane that has never completed. That matters more than usual here because the manifest comparison is the entire evidentiary basis for the 102 result: if it is wrong, a rollback that lost files still reports 0 differing files. Worth noting one real gap while you are writing them: directoryTreeManifest does not record empty directories, so a rollback that lost one would pass. Reproduced by execution against the PR's file list.
| # customInstall (success tail) delete the backup. | ||
| # | ||
| # What this does NOT cover (documented in docs/windows-support.md): failures | ||
| # that bypass NSIS's Abort path — a hard kill of the installer process or |
There was a problem hiding this comment.
[P3] Replace the em dashes with ASCII hyphens — they make this file uncompilable on macOS and Linux. Compiling it with electron-builder's own cached pinned NSIS 3.0.4.1 makensis, invoked exactly as NsisTarget invokes it (-WX -INPUTCHARSET UTF8 -, script on stdin, includes from disk), fails with Bad text encoding: installer_hook.nsh:24 and aborts; replacing the four U+2014 characters at lines 24, 25, 33 and 95 with -- compiles cleanly with no warnings under -WX. That makensis build is NSIS_CHAR_SIZE=1, so this is a POSIX-toolchain limitation rather than anything wrong with the file, and Windows' Unicode makensis is unaffected — the repo's package:windows-x64 guard on platform !== 'win32' is why CI does not see it. It costs nothing to fix and it is what let me verify the rest of the syntax. Reproduced by execution on macOS.
| * port — reserve-then-release had a race window in which another process | ||
| * could take the port, leaving Electron listening elsewhere while the | ||
| * verifier polled the stale number for its full deadline ("did not expose | ||
| * CDP ... fetch failed", observed repeatedly on busy CI runners). |
There was a problem hiding this comment.
[P3] Land the CI-stability fixes first, as their own PR. waitForDevToolsPort, the renderer-discovery timeout going 30s to 90s, and the taskkill exit-128 tolerance in verify-windows-autoupdate.mjs are three independent CI infrastructure fixes with nothing to do with installer backup, and a revert of "transactional installer backup" should not also revert them. They are here for an understandable reason — five of this branch's commits are chasing the same flakes — but that is the argument for separating them, not against: as long as they ride along, this PR cannot get its own rollback step to execute, which is the P1 above. Land them ahead of this, get one green Windows lane, then rebase this on top. Note #3241 is fixing the same CDP-port discovery race in the same file, so coordinate rather than diverge.
|
@Joob1n Thanks for flagging this before the conflict decided it for us — and for the One disclosure first: half an hour before reading your comment I pushed d1aee1a to this PR, which already lands one of your two orthogonal items — a per-attempt bound on the CDP poll ( Given that, the split that leaves no overlap at file level:
Merge order then stops mattering: whichever lands second rebases trivially because the file sets are disjoint. If the maintainers prefer the other ordering (#3241 first, this PR rebases its verifier half onto yours), that also works — your call and theirs; no attachment to authorship here, only to not paying another flake-priced CI cycle. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 551eb922. The new commit is unrelated to my standing findings — it adds waitForUninstallRegistrationToClear to the verification harness — so I checked apps/desktop/build/installer.nsh and it is byte-identical to the head I reviewed. All my earlier findings still stand and I am not re-filing them inline: .onInstFailed unreachable on the Quit paths, Rename sharing an error flag with RMDir /r, ${If} ${FileExists} "$INSTDIR\*.*" being true for an empty directory, CopyFiles /SILENT running before CHECK_APP_RUNNING, and the backup directory's name prefix colliding with app-running detection.
The new commit itself is a good catch and correctly reasoned. "Files gone" genuinely is not "uninstall finished": launched without _?=, the NSIS uninstaller copies itself to %TEMP% and detaches, and DeleteRegKey is its last action — so waitUntilMissing returns while a later install's fresh registration can still be deleted by the previous step's stale uninstaller. Treating the registration's disappearance as the completion signal is the right observable, and folding the duplicated readUninstallDisplayVersion from the rollback script into one shared readUninstallDisplayVersions is the right seam.
One defect in the new code, inline. Worth noting alongside it: #3241 is fixing exactly this class of bug in listInstalledProcesses — an unbounded PowerShell call under a polling deadline — in this same file, so the two branches should not diverge on it.
Reviewed with Claude Opus as an analysis assistant; verified by reading the new commit, runCommand's timeout handling, and installer.nsh at this head. The Windows paths were not executed.
| Where-Object { $_.DisplayName -eq 'Maka' } | ||
| @($entries | ForEach-Object { $_.DisplayVersion }) -join ',' | ||
| `; | ||
| const { stdout } = await run('powershell', ['-NoProfile', '-NonInteractive', '-Command', script]); |
There was a problem hiding this comment.
[P2] Bound this PowerShell call, or the enclosing 120-second deadline cannot fire. runCommand only arms a timer when options.timeoutMs is defined — with it undefined, deadline is null and the child is never killed — and waitForUninstallRegistrationToClear awaits this call at the top of its loop, checking Date.now() >= deadline only afterwards. So a Get-ChildItem over the Uninstall key that hangs makes the wait hang forever rather than failing at 120 seconds, and the workflow timeout becomes the real bound, cancelling the run with no diagnostic pointing here. That is the same failure the sibling listInstalledProcesses has and that #3241 is fixing in this file by giving it an explicit per-probe budget — the two branches should agree. Confirmed by reading runCommand at this head; not reproduced, since it needs a registry query to actually stall. Pass a timeoutMs well under the 120-second budget so a stalled probe leaves room for several more attempts, and let a probe failure be tolerated across iterations rather than ending the wait. Regression test: inject a run that never settles and assert waitForUninstallRegistrationToClear rejects within its own timeout.
apache#3265 fixes the same reserve-then-release race by reading the port from Chromium's `DevTools listening on …` stderr line, which is fresh per launch and therefore cannot go stale — the failure mode this PR had to guard against explicitly, because the upgrade-lifecycle check reuses one user-data directory across two app versions with a kill in between. That mechanism needs no delete-before-spawn convention, so it wins on merit and the port half goes there, along with the per-attempt poll bound both PRs arrived at independently. What remains is disjoint at file level and nobody else has it: the Windows process probe that hung a job for 59 minutes past a 120-second deadline, and the poll semantics that let a stalled probe be an unknown round rather than a verdict. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@liugddx — agreed, taking your split. #3241 is now narrowed to No need to apologise for the collision — the timestamps say it was independent work: your Two things from my lane that are yours now, both with data attached. The relaunch loop needs the same treatment as the probe, and it is proven.
The bounded probe half is in #3241 and reaches your file for free once it lands. The loop half is three lines in your file — catch the probe error, keep polling, and report at the deadline whether the process table was ever read, since "never observed" and "did not relaunch" are different faults and only one is about Maka. Shape is in c611cfd's parent if useful; adapt or ignore. The next stall after that one is Also worth knowing if your lane goes red on something that looks like a renderer that never appears: #3279 tracks an intermittent product-side fault where the upgraded install's own Runtime Host stops responding during startup. Not a verifier bug, and not fixed. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The mechanism works, and the evidence for that is real rather than asserted: in run 32346230451 the failpoint fired, .onInstFailed's SetErrorLevel propagated out as a genuine process exit code, and the rename path restored 605 files byte-for-byte. The same head's end-to-end update also passed, which incidentally proves the part I would have doubted most — that CopyFiles-backing-up a running install is viable, because CHECK_APP_RUNNING runs after customInit. The registry snapshot is also more thorough than it looks: I checked it against this project's actual electron-builder configuration and the eleven captured values are complete for it, with the absent ones (HelpLink, MenuDirectory, the second uninstall key, file associations) genuinely not applicable. That came from reading the templates, not guessing.
My concern is that the backup half is careful and the restore half is not, and the exit codes do not distinguish the two.
Architecturally, the restore sequence is ordered against itself: it destroys before it recovers. When the failpoint fires, $INSTDIR holds a fully extracted, launchable new version — custom_files_post_decompression runs after decompression and before the uninstaller and registry are written. The first thing rollback does is RMDir /r that, and only then attempt a rename. If both the rename and the copy fallback fail, the machine has neither version, where doing nothing would have left a working tree. A same-volume two-step swap — rename $INSTDIR aside, rename the backup into place, delete the aside copy only on success — has no window in which both are absent. That is worth restructuring around rather than patching, because three of the findings below are consequences of the current ordering.
Two of the three findings I raised on the earlier head are unchanged and still stand; both are the same anti-pattern #3241 is currently naming and forbidding, which is worth reconciling between the two PRs before either lands. runCommand's missing default timeout in verify-packaged-app.mjs is unchanged too, and I accept the argument in its comment that codesign and notarization have no honest upper bound — I am not re-raising it.
On the NSIS uninstaller race I owe you a correction. I asserted that an uninstaller launched during an upgrade detaches and deletes registry keys last. I read the pinned app-builder-lib@26.15.3 templates: installUtil.nsh invokes the old uninstaller with _?=$installationDir, so it does not self-copy and ExecWait genuinely waits. The upgrade path was never exposed. The harness's own standalone uninstall does omit _?=, which is where the race actually lived — and your waitForUninstallRegistrationToClear barrier addresses exactly that, from a correct diagnosis. One precision issue with the barrier is noted below.
Finally, a merge gate rather than a code comment: release-windows-check has not passed once on this branch, and the head run is still in flight with the rollback step pending. docs/windows-support.md now claims a gate proves transactional upgrade and drops the previous disclaimer, so that claim needs one green run behind it before merge.
AI disclosure: this review was produced with Claude Code (Opus 5) with a subagent covering security, correctness/resource bounds, integration and simplification. I re-derived the restore-path findings myself by reading installer.nsh at 23e8a8b, and confirmed the current check status. The subagent additionally reports reading the pinned NSIS templates and correlating the CI run; the NSIS wildcard semantics behind the fallback finding are inference, not execution, and are labelled as such. Per AGENTS.md this is not independent human review.
| WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1 | ||
| WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1 | ||
| DetailPrint "Previous installation restored" | ||
| SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK} |
There was a problem hiding this comment.
[P1] Make this exit code conditional on something actually having been restored. Every registry writeback above is guarded by ${If} $makaPrev<X> != "", but WriteRegDWORD NoModify/NoRepair and this SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK} are unconditional. customInit captures the snapshot only after confirming an upgrade via ${FileExists} "$INSTDIR\${APP_EXECUTABLE_FILENAME}", and it does not fail closed when the registry read comes back empty — so if the uninstall key is already gone (a user cleaned it, or the standalone-uninstall race your own barrier now guards), the destructive upgrade proceeds with an empty snapshot, all eleven guards skip, and rollback reports "Previous installation restored" having restored nothing. What is left is a stub uninstall key holding only NoModify and NoRepair. That is self-perpetuating rather than cosmetic: installUtil.nsh's uninstallOldVersion returns early when it cannot read UninstallString, so the next upgrade skips the uninstall entirely and extractUsing7za merges the new tree over the old one; meanwhile the app is absent from Apps and Features, so the user cannot uninstall it either. A subagent reports that run 32346230451 — whose installer.nsh is byte-identical to this head — produced exactly this state: 605 files correct, exit 102 accepted, DisplayVersion empty. The follow-up commit added a harness-side precondition assertion but did not change the installer. Fix: fail closed in customInit with 101 when the snapshot is empty on a confirmed upgrade, while nothing destructive has happened yet, and make 102 conditional on the snapshot having been non-empty and written back. Regression test: in the rollback verifier, delete the uninstall key after installing the candidate, then upgrade with the failpoint, and assert the exit code is not 102.
| # our own open handle (same precaution as the template's uninstaller). | ||
| SetOutPath $PLUGINSDIR | ||
| ClearErrors | ||
| RMDir /r "$INSTDIR" |
There was a problem hiding this comment.
[P2] Do not delete the only recoverable tree before recovery has succeeded. At the moment the failpoint fires, $INSTDIR contains a fully extracted and launchable new version, and this is the first thing rollback removes — before the rename that is supposed to bring the old one back. If the rename and the copy fallback both fail, the machine ends with neither version installed, a sibling backup directory, and exit 103, which a silent install surfaces to nobody. Not rolling back at all would have left a usable tree. The same-volume swap avoids the window entirely: Rename "$INSTDIR" "$INSTDIR.failed-upgrade", then Rename "$makaBackupDir" "$INSTDIR", then remove the aside copy only after that succeeds — every intermediate state has at least one complete installation present. Confirmed by reading the ordering here and the failpoint's position in the pinned template. Regression test: hold an exclusive handle on a file under $INSTDIR, trigger the failpoint, and assert that whatever the exit code, a launchable Maka.exe exists — old or new — rather than an empty directory.
| # new-version files behind, merging the backup over them would produce a | ||
| # mixed tree that the success exit code would then misreport — and delete | ||
| # the only recovery source. Keep the backup and report failure instead. | ||
| ${If} ${FileExists} "$INSTDIR\*.*" |
There was a problem hiding this comment.
[P2] This residue check cannot distinguish an empty directory from a populated one, so the fallback is unreachable in the case its own comment describes. IfFileExists "dir\*.*" is the standard NSIS idiom for "does this directory exist", because FindFirstFile matches . — it returns true for an empty directory. The most common partial failure of RMDir /r on Windows is precisely that children are removed but RemoveDirectory fails because something holds a handle on the directory itself: a scanner, an Explorer window, the indexer — the very example this comment gives. That leaves an empty $INSTDIR, so the rename fails on an existing target, $R8 becomes 1, and this check then reports "new-version residue" and returns 103 without ever reaching CopyFiles. The fallback is reachable only in the narrower case where $INSTDIR was fully removed and the rename still failed. Inference rather than execution — I have no makensis here and the NSIS wildcard semantics are the basis — so please verify before acting. A real emptiness test would be a non-recursive RMDir "$INSTDIR" (which succeeds only when empty) followed by re-testing existence, or ${GetSize} "$INSTDIR" "/S=0K" against zero. Regression test: the held-handle variant above, asserting rollback still completes and returns 102 when only an empty directory remains.
| RMDir /r "$makaBackupDir" | ||
| CreateDirectory "$makaBackupDir" | ||
| CopyFiles /SILENT "$INSTDIR\*.*" "$makaBackupDir" | ||
| ${If} ${Errors} |
There was a problem hiding this comment.
[P2] Verify the backup before crossing the destructive boundary — the error flag alone does not establish it. This is the transaction's commit point, and its only gate is the error flag after CopyFiles /SILENT. That lowers to SHFileOperation, whose return value NSIS checks while fAnyOperationsAborted goes unread, and /SILENT sets FOF_SILENT|FOF_NOCONFIRMATION without FOF_NOERRORUI. SHFileOperation is also MAX_PATH-bound, and the backup path is twenty characters longer than $INSTDIR — so a path that fits in the source tree may not fit under .pre-upgrade-backup. Any partial copy passes with a clean error flag, the old installation is then destroyed, and rollback restores a truncated tree while still reporting 102. Nothing in the shipped installer would notice; the harness manifest only exists in CI. Even a minimal existence check — ${IfNot} ${FileExists} "$makaBackupDir\${APP_EXECUTABLE_FILENAME}" then exit 101 — converts this from silent corruption into a fail-closed abort taken while the old install is still intact; comparing ${GetSize} on both trees would be stronger. "Missing integrity check" is confirmed by reading the code at this head; the specific SHFileOperation under-reporting is inference. Regression test: plant a file under $INSTDIR whose backup path exceeds 260 characters, and assert the upgrade exits 101 with the old install intact rather than exiting 102 with a short tree.
| Where-Object { $_.DisplayName -eq 'Maka' } | ||
| @($entries | ForEach-Object { $_.DisplayVersion }) -join ',' | ||
| `; | ||
| const { stdout } = await run('powershell', ['-NoProfile', '-NonInteractive', '-Command', script]); |
There was a problem hiding this comment.
[P2] Still unbounded, and now inside a poll loop — this is the same finding from the previous head, and it is the exact anti-pattern #3241 is currently naming. run('powershell', [...]) here takes no options object at all, so no timeoutMs reaches runCommand, which arms its kill timer only when one is defined. waitForUninstallRegistrationToClear calls this at the top of its loop and checks its 120s deadline only after the await returns, so a wedged PowerShell means the deadline never fires and the lane hangs to the 90-minute workflow timeout instead of failing. scripts/verify-windows-installer-rollback.mjs:80 has the same shape. #3241 is landing a bounded probe and a tolerate-one-failure loop for exactly this; the two PRs should agree on the rule rather than shipping opposite examples of it. Confirmed by reading both files at this head and #3241 at c611cfd. Regression test: inject a run that never settles and assert the wait rejects at its own deadline.
| * of seconds after `waitUntilMissing` saw the files disappear. Anything that | ||
| * installs into that window gets its freshly written registration deleted by | ||
| * the stale uninstaller — observed as an empty uninstall entry two steps | ||
| * later. The registration's disappearance IS the completion signal: once the |
There was a problem hiding this comment.
[P3] The barrier releases one instruction before the uninstaller's last one, and this comment states otherwise. uninstaller.nsh deletes UNINSTALL_REGISTRY_KEY at line 250 and INSTALL_REGISTRY_KEY at line 254 — so when the uninstall registration disappears, the detached copy has one more destructive registry call to make, not zero. At a 2s poll the window is two adjacent registry calls wide and the practical collision probability is negligible, so this is not a defect today. It matters because this comment is precisely what the next person will read when deciding whether the barrier is still sufficient, and as written it would tell them yes when the premise has changed. Either restate it accurately, or make the barrier wait for both keys to be gone. Confirmed against the pinned app-builder-lib@26.15.3 templates.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
COMMENT — the three new commits are all harness work, and the transaction they are meant to prove is byte-for-byte unchanged.
I diffed d1aee1ae4...b22e98ee7: three commits, four files, all under scripts/. apps/desktop/build/installer.nsh has the same blob hash at both heads (ace1fd526…), so every finding I filed against it still stands exactly as written and I am not re-posting any of it — the existing threads are still the right place. That is two P1s (.onInstFailed never fires for the failure modes the PR names, and the rollback exit code is unconditional while every writeback above it is guarded), five P2s, and one P3.
Status on the other P1, the CI evidence: check-runs on this head lists audit, package, test and windows_sandbox_w0_protocol — release-windows-check still has no green run anywhere on this branch, so the word "proven" in the title is still unbacked.
What the delta does do is real and I have no objection to any of it in isolation: waitForUsableRenderer correctly makes one stalled probe non-fatal rather than fatal, waitForUninstallRegistrationToClear identifies a genuine race (the detached uninstaller deletes the registry keys as its last act, after waitUntilMissing has already seen the files go), and the version-mismatch evidence capture is the right instinct. biome format is clean on all four files.
Two new P2s inline, both the same shape as the one I have already raised twice on this PR: an await with no bound sitting inside a loop or path whose deadline is supposed to be the authority. runCommand arms a kill timer only when timeoutMs is passed, and it is still not passed at verify-windows-installer-lifecycle.mjs:40 or at the new :100, so waitForInstalledProcessesToExit's 60s and waitForUninstallRegistrationToClear's 120s are both advisory. This is now the fourth site; it would be cheaper to give runCommand a default bound than to keep finding them.
On scope: my earlier P3 asked for the CI-stability work to land as its own PR. Three more commits of it have landed here instead, and the PR is now roughly half harness hardening under a title about installer transactions. Splitting it would let the harness fixes merge on their own evidence today, and leave this PR to stand or fall on installer.nsh and one green release-windows-check run.
AI disclosure: this review was assisted by Claude (Opus) for code search and cross-checking. Every finding I re-derived myself against the source at b22e98ee7.
| let lastError; | ||
| for (;;) { | ||
| try { | ||
| state = await evaluateRenderer(webSocketDebuggerUrl); |
There was a problem hiding this comment.
[P2] Bound this probe, or the deadline this whole function is built around cannot fire. evaluateInRenderer awaits the WebSocket handshake with only open and error listeners and no timer (verify-packaged-app.mjs:193-196); its timeoutMs = 10_000 at :187 arms only after the socket is open and covers the CDP round trip alone. A DevTools port that is bound but wedged — TCP accepts, nothing responds, no error event — is exactly the Windows-runner state this commit exists to survive, and in it this await never settles, so Date.now() >= deadline below is never reached and the job dies on the workflow timeout instead of on this function's own message. The fix is a timeout on the open await, which then makes each stalled probe a caught error and the retry loop do what its comment says. Confirmed by reading the source at this head. Regression test needed: point waitForUsableRenderer at a URL served by a listener that accepts and never speaks, and assert it rejects with the deadline message inside deadlineMs.
| // executable timestamps (rewritten ⇒ extraction ran). | ||
| const evidence = [`relaunched processes: ${JSON.stringify(relaunched)}`]; | ||
| try { | ||
| const commandLines = await run('powershell', [ |
There was a problem hiding this comment.
[P2] Pass a timeoutMs here — an unbounded evidence probe can destroy the evidence it was added to collect. runCommand arms a kill timer only when options.timeoutMs is defined (verify-packaged-app.mjs:21-33), so this Get-CimInstance Win32_Process call has no bound. It runs on the machine in the exact wedged state the surrounding comment describes — a Maka.exe running from the install directory while the on-disk bytes are stale — and WMI process enumeration on a busy or half-broken Windows box is a well-known place to hang. If it does, the throw below never happens: instead of a version-mismatch assertion carrying four kinds of state, the run dies on the workflow timeout with none of it. The same applies to readUninstallDisplayVersions two calls later. Something like 30s is enough here, and a timeout must not abort the rest of the capture — the existing catch already handles that. Confirmed by reading the source at this head. Regression test needed: inject a run that never resolves and assert the mismatch error is still thrown with the remaining evidence lines.
Reworked from review (apache#3265): the restore is now a two-step same-volume swap ordered so every intermediate state keeps at least one complete installation on disk - the extracted new tree is moved aside, never deleted, before the backup is moved in, and is only discarded after the restore and registry write-back completed. Success is judged by the filesystem, not the error flag; a blocked-but-empty $INSTDIR shell is distinguished from real residue by enumeration and recovered through the copy path. Fail closed (101) when the upgrade has no uninstall registration and no adoptable backup: with nothing to restore, a failed upgrade would leave files without an uninstall entry and the template would silently merge trees on the next attempt. The backup is verified (executable witness) before anything destructive runs and marked complete only then; the registry snapshot is also persisted outside the keys the upgrade deletes, so an attempt that dies on a hookless template Quit path leaves a state the next run adopts and completes. Every kept-backup path writes RECOVERY-README.txt and shows a message when interactive. 102 is reported only after the filesystem witness and the write-back. The gate now pins the boundaries instead of assuming them: the Abort failpoint proves byte-identical restore (102); a Quit failpoint at the same moment proves no hook fires and the backup survives; a rerun proves adoption completes the upgrade; a deleted registration proves the 101 refusal leaves files untouched; the no-failpoint control run proves normal upgrades are unaffected. Title and docs now say what is covered - Abort-path rollback with backup retention - and spell out the Quit gap, the supported rerun recovery, and manual recovery steps. ASCII-only so the POSIX makensis toolchain can compile the include. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
b22e98e to
7c3a8f2
Compare
liugddx
changed the title
|
@Astro-Han Thank you — this round changed the PR's shape, and both P1 "two words" objections were correct. Head 3819778489 (P1, 3819778502 (P1, "proven" without a green run) — Accepted. "Proven" is gone from the title, body, and both docs languages. The body commits to posting the run log (manifest counts, 3820922148 (P1, unconditional 102 / empty snapshot) — Accepted: 3820922156 (P2, destroys before it recovers) — Restructured as you proposed: two-step same-volume swap (new tree aside → backup in → write-back → only then discard the aside), with the undo path bringing the aside back if the backup cannot land. Every intermediate state keeps at least one complete installation on disk. Both renames retry 5×1s — the template's own cadence for destructive loops. 3819778512 / 3820922163 (P2, empty directory vs residue) — Accepted, including your LogicLib source read: emptiness is now decided by 3819778516 (P2, shared error flag) — Accepted: 3819778505 (P2, leftover backup semantics) — Implemented via a completeness marker written only after verification: a complete backup from an earlier attempt is adopted, never overwritten with a possibly-torn tree; "backup present, 3820922170 (P2, unverified backup at the commit point) — Accepted in the minimal-witness form you suggested: 3819778524 / 3819778529 (P2, backup timing and 101/103 visibility) — Visibility: 3819778535 (P2, tests described but not committed) — Committed in #3327 as 3820232602 / 3820922173 / 3821101776 / 3821101780 (P2, unbounded probes) — All bounded in #3327 (per-site 3819778543 (P3, encoding) — installer.nsh is ASCII-only, byte-scanned in the local gate. Thank you for compiling it with the pinned makensis — that also retired the syntax risk from the PR body. 3819778548 (P3, split) — Done: #3327 carries every harness change; this PR is installer.nsh, its verifier, workflow wiring, and docs. 3820922176 (P3, barrier comment) — Restated precisely: the uninstall key goes at uninstaller.nsh:250 and the install key at :254, so the wait's release point is one registry call before the true end; the residual window and the premise to re-check are in the comment. Remaining stated gaps (in the PR body, not hidden): the 103 branches, the empty-shell copy path, and an Abort during an adopted-backup run are not deterministically executed in CI — they need a live handle holder. If you'd like the held-handle regression (a child process with its CWD inside |
…ntracts Split out of apache#3265 at its reviewer's request so these fixes merge on their own evidence and that PR stands on the installer transaction alone. Contents: - Read the CDP port from the DevTools stderr announcement (waitForDevToolsPort) instead of pre-reserving one; widen renderer discovery to 90s with per-probe AbortSignal bounds and errno cause chains (four observed CI failures in this family). - waitForUsableRenderer: poll the renderer-usable state with the deadline as the sole authority — one stalled Runtime.evaluate used to fail the whole gate (run 32352924376); the WebSocket handshake now has its own bound so a port that accepts but never speaks fails the probe, not the lane. - Tolerate taskkill exit 128 when the relaunched instance already exited; the authoritative assertion remains waitForInstalledProcessesToExit. - Match the versioned uninstall DisplayName ('Maka 0.1.11'): the -eq 'Maka' filter matched nothing, deterministically, and every reader of the scan was blind. - Bound every PowerShell probe that runs under a polling deadline (the anti-pattern apache#3241 names), and let waits tolerate one failed probe: a failed enumeration is never treated as 'no processes'. - waitForUninstallRegistrationToClear: a detached uninstaller deletes its registry keys after waitUntilMissing sees the files disappear; wait for the registration to clear before the next install, with the one-registry-call residual window stated precisely. - directoryTreeManifest/diffTreeManifests shared exports for the rollback gate, now recording empty directories so their loss is visible; capture upgrade-state evidence on a relaunch version mismatch. - Commit the table-driven contract tests as scripts/verify-windows-harness.test.mjs and wire them into the CI planner test step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
The upgraded-app smoke pipes stderr but only waitForDevToolsPort's temporary listener ever read it: once removed, the paused stream lets Chromium's --enable-logging=stderr output fill the pipe and block the child, and the evidence the pipe exists to preserve is lost. Attach the same persistent collector every sibling smoke uses and append its tail to renderer-readiness failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Reworked from review (apache#3265): the restore is now a two-step same-volume swap ordered so every intermediate state keeps at least one complete installation on disk - the extracted new tree is moved aside, never deleted, before the backup is moved in, and is only discarded after the restore and registry write-back completed. Success is judged by the filesystem, not the error flag; a blocked-but-empty $INSTDIR shell is distinguished from real residue by enumeration and recovered through the copy path. Fail closed (101) when the upgrade has no uninstall registration and no adoptable backup: with nothing to restore, a failed upgrade would leave files without an uninstall entry and the template would silently merge trees on the next attempt. The backup is verified (executable witness) before anything destructive runs and marked complete only then; the registry snapshot is also persisted outside the keys the upgrade deletes, so an attempt that dies on a hookless template Quit path leaves a state the next run adopts and completes. Every kept-backup path writes RECOVERY-README.txt and shows a message when interactive. 102 is reported only after the filesystem witness and the write-back. The gate now pins the boundaries instead of assuming them: the Abort failpoint proves byte-identical restore (102); a Quit failpoint at the same moment proves no hook fires and the backup survives; a rerun proves adoption completes the upgrade; a deleted registration proves the 101 refusal leaves files untouched; the no-failpoint control run proves normal upgrades are unaffected. Title and docs now say what is covered - Abort-path rollback with backup retention - and spell out the Quit gap, the supported rerun recovery, and manual recovery steps. ASCII-only so the POSIX makensis toolchain can compile the include. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
7c3a8f2 to
4f9a118
Compare
…aunch Run 32378497920: taskkill /T /F on the force-run instance exceeded its 30s bound on a wedged runner and failed the gate, even though the authoritative assertion - waitForInstalledProcessesToExit, which fails with the live process list if anything from the install tree still runs - was one line below. Treat a kill that overran its bound like exit 128: the kill is the mechanism, the exit wait is the assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Reworked from review (apache#3265): the restore is now a two-step same-volume swap ordered so every intermediate state keeps at least one complete installation on disk - the extracted new tree is moved aside, never deleted, before the backup is moved in, and is only discarded after the restore and registry write-back completed. Success is judged by the filesystem, not the error flag; a blocked-but-empty $INSTDIR shell is distinguished from real residue by enumeration and recovered through the copy path. Fail closed (101) when the upgrade has no uninstall registration and no adoptable backup: with nothing to restore, a failed upgrade would leave files without an uninstall entry and the template would silently merge trees on the next attempt. The backup is verified (executable witness) before anything destructive runs and marked complete only then; the registry snapshot is also persisted outside the keys the upgrade deletes, so an attempt that dies on a hookless template Quit path leaves a state the next run adopts and completes. Every kept-backup path writes RECOVERY-README.txt and shows a message when interactive. 102 is reported only after the filesystem witness and the write-back. The gate now pins the boundaries instead of assuming them: the Abort failpoint proves byte-identical restore (102); a Quit failpoint at the same moment proves no hook fires and the backup survives; a rerun proves adoption completes the upgrade; a deleted registration proves the 101 refusal leaves files untouched; the no-failpoint control run proves normal upgrades are unaffected. Title and docs now say what is covered - Abort-path rollback with backup retention - and spell out the Quit gap, the supported rerun recovery, and manual recovery steps. ASCII-only so the POSIX makensis toolchain can compile the include. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
4f9a118 to
83ba1fd
Compare
Run 32380815350 failed at the first packaged smoke with the app's own '[runtime-host] fatal: Runtime Host stopped responding during startup' (product-level, tracked in apache#3279); nothing in this PR executes at that point. Evidence posted to apache#3279.
) Run 32382283646 failed in the pre-existing autoupdate gate: a Runtime Host execution candidate from the old app survived the quit and the NSIS upgrade bailed before touching anything (backup present, files and registration untouched). Product-level, filed as apache#3340 with the full process-command-line evidence; nothing in this PR executes at the failure point.
The gate's first full execution (run 32384536035) proved the covered path - Abort at the worst moment, 607 files restored with 0 diffs, registration back, launchable, control upgrade clean - and then failed its own scenario-3 pin: a bare template-style Quit measured exit 2 (the silent installer's generic failure code), not the 0 the NSIS source reading suggested. Pin the measured value, and assert the state (extracted files, cleared registration, retained backup with marker and recovery note) before the exit code so a future drift reports what actually happened rather than just a number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
The promised L3 evidence: So: Abort at the worst moment → 607 files restored with 0 diffs, registration restored (exit 102 asserted exactly), launchable at the old version; normal upgrade unaffected; the hookless-Quit gap pinned at its measured exit code (2 — my earlier source-reading said 0; the gate's own first execution corrected it, and the pin now asserts state before exit code so any future drift reports what actually happened); rerun adoption completes the upgrade; registration-less upgrade refused at 101 with files untouched. One earlier lane run also passed the same autoupdate stage and then failed only the old scenario-3 pin — that run additionally caught a real product bug on its previous attempt: an orphaned Runtime Host surviving |
|
All lanes are green at head |
3 participants
Summary
Adds Abort-path rollback with backup retention to the one-click NSIS installer, a release gate that exercises it scenario by scenario, and docs that state the covered boundary precisely. Retitled from "transactional / proven" per review: the restore hangs off
.onInstFailed, which onlyAbortreaches, and the claim now says exactly that.Stacked on #3327 (harness hardening, split out per review) — the diff here is installer.nsh, the rollback verifier, workflow wiring, and docs only.
Mechanism (apps/desktop/build/installer.nsh)
$INSTDIRto a same-volume sibling, verify the copy (executable witness) before the destructive boundary, mark it complete, writeRECOVERY-README.txtinto it now (the hookless Quit exits run none of our code after a failure), and persist the registry snapshot outside the keys the upgrade deletes.$INSTDIRshell is distinguished from real residue byFindFirstenumeration (IfFileExists "dir\*.*"is true for an empty directory) and recovered through the copy path. Success is judged by the filesystem, not the error flag. 102 only after the witness and the write-back; 103 keeps the backup and refreshes the recovery note (plus an interactive message).Gate scenarios (scripts/verify-windows-installer-rollback.mjs)
Quitat the same moment → exit 0, no hook fires, extracted files stay, registration stays gone, backup + README retained. This is the template's real failure shape and it is pinned, not assumed.Boundaries (stated in docs/windows-support.md, EN+zh, with manual recovery steps)
Quitbranches (old-uninstaller failure, extraction retry exhaustion/cancel) get no automatic rollback — backup retention + rerun adoption is the supported recovery, and scenario 3+4 prove it.Verification
node --check+ biome on all scripts; installer.nsh byte-scanned ASCII-clean; harness contract tests (in test(windows): harden the release-verification harness and pin its contracts #3327) 36 pass / 1 skip; an independent review pass gated this push (its blocking finding — README missing on hookless Quit paths — is fixed above).Quit→exit-0 and.onInstFailed→SetErrorLevelpins —release-windows-checkon this PR. The run log (manifest counts,exit code 102,0 differing files, control run) will be posted here once green, before any "verified" wording ships anywhere else.Part of #2142.