fix(desktop): ignore stale backend exits - #38292
Conversation
246476b to
abbdf50
Compare
|
Rebased this PR onto current Conflict resolution kept the newer main changes for profile backend pooling, WebSocket connection probing, and the expanded desktop platform test set, while preserving this PR's generation/process-owner guard for the primary backend connection state. The update reaper now excludes both the primary backend from Fresh validation on the rebased head GitHub now reports the PR as mergeable. |
austinpickett
left a comment
There was a problem hiding this comment.
Hermes Agent Review — ✅ Approve
Verified locally vs origin/main; ran the new test suite (3/3 pass). Paid special attention to the correctness of the stale-exit heuristic, which is the crux.
The stale-exit detection is sound. The PR extracts primary-backend lifecycle into backend-connection-state.cjs using a generation counter + per-process/per-attempt ownership tokens:
clearForCurrentProcess(owner)returnstrueonly if BOTHowner.generation === generationANDowner.process === process.attachProcess()captures{generation, process}at spawn;resetHermesConnection()→invalidate()bumpsgenerationand nullsprocess. So a lateexit/errorfrom a superseded backend fails both checks → returnsfalse→ the new guard (first statement in both theerrorandexithandlers) bails out without nulling the live connection. That's exactly the targeted bug.- The double guard (generation + process identity) is belt-and-suspenders: generation alone suffices, but the extra identity check harmlessly defends future code paths that might swap process without bumping generation. No false positives — a genuine crash of the current backend matches both and clears normally (verified by test 2).
- The promise-attempt path (
clearPromiseForAttempt(attempt), identity checkattempt.promise !== promise) guards the.catch()rejection so a stale attempt can't wipe the new attempt'sconnectionPromiseor fire a misleadingbackend.errorprogress update (test 3).
I confirmed the main.cjs hunks are a faithful mechanical translation of the old module-level hermesProcess/connectionPromise globals into the new accessors (resetHermesConnection, teardownPrimaryBackendAndWait, startHermes single-flight via getPromise(), the bootstrap-reset IPC clearPromise(), before-quit).
node --test apps/desktop/electron/backend-connection-state.test.cjs => 3 pass, 0 fail
Tests exercise the real supersede race (attach → invalidate → new attempt → assert stale clear rejected and live promise survives), not smoke checks. Registered in test:desktop:platforms. No out-of-scope hunks, no new I/O/env/spawn surface (pure in-memory state machine).
Non-blocking observations (not defects): the error handler doesn't re-check !backendReady before rejecting in the stale branch (harmless — rejectBackendStart?. is a no-op once settled); clearPromise() nulls the latched promise without bumping generation, which is intentional for the failure-recovery reset path.
Cluster note: same desktop module-extraction family as #37471/#39554/#38589/etc. — textual conflicts expected, no semantic overlap.
Reviewed by Hermes Agent (local node --test + origin/main verification).
abbdf50 to
1d210d3
Compare
1d210d3 to
7ea439f
Compare
|
Thanks for the focused Desktop lifecycle work. This is an automated hermes-sweeper review: the reported reconnect ordering is already implemented on current
The PR’s |
Summary
error/exitevents after a reconnect switches to a newer connectionRoot cause
resetHermesConnection()cleared the global connection promise and killed the old local backend, but the old process could emitexitafter a newer remote connection had already started. That stale handler cleared the new connection state and broadcast a backend exit, which could push Desktop back into the boot path and amplify into repeated local spawns/port conflicts.Validation
node --test electron/backend-connection-state.test.cjsnpm run test:desktop:platformsnode --check electron/main.cjs; node --check electron/backend-connection-state.cjs; node --check electron/backend-connection-state.test.cjsgit diff --checkNote: local
npm run type-checkcould not complete in this checkout becausetscis not installed on PATH; CI is running the full workflow on the rebased head.Closes #38266