You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "liugddx (via GitHub)" <gi...@apache.org> on 2026/08/20 00:18:54 UTC
[PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
liugddx opened a new pull request, #3265:
URL: https://github.com/apache/maka/pull/3265
## What & why
Closes the #2142 Phase 3 item *"Design transactional backup/restore and prove a deterministic mid-install failure leaves the previous installation byte-identical and launchable"*.
electron-builder's default one-click upgrade runs the **old uninstaller first** — which removes the entire install directory and the uninstall registry keys — and only then extracts the new files (installSection.nsh:52 → installUtil.nsh:224 → uninstaller.nsh:185-187, 250-254 in the pinned app-builder-lib templates). Any failure after that point strands the machine with no working install and no rollback source. Until now the docs could only disclaim this.
## What this does
**1. Transactional upgrade hook (`apps/desktop/build/installer.nsh`, via `nsis.include`).**
- `customInit` (before anything destructive — the app kill and old-uninstaller both run later, inside the install section): back up `$INSTDIR` to a same-volume sibling and snapshot exactly the registry values `registryAddInstallInfo` owns. A backup failure **fails the upgrade closed (exit 101)** with the old install untouched: no backup, no upgrade.
- `.onInstFailed` (any script `Abort`): remove the partial install, restore the backup (rename; copy fallback; the backup is kept for manual recovery if the restore itself errors) and re-create the snapshotted registry state — **exit 102** on a completed restore, 103 otherwise. Idempotent, and inert for fresh installs.
- `customInstall` (success tail): delete the backup.
- **Deterministic test-only failpoint**: `MAKA_INSTALLER_TEST_FAILPOINT=after-extract` (runtime `ReadEnvStr`, exact-value gate) aborts in `customFiles_*` — after extraction, at the worst possible moment (old files gone, new uninstaller/registry not yet written). Same security narrative as `MAKA_UPDATE_TEST_FEED`: setting the variable requires same-user code execution and can only make an upgrade fail *and roll back* — self-denial-of-service; unset, the added behavior is one `ReadEnvStr`, the backup, and the post-success backup removal. Functions read only variables assigned in `customInit`, so nothing depends on the include's position relative to template defines.
**2. Evidence harness (`scripts/verify-windows-installer-rollback.mjs`).** Installs the candidate, records a full per-file SHA-256 tree manifest (`directoryTreeManifest`/`diffTreeManifests`, new exports next to `sha256File`; nothing skipped, non-file entries throw), then:
- runs the version-bumped next installer with the failpoint armed and asserts the exit code is **exactly 102** (distinguishing 101/103/0/other with named diagnoses);
- asserts zero missing/extra/changed files against the pre-upgrade manifest, no backup residue, the uninstall registry `DisplayVersion` restored, and the restored app **launching** (CDP renderer-usable) at the old `ProductVersion`;
- control run: the very same installer without the failpoint must succeed, report the next version, and leave no backup — the transactional machinery must not disturb normal upgrades (the existing pinned-upgrade and auto-update gates in the same lane are further regression cover);
- silent uninstall; cleanup mirrors the installer-lifecycle verifier's AggregateError chain.
**3. CI.** Both Windows workflows run the proof after the auto-update verification (release: before asset upload, so a regression blocks the release), reusing the candidate installer and the `release-autoupdate-next` build — whose NSIS code, compiled from the same config, is what actually executes during the upgrade. Timeouts: check 75→90, release 60→75.
**4. Docs (`docs/windows-support.md`, EN+zh).** The proven boundary is stated exactly: transactional **Abort-path** rollback with byte-identical file restore (file contents, not ACLs/timestamps), registration restore, and a launchable old version. Explicitly still unproven: a hard kill of the installer process or power loss (the backup copy is not forced through volatile device caches, same terms as the existing durability boundary), installer failure branches that exit without aborting, and business-data migration.
## Verification
- Local (this machine lacks VS Build Tools, so packaged verification is CI-scoped): `node --check` on all touched scripts; table-driven tests of `directoryTreeManifest`/`diffTreeManifests` (sorted POSIX paths, sha256 shape, identical→empty diff, change/delete/add detection) and the platform guard; biome clean.
- **Authoritative end-to-end evidence = this PR's own `release-windows-check` lane**: package → verify → pinned-upgrade → autoupdate-next → autoupdate → **rollback**, on windows-2025. The rollback step's log must show the manifest file count, `exit code 102`, `0 differing files`, the restored-version assertions, and the control-run success. The `.nsh` compiles for the first time in that lane (no local makensis); expect the first run to be the real syntax/behavior check and iterate from its log if needed.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Fable 5 (Anthropic), driven by the human contributor of record, authored the NSIS transactional hook, the rollback harness and manifest tooling, the CI wiring, and the documentation updates, all under human review against the pinned app-builder-lib NSIS templates. Commits carry `Generated-by: Claude Fable 5` trailers; the final squash commit must retain them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5384744634
Hi — this PR was behind `main` and could not be merged as-is. Since `maintainer_can_modify` was enabled, I rebased it onto current `main` and force-pushed the result; the rebase applied with **zero conflicts**, so your changes are unchanged in content (author attribution is preserved — only the commit SHAs moved).
Please `git fetch && git reset --hard <your-remote>/<branch>` before your next push so you do not re-introduce the old base.
CI should now run against the rebased head. I will pick this up for review once the checks are terminal.
---
*AI-assisted maintenance action. It is not a review and does not count as the required human review under `CONTRIBUTING.md` §Review.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5359024356
All lanes are green at head `77f81e292`: `test`, `audit`, and `release-windows-check` with every rollback scenario executed (evidence log in the comment above). Together with #3327 (also fully green), this round addresses all previously filed findings; ready for re-review. Merge order per the split: #3327 first, then this rebases trivially (it already sits on that branch's commits).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5384881864
@Astro-Han Addressed in `8f003ad59`.
The machine-wide identity assumption was wrong. The existing global registration-clear barrier did prevent a pre-existing Maka registration from reaching the destructive scenario, but it did not justify a later `DisplayName` wildcard or close the race where another registration appears after that barrier.
The rollback verifier now derives ownership from the fixture's exact quoted `UninstallString` executable path:
- it parses the first quoted executable from each uninstall command;
- canonicalizes it with `GetFullPath`;
- compares it to the isolated fixture's `Uninstall Maka.exe` using `OrdinalIgnoreCase`;
- returns `DisplayVersion` only for that exact key;
- deletes only the matched key's `PSPath` with `-LiteralPath`.
All seven scenario reads and the registration-less deletion use this scoped helper. The startup-wide barrier remains as a fail-closed precondition for machines that already have Maka registered. The platform-neutral regression test asserts the exact path binding, rejects the former `DisplayName` wildcard, and pins deletion to the matched key.
Fresh exact-head evidence is green:
- [Core CI 32625399097](https://github.com/apache/maka/actions/runs/32625399097), 15m28s
- [dependency audit 32625399104](https://github.com/apache/maka/actions/runs/32625399104), 45s
- [Release Windows check 32625399096](https://github.com/apache/maka/actions/runs/32625399096), 14m00s
- rollback: 234 entries / 0 diffs; registry-mismatch 103 recovery, stale/incomplete 101 refusals, hookless Quit retention, exact adoption, and registration-less 101 all passed through the real Windows registry provider.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5366142571
@Astro-Han #3327 is now merged, and this PR has been rebuilt cleanly on current `main` as requested. New exact head: `442d8a17e`.
The old stacked harness commits and the two CI-only retrigger commits are gone. The history is now exactly two substantive commits:
- `92b7b8342` — Abort-path installer rollback with backup retention
- `442d8a17e` — measured hookless-Quit exit-code contract
The net diff is limited to seven rollback-owned files: the NSIS hook, rollback verifier, electron-builder wiring, the two Windows release workflow call sites, root script wiring, and Windows support docs. No #3327 harness implementation is duplicated.
Local verification after rebuilding on current main:
- full `npm run rebuild` passed
- rollback-adjacent planner/harness tests: 31 passed, 1 symlink-privilege skip
- syntax, Biome, and diff checks passed
- release policy: every check passed except the unrelated main-branch Eval Git-fixture cleanup test, which reproducibly hits a local Windows `EBUSY` removing its temp repository; that file is outside this PR diff
Fresh `test`, `audit`, and Windows `package` checks are running now. PTAL on the narrowed exact head when they finish.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5369556869
@Astro-Han PTAL on exact head `68520adcc8dd4ef42447ab065279be9b075aece6`.
The branch is rebased onto main with #3327/#3382, all four active threads have exact-head replies, and all gates are green:
- CI: [32477989467](https://github.com/apache/maka/actions/runs/32477989467), 13m08s
- dependency audit: [32477989418](https://github.com/apache/maka/actions/runs/32477989418)
- Release Windows check: [32477989412](https://github.com/apache/maka/actions/runs/32477989412), 23m21s
- rollback: 607 entries / 0 diffs, registry-mismatch 103+recovery, stale and incomplete backup 101 refusals, Quit retention/adoption, and registration-less 101 all passed
The PR description and EN/zh docs now state the Abort/Quit and conditional 103 boundaries directly. Independent local author-gate found no remaining P0-P2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5384360809
Independent review of `68520adcc8dd4ef42447ab065279be9b075aece6`.
## [P3] `verify-windows-installer-rollback.mjs` assumes it owns the machine's Maka uninstall registrations
`scripts/verify-windows-installer-rollback.mjs:98-110`:
```powershell
Get-ChildItem 'HKCU:\Software\Microsoft\Windows\CurrentVersion\Uninstall' |
Where-Object {
$name = ($_ | Get-ItemProperty).DisplayName
$name -eq 'Maka' -or $name -like 'Maka *'
} |
Remove-Item -Recurse -Force
```
The install directory is properly isolated — `mkdtemp` + `/D=` + an isolated environment. **The registry is not.** That wildcard delete is unscoped: on any machine with a real Maka installed, the fail-closed scenario at `:491` removes the *production* uninstall registration along with the fixture's.
The two read helpers make the same assumption in the other direction. `readUninstallDisplayVersions` returns every matching registration joined together, so:
- the Quit-gap pin at `:447-451` asserts `orphanRegistration !== ''` fails the run — a production registration is present, so this always throws;
- the precondition at `:235-238` splits the joined value on `,` and looks for the candidate version, which a foreign registration perturbs.
On a clean CI runner none of this shows. **That is exactly why it is worth naming**: the script reads as machine-agnostic and it is not. Anyone who runs it on a developer box with Maka installed gets a failure that looks like a rollback defect, and loses their production uninstall entry on the way.
Right now the only thing preventing the destructive case from being reached in a normal run is assertion *ordering* — the Quit assertion throws before the wildcard delete executes. That is luck, not design.
Suggested fix: a precondition at the top of the script that enumerates Maka uninstall registrations and refuses to run if any predate this run. Fail closed, before touching anything.
## What we checked and found nothing on
**Template hook points**, verified against the `app-builder-lib` version pinned in this repo rather than from memory:
- `customInit` is spliced *after* `initMultiUser` in `installer.nsi`'s `.onInit`, so `$INSTDIR` is already valid.
- `customFiles_x64` exists at `extractAppPackage.nsh:58`, positioned after decompress and before `registryAddInstallInfo`. This is what makes the PR's stated worst moment — "new files landed, old files deleted, registry not yet written" — an accurate description rather than an approximation.
- `customInstall` (which deletes the backup) runs *after* `registryAddInstallInfo`, so the success path never clears the backup while the registry write is still outstanding.
- `.onInstFailed` is unoccupied in the template, and an `Abort` inside a section reaches it.
**Interrupted rollback.** We pushed every interruption point of the two-step rename. Killed between step 1 and step 2: `$INSTDIR` is absent, the backup retains its marker intact, the aside copy is intact. The next installer run sees no `$INSTDIR\Maka.exe`, takes the fresh-install path, and the success-path `customInstall` clears both the backup and the aside leftovers. **State converges; leftovers do not accumulate.**
**Backup retention.** At most one backup directory. A mismatched marker refuses the upgrade (fail closed, with a test). A matching marker is adopted rather than overwritten. Success deletes it. Bounded, and it cannot be mistaken for the current version because the marker is checked against the snapshot version before adoption.
**Privilege shape.** This is a per-user install — `HKCU` plus user-owned directories — so backup and install directory share one security context. The "unprivileged user tampers with the backup so rollback writes it into a system location" shape **does not exist here, because nothing elevates.** The failpoint environment variable is read-only, can only affect the caller's own run, is documented, and follows the existing `MAKA_UPDATE_TEST_FEED` precedent.
**Registry consistency.** The snapshot covers every value the template deletes. The required values (`DisplayName`, `UninstallString`, `NoModify`, `NoRepair`) are read back and verified after being rewritten, so exit code 102 cannot be reported unverified. `EstimatedSize` is deliberately not restored, which is cosmetic and carries a comment saying so.
**Honest scope.** The documentation states plainly that the Quit path (the template's own failure branch, which has no hook), a hard kill, and power loss are not covered — and then uses the failpoint to pin the Quit gap and demonstrate that a re-run recovers. **The claimed coverage and the actual coverage match.** No overstatement.
## Candidates raised and withdrawn
- **The sibling backup rebuilt on the 102 path carries a self-referential marker.** That is the restored tree copied back; the marker content matches the snapshot version, so the next adoption behaves correctly. Withdrawn.
- **The marker is a plaintext version string and could be forged.** Per-user context: anyone able to edit the marker can already edit the install directory. No privilege is gained. Withdrawn.
- **+1258/−12, almost entirely additions — is this patch-on-patch?** No: this is the repository's *first* rollback mechanism, so there is no prior layer being stacked on. The 12 deleted lines are the documentation statement that rollback was not yet promised. **Net entropy decrease — a known exposure is removed**, and 613 lines of NSIS plus 562 lines of verification are its legitimate price.
## Gate state
- exact head `68520adc`, non-draft.
- **`mergeable = CONFLICTING` / `mergeStateStatus = DIRTY`** — needs a rebase. (Note for anyone checking via a bulk `gh pr list --json mergeable`: that returns `UNKNOWN` because GitHub computes it lazily; it must be re-polled per PR.)
- check-runs on this exact head: `test`, `audit`, `package` all `completed / success`. Within the `test` run, `Release Windows check` → `Prove deterministic mid-install failure rollback` is `success` (run 32477989412), so **the full seven-scenario gate has genuinely executed on a clean runner** — this is a real green, not an absent one.
- Existing review endpoints read separately: zero comments, zero reviews, zero inline comments. Nothing to deduplicate against.
- We did **not** run the installer on a real Windows machine, and the reason is the finding above rather than convenience.
No overall verdict offered. Reviewed 2026-08-23 13:20 UTC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #3265:
URL: https://github.com/apache/maka/pull/3265
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3265:
URL: https://github.com/apache/maka/pull/3265#discussion_r3820922163
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
Review 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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
Review 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 wh
atever the exit code, a launchable `Maka.exe` exists — old or new — rather than an empty directory.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
Review 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 uninst
all 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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
+FunctionEnd
+
+Function .onInstFailed
+ Call makaRestoreFromBackup
+FunctionEnd
+
+!macro customInit
+ StrCpy $makaBackupDir "$INSTDIR.pre-upgrade-backup"
+ StrCpy $makaInstallRegKey "${INSTALL_REGISTRY_KEY}"
+ StrCpy $makaUninstallRegKey "${UNINSTALL_REGISTRY_KEY}"
+ StrCpy $makaFailpointArmed "0"
+ ReadEnvStr $R9 MAKA_INSTALLER_TEST_FAILPOINT
+ # S== is case-sensitive: the accepted value is a single exact token.
+ ${If} $R9 S== "after-extract"
+ StrCpy $makaFailpointArmed "1"
+ ${EndIf}
+ ${If} ${FileExists} "$INSTDIR\${APP_EXECUTABLE_FILENAME}"
+ # Snapshot the registry values the template's upgrade path deletes, while
+ # they still exist (the old uninstaller runs inside the install section,
+ # after this hook). The set mirrors registryAddInstallInfo, including the
+ # conditional MenuDirectory (reading a missing value yields "" and the
+ # write-back skips empty values).
+ ReadRegStr $makaPrevInstallLocation SHELL_CONTEXT "$makaInstallRegKey" InstallLocation
+ ReadRegStr $makaPrevKeepShortcuts SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts
+ ReadRegStr $makaPrevShortcutName SHELL_CONTEXT "$makaInstallRegKey" ShortcutName
+ ReadRegStr $makaPrevMenuDirectory SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory
+ ReadRegStr $makaPrevDisplayName SHELL_CONTEXT "$makaUninstallRegKey" DisplayName
+ ReadRegStr $makaPrevUninstallString SHELL_CONTEXT "$makaUninstallRegKey" UninstallString
+ ReadRegStr $makaPrevQuietUninstallString SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString
+ ReadRegStr $makaPrevDisplayVersion SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion
+ ReadRegStr $makaPrevDisplayIcon SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon
+ ReadRegStr $makaPrevPublisher SHELL_CONTEXT "$makaUninstallRegKey" Publisher
+ ReadRegStr $makaPrevComments SHELL_CONTEXT "$makaUninstallRegKey" Comments
+ ClearErrors
+ # An interrupted earlier run may have left a stale backup; replace it so
+ # the backup always mirrors the install being upgraded right now.
+ RMDir /r "$makaBackupDir"
+ CreateDirectory "$makaBackupDir"
+ CopyFiles /SILENT "$INSTDIR\*.*" "$makaBackupDir"
+ ${If} ${Errors}
Review 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.
##########
scripts/verify-windows-installer-lifecycle.mjs:
##########
@@ -85,6 +85,55 @@ export async function waitUntilMissing(
}
}
+/**
+ * Every HKCU `DisplayVersion` registered under the Maka display name, joined
+ * with commas ('' when none). One PowerShell scan, shared by the harnesses
+ * that assert on — or wait out — the uninstall registration.
+ */
+export async function readUninstallDisplayVersions({ run = runCommand } = {}) {
+ const script = String.raw`
+$entries = Get-ChildItem 'HKCU:\Software\Microsoft\Windows\CurrentVersion\Uninstall' |
+ ForEach-Object { Get-ItemProperty $_.PSPath } |
+ Where-Object { $_.DisplayName -eq 'Maka' }
+@($entries | ForEach-Object { $_.DisplayVersion }) -join ','
+`;
+ const { stdout } = await run('powershell', ['-NoProfile', '-NonInteractive', '-Command', script]);
Review 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.
##########
scripts/verify-windows-installer-lifecycle.mjs:
##########
@@ -85,6 +85,55 @@ export async function waitUntilMissing(
}
}
+/**
+ * Every HKCU `DisplayVersion` registered under the Maka display name, joined
+ * with commas ('' when none). One PowerShell scan, shared by the harnesses
+ * that assert on — or wait out — the uninstall registration.
+ */
+export async function readUninstallDisplayVersions({ run = runCommand } = {}) {
+ const script = String.raw`
+$entries = Get-ChildItem 'HKCU:\Software\Microsoft\Windows\CurrentVersion\Uninstall' |
+ ForEach-Object { Get-ItemProperty $_.PSPath } |
+ Where-Object { $_.DisplayName -eq 'Maka' }
+@($entries | ForEach-Object { $_.DisplayVersion }) -join ','
+`;
+ const { stdout } = await run('powershell', ['-NoProfile', '-NonInteractive', '-Command', script]);
+ return stdout.trim();
+}
+
+/**
+ * An NSIS uninstall is not over when the files are gone. Launched without
+ * `_?=`, "Uninstall Maka.exe" copies itself to %TEMP% and detaches; the copy
+ * removes $INSTDIR first and deletes the uninstall/install registry keys as
+ * its LAST action (uninstaller.nsh: RMDir /r, then shortcuts, SHChangeNotify
+ * and app-data handling, then DeleteRegKey), which on a busy runner lands tens
+ * 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5353011293
@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 `DevTools listening on ws://127.0.0.1:<port>/` from stderr. They will conflict in `scripts/verify-packaged-app.mjs` and `scripts/verify-windows-autoupdate.mjs`.
Your mechanism is better on the axis that bit me: a stderr line is fresh per launch, so it cannot go stale. The `DevToolsActivePort` file can — Chromium removes it only on a clean exit, and the upgrade-lifecycle check reuses one user-data directory across two app versions with a kill in between, so my first CI run polled the previous instance's dead port for the full deadline. I handle it by deleting the file before spawning, but yours does not need the guard at all. You also already piped stderr at the smoke site that had `stdio: ['ignore','ignore','ignore']`, which is the one place a stderr-based reader would otherwise be blind.
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:
- **Each poll attempt needs its own bound.** A `fetch` against a bound-but-unresponsive endpoint hangs for undici's 300-second headers default, so one run reported *355 seconds* against a 120-second deadline. A 90s or 120s loop deadline cannot hold without a per-attempt abort.
- **The Windows process probe is unbounded and sits inside two deadline-bounded loops.** `listInstalledProcesses` runs `Get-CimInstance Win32_Process` with no timeout; it stalled while NSIS was relaunching the upgraded app and hung a job for 59 minutes past a 120-second deadline. Bounding it turned that into a 74-second failure, and the loops now treat a stalled probe as an unknown round so the deadline stays the authority.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5353268629
@Joob1n Thanks for flagging this before the conflict decided it for us — and for the `DevToolsActivePort` staleness analysis; the reuse-one-user-data-dir-across-versions case is exactly why this PR went with the per-launch stderr line.
One disclosure first: half an hour before reading your comment I pushed d1aee1ae4 to this PR, which already lands one of your two orthogonal items — a per-attempt bound on the CDP poll (`AbortSignal.timeout(2000)` per `fetch`, 250ms spacing, loop deadline stays the authority) plus errno cause-chain + app-stderr evidence on failure. The motivating data matches yours: our last lane failure reported "90 seconds" but ran ~6 minutes, same unbounded-connect overshoot you measured as 355s against a 120s deadline. Sorry for the collision — it was reacting to our own red lane, not lifting from #3241. If you'd rather the bound carry your numbers/shape, happy to adjust to whatever review converges on.
Given that, the split that leaves no overlap at file level:
- **#3265 carries**: stderr port discovery + per-attempt poll bound + failure evidence (all already in this PR's `verify-packaged-app.mjs` / `verify-windows-autoupdate.mjs` changes, CI is running on them now).
- **#3241 narrows to**: the bounded `listInstalledProcesses` probe and stalled-probe-as-unknown-round loop semantics in `verify-windows-installer-lifecycle.mjs` — fully orthogonal, this PR does not touch that file, and your 59-minute-hang → 74-second-failure result is evidence nobody else has. That fix stands on its own regardless of merge order.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3265:
URL: https://github.com/apache/maka/pull/3265#discussion_r3821101776
##########
scripts/verify-packaged-app.mjs:
##########
@@ -209,6 +259,45 @@ export function isPackagedRendererUsable(rendererState) {
);
}
+/**
+ * Poll a freshly booted packaged app over CDP until its renderer reports the
+ * usable state. One evaluation can stall past its own socket timeout while
+ * the renderer is still booting — observed on the Windows release runners,
+ * where a single timed-out `Runtime.evaluate` used to fail the whole gate.
+ * The deadline here is the authority: an individual failed probe is retried,
+ * not fatal, and only the deadline (or child exit) fails the wait. The last
+ * probe error or renderer state is reported as evidence either way.
+ */
+export async function waitForUsableRenderer(
+ webSocketDebuggerUrl,
+ child,
+ { deadlineMs = 30_000, description = 'Packaged renderer' } = {},
+) {
+ const deadline = Date.now() + deadlineMs;
+ let state;
+ let lastError;
+ for (;;) {
+ try {
+ state = await evaluateRenderer(webSocketDebuggerUrl);
Review 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`.
##########
scripts/verify-windows-autoupdate.mjs:
##########
@@ -412,17 +411,70 @@ export async function verifyWindowsAutoupdate(
await delay(1_000);
}
const productVersion = await readInstalledProductVersion(installedExecutable, { run });
- assertWindowsProductVersion(productVersion, nextVersion);
+ try {
+ assertWindowsProductVersion(productVersion, nextVersion);
+ } catch (error) {
+ // Nondeterministic CI state observed once: a Maka.exe process is
+ // running from the install directory while the on-disk executable is
+ // still the previous version. The only code path in the NSIS template
+ // that launches the app is StartApp at full Section success, which
+ // contradicts old bytes on disk — so on mismatch, capture the raw
+ // state needed to attribute the launch and the transaction stage:
+ // each process's command line (StartApp passes `--updated`), the
+ // pre-upgrade backup directory (exists ⇒ the upgrade quit between
+ // customInit and customInstall), the uninstall registration, and the
+ // executable timestamps (rewritten ⇒ extraction ran).
+ const evidence = [`relaunched processes: ${JSON.stringify(relaunched)}`];
+ try {
+ const commandLines = await run('powershell', [
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on code in PR #3265:
URL: https://github.com/apache/maka/pull/3265#discussion_r3830003612
##########
.github/workflows/release-windows-check.yml:
##########
@@ -105,3 +108,10 @@ jobs:
npm run verify:windows-autoupdate -- \
"apps/desktop/release/Maka-${version}-win-x64.exe" \
apps/desktop/release-autoupdate-next
+
+ - name: Prove deterministic mid-install failure rollback
Review Comment:
Fresh exact-head evidence is now green: [Release Windows check 32477989412](https://github.com/apache/maka/actions/runs/32477989412) passed in 23m21s on `68520adc`.
The rollback step executed after the successful `0.1.11 -> 0.1.12` autoupdate and reported:
- `manifest covers 607 files`
- `0 differing files across 607 entries`
- restored registration and launchability
- registry mismatch returned 103 and the no-failpoint recovery rerun completed
- stale-version backup refusal passed
- matching marker without `Maka.exe` refusal passed
- hookless Quit retention and exact adoption recovery passed
- registration-less/no-backup refusal passed
- `verified Abort-path rollback and its boundaries for 0.1.11 -> 0.1.12`
The PR body now links this run. The old `transactional/proven` wording remains removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on code in PR #3265:
URL: https://github.com/apache/maka/pull/3265#discussion_r3829867992
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
+FunctionEnd
+
+Function .onInstFailed
Review Comment:
Addressed at exact head `68520adcc8dd4ef42447ab065279be9b075aece6`. The PR title, installer header, and EN/zh docs now explicitly limit the mechanism to Abort-path rollback; the template Quit branches are stated as having no automatic rollback. The Windows verifier injects a separate hookless Quit at the same worst point, asserts the measured exit 2 and retained backup/README with no registration, then proves a rerun adopts only an exact marker/snapshot match. Fresh Windows L3 is running on this head.
##########
.github/workflows/release-windows-check.yml:
##########
@@ -105,3 +108,10 @@ jobs:
npm run verify:windows-autoupdate -- \
"apps/desktop/release/Maka-${version}-win-x64.exe" \
apps/desktop/release-autoupdate-next
+
+ - name: Prove deterministic mid-install failure rollback
Review Comment:
The `transactional` and `proven` claims have been removed from the title, installer/workflow/builder comments, verifier diagnostics, and EN/zh rollback documentation. Exact-head Windows evidence is intentionally still marked pending in the PR body. Fresh `Release Windows check` run 32477989412 is running now; I am leaving this verification thread open until its rollback step completes and the exact log evidence is posted.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
+FunctionEnd
+
+Function .onInstFailed
+ Call makaRestoreFromBackup
+FunctionEnd
+
+!macro customInit
+ StrCpy $makaBackupDir "$INSTDIR.pre-upgrade-backup"
+ StrCpy $makaInstallRegKey "${INSTALL_REGISTRY_KEY}"
+ StrCpy $makaUninstallRegKey "${UNINSTALL_REGISTRY_KEY}"
+ StrCpy $makaFailpointArmed "0"
+ ReadEnvStr $R9 MAKA_INSTALLER_TEST_FAILPOINT
+ # S== is case-sensitive: the accepted value is a single exact token.
+ ${If} $R9 S== "after-extract"
+ StrCpy $makaFailpointArmed "1"
+ ${EndIf}
+ ${If} ${FileExists} "$INSTDIR\${APP_EXECUTABLE_FILENAME}"
Review Comment:
Addressed at exact head `68520adcc8dd4ef42447ab065279be9b075aece6`. A leftover backup is adoptable only when it contains `Maka.exe` and its marker exactly matches the live or persisted snapshot `DisplayVersion`; missing, stale, unreadable, or marker-only backups fail closed with 101 before the destructive boundary. The verifier now constructs both an old-version complete backup and a current-version marker with `Maka.exe` removed, and asserts each leaves the installed version and registration unchanged. Registry-mismatch 103 also retains the live restored tree, aside, and snapshot; sibling recopy/adoption is explicitly best effort rather than unconditional.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
Review Comment:
Addressed at exact head `68520adcc8dd4ef42447ab065279be9b075aece6`. `customInit` now requires non-empty `DisplayName`, `UninstallString`, and `DisplayVersion` from the live registration or an exact-version adoptable snapshot, otherwise it exits 101 before destructive work. After restore, 102 is gated by read-back equality for `DisplayName`, `UninstallString`, `NoModify=1`, and `NoRepair=1`. A deterministic registry-mismatch failpoint removes `UninstallString`; the verifier requires 103, retained recovery evidence, and a clean recovery rerun. Cleanup and 102 occur only after that proof.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3265:
URL: https://github.com/apache/maka/pull/3265#discussion_r3819778502
##########
.github/workflows/release-windows-check.yml:
##########
@@ -105,3 +108,10 @@ jobs:
npm run verify:windows-autoupdate -- \
"apps/desktop/release/Maka-${version}-win-x64.exe" \
apps/desktop/release-autoupdate-next
+
+ - name: Prove deterministic mid-install failure rollback
Review 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/windo
ws-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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
+FunctionEnd
+
+Function .onInstFailed
Review 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 `$INSTD
IR.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 `Quit`s rather than `Abort`s 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/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
+FunctionEnd
+
+Function .onInstFailed
+ Call makaRestoreFromBackup
+FunctionEnd
+
+!macro customInit
+ StrCpy $makaBackupDir "$INSTDIR.pre-upgrade-backup"
+ StrCpy $makaInstallRegKey "${INSTALL_REGISTRY_KEY}"
+ StrCpy $makaUninstallRegKey "${UNINSTALL_REGISTRY_KEY}"
+ StrCpy $makaFailpointArmed "0"
+ ReadEnvStr $R9 MAKA_INSTALLER_TEST_FAILPOINT
+ # S== is case-sensitive: the accepted value is a single exact token.
+ ${If} $R9 S== "after-extract"
+ StrCpy $makaFailpointArmed "1"
+ ${EndIf}
+ ${If} ${FileExists} "$INSTDIR\${APP_EXECUTABLE_FILENAME}"
Review 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 /r`s 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, n
ot 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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
Review 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 `Renam
e`, and decide success by testing for `$INSTDIR\${APP_EXECUTABLE_FILENAME}`.
##########
scripts/verify-packaged-app.mjs:
##########
@@ -386,6 +436,47 @@ export async function assertPackagedResources(
}
}
+/**
+ * Recursive content manifest of a directory tree: POSIX-normalized relative
+ * paths, sorted, each with its file's SHA-256. Nothing is skipped — an install
+ * tree has no entries whose drift would be acceptable — and anything that is
+ * not a plain file or directory (symlinks, junctions, devices) throws: an
+ * install tree must not contain them, and silently hashing a link target would
+ * make two different trees compare equal.
+ */
+export async function directoryTreeManifest(rootDirectory) {
Review 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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
Review 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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ ClearErrors
+ CreateDirectory "$INSTDIR"
+ CopyFiles /SILENT "$makaBackupDir\*.*" "$INSTDIR"
+ ${If} ${Errors}
+ DetailPrint "Restore copy failed; keeping $makaBackupDir for manual recovery"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_FAILED}
+ Return
+ ${EndIf}
+ # Best-effort backup cleanup: a scanner holding the now-redundant backup
+ # must not demote a restore that has already completed.
+ ClearErrors
+ RMDir /r "$makaBackupDir"
+ ClearErrors
+ ${EndIf}
+ # Re-create the registry state the old uninstaller removed. Only values that
+ # existed before the upgrade are written back; NoModify/NoRepair are
+ # template constants in every shipped version. EstimatedSize is cosmetic and
+ # intentionally not restored.
+ ${If} $makaPrevInstallLocation != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" InstallLocation "$makaPrevInstallLocation"
+ ${EndIf}
+ ${If} $makaPrevKeepShortcuts != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts "$makaPrevKeepShortcuts"
+ ${EndIf}
+ ${If} $makaPrevShortcutName != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" ShortcutName "$makaPrevShortcutName"
+ ${EndIf}
+ ${If} $makaPrevMenuDirectory != ""
+ WriteRegStr SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory "$makaPrevMenuDirectory"
+ ${EndIf}
+ ${If} $makaPrevDisplayName != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayName "$makaPrevDisplayName"
+ ${EndIf}
+ ${If} $makaPrevUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" UninstallString "$makaPrevUninstallString"
+ ${EndIf}
+ ${If} $makaPrevQuietUninstallString != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString "$makaPrevQuietUninstallString"
+ ${EndIf}
+ ${If} $makaPrevDisplayVersion != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion "$makaPrevDisplayVersion"
+ ${EndIf}
+ ${If} $makaPrevDisplayIcon != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon "$makaPrevDisplayIcon"
+ ${EndIf}
+ ${If} $makaPrevPublisher != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Publisher "$makaPrevPublisher"
+ ${EndIf}
+ ${If} $makaPrevComments != ""
+ WriteRegStr SHELL_CONTEXT "$makaUninstallRegKey" Comments "$makaPrevComments"
+ ${EndIf}
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoModify 1
+ WriteRegDWORD SHELL_CONTEXT "$makaUninstallRegKey" NoRepair 1
+ DetailPrint "Previous installation restored"
+ SetErrorLevel ${MAKA_EXIT_ROLLBACK_OK}
+FunctionEnd
+
+Function .onInstFailed
+ Call makaRestoreFromBackup
+FunctionEnd
+
+!macro customInit
+ StrCpy $makaBackupDir "$INSTDIR.pre-upgrade-backup"
+ StrCpy $makaInstallRegKey "${INSTALL_REGISTRY_KEY}"
+ StrCpy $makaUninstallRegKey "${UNINSTALL_REGISTRY_KEY}"
+ StrCpy $makaFailpointArmed "0"
+ ReadEnvStr $R9 MAKA_INSTALLER_TEST_FAILPOINT
+ # S== is case-sensitive: the accepted value is a single exact token.
+ ${If} $R9 S== "after-extract"
+ StrCpy $makaFailpointArmed "1"
+ ${EndIf}
+ ${If} ${FileExists} "$INSTDIR\${APP_EXECUTABLE_FILENAME}"
+ # Snapshot the registry values the template's upgrade path deletes, while
+ # they still exist (the old uninstaller runs inside the install section,
+ # after this hook). The set mirrors registryAddInstallInfo, including the
+ # conditional MenuDirectory (reading a missing value yields "" and the
+ # write-back skips empty values).
+ ReadRegStr $makaPrevInstallLocation SHELL_CONTEXT "$makaInstallRegKey" InstallLocation
+ ReadRegStr $makaPrevKeepShortcuts SHELL_CONTEXT "$makaInstallRegKey" KeepShortcuts
+ ReadRegStr $makaPrevShortcutName SHELL_CONTEXT "$makaInstallRegKey" ShortcutName
+ ReadRegStr $makaPrevMenuDirectory SHELL_CONTEXT "$makaInstallRegKey" MenuDirectory
+ ReadRegStr $makaPrevDisplayName SHELL_CONTEXT "$makaUninstallRegKey" DisplayName
+ ReadRegStr $makaPrevUninstallString SHELL_CONTEXT "$makaUninstallRegKey" UninstallString
+ ReadRegStr $makaPrevQuietUninstallString SHELL_CONTEXT "$makaUninstallRegKey" QuietUninstallString
+ ReadRegStr $makaPrevDisplayVersion SHELL_CONTEXT "$makaUninstallRegKey" DisplayVersion
+ ReadRegStr $makaPrevDisplayIcon SHELL_CONTEXT "$makaUninstallRegKey" DisplayIcon
+ ReadRegStr $makaPrevPublisher SHELL_CONTEXT "$makaUninstallRegKey" Publisher
+ ReadRegStr $makaPrevComments SHELL_CONTEXT "$makaUninstallRegKey" Comments
+ ClearErrors
+ # An interrupted earlier run may have left a stale backup; replace it so
+ # the backup always mirrors the install being upgraded right now.
+ RMDir /r "$makaBackupDir"
+ CreateDirectory "$makaBackupDir"
+ CopyFiles /SILENT "$INSTDIR\*.*" "$makaBackupDir"
Review 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 `CopyFi
les` 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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
Review 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.
##########
apps/desktop/build/installer.nsh:
##########
@@ -0,0 +1,236 @@
+# Transactional upgrade backup/rollback for the one-click NSIS installer.
+#
+# Why this exists: electron-builder's default upgrade sequence runs the OLD
+# uninstaller (which removes the whole $INSTDIR and the uninstall registry
+# keys) before extracting the new files. A failure after that point strands the
+# machine with no working install and no rollback source. This include makes
+# the upgrade transactional on the Abort path:
+#
+# customInit (before anything destructive) back up $INSTDIR to a
+# same-volume sibling and snapshot the registry values the
+# template will delete; a backup failure fails the upgrade
+# CLOSED (exit 101) with the old install untouched.
+# customFiles_* (after extraction, worst case: old files gone, new
+# uninstaller/registry not yet written) test-only
+# deterministic failpoint: MAKA_INSTALLER_TEST_FAILPOINT
+# set to exactly "after-extract" aborts here.
+# .onInstFailed (any script Abort) remove the partial $INSTDIR, restore
+# the backup and the registry snapshot; exit 102 when the
+# restore completed, 103 when it did not (the backup
+# directory is then kept for manual recovery).
+# 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
+# power loss — and template failure branches that exit via Quit instead of
+# Abort. The backup copy is not forced through volatile device caches, in line
+# with the repository's Windows durability boundary.
+#
+# Security posture of the failpoint (same narrative as MAKA_UPDATE_TEST_FEED):
+# setting an environment variable on the installer's process already requires
+# code execution as the same user, which under the per-user install model can
+# rewrite the installation directory directly. The failpoint can only make an
+# upgrade fail and roll back — self-denial-of-service — and it is read at run
+# time (ReadEnvStr), never baked in at compile time. With the variable unset
+# the only added behavior is one ReadEnvStr, the pre-upgrade backup, and the
+# post-success backup removal.
+#
+# Compile-order note: this file is prepended to the generated script, before
+# common.nsh/multiUser.nsh. Product defines arrive as makensis -D flags and are
+# safe everywhere; template defines (INSTALL_REGISTRY_KEY, APP_EXECUTABLE_
+# FILENAME, ...) are only safe inside !macro bodies, which expand at their
+# insertion points. Functions therefore read exclusively from the $maka*
+# variables assigned in customInit.
+
+!ifndef BUILD_UNINSTALLER
+
+!include LogicLib.nsh
+
+!define MAKA_EXIT_BACKUP_FAILED 101
+!define MAKA_EXIT_ROLLBACK_OK 102
+!define MAKA_EXIT_ROLLBACK_FAILED 103
+
+Var makaBackupDir
+Var makaFailpointArmed
+Var makaInstallRegKey
+Var makaUninstallRegKey
+Var makaPrevInstallLocation
+Var makaPrevKeepShortcuts
+Var makaPrevShortcutName
+Var makaPrevMenuDirectory
+Var makaPrevDisplayName
+Var makaPrevUninstallString
+Var makaPrevQuietUninstallString
+Var makaPrevDisplayVersion
+Var makaPrevDisplayIcon
+Var makaPrevPublisher
+Var makaPrevComments
+
+# Restores the pre-upgrade state captured by customInit. Reads only variables
+# (see the compile-order note above). Idempotent: without a backup directory it
+# returns immediately, so a fresh-install Abort behaves exactly as upstream
+# does today.
+Function makaRestoreFromBackup
+ ${IfNot} ${FileExists} "$makaBackupDir\*.*"
+ Return
+ ${EndIf}
+ DetailPrint "Restoring the previous installation from $makaBackupDir"
+ # Move the working directory out of $INSTDIR so the removal cannot fail on
+ # our own open handle (same precaution as the template's uninstaller).
+ SetOutPath $PLUGINSDIR
+ ClearErrors
+ RMDir /r "$INSTDIR"
+ Rename "$makaBackupDir" "$INSTDIR"
+ StrCpy $R8 "0"
+ ${If} ${Errors}
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${IfNot} ${FileExists} "$INSTDIR\*.*"
+ StrCpy $R8 "1"
+ ${EndIf}
+ ${If} $R8 == "1"
+ # The rename path failed (e.g. a scanner held a handle). Fall back to
+ # copying ONLY into a verified-empty target: if the partial removal left
+ # 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\*.*"
+ DetailPrint "New-version residue blocks a clean restore; keeping $makaBackupDir for manual recovery"
Review 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.
##########
scripts/verify-packaged-app.mjs:
##########
@@ -74,21 +73,52 @@ export async function assertMissing(path) {
throw new Error(`Forbidden release resource exists: ${path}`);
}
-export async function reserveTcpPort() {
- const server = createServer();
- await new Promise((resolvePromise, reject) => {
- server.once('error', reject);
- server.listen(0, '127.0.0.1', resolvePromise);
- });
- const address = server.address();
- if (!address || typeof address === 'string') {
- server.close();
- throw new Error('Could not reserve a CDP port.');
- }
- await new Promise((resolvePromise, reject) => {
- server.close((error) => (error ? reject(error) : resolvePromise()));
+/**
+ * The authoritative CDP port: Chromium announces it on stderr once the
+ * DevTools socket is actually bound. Callers spawn with
+ * `--remote-debugging-port=0` and wait for this instead of pre-reserving a
+ * 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).
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5358474003
The promised L3 evidence: `release-windows-check` is green at head `77f81e292` — run [32387103810](https://github.com/apache/maka/actions/runs/32387103810) (`package` lane, 22m54s), first full execution of every scenario:
```
[verify-windows-installer-rollback] installing candidate 0.1.11
[verify-windows-installer-rollback] asserting the candidate registered its uninstall entry
[verify-windows-installer-rollback] manifest covers 607 files
[verify-windows-installer-rollback] upgrading to 0.1.12 with the after-extract failpoint armed
[verify-windows-installer-rollback] asserting the previous installation is byte-identical
[verify-windows-installer-rollback] 0 differing files across 607 entries
[verify-windows-installer-rollback] asserting the uninstall registration was restored
[verify-windows-installer-rollback] asserting the restored installation launches
[verify-windows-installer-rollback] control run: the same installer must succeed without the failpoint
[verify-windows-installer-rollback] gap pin: a Quit at the worst moment leaves no hook and keeps the backup
[verify-windows-installer-rollback] recovery: rerunning the installer adopts the backup and completes the upgrade
[verify-windows-installer-rollback] fail closed: an upgrade with no registration and no backup is refused
[verify-windows-installer-rollback] uninstalling
[verify-windows-installer-rollback] verified Abort-path rollback and its boundaries for 0.1.11 -> 0.1.12
```
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 `quitAndInstall` and blocking the NSIS upgrade entirely, filed as #3340 with per-process command-line evidence. It is a probabilistic hazard for every Windows lane run (and for real users' updates) until fixed; I've started on it separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5354229217
@liugddx — agreed, taking your split. #3241 is now narrowed to `scripts/verify-windows-installer-lifecycle.mjs` only (`c611cfd`); the port discovery, the per-attempt poll bound and the renderer-readiness deadline all left that branch, so the file sets are disjoint and merge order stops mattering. Title and description rewritten to match.
No need to apologise for the collision — the timestamps say it was independent work: your `d1aee1a` landed about three minutes after I posted here, which is not enough time to read, implement and commit. We even picked the same 2000ms. Keep your shape; it is the one with CI evidence behind it on your lane.
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.** `verify-windows-autoupdate.mjs`'s `waiting for the upgraded app to relaunch automatically` calls `listInstalledProcesses` in a poll loop; that query stalls exactly there, while NSIS is handing off. Measured at the same step across three revisions of my branch:
| | result |
| --- | --- |
| unbounded probe | hung 59 minutes past the 120s deadline; runner cancelled the job |
| bounded probe, loop aborts on probe failure | failed in 74 seconds |
| bounded probe, loop treats a failed probe as an unknown round | **relaunch detected in 27 seconds** |
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](https://github.com/apache/maka/pull/3241/commits) if useful; adapt or ignore.
**The next stall after that one is `taskkill`.** With the loop fixed, my last lane run got past the relaunch and died at `stopping the relaunched instance`: `taskkill /PID 6612 /T /F did not finish within 30000ms`, with a second `taskkill … exit code 255` during cleanup. It is bounded, so it fails cleanly rather than hanging — but 30 seconds to kill a just-relaunched process tree suggests the same process-table churn, and it will redden this lane for whoever gets there first. I have not touched it.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(release): transactional installer backup with proven mid-install rollback [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3265:
URL: https://github.com/apache/maka/pull/3265#discussion_r3820232602
##########
scripts/verify-windows-installer-lifecycle.mjs:
##########
@@ -85,6 +85,55 @@ export async function waitUntilMissing(
}
}
+/**
+ * Every HKCU `DisplayVersion` registered under the Maka display name, joined
+ * with commas ('' when none). One PowerShell scan, shared by the harnesses
+ * that assert on — or wait out — the uninstall registration.
+ */
+export async function readUninstallDisplayVersions({ run = runCommand } = {}) {
+ const script = String.raw`
+$entries = Get-ChildItem 'HKCU:\Software\Microsoft\Windows\CurrentVersion\Uninstall' |
+ ForEach-Object { Get-ItemProperty $_.PSPath } |
+ Where-Object { $_.DisplayName -eq 'Maka' }
+@($entries | ForEach-Object { $_.DisplayVersion }) -join ','
+`;
+ const { stdout } = await run('powershell', ['-NoProfile', '-NonInteractive', '-Command', script]);
Review 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 le
t 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(win): Abort-path installer rollback with backup retention [maka]
Posted by "liugddx (via GitHub)" <gi...@apache.org>.
liugddx commented on PR #3265:
URL: https://github.com/apache/maka/pull/3265#issuecomment-5356755512
@Astro-Han Thank you — this round changed the PR's shape, and both P1 "two words" objections were correct. Head `7c3a8f23c` (stacked on #3327, which is the split you asked for). Disposition of every finding, by ID:
**3819778489 (P1, `.onInstFailed` unreachable on real failure paths)** — Accepted in full. Retitled to *Abort-path rollback with backup retention*; docs (EN+zh) now open the boundary paragraph with exactly your framing: the template's failure branches exit via `Quit`, NSIS gives no hook, **no automatic rollback runs there**. Your regression suggestion is implemented as a second failpoint value (`after-extract-quit`) and the gate pins the whole shape: exit 0, extracted files stay, registration stays gone, backup retained. I went one step further than retention: the registry snapshot is persisted in a sibling key (`Software\Maka\PreUpgradeSnapshot`, untouched by the uninstaller's `DeleteRegKey`s), so **rerunning the installer adopts the backup and completes the upgrade** — scenario 4 asserts that end to end. The next-launch sentinel recovery stays out of scope, now honestly labeled.
**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, `exit code 102`, `0 differing files`, control run) here once `release-windows-check` is green; #3327 exists so this PR's lane can get there on its own.
**3820922148 (P1, unconditional 102 / empty snapshot)** — Accepted: `customInit` now fails closed (101) on a confirmed upgrade with no registration and no adoptable backup, before anything destructive; 102 is set only after the filesystem witness and the write-back. Your regression test is scenario 5 (delete the registration, upgrade, assert 101 with files untouched). One correction to the evidence trail: the "DisplayVersion empty" your subagent read from run 32346230451 came through my own scan bug — the filter matched `DisplayName -eq 'Maka'` but electron-builder's default is `"${productName} ${version}"` (NsisTarget.js:473), so the scan was blind and the registry may in fact have been restored. Fixed in #3327; the fail-closed change is right regardless.
**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 `FindFirst` enumeration skipping `.`/`..`, an empty shell is cleared with a non-recursive `RMDir` and, if it persists, the backup is **copied into it** — the exact held-handle case now ends in 102, not 103.
**3819778516 (P2, shared error flag)** — Accepted: `ClearErrors` before each operation, and success is judged by `$INSTDIR\Maka.exe` existing, not by the flag.
**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, `$INSTDIR` broken" resolves through adoption + the section's normal flow; fresh-install aborts cannot resurrect a stale backup (the restore is armed per-run, only after this run verified or adopted a backup).
**3820922170 (P2, unverified backup at the commit point)** — Accepted in the minimal-witness form you suggested: `Maka.exe` must exist in the backup before the destructive boundary, else 101 with the old install intact. `${GetSize}` tree comparison noted as a stronger follow-up.
**3819778524 / 3819778529 (P2, backup timing and 101/103 visibility)** — Visibility: `RECOVERY-README.txt` is written into the backup **at creation time** (the hookless Quit paths run none of our code after the failure, so failure-time writes cannot help them), refreshed on 103, plus an interactive MessageBox; docs carry the manual steps in both languages. Backup timing before `CHECK_APP_RUNNING` is kept, leaning on the evidence you cited (the e2e run proving `CopyFiles` of a running install is viable); a free-space precheck is not in this round — disk-full now lands on the fail-closed 101 with the old install intact rather than a mid-upgrade failure. Happy to add the precheck as a follow-up if you want it in-tree.
**3819778535 (P2, tests described but not committed)** — Committed in #3327 as `scripts/verify-windows-harness.test.mjs`, wired into the CI planner step so they run on every PR; your empty-directory manifest gap is fixed (recorded with a trailing slash, loss shows as `missing`) and tested.
**3820232602 / 3820922173 / 3821101776 / 3821101780 (P2, unbounded probes)** — All bounded in #3327 (per-site `timeoutMs`, keeping your earlier acceptance of no `runCommand` default for codesign/notarize); the waits tolerate a failed probe and retry to their own deadline, and a failed enumeration is never treated as "no processes"; the WebSocket handshake has its own bound so an accepting-but-mute port fails one probe, not the lane. Contract tests cover the tolerate-and-deadline behavior.
**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 `$INSTDIR` during the failpoint), I'd rather land it as a follow-up than grow this round further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org