Skip to content

fix(desktop): guard git-based self-update against hung processes - #39096

Closed
foras910521-lab wants to merge 1 commit into
NousResearch:mainfrom
foras910521-lab:fix/desktop-git-timeout-guard
Closed

fix(desktop): guard git-based self-update against hung processes#39096
foras910521-lab wants to merge 1 commit into
NousResearch:mainfrom
foras910521-lab:fix/desktop-git-timeout-guard

Conversation

@foras910521-lab

Copy link
Copy Markdown
Contributor

Summary

  • Cap git operations in the Desktop self-update path at 15s via GIT_OPERATION_TIMEOUT_MS
  • On timeout, kill the spawned child with SIGTERM and resolve with timedOut=true so the UI can recover
  • Clear the pending timeout when the child exits/errors to avoid double-firing
  • Add tests/test_desktop_update_safety.py to guard the timeout hook and kill path

Why

The Desktop self-update path shells out to git. If git hangs (slow remote, network glitch, host refusing connection) the spawned child runs forever and the update UI never recovers. This is an infinite-spinner failure mode observed when the network is unreliable.

Diff scope

  • apps/desktop/electron/main.cjs: runGit() now wraps spawn in a timeout that fires SIGTERM and resolves with timedOut=true
  • tests/test_desktop_update_safety.py: pins the timeout hook + kill path

Out of scope

The companion fix that makes the Python backend reachable in the packaged app (dist/** added to build.asarUnpack, guarded by tests/test_desktop_packaging.py) is split out into #39093 to keep this change small and reviewable.

Test plan

The Desktop self-update path shells out to git. If git hangs (slow
remote, network glitch, host refusing connection) the spawned child
runs forever and the update UI never recovers.

- Cap git operations at 15s via GIT_OPERATION_TIMEOUT_MS
- On timeout, kill the child with SIGTERM and resolve with timedOut=true
- Clear the pending timeout once the child exits/errors to avoid double-firing

Tests:
- tests/test_desktop_update_safety.py pins the timeout hook + kill path

The companion fix (dist/** unpack so the Python backend can reach
app.asar.unpacked/dist/index.html) is split out into a separate PR
to keep this change small and reviewable.

@YoussefEttamimi YoussefEttamimi 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.

Reviewed and this looks good to merge.

What I checked:

  • runGit() now uses a settled guard to avoid double resolution.
  • The timeout path appends an actionable stderr message, attempts child.kill('SIGTERM'), and resolves with { code: null, timedOut: true }.
  • The normal error and exit paths clear the timer, so successful/failed git operations should not leave a pending timeout behind.
  • Existing callers already treat code !== 0 as failure, so timeout should recover the update UI instead of hanging forever.

Checks I ran/verified during review:

  • python -m pytest tests/test_desktop_update_safety.py -q — passed
  • node --check apps/desktop/electron/main.cjs — passed

Non-blocking suggestions for a follow-up:

  • Validate HERMES_DESKTOP_GIT_TIMEOUT_MS so invalid/zero/negative values fall back to a sane default instead of producing an immediate or unexpected timeout.
  • Consider replacing the textual Python guard with a JS behavioral test that mocks spawn and verifies SIGTERM, timedOut: true, timer cleanup, and no double resolution.

No blocking issues found.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 4, 2026
@foras910521-lab foras910521-lab closed this by deleting the head repository Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants