Follow-up: bounded simplification of the Windows installer rollback (#3265)

[Astro-Han]

Context

apps/desktop/build/installer.nsh and scripts/verify-windows-installer-rollback.mjs landed in #3265 (merged as 9de05e26647f6c63e4c6b67cb7b882d0f77a4290). Together they add a Windows installer transaction: back up the old program bytes and registry registration before the destructive phase, restore them if the install fails, retain recoverable state across a hookless Quit, and prove those guarantees on a real Windows runner.

A follow-up simplification audit was run against the merged code. This issue records what it found, so that the work can be picked up independently. The PR itself is not being reverted or reopened.

Read this before proposing a deletion

The audit began from the hypothesis that automatic rollback might be dead code — that nothing in the shipped install path can actually reach .onInstFailed, and that the hook existed only to serve the test failpoints. That hypothesis is false, and it is the most likely wrong turn for anyone approaching this code fresh.

The pinned electron-builder 26.15.3 one-click section runs the old uninstaller and then executes built-in NSIS File instructions — the optional uninstaller icon into $INSTDIR, the embedded app package into $PLUGINSDIR, and the generated uninstaller into $INSTDIR. NSIS documents that when a File cannot be extracted, created, or overwritten and the user answers Cancel, the installer aborts and .onInstFailed runs. Disk-full, insufficient permissions, or a locked target therefore reaches the hook after the old installation has already been destroyed.

It is true that the destructive section contains no explicit post-destruction Abort outside the PR's own test instrumentation. That observation is real, but "no explicit Abort" is not "no .onInstFailed entry."

One specific near-miss worth naming, because it looks like supporting evidence and is not: include/extractAppPackage.nsh has a copy-retry Cancel branch whose label is AbortExtract7za, but the instruction it executes is Quit, which bypasses callbacks entirely. It is a counterexample, not a confirmation.

Removing automatic rollback would remove real protection.

Where the lines actually are

The framing "1,388 lines to fix an empty-directory check" does not survive contact with the code. The FindFirst helper that fixes the vacuously-true ${If} ${FileExists} "$INSTDIR\*.*" condition is 21 lines, and it is one contingency inside the new transaction rather than the reason the transaction exists.

apps/desktop/build/installer.nsh, 630 lines:

Lines	Responsibility
118	License, design/limits contract, constants, 18 state variables
21	makaDirHasEntries — the FindFirst fix
52	Recovery README/UI and persisted-backup identity validation
38	Required registry read-back verification
195	Abort restore state machine
157	Pre-destructive snapshot, backup creation/verification, prior-run adoption
33	Test failpoints and architecture hooks
16	Success cleanup

Roughly 350 lines are backup/snapshot/adoption lifecycle and roughly 230 are restore and verification. These categories are interdependent, so "everything except the 21-line fix is incidental" is not a valid decomposition.

Item 1 — Reuse the existing packaged-app verifier

This is the only item with no product-contract consequences, and it is the one suitable for an outside contributor.

scripts/verify-windows-installer-rollback.mjs:176 defines a bespoke assertLaunchable, called once at :367. The repository already owns this authority: verifyPackagedWindowsApp in scripts/verify-windows-x64.mjs:101, which scripts/verify-windows-installer-lifecycle.mjs:380 uses on the same artifact shape (see :427 for the artifactContract: 'legacy-baseline' option). It launches the installed app, waits for a usable renderer, and verifies the product version — which is exactly what the local wrapper does, and no rollback-specific semantics live in it.

Replace the local implementation and its CDP/home-directory imports with:

verifyPackagedWindowsApp(installDirectory, {
  workingDirectory,
  expectedVersion,
  artifactContract: 'legacy-baseline',
})

Estimated net deletion: 35–45 lines.

You do not need a Windows machine. .github/workflows/release-windows-check.yml path-filters on scripts/verify-windows-installer-rollback.mjs, so touching that file runs the real rollback gate on a hosted Windows runner. Locally, node --test scripts/verify-windows-harness.test.mjs covers the harness unit tests.

Items 2–4 — Maintainer decisions, not cleanup

Each of these deletes lines by consciously weakening a promise. They need a maintainer to make the call first; please do not send a PR for these without that decision recorded here.

2. Empty-shell salvage (45–55 NSIS lines). If the partial-tree rename still fails after retries, return 103 immediately and retain the verified sibling backup plus the recovery note — the same behaviour that already applies to a populated directory that cannot be moved. This drops automatic healing for a held-but-empty shell, a branch the Windows gate does not currently exercise. Decision: is "verified backup plus documented manual recovery" acceptable there?

3. Second backup after registry write-back failure (60–75 lines). By the time this branch runs, $INSTDIR\Maka.exe is already the verified old tree; the extra copy exists only so a later installer run can adopt it. A registry policy or ACL that rejected the first writes is unlikely to be repaired by immediately rerunning the same installer. Decision: narrow the 103 promise from "may self-heal on rerun" to explicit manual recovery.

4. Rerun recovery after Quit (150–190 lines). This is the largest single block, and it contradicts the merged documentation's supported "rerun the installer to recover" promise. Note that backup retention cannot be removed regardless — a Quit or hard kill leaves the copy because no callback can run. What is deletable is the persisted registry snapshot, marker/version adoption, and conflict handling that let a later process adopt it safely. Decision: is cross-run automatic adoption a supported product promise?

Maximum if all four land: roughly 290–365 lines. There is no evidence-backed path to deleting ~1,000 lines while retaining current behaviour.

Two things deliberately ruled out

Do not fold the rollback verifier into verify-windows-installer-lifecycle.mjs. They share mechanics but own different acceptance contracts — lifecycle proves install → upgrade → smoke → uninstall, rollback proves nonzero exit codes across seven abnormal filesystem/registry states. Folding preserves nearly every scenario while turning a 510-line lifecycle authority into an approximately 1,000-line modeful one: perhaps 80–120 lines of boilerplate saved, no concepts removed, worse failure ownership. The 637 lines are also not duplicated unit tests — the file already imports the shared manifest/CDP/process/lifecycle helpers.

Do not replace the backup with a manifest or state record. A manifest records identity, not bytes, and the old uninstaller deletes the old tree before the new package commits. The explicit 11-value registry snapshot likewise mirrors values the pinned template deletes; shrinking it converts "restore prior registration" into a weaker, newly specified contract.

The actually-fundamental fix, for the record

Stage and validate the new application in a sibling version directory, then atomically swap directory identity and write registry state last. Rollback becomes the inverse rename and the old tree is the backup — no snapshot, no adoption, no rollback-of-the-rollback.

electron-builder 26.15.3 owns the destructive uninstaller-first sequence, so this needs an upstream change or a maintained custom template. Doing it only inside installer.nsh would add another installer authority and likely more code, not less. This is a separate architecture project, not a line-reduction patch.

Also worth correcting

The merged docs and comments should name built-in File failure/Cancel as the real production .onInstFailed entry, and state that the gate simulates it with an explicit Abort at a later point rather than executing a genuine disk-full or permission failure.

---

Prepared from an AI-assisted simplification audit of the merged code. Line counts, the verifyPackagedWindowsApp call sites, and the CI path filter were verified against main; the File-failure entry path is established from official NSIS semantics plus the exact generated 26.15.3 template ordering, and is source-inferred rather than executed.

[Astro-Han]

Scope note on labels, added by the issue author.

This issue was briefly tagged help wanted and that tag has been removed. The reasoning for removing it is worth recording, because it is about the shape of the work rather than its difficulty.

Only Item 1 — reusing verifyPackagedWindowsApp in place of the bespoke assertLaunchable, roughly 35–45 lines — is genuinely self-contained. It changes no published guarantee, and CI runs the real Windows rollback gate for anyone who touches that file, so no local Windows machine is needed.

Items 2 through 4 look like deletions but are product decisions wearing deletion clothing. Each one trades automatic recovery for documented manual recovery, and each needs NSIS familiarity plus Windows fault injection to validate. Labelling the issue as a whole help wanted would invite a contributor to make a maintainer's product tradeoff on their behalf — and, worse, would put them on the same wrong path the audit started from, without the NSIS background that got the audit back off it.

If you would like to pick up Item 1 on its own, say so here and it can be split into its own scoped issue.

[Astro-Han]

Closing as a duplicate of #3575, which covers the same audit with a finer per-item decomposition and should be treated as the canonical issue.

The duplication is my doing, not a disagreement: I filed this one from the audit report while the auditor was independently writing up #3575 after I asked for a contributor-readable version. My instruction was ambiguous about who was filing.

Everything actionable here also appears in #3575. Please continue there.