Stop a closed terminal from crashing the desktop app through its own logging - #649
Merged
Merged
Conversation
…n logging
Closing the terminal that launched `copilot` killed the running app:
Uncaught Exception:
Error: write EPIPE
at afterWriteDispatched (node:internal/stream_base_commons:159:15)
...
at console.error (node:internal/console/constructor:444:26)
at replyWithError (node:electron/js2c/browser_init)
Read that bottom-up, because the order is the point: the main process was
REPORTING an IPC error, and the reporting is what killed it. A failure in the
error path converts a handled problem into a crash.
Why a closed terminal reaches the app at all: `tools/cli/bin/copilot.mjs:121`
spawns it with `stdio: "inherit"`, so the GUI writes straight to the launching
terminal — and the window outlives that terminal. Close it (or end the ssh
session, or quit the shell) and stdout is a pipe with no reader, so the next
`console.*` raises EPIPE. With no listener on the stream Node promotes that to
an uncaught exception. This is the documented install path — CLI-first, no DMG —
so it is the normal way to run the app, not an edge case.
There are ~30 `console.*` sites in `main/`, so guarding call sites is not the
fix; attaching a stream listener is. EPIPE is swallowed because there is
genuinely nothing to do — the reader is gone and there is nowhere to log the
fact that there is nowhere to log. Everything else is RE-THROWN: a disk-full
stderr is a real failure, and swallowing every stream error to fix one of them
is how the next one goes unnoticed.
Installed before `applyBrandIdentity`, because the write it protects can be the
first one.
The tests assert the stream does or does not THROW rather than that a listener
was registered — "a listener exists" passes just as well over a listener that
re-throws everything. The disposal case is the control: with the guard removed
the same emit throws again, which proves the passing case is the guard working
and not PassThrough being tolerant on its own.
Tests: 1545 passed (126 files) via `npm run test --workspace @0x-copilot/desktop`.
Note that bare `vitest` is NOT that command — the script builds the
workspace-commit-helper binary first, and without it 15 helper tests fail on
ENOENT spawning a binary this repo never produced.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Surfaced by a crash dialog during testing:
Read it bottom-up, because the order is the whole point: the main process was reporting an IPC error, and the reporting is what killed it. A failure in the error path converted a handled problem into a dead app.
Why a closed terminal reaches the app
tools/cli/bin/copilot.mjs:121spawns the app withstdio: "inherit", so the GUI writes straight to the terminal that rancopilot— and the window outlives that terminal. Close it (or end the ssh session, or quit the shell) and stdout is a pipe with no reader. The nextconsole.*raises EPIPE, and with no listener on the stream Node promotes it to an uncaught exception.This is the documented install path — CLI-first, no DMG — so it is the normal way to run the app, not an edge case. There was no EPIPE guard and no
uncaughtExceptionhandler anywhere inmain/.The fix
There are ~30
console.*sites inmain/, so guarding call sites is not the fix; attaching a stream listener is.EPIPE is swallowed because there is genuinely nothing to do — the reader is gone, and there is nowhere to log the fact that there is nowhere to log. Everything else is re-thrown. A disk-full stderr is a real failure, and swallowing every stream error to fix one of them is how the next one goes unnoticed.
Installed before
applyBrandIdentity, because the write it protects can be the first one.On the tests
They assert the stream does or does not throw, rather than that a listener was registered — "a listener exists" passes just as well over a listener that re-throws everything. The disposal case is the control: with the guard removed the same emit throws again, which proves the passing case is the guard working and not
PassThroughbeing tolerant on its own.Verification
npm run test --workspace @0x-copilot/desktop→ 126 files, 1545 passed, 1 todo, 0 failed. Typecheck clean.Worth recording, because it cost a detour: bare
vitestis not that command. The script isbuild:workspace-commit-helper && build:workspace-fs && test:native && vitest run, and without the build step 15 helper tests fail on ENOENT spawning a binary this repo never produced. Those failures are an artifact of running the tests wrong, not a defect.🤖 Generated with Claude Code