fix(proxy): a lost stdout/stderr reader must not wedge the proxy - #333
Closed
codeslake wants to merge 1 commit into
Closed
fix(proxy): a lost stdout/stderr reader must not wedge the proxy#333codeslake wants to merge 1 commit into
codeslake wants to merge 1 commit into
Conversation
When the last reader of the proxy's stdout/stderr pipe goes away — a closed terminal, a rotated log, a killed `tee` — the next write raises EPIPE. That arrives as an asynchronous 'error' event, not a synchronous throw, so Node promotes it to uncaughtException. The self-heal handler then formats err.stack and writes it to the same dead stderr, raising the next EPIPE: the mechanism that exists to keep the proxy up is what takes it down. Measured on a live proxy: 100% CPU, 22 minutes of CPU time burned, 18 connections accepted and none answered — while /health kept returning 200 in 0.35s with every self-reported field green, so no health-based check saw it. Install 'error' listeners on process.stdout/stderr that swallow EPIPE, and route the self-heal handlers' own logging through a guarded write so a log line can never become the next uncaught exception. Both halves are needed: the try/catch covers a synchronous throw on a destroyed stream, the listeners cover the asynchronous event that caused the outage. Removed again in removeSelfHeal, so an embedded host process regains Node's default behaviour. The test reproduces the outage rather than simulating it: it removes the only reader of the child's stdio, raises an uncaught exception from a check-phase callback, then asserts the proxy still serves and does not spin. Mutation-checked — deleting the two listeners fails it with "burned 2.00s of CPU in 1.5s wall". Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Closing — this belongs inside #304, not as a separate PR. Moving the commit onto that branch. — Proxy Builder |
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.
What broke
A live proxy stopped carrying traffic for 27 minutes. It was not down — it was pegged at 100% CPU, accepting connections and answering none, while
/healthkept returning200in 0.35s with every self-reported field green (https_proxy=127.0.0.1:8118,https_proxy_measured=true).The trigger was ordinary: a leftover
... | tee <file>wrapper was killed. Thatteewas the only reader of the pipe the proxy holds as stdout/stderr.Why one dead log reader takes the proxy down
sampleon the wedged process, verbatim shape:The cycle:
EPIPEEPIPEon a stream arrives as an asynchronous'error'event, not a synchronous throw, and with no listener Node promotes it touncaughtExceptioninstallSelfHeal's handler answersuncaughtExceptionby formattingerr.stackand writing it to that same dead stderrThe mechanism that exists to keep the proxy up is what takes it down. A
try/catcharound the write cannot help, because step 2 is not a throw.Measured: 22 minutes of CPU time burned, 18 connections accepted, none answered.
The fix
'error'listeners onprocess.stdout/process.stderrthat swallowEPIPEandERR_STREAM_DESTROYED, so the async half never reachesuncaughtException.removeSelfHeal, matching what that function already does for the other two — an embedded host process that ran forward mode earlier must get Node's default behaviour back, not keep an EPIPE swallower installed by a proxy that has closed.Losing a log reader is ordinary — a closed terminal, a rotated file, a killed
tee. It should cost the log line and nothing else.Tests
test/proxy-stdio-epipe.test.mjsreproduces the outage rather than simulating it: it removes the only reader of a child proxy's stdio, raises an uncaught exception from a check-phase callback (where the real one came from), then asserts the proxy still serves and does not spin.burned 2.00s of CPU in 1.5s wall; restoring them passes. Both directions run and recorded./healthassertion inside the test passes even in the broken state — independently reproducing the field observation that/healthis not a liveness signal for this failure.Non-Functional Requirements
proxy/server.mjs.safeWriteis a 1-line local closure with 3 call sites in the same function; inlining it three times would be worse.Note for reviewers
Branched from
upstream/mainand self-contained on it. An earlier draft routed the handlers through asay()helper — that helper does not exist onmain(it lives on another of our branches), and the resultingReferenceError: say is not definedinside theuncaughtExceptionhandler killed the proxy outright. Caught by the test before commit; the landed version depends on nothing outside this diff.🤖 Generated with Claude Code