fix(desktop): dispose orphaned PTYs on renderer crash/reload (follow-up to #48421) - #48732
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Disposes orphaned PTYs on renderer crash/reload by matching webContentsId and calling disposeTerminalSession before the reload/suppress branches. Follows the before-quit sweep pattern. Clean 1-file fix in the Electron main process.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the renderer-crash lifecycle and reusing the existing idempotent PTY disposer. The underlying gap remains on current main: apps/desktop/electron/main.ts:7312-7335 reloads a crashed/oom renderer, while terminal cleanup is otherwise tied to event.sender.once('destroyed', ...) at apps/desktop/electron/main.ts:8612.
Problems
- The diff changes
apps/desktop/electron/main.cjs, but Desktop's source was renamed and substantially migrated toapps/desktop/electron/main.tsin39d09453f95e8aefc0c97e5d9b30ff341cae9ed8. The current bundle path therefore receives none of this PR as written. - No current Electron test covers
render-process-gonedisposal; the PR adds none.
Suggested changes
- Port the sweep to
apps/desktop/electron/main.ts:7312, before the limiter/reload path, and scope it with the event sender id recorded atapps/desktop/electron/main.ts:8597. - Add focused coverage for disposal before both reload and reload suppression.
This is an automated hermes-sweeper review.
| // would leak a live shell (plus children/handles) on every crash. Dispose | ||
| // them here, scoped by the stored webContentsId, mirroring the before-quit | ||
| // sweep. Runs before the reload/suppress branches so it covers both. | ||
| const goneWebContentsId = mainWindow?.webContents?.id |
There was a problem hiding this comment.
This source file was renamed to apps/desktop/electron/main.ts by 39d09453f95e8aefc0c97e5d9b30ff341cae9ed8; current Desktop bundles the TypeScript entrypoint, so this block must be ported there to affect the product. While porting, scope with the render-process-gone event sender id, which is the id stored for terminal sessions.
d4aea69 to
464bb20
Compare
|
Addressed the current-source and test requests in
Verification: all Electron Vitest tests passed (418 passed, 1 skipped), Desktop typecheck passed, touched-file ESLint has 0 errors, and |
Follow-up to merged #48421, which disposes live PTY sessions during app quit. The parallel
render-process-gonecrash/reload path still leaves renderer-owned shells running because reloading reuses the samewebContents, so the per-sessiondestroyedlistener does not fire.Fix
apps/desktop/electron/main.ts.event.sender.id, matching thewebContentsIdstored when each terminal starts, so another renderer's PTYs are untouched.setImmediatescheduling unchanged.The recovery logic is extracted into a small behavior-testable helper rather than testing
main.tssource text.Validation
npx --no-install vitest run --project electron: 38 files, 418 passed, 1 skipped.npm run typecheck: passed for renderer and Electron configs.main.tspadding warnings on untouched lines.git diff --check: passed.Codex adversarial review:
REWORKEDthen approved. The rework added independent isolation for a throwing terminal disposer and a throwing error reporter, with regressions for both.Coordination
Open #43716 still touches the old
main.cjscrash block for the distinctkilledreason; it does not overlap this current-source PTY cleanup.