Skip to content

fix: make macOS desktop self-update swap+relaunch fail-fast and recoverable - #43265

Open
OmarB97 wants to merge 1 commit into
NousResearch:mainfrom
OmarB97:fix/desktop-mac-swap-failfast
Open

fix: make macOS desktop self-update swap+relaunch fail-fast and recoverable#43265
OmarB97 wants to merge 1 commit into
NousResearch:mainfrom
OmarB97:fix/desktop-mac-swap-failfast

Conversation

@OmarB97

@OmarB97 OmarB97 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Why

Revives #38410, which was closed after an old force-push left its branch without the changes the description promised. The original commit was recovered from the fork's pre-force-push branch head and rebased onto current main.

The macOS in-app updater can quit Hermes.app and never reinstall/reopen — matching the reported "Update now does not actually update" behavior. The detached swap script gated only ditto on success and fell through silently when the bundle swap failed, leaving the user on the old build (or with no app at all) after a dead quit.

What changed

  • apps/desktop/electron/main.cjs: the applyUpdatesPosixInApp swap script now runs set -euo pipefail, detects ditto and destination-replace failures immediately, cleans up the .hermes-update-old staging copy via an EXIT trap, and on any swap failure falls back to opening the freshly rebuilt bundle directly instead of leaving the user with a dead quit.

Unlike #38410, this does not touch hermes_state.py: the PRAGMA busy_timeout = 5000 half of that PR is superseded by the BEGIN IMMEDIATE + jitter-retry redesign (#3385), which deliberately keeps the SQLite busy handler short and handles contention with application-level retries.

How to review

Review the swapScript template in applyUpdatesPosixInApp (apps/desktop/electron/main.cjs, ~line 1815). The invariant: on ANY swap failure the user ends up with a working app launched (the rebuilt bundle via the open "$SRC" fallback) and no orphaned .hermes-update-* staging copies.

Evidence

Functional simulation of the rendered swap script with /usr/bin/open and /usr/bin/xattr stubbed:

  • Happy path: destination replaced with the new bundle, relaunch targets the destination, no staging leftovers, rc=0.
  • ditto-failure path (source missing): destination untouched (old bundle intact), [updates] ditto failed logged to stderr, fallback open targets the rebuilt source bundle, no leftovers, rc=0.

Verification

  • node --check apps/desktop/electron/main.cjs
  • bash -n on the rendered swap script
  • The two functional simulations above
  • Trigger the macOS in-app update path; confirm a forced swap failure (e.g. make the staging ditto fail) opens the rebuilt release/mac-arm64/Hermes.app instead of dead-quitting, and a normal update still replaces /Applications/Hermes.app.

Risks & gaps

  • If moving the old bundle aside fails AND rm -rf of it also fails, set -e aborts with the original app intact but no relaunch (the user reopens manually) — strictly better than the prior silent fall-through.
  • The open "$SRC" fallback launches the rebuilt bundle from the build tree rather than /Applications; this avoids a no-app state but may not match the user's preferred installed location (same trade-off as reviewed in fix: make macOS desktop self-update swap+relaunch fail-fast and recoverable #38410).

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Jun 10, 2026

OmarB97 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Maintainer-ready after refresh.

I merged current upstream/main into this macOS desktop self-update swap failfast PR and pushed head 4648a5ca9e0082f4605a219d6c03870c05898bd7.

Verification:

  • npm --prefix apps/desktop run test:desktop:platforms (144 passed, 1 skipped)
  • npm --prefix apps/desktop run typecheck
  • npm exec eslint -- electron/main.cjs
  • git diff --check upstream/main...HEAD

MeshBoard merge dry-run passes with green checks and a fresh base. I attempted the actual merge through meshctl pr merge, but GitHub denied MergePullRequest for my account, so this is ready for a maintainer to merge.

…erable

The macOS in-app updater's detached swap script ran with plain 'set -u'
and a success-gated ditto: when ditto or the destination move failed,
the script fell through silently -- the app had already quit, the old
bundle stayed (or was left moved aside), and 'open "$DST"' either
relaunched the stale build or nothing at all. This matches the reported
"Update now does not actually update" behavior.

Make the swap fail-fast and recoverable:
- set -euo pipefail so unexpected failures stop the script instead of
  compounding.
- Detect ditto and destination-replace failures explicitly; on either,
  fall back to opening the freshly rebuilt bundle directly so the user
  is never left with a dead quit.
- Clean up the .hermes-update-old copy via an EXIT trap so it is
  removed on every exit path.

Recovered from the pre-force-push head of desktop-mac-swap-fix
(NousResearch#38410, which lost this work to a force-push
and was closed). The sqlite busy_timeout half of that PR is
intentionally not revived: the BEGIN IMMEDIATE + jitter-retry redesign
(NousResearch#3385) deliberately keeps the connection busy handler short, and a 5s
busy_timeout would override that design.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@OmarB97
OmarB97 force-pushed the fix/desktop-mac-swap-failfast branch from 4648a5c to a608117 Compare July 11, 2026 01:34
@OmarB97

OmarB97 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed onto upstream/main (rebase). This branch's prior head (4648a5ca9e) was itself a June 11 merge-refresh that's since gone stale — live merge-base against that head is 484f484c25, 3734 commits behind current upstream/main. One conflict: apps/desktop/electron/main.cjsmain.ts — upstream fully ts-ified electron/ since this PR last refreshed (zero .cjs files remain there now), so the swap+relaunch fail-fast hunk (17 insertions / 7 deletions: set -euo pipefail, the ditto/mv failure fallbacks that open the freshly-rebuilt bundle instead of leaving the user with a dead quit, and the hermes-update-old cleanup trap) was carried onto the renamed file verbatim, no logic changes. The prior 2-commit history (original fix + June's merge commit) collapses to a single commit riding directly on upstream/main's current tip. Original head 4648a5ca9e -> refreshed a608117134. Same 1-file diff as the original PR. npm run typecheck (both tsconfig projects, apps/desktop) clean. Checks re-running.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviving the macOS recovery path. The underlying defect remains on current main: apps/desktop/electron/main.ts:3028 does not check the new-bundle move before deleting the old bundle and opening the destination.

Problems

  • apps/desktop/electron/main.ts:3016 adds set -euo pipefail, but the retained mv "$DST" "$DST.hermes-update-old" || rm -rf "$DST" path has no fallback if both commands fail. The script exits before open "$SRC", after the app has already quit.
  • apps/desktop/electron/main.ts:3024 cleans only .hermes-update-old. A failed ditto or failed destination replacement can leave .hermes-update-new, contrary to the stated staging-cleanup invariant.
  • No repository test covers this macOS script; apps/desktop/AGENTS.md:179 calls for exercising the actual failure rungs.

Suggested changes

  • Add an explicit recovery branch for failure to displace the destination, and clean both staging paths on every exit.
  • Commit the rendered-script simulations as an automated test covering success plus each failure branch.

Automated hermes-sweeper review.

// then atomically replace the running .app, clear quarantine, and reopen it.
// If the swap fails, fall back to launching the freshly rebuilt bundle
// directly so we do not leave the user with a dead quit.
const swapScript = `#!/bin/bash

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set -e makes the existing mv "$DST" "$DST.hermes-update-old" || rm -rf "$DST" sequence fatal if both commands fail, before either explicit open "$SRC" fallback. Handle that displacement failure explicitly so the recovery invariant holds.

for _ in $(seq 1 240); do
kill -0 "$APP_PID" 2>/dev/null || break
sleep 0.5
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trap removes only .hermes-update-old; a failed ditto or destination replacement can leave .hermes-update-new. Clean both staging paths so the stated no-orphaned-staging invariant is true.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/install-update Installer, updater, packaging, wheels, doctor labels Jul 14, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Summary

Three PRs address or reference the macOS desktop self-update failure. #35607 introduced the POSIX rebuild/swap/relaunch path, #38410’s surviving diff only ad-hoc re-signs the copied installer and does not implement its described recovery changes, and #43265 hardens the swap script but still leaves documented failure paths uncovered.

Related pull requests

  • #35607 [merged] related — (+340/-14) — merged reference implementation: introduced the macOS/Linux in-app update flow, including rebuilding the desktop bundle and swapping/relaunching it, along with installer-path and setup fixes. It remains relevant because its detached swap script is the implementation in which the reported dead-quit failure originates.
  • #38410 [closed] related — (+3/-0) — closed, not the promised recovery fix: the surviving diff only invokes ad-hoc codesigning after copying the bootstrap installer; it contains none of the described swap fail-fast or SQLite changes. It remains relevant as provenance for the recovery attempt, but was closed because its sole commit was superseded by #38446 and the intended updater work was lost.
  • #43265 related — (+17/-7) — keep open: revives the intended swap recovery by adding fail-fast shell behavior and opening the rebuilt bundle when ditto or final replacement fails. This agrees with the contributor keep_open review on #43265: the diff still exits without recovery if both destination displacement commands fail, cleans .hermes-update-old but not .hermes-update-new, and adds no automated coverage for the required failure rungs.

Suggested consolidation

Keep #43265 open rather than merging it yet; address the contributor review by adding explicit recovery when the existing destination cannot be displaced, cleaning both staging paths on every exit, and committing rendered-script tests for success and each failure branch. Once those blockers are resolved and verified, merge #43265 as the focused fix; #35607 remains the merged reference implementation, and closed #38410 is not a diff-level duplicate requiring further action.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 21 kB of PR diffs, 7 kB of issue/PR text, 7 kB of discussion (8 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants