fix: keep the forked child's output out of the MCP transport - #4
Merged
Conversation
stdout is the protocol stream, and the child inherits it across the fork.
runInProcess() buffered PHPUnit's own output, but a test that fatals or calls
exit() never reaches the matching ob_get_clean(): PHP flushes every active
buffer during shutdown, straight to fd 1. Whatever user code had printed landed
in front of the next JSON-RPC frame and, unterminated, glued itself to it --
`</html>{"jsonrpc":...}`. Clients that frame on newlines cannot parse that line,
so they block until their own timeout on a result that already arrived. Measured
at five minutes per call, and no verdict, for a run that took two seconds.
The child now seals stdout immediately after the fork with a never-ended output
buffer whose callback returns nothing, so any phase -- destructors and shutdown
included -- writes zero bytes.
Sealing alone would trade a loud bug for a silent one: a debug echo, and the
error page a framework renders on its way down, would both vanish. So the seal
also registers a shutdown hook that ships the buffered output over the result
socket, and the crash payload carries it under the existing `echo` key together
with a message naming the fatal when PHP recorded one. The bytes that used to
corrupt the channel are now the diagnostic that explains the crash. The hook
fires only on the crash path: a completed run writes its payload and dies by
SIGKILL, which runs no shutdown function.
Transport purity is asserted in tearDown for every integration test rather than
only the one written for it -- a leak is a property of the transport, so it
should fail wherever it appears, not only where someone thought to look.
Co-Authored-By: Max <noreply>
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 #3.
The failure
stdout is the protocol stream, and the forked test child inherits it.
runInProcess()buffers PHPUnit's own output, but a test that fatals or callsexit()never reaches the matchingob_get_clean()— PHP flushes every active buffer during shutdown, straight to fd 1. Whatever user code had printed landed in front of the next JSON-RPC frame and, being unterminated, glued itself to it:A client that frames on newlines cannot parse that line. It blocks until its own timeout on a result that already arrived — measured at five minutes per call, and no verdict, for a run the server had finished in two seconds. Only crashing tests trigger it, which is exactly what happens during TDD.
The change
Seal. The child seals stdout immediately after the fork: a never-ended output buffer whose callback returns nothing. The shutdown flush still runs and still writes zero bytes, so no phase — destructors and shutdown functions included — can reach the transport.
Report, don't swallow. Sealing alone would trade a loud bug for a silent one: a debug
echo, and the error page a framework renders on its way down, would both vanish. The same hook ships the buffered output over the result socket instead. The crash payload now carries it under the existingechokey, alongside a message naming the fatal when PHP recorded one:The bytes that used to corrupt the channel are now the diagnostic that explains the crash.
The hook only ever fires on the crash path. A completed run writes its payload and dies by
SIGKILLinterminateChild(), which runs no shutdown function at all; a flag covers the posix-less fallback there, whereexit(0)does run them.Tests
ServerStdioTest::testChildOutputNeverReachesTheProtocolStreambuilds a fixture project whose only test echoes an HTML marker and then callsexit(1)— both halves of the bug at once: output from user code, and a child that dies before shipping a result. It asserts the transport carries only parseable frames, and that the crash result reports the child's output and its cause of death.The assertion is deliberately not a substring check for the marker on raw stdout: once the output is reported rather than discarded, the marker travels inside the frame as data, which is the point. What must never happen is it reaching the stream as bytes of its own — which is what frame purity states.
Transport purity is also asserted in
tearDownfor every integration test, not only the one written for it. A leak is a property of the transport, so it should fail wherever it appears rather than only where someone thought to look. Reverting just the source change fails the suite twice — once in the test body, once in the shared check.Suite: 14 tests, 80 assertions, green. Verified red without the source change.
Not in scope
The fork-less fallback (no
pcntl) runs tests in the daemon itself and has the same exposure. Left alone deliberately: sealing the daemon's own stdout risks silencing the transport, and a test that callsexit()there kills the daemon regardless. Worth its own issue.The client-side half — an adapter that waits out its full timeout rather than reporting that it received 46KB it could not parse — is
Digital-Process-Tools/claude-supertool#1924, with observability follow-ups in#1927.