fix(terminal): guard the terminal worker against process-level errors - #3207
fix(terminal): guard the terminal worker against process-level errors#3207zxhggg wants to merge 2 commits into
Conversation
The terminal worker utilityProcess owns every terminal, SSH, SFTP, and
port-forwarding session, but it installed no uncaughtException or
unhandledRejection handler. Any stray async throw inside it — a socket that
errors after its `once("error")` handler was consumed, a throw from a
setTimeout callback, a rejected promise on a fire-and-forget IPC listener —
exited the process with code 1. terminalWorkerManager.handleExit() then tore
down *every* live session at once and reported "Terminal worker exited with
code 1" to each of them.
The main process already installs exactly these guards, for exactly this
reason; processErrorGuards.cjs even says an ssh2 connection-level error is
"never a reason to kill the entire multi-session app". Once sessions moved
into the worker, the worker became the process that has to survive them.
Install the same guards there, arming runtime protection immediately since
the worker has no window lifecycle to wait on, and forward suppressed errors
to the parent so main can record them.
A worker exit dropped every session at once but left no forensic trace:
handleExit() never reached crashLogBridge, and the only onWorkerExit listener
was port forwarding's tunnel-status cleanup. Bug reports could therefore only
carry the symptom ("Terminal worker exited with code 1"), never the cause —
the worker's stderr goes to the main process's inherited stderr, which is
discarded under the Windows GUI subsystem.
Fan worker-process-error reports out to main and write both those and worker
exits to the crash log.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3b7b046c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // The worker has no window lifecycle: it is live the moment it is forked. | ||
| // Arm runtime protection immediately, otherwise classifyProcessError() reads | ||
| // every runtime error as a pre-startup failure and still lets the process die. | ||
| controller.completeMainWindowStartup({ windowShown: true }); |
There was a problem hiding this comment.
If a generic exception occurs while loading or initializing any bridge between this call and runtime.start(), the controller already classifies it as a post-startup error and suppresses it. Node then unwinds main() without reaching the parentPort.on("message", ...) registration, while the utility process remains alive; because terminalWorkerManager.request() has no timeout, terminal requests remain pending indefinitely instead of the worker exiting and being replaced. Keep startup errors fatal and arm runtime protection only after runtime.start() succeeds.
Useful? React with 👍 / 👎.
1 participant
Summary
Every terminal, SSH, SFTP, and port-forwarding session lives in one
utilityProcess, and that process installs nouncaughtException/unhandledRejectionhandler. So a single stray async error anywhere in the workerexits it with code 1, and
terminalWorkerManager.handleExit()then closes everylive session at once with
Terminal worker exited with code 1.The main process already installs these guards for exactly this reason —
processErrorGuards.cjsopens by saying an ssh2 connection-level error is"never a reason to kill the entire multi-session app", and the test file covering it
is even named
mainProcessErrorGuards.test.cjs. When sessions moved into the worker,the worker became the process that has to survive them, and it is the one process
without the guard.
This PR installs the same guards there, and makes worker crashes visible in the
crash log so future reports can carry the cause rather than only the symptom.
Type of Change
Related Issue (optional)
Closes #3206
Changes Made
electron/terminalWorker/process.cjs— newinstallWorkerProcessErrorGuards(),called at the top of
main(). It reusesprocessErrorGuards.cjsunchanged and callscompleteMainWindowStartup({ windowShown: true })immediately: the worker has no windowlifecycle, so without arming runtime protection
classifyProcessError()would read everyruntime error as a pre-startup failure and still let the process die. Suppressed errors
are forwarded to the parent as a
worker-process-errormessage.electron/bridges/terminalWorkerManager.cjs— handle that message kind and fan it outthrough a new
onWorkerProcessError()subscription, following the existingonTerminalInterceptorWarning()pattern (listener set, dispatch branch, dispose clear,export).
electron/main/registerBridges.cjs— write both worker exits and suppressed workererrors to
crashLogBridge. PreviouslyhandleExit()never reached the crash log and theonly
onWorkerExitlistener was port forwarding's tunnel cleanup, so a crash that droppedevery session left no record at all; the worker's stderr goes to the main process's
inherited stderr, which is discarded under the Windows GUI subsystem.
Tests — 5 cases in
process.test.cjs(ssh2levelerror, generic runtime error andrejection,
ECONNRESETreporting,EPIPEignored without a report, uninstall) and 1 interminalWorkerManager.test.cjs(fan-out + dispose).Not fixed here
While tracing this I found specific unguarded throw sites that the guard now catches but
that are worth fixing at the source separately, so this PR stays reviewable:
terminalWorker/runtime.cjs—handleMessage()andhandleSend()have notry/catch.handleRequest()does, so only the fire-and-forgetipcMain.onpath (netcatty:write,resize,flow,close,interrupt) is exposed.bridges/terminalBridge.cjswriteToSession()—setTimeout(sendChunk, ...); a throw ina timer callback is uncatchable by any surrounding
try/catch.bridges/sshBridge.cjsjump-host chain —conn.once('error', ...)only. Afterready,settledswallows the first error and theoncelistener is then gone, so a seconderroron that client reaches an EventEmitter with no listener and throws.Screenshots / Demo
No UI change. Behavioral change: a stray async error in the worker is now logged and
suppressed instead of terminating the process and disconnecting every session.
Testing
npm run dev)npm run lint)npm test)npm run generate:capability-tools) — not applicable, no capability catalog changePlease read the three unchecked boxes — I want to be precise about what I did and did not run.
I could not run
npm ciin my environment, sonpm run dev,npm run lint, and the fullnpm testdid not run. What I did run, withnode --testdirectly on the suites that haveno native/third-party dependencies:
The 2 failures are
Cannot find module 'ssh2-sftp-client'andCannot find module 'ajv/dist/2020'. I confirmed they are environmental by stashing thisbranch and re-running on clean
main: the same 2 fail identically there. All 6 newtests pass, and
node --checkis clean on all three changed source files.CI runs the real suite — please treat that as the authoritative result over my partial run.
Checklist