Repository navigation
fix(v3/updater): keep the staged payload owned by the swap helper during restart - #6350
Conversation
Restart spawns a detached helper that waits for this process to exit before renaming the staged payload over the installed application, then returns without retaining any claim on the staging directory. A DownloadAndInstall arriving during that wait begins with discardStaging, which removes the very directory the helper is about to install from. os.RemoveAll is depth-first, and a staged .app bundle is a directory, so the helper is left holding a path whose contents have been unlinked — and os.Rename succeeds on a directory whether or not it still holds anything. The swap reports success and discards the backup, leaving an empty application with nothing to roll back to. Restart also accepts a second call while the first helper is pending. The built-in window starts a Restart per click, so a double click starts a second helper against the same target. Restart now reads the staged path, takes the staging directory and marks a restart as pending in one critical section, and returns ErrRestartPending if one already is. discardStaging makes the same check under the same lock, so DownloadAndInstall returns ErrRestartPending rather than deleting a payload a helper owns; either Restart finds nothing staged or the download finds the helper's claim. A helper that fails to start, or never signals readiness, hands ownership back, so a failed spawn does not strand the payload and block every later attempt.
|
Caution Review failedThe pull request is closed. No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Claim and release restart ownership v3/pkg/updater/updater.go, v3/pkg/updater/updater_test.go, v3/UNRELEASED_CHANGELOG.md |
Restart claims the staged payload and rejects another restart during the handoff. If helper startup or readiness fails, the updater releases the claim. Tests cover failed handoffs and repeated restarts. |
Protect staging during handoff v3/pkg/updater/updater.go, v3/pkg/updater/updater_test.go |
DownloadAndInstall returns staging-discard errors. While a restart is pending, the updater preserves the staged payload and returns ErrRestartPending. Tests cover payload preservation and interleaved downloads. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
- [v3] updater: a damaged or emptied staged payload installs over a working app, and the backup is discarded #6351 — The change prevents deletion of the staged payload during a pending handoff, but does not add payload validation or rollback.
Suggested labels: Bug, Documentation, v3
Suggested reviewers: leaanthony
Merge Risk: 
6e50e
-
The change stops a download or a second restart from deleting or contending for the staged update while the swap helper is waiting to install it. If the helper fails to start or never signals readiness, a later download or restart is still allowed. No merge-blocking issues were identified. The only gap is that Windows and Linux runtime behavior was not exercised.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: preserving staged payload ownership during a pending restart. Description check The description includes the bug context, linked issue, implementation details, test coverage, test commands, environment, and completed checklist items. It clearly documents that Windows and Linux we… Linked Issues check Issue [ #6349] requires ownership protection for staged payloads and refusal of duplicate restarts.Restartnow claimsstagingDirand setsrestartingin one lock section.discardStagingreturns…Out of Scope Changes check The changes stay within issue [ #6349]. The updater changes implement staging ownership and duplicate-restart protection. The tests verify those behaviors. The unreleased changelog entry documents the …Full details: Description check
-
Explanation
The description includes the bug context, linked issue, implementation details, test coverage, test commands, environment, and completed checklist items. It clearly documents that Windows and Linux were not fully tested.
Full details: Linked Issues check
-
Explanation
Issue [
#6349] requires ownership protection for staged payloads and refusal of duplicate restarts.Restartnow claimsstagingDirand setsrestartingin one lock section.discardStagingreturnsErrRestartPendingwithout deletion. A secondRestartreturnsErrRestartPending. Failed helper startup and failed readiness reclaim staging. The added tests cover concurrent download, failed handoff, helper-path preservation, and duplicate restart behavior.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue [
#6349]. The updater changes implement staging ownership and duplicate-restart protection. The tests verify those behaviors. The unreleased changelog entry documents the fix. No unrelated change is shown.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit guards the payload tight,
While helpers wait through shutdown night.
No second restart joins the queue,
Failed starts release the claim anew.
Safe files wait for the swap to bloom.
Comment @coderabbitai help to get the list of available commands.
github-actions
Bot
added
Bug
Description
Updater.Restarthands the staged payload to a detached swap helper, which waits for the app to exit before renaming the payload over the installed application. Until now the updater kept no claim on the staging directory once the helper had started, which caused two problems:DownloadAndInstallduring shutdown began withdiscardStagingand deleted the directory the helper was about to install from. A staged.appis a directory andos.Renamesucceeds on an empty one, so the helper reported success, discarded the backup and left an empty application.Restart(for example a double click in the built-in window, which runsgo u.Restart(ctx)on every click) started a second helper against the same target.This PR makes
Restartread the staged path, take the staging directory and mark a restart as pending in one critical section. If a restart is already pending it returns the newErrRestartPending.discardStagingchecks the same flag under the same lock, soDownloadAndInstallreturnsErrRestartPendingand deletes nothing. If the helper fails to start, or never signals that it is ready, ownership is handed back so later attempts still work.DownloadAndInstallreturnsErrRestartPendingwithout anEventError, the same way it already handlesErrNoPendingReleaseandErrDownloadInProgress: no flow has started at that point. If an app'sShouldQuitvetoes the quit, the restart stays marked as pending until the process exits, because the helper is still waiting on the PID. This is documented on the field.Fixes #6349
Type of change
How Has This Been Tested?
New unit tests in
v3/pkg/updater/updater_test.go. The last two make the interleavings deterministic through the package's existing test seams, with no sleeps. Both were run and seen to fail before the single-critical-section locking inRestart/discardStaging("helper handed a staged path that does not exist" and "second Restart: … helper did not signal readiness"), and both pass with it. All four pass now:TestRestart_StagedPayloadSurvivesConcurrentDownload: aDownloadAndInstallafterRestartreturnsErrRestartPendingand the staged payload is still there.TestRestart_FailedHandoff_ReturnsStagingOwnership: if the spawn fails or the helper never becomes ready, ownership is handed back.TestRestart_HelperNeverHandedDiscardedPayload: runs aDownloadAndInstallinsideRestartthrough theselfExecutabletest seam, then checks that the path handed to the helper still exists.TestRestart_SecondCallRefusedWhileRestartPending: callsRestartagain from inside the first call's readiness wait. The second call getsErrRestartPending, only one helper is started, and the first call's claim is still in place.Commands:
go vetonly; not run on Windows)Test Configuration
Built from source at master b6167d9 + this commit; macOS arm64, Go 1.26.3.
Checklist:
website/src/pages/changelog.mdxwith details of this PR (v3 changelog entries are added automatically)Summary by CodeRabbit