Repository navigation
fix(cli): end the long-lived stream cleanly on EPIPE - #2479
Merged
Merged
Conversation
A reader that exits early (| head, | grep -m1, quitting less) made the next stdout write fail with EPIPE, and with no 'error' listener on stdout that was an uncaught event that crashed the CLI. The stream path now listens on stdout for its lifetime: EPIPE unsubscribes and resolves, any other stdout error unsubscribes and rejects into the CLI error path, and every listener is detached on each exit, including start() throwing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
2 tasks done
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation addresses the reported failure with focused cleanup and comprehensive tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes CLI stream crashes when downstream readers close stdout early.
Changes:
- Handles
EPIPEas a clean stream termination. - Propagates other stdout errors and consistently removes listeners.
- Adds coverage for errors, signals, cleanup, and startup failures.
| File | Description |
|---|---|
clients/cli/src/handlers/consume-outcome.ts |
Adds stream-scoped stdout error handling and cleanup. |
clients/cli/__tests__/consume-outcome-stream.test.ts |
Tests stream termination and listener cleanup paths. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Copilot review loop closed: round 1 was clean (no inline comments, no findings in the body, no suppressed comments), so no further round was requested. |
2 tasks done
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
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.
Closes #2412
Problem
consumeMethodOutcome's long-lived stream path (resources/subscribeand the other stream methods) registered only SIGINT/SIGTERM handlers. When the reader exits early (| head,| grep -m1, quittingless), the next stdout write fails with EPIPE. With no'error'listener on stdout, that became an unhandled'error'event that crashed the process. The code carried its ownTODO(#1432)for this.Reproduced on
v2/mainby piping a stream outcome that writes every 5ms intohead -n 2:With this branch, the same run prints the two lines and exits 0.
Change
clients/cli/src/handlers/consume-outcome.ts: the stream path now listens for stdout'error'for as long as the stream is open, and attaches that listener beforestart()runs.start()throwing): removes all three listeners.The TODO is gone, and the doc comment now describes this behavior.
Tests
New
clients/cli/__tests__/consume-outcome-stream.test.tscovers:Errorvalue on stdout being treated as a failure, not as a broken pipestart()throwing and leaving no listeners behindLike the existing test in
run-method.test.ts, these call the registered listeners directly instead of emitting process-wide.npm run local:gateis green. An earlier run failed on a webSkillsScreentest under load. This diff touches no web code, and that test passes 3/3 when run alone.🤖 Generated with Claude Code