test(mcp): report why the server did not answer, instead of hanging on it (#350) - #352
Open
Ercaner1988 wants to merge 1 commit into
Open
test(mcp): report why the server did not answer, instead of hanging on it (#350)#352Ercaner1988 wants to merge 1 commit into
Ercaner1988 wants to merge 1 commit into
Conversation
🌱 graft blast radiusNothing outside this diff depends on it. 0 areas changed; no indexed dependents at depth 2.
Open the interactive graph → — click an area to see its dependent symbols at file:line. |
…n it rpc() gave up after a fixed 15s, returned whatever had arrived, and dropped the child's stderr. Callers dereferenced into `undefined`, so a slow server and a crashed one both surfaced as `Cannot read properties of undefined (reading 'result')` — naming neither the timeout nor the request. The crash case was worse than badly reported. `await once(child, 'exit')` after `child.kill()` only settles while the child is still alive to be killed; a server that died on its own has already fired `exit` and will not fire it again, so the await hangs forever. Replacing the server with something that writes to stderr and exits 3: the file was still running when I killed it at 120s, with none of its five tests having reported. It now fails in 0.1s with the exit code and the server's own stderr in the message. - Throw instead of returning short: how many of how many, and how long it took. - Keep the child's stderr and put it in that error. It was piped already; it was just never read. - Await a `close` promise created at spawn time, so it settles whether the child crashed or was killed. This is the hang. - Stop waiting the moment the child quits — nothing can arrive after that. - Deadline 15s to 60s, named. Only a live-but-silent server can reach it now, and the loop leaves as soon as the replies are in, so it costs nothing when things work. 15s was the number that flaked: ~2s of work has less headroom than it looks with ~30 test files running in parallel. `close` rather than `exit` throughout: it fires once stdio is drained, so a server that answers and then quits is not misread as having quit without answering. Fixes trailhq#350 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ercaner1988
force-pushed
the
mcp-rpc-diagnosis
branch
from
September 11, 2026 01:27
1375f68 to
3aa15ab
Compare
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.
Fixes #350.
rpc()gave up after a fixed 15s, returned whatever had arrived, and dropped the child's stderr. Callers then dereferenced intoundefined, so a slow server and a crashed one produced the sameCannot read properties of undefined (reading 'result').Chasing that turned up something worse than the bad message, which is why this PR is a little larger than the issue implies.
A crashed server did not fail the run — it hung it
The last two lines of the helper were:
That works only while the child is still alive when
kill()is called. If the server has already died on its own — a bad import, a missing native build, a throw during startup —exithas fired and will not fire again, so theawaitnever settles. Isolated, without any of graft in the way:End to end, with the server replaced by something that writes to stderr and exits 3:
After
and, when the server is up but silent (deadline dropped to 100ms to force it):
Those two are now different sentences, which is the whole point of the issue.
What changed, in one helper
closepromise created at spawn time, so it settles whether the child crashed on its own or was killed at the end. This is the hang.closerather thanexitthroughout: it fires once stdio is drained, so a server that answers and then quits is not misread as having quit without answering.Verified
test/mcp-server.test.ts: 5 of 5 pass, 1.5–3.7s each.upstream/mainon this machine, which are Savings are under-counted ~1000x outside en-US: the footer is written with toLocaleString() and read back with a comma-only regex #338 (fixed separately in fix: pin rendered numbers to one locale so graft can read its own savings footer (#338) #345) and have nothing to do with this file. (I measuredmainat 1220/1207/8, but atde8456e, which is older than this branch's base. The one extra test here isthe brain build worker …, added by270edccin between. This PR adds no tests.)Environment
Node v24.16.0, Windows 11 x64, on
f9e6539(0.18.0 plus the brain commits after it).🤖 Generated with Claude Code