Repository navigation
Stdio: keep a bounded tail of a child's stderr - #13
Merged
Merged
Conversation
`ProcessLaunch.inheritStderr` offered two choices — pass the child's stderr through to ours, or drop it. Neither helps a caller diagnosing a child that dies: the explanation is usually the last thing the child wrote, and with `.discard` it is gone while with `.inherit` it is mixed into this process's own stderr, where a library cannot read it back. `ProcessLaunch.stderr: StderrDisposition` adds `.capture(maxBytes:)` beside `.inherit` and `.discard`. Both stdio transports implement it and expose `capturedStandardError()`. - Everything is read, only the tail is kept. Draining matters as much as capturing: a child whose stderr nobody reads can block on a full pipe or take `EPIPE`, so the bytes past the limit are read and dropped rather than left. - `inheritStderr` remains, as a two-way view over the new property, and the old initializer keeps working — existing callers need no change, and `.discard` stays the default. - Decoding the tail is deliberately lossy: keeping the *last* bytes means the first may be half a character, and a diagnostic with one replacement character beats no diagnostic. Prompted by porting acpx's cold-start stderr capture (0.12.1) to SwiftACP, where "agent exited with code 1" is reported today with no way to say why — the agent's own stderr is exactly what explains it. Tests: a dying child's message is captured, only the tail is kept under a small limit, a chatty child still completes its work (the drain, not just the buffer), and nothing is captured by default. Both transports covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: db7af626e3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
They were appended after the file's closing `#endif`, so they sat outside `#if Subprocess` — where `StdioTransport`, `ProcessLaunch` and `LineFraming` are not imported. The default build compiles that file, so every platform failed on missing types while the trait-enabled build I ran locally was fine. Verified both ways now: `swift test` and `swift test --traits Subprocess`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
From review: a child can exit with bytes still queued in its stderr pipe, and `terminationHandler` has no specified ordering against the readability drain — so `await waitForExit(); capturedStandardError()` could return a tail missing the child's last line, which is usually the one that explains the exit. The original test polled, which hid the question rather than answering it. `waitForExit()` now resumes only once a captured stderr has reached EOF: the capture marks `awaitingStderrEOF`, an empty read clears it and releases any waiters, and termination holds them until then. Nothing changes for `.inherit` or `.discard`. Honest about the evidence: I could not reproduce the race on macOS. The test reads the tail straight after `waitForExit()` with no polling, and it passes without this change too — even with ~800 KB of backlog, the drain wins here. It is kept because the guarantee is what callers depend on, and the ordering is unspecified rather than safe; the comment on the test says exactly that rather than implying it reproduces a failure. The `Subprocess` transport does not share the race: its stderr drain and message pump are in one task group, so `run` returns — and the inbound stream finishes — only after both complete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Linux CI timeout was mine. `waitForExit()` blocked until a captured stderr reached EOF, and that EOF is not guaranteed: on Linux a child that writes one line and exits immediately never delivered the empty read, so `processTransportCapturesTheStderrTail` hung and took the whole test process with it — the job was cancelled after 25 minutes with `swift-test` and the xctest bundle left as orphans. The rest of the run had already passed, which is why the job showed no failing test. Two fixes, both of which the previous commit should have had: - **The wait is bounded.** Once the child has exited, EOF gets a short grace period (250 ms) and then waiters are released regardless. A tail missing its last bytes is a far better outcome than a caller that never wakes — and EOF can legitimately never arrive when a grandchild inherited stderr and holds the write end open. - **`close()` tears the capture down.** It cancels the readability handler and closes the read end. Leaving a handler installed keeps its dispatch source alive past the transport, which on Linux can keep the process itself from exiting — quite possibly the second half of what the runner saw. The guarantee the review asked for still holds in the normal case: EOF arrives straight after exit, and `capturedStandardError()` read after `waitForExit()` sees the child's last words. 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.
Why
ProcessLaunch.inheritStderroffers two choices: pass the child's stderr through to ours, or drop it. Neither helps a caller diagnosing a child that dies. The explanation is almost always the last thing the child wrote — and.discardthrows it away, while.inheritmixes it into this process's own stderr where a library cannot read it back.Prompted by porting acpx's cold-start stderr capture (0.12.1) to SwiftACP. Today a failed agent is reported as
Codex process has exited with code 1with no way to say why, which has already cost real debugging time there — the agent's own stderr is exactly what explains it.What
ProcessLaunch.stderr: StderrDisposition—.inherit,.discard,.capture(maxBytes:)— implemented by both stdio transports, each exposingcapturedStandardError().EPIPE. Bytes past the limit are read and dropped rather than left unread. One of the tests covers exactly this — a child that writes 2000 stderr lines under a 64-byte limit still finishes its work.inheritStderrstays as a two-way view over the new property, the old initializer keeps working, and.discardremains the default.optional_data_string_conversionrule is waived at that one line, with the reason in place.Verification
All tests pass with and without the
Subprocesstrait,swiftlint --strictclean on 64 files.Six new tests across both transports: a dying child's message is captured, only the tail survives a small limit, a chatty child is not stalled, and nothing is captured by default.