Skip to content

fix(desktop): preserve the packaged app when an Electron rebuild fails#44234

Open
AIalliAI wants to merge 1 commit into
NousResearch:mainfrom
AIalliAI:fix/44225-preserve-desktop-app-on-failed-pack
Open

fix(desktop): preserve the packaged app when an Electron rebuild fails#44234
AIalliAI wants to merge 1 commit into
NousResearch:mainfrom
AIalliAI:fix/44225-preserve-desktop-app-on-failed-pack

Conversation

@AIalliAI

@AIalliAI AIalliAI commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Fixes #44225

Problem

hermes update (including the desktop GUI's Update Now) rebuilds the desktop app via hermes desktop --build-only after git pull. electron-builder's beforePack hook (apps/desktop/scripts/before-pack.cjs) wipes appOutDir — e.g. release/win-unpacked/ containing the live Hermes.exe — before staging, and the Electron download/extract runs after that wipe. So any pack failure (corrupt cached Electron zip, blocked/timed-out download) destroys the only executable the user has: the desktop shortcut becomes a dead link and the app is gone until a manual rebuild succeeds.

The CLI's existing retry ladder (cache purge → retry → mirror fallback) reduces how often the pack fails, but restores nothing when all retries fail.

Fix

In the packaged-build path of _cmd_desktop (so it covers hermes update, GUI-triggered updates, the installer's headless --update rebuild, and manual hermes desktop --force-build alike):

  • Before the pack: move the current unpacked app aside with a cheap same-volume rename into release/.rebuild-backup/<name> (_backup_desktop_unpacked_app). The beforePack hook's stale-dir cleanup semantics are preserved — it simply finds a clean tree.
  • On final failure (after all existing retries): restore the parked copy over any partial tree, so the existing install and its shortcuts keep working (_restore_desktop_unpacked_app), then fail with the same exit code as before.
  • On success: discard the backup. If a zero-exit pack somehow produced no launchable app, restore instead of discarding.

The holder directory is deliberately not a sibling rename: _purge_electron_build_cache rmtree's release/*-unpacked between retries and _desktop_packaged_executable globs mac*, so a sibling name would either get purged mid-retry or be mistaken for the freshly-built app. There's a regression test pinning each of those interactions.

All helpers are best-effort and never raise; if the backup rename fails, behavior is exactly what it was before this change.

This is the issue's proposed option A/B, but placed in the CLI pack path rather than _cmd_update_impl (option A would miss manual rebuilds) or before-pack.cjs (electron-builder has no on-failure hook to drive a restore from).

Tests

8 new tests in tests/hermes_cli/test_gui_command.py: backup/restore unit coverage (including the macOS Hermes.app → appOutDir resolution), the purge-glob and detection-glob non-interference pins, and end-to-end cmd_gui runs for fail-restores / success-discards / zero-exit-without-artifact-restores.

tests/hermes_cli/test_gui_command.py (36) and tests/hermes_cli/test_cmd_update.py (29) all pass.

🤖 Generated with Claude Code

@claude
fix(desktop): preserve the packaged app when an Electron rebuild fails
278ae9f 
before-pack.cjs deliberately wipes appOutDir (release/win-unpacked with
the live Hermes.exe) so packs always stage into a clean tree, but the
Electron download/extract runs after that wipe — so a pack that fails
(corrupt cached zip, blocked download, proxy timeout) destroyed the only
executable the user had, and the desktop shortcut became a dead link.
hermes update triggers exactly this rebuild after git pull, including
from the GUI's Update Now button.

Park the current unpacked app under release/.rebuild-backup/ (cheap
same-volume rename) before invoking the pack, restore it if every retry
fails (or if a zero-exit pack produces no launchable app), and discard
it once a fresh build exists. The holder directory sits outside both the
release/*-unpacked glob that _purge_electron_build_cache clears between
retries and the mac* glob _desktop_packaged_executable uses for
detection, so the parked copy can't be purged mid-retry or mistaken for
a fresh build. Best-effort: if the rename fails, behavior is exactly as
before.

Fixes NousResearch#44225

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@liuhao1024

liuhao1024 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Code Review — looks solid

The backup-before-pack approach is well-designed. A few things verified:

  1. Backup location avoids purge glob: release/.rebuild-backup/ sits under release/ but doesn't match release/*-unpacked, so _purge_electron_build_cache rmtree won't destroy it. Confirmed.

  2. Backup doesn't masquerade as packaged app: The macOS mac* glob for _desktop_packaged_executable won't match .rebuild-backup/ since it doesn't start with mac. Confirmed.

  3. Race between backup and pack: _backup_desktop_unpacked_app runs synchronously before subprocess.run([npm, "run", "pack"]), so the backup is complete before the pack wipes appOutDir. No race.

  4. Win32 lock handling: The backup rename happens after _stop_desktop_processes_locking_build, so the Hermes.exe file handle is released before the rename. Correct order.

  5. Best-effort semantics: All three helper functions (_backup, _restore, _discard) catch OSError and return None/False — the build proceeds exactly as before if backup fails. Good.

  6. Edge case: pack succeeds but produces no artifact: The post-build check _desktop_packaged_executable(desktop_dir) is None correctly restores the backup. This handles misconfigured targets.

One minor note: _restore_desktop_unpacked_app does shutil.rmtree(original) then backup.rename(original). If the rmtree succeeds but the rename fails (e.g., disk full), the backup is still intact at its .rebuild-backup/ path — the user would have lost the partial build output but can manually recover. Acceptable for best-effort.

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

alt-glitch commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Related: competing fix for #44225 alongside #44238. This PR parks the unpacked app in a release/.rebuild-backup/ holder dir (deliberately not a sibling .bak rename, which the retry-purge / mac-glob paths would clobber); #44238 uses the sibling .bak rename. Same goal, different mechanism — maintainer to pick one.

@alt-glitch alt-glitch mentioned this pull request Jun 11, 2026
Closed
13 tasks
@AIalliAI

AIalliAI commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Requesting maintainer review — this is ready to land from my side. Standalone fork CI is pending first-run approval here; the rollup branch in #44061 carrying this session's batch is fully green on upstream CI (all test shards, typecheck, e2e).

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

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

hermes update destroys Hermes Desktop executable on failed Electron rebuild (desktop shortcut becomes dead link)

3 participants

@AIalliAI @liuhao1024 @alt-glitch