test(node): read SSE streams until expected events arrive instead of assuming one fetch chunk - #2662
test(node): read SSE streams until expected events arrive instead of assuming one fetch chunk#2662claude[bot] wants to merge 3 commits into
Conversation
…assuming one fetch chunk Two tests in packages/middleware/node/test/streamableHttp.test.ts called reader.read() once and asserted that multiple SSE events were present in that single chunk. fetch makes no such guarantee: on Node 26.7 the events arrive in separate reads, so the tests fail while the middleware behaves correctly. Adds a readSSEUntil(response, predicate, timeoutMs) helper that accumulates decoded chunks until the predicate is satisfied, the stream ends, or the timeout cancels the reader — the same pattern the neighboring "multiple notifications while disconnected" test already uses — and applies it to the three single-read sites in the two affected tests. Fixes #2661 Approach credit: jstar0, who reported the failure and prepared an equivalent branch but could not open a PR. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLGiVsm3WDL3j3Dnepy7Ci
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only fix that replaces single-chunk read() assumptions with a read-until-predicate loop, correctly addressing the Node 26.7 SSE chunking flakiness from #2661. The two inline comments are nits (a comment-wording inaccuracy and a suggested cleanup) and don't block merge.
Extended reasoning...
Overview
Single-file, test-only change to packages/middleware/node/test/streamableHttp.test.ts. It adds a readSSEUntil(response, predicate, timeoutMs) helper and applies it to the three sites in two tests that previously called reader.read() once and asserted on multiple SSE events landing in that single chunk — an assumption fetch never guaranteed and that breaks on Node 26.7. No production code, exports, or protocol behavior are touched, so the absence of a changeset is correct.
Security risks
None. The diff contains only test code — no auth, crypto, input handling, or network-facing production paths are modified.
Level of scrutiny
Low. This is test-infrastructure hardening following an established pattern already present in the same file (the neighboring disconnected-replay test uses the same read-until loop). The main correctness question is the helper's stream mechanics, which I verified: the timeout cancels the reader (making read() resolve done and breaking the loop rather than hanging), decoder.decode(value, { stream: true }) handles multi-byte splits, and releaseLock() in the finally block is what allows the replay test's subsequent sseResponse.body!.cancel() disconnect simulation to work — the diff consistently switched that call from reader.cancel() to body.cancel() to match.
Other factors
The bug hunting system's two findings are both nits: one comment misattributes chunk-splitting to a priming event that the standalone GET stream never writes (writePrimingEvent has a single call site in the POST handler), and one suggests replacing the surviving inline readWithTimeout copies (lines 1607 and 2403) with the new helper. Neither affects test correctness — the tests pass either way, and the helper is strictly more robust than the code it replaces. A candidate issue about a sibling single-read test was examined and ruled out by verifiers. The full package suite (99 tests) passes per the PR description, and the change reduces rather than adds flakiness risk.
…ming event Review nit: the chunk-splitting note misattributed the split to a priming event, but writePrimingEvent's only call site is the POST handler path. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLGiVsm3WDL3j3Dnepy7Ci
There was a problem hiding this comment.
LGTM — test-only fix that replaces single-read SSE assertions with a read-until-predicate helper, correctly addressing the Node 26.7 chunking failures in #2661; the remaining inline comments are non-blocking nits. Thanks for already correcting the priming-event comment in 30a4cc6.
Extended reasoning...
Overview
Single-file, test-only change in packages/middleware/node/test/streamableHttp.test.ts. It adds a readSSEUntil(response, predicate, timeoutMs) helper that accumulates decoded chunks until the expected text appears, and applies it to the three single-read sites in the two tests that failed on Node 26.7 (#2661). No production code, no public API, no changeset needed.
Security risks
None. The diff touches only test code; no auth, crypto, network-facing, or production transport logic is modified.
Level of scrutiny
Low-to-moderate: the risk profile of a test-only PR is limited to masking real failures or introducing flakiness. I checked both directions: the helper still fails deterministically (timeout cancels the reader, assertions on the accumulated text then fail) if the middleware misbehaves, and it is chunking-agnostic by construction — decoder.decode(value, { stream: true }) handles multi-byte splits, the timeout is cleared and the reader lock released in finally, and the replay test's disconnect simulation was correctly updated from reader.cancel() to sseResponse.body!.cancel() since the helper releases its lock on return.
Other factors
All findings across both review runs are nits (stale prose in a doc note, a surviving inline copy of the read-until loop, and an unmigrated single-read in the pre-parsed batch test — currently green only due to handler-completion ordering). One prior nit (the priming-event comment misattribution) was already fixed in commit 30a4cc6. None of these block merge; the inline comments give the author drop-in fixes if they want to sweep them up. The PR's approach matches the pattern an adjacent test in the same file already used, and the package's full test suite passes per the PR description.
…o readSSEUntil Review follow-up, three non-blocking nits in one sweep: - the pre-parsed batch test still read one chunk over two separately-written batch responses (the #2661 pattern) - the two surviving inline readWithTimeout closures now use the helper (one copy had drifted: fresh TextDecoder per chunk without { stream: true } and no releaseLock); the 5s bound is preserved via the timeoutMs parameter - readSSEEvent's doc note now points at readSSEUntil instead of suggesting manual multi-reads Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLGiVsm3WDL3j3Dnepy7Ci
There was a problem hiding this comment.
LGTM — test-only robustness fix, and the nits from earlier review rounds have all been addressed by the follow-up commits.
What was reviewed: the readSSEUntil helper (timeout cancels the reader, decode(..., {stream: true}), lock released in finally so the replay test's body.cancel() still works); the predicate choices at each migrated site (the batch test waits for both req-1 and req-2; the pre-parsed batch test's batch-1 predicate suffices since the tools payload arrives in the same frame); and the remaining single-read() sites in the file, which each await exactly one server-side write, so only intra-write segmentation risk remains — out of scope for #2661.
Extended reasoning...
Overview
PR #2662 changes a single test file, packages/middleware/node/test/streamableHttp.test.ts, to fix Node 26.7 test flakiness (#2661). It adds a readSSEUntil(response, predicate, timeoutMs) helper that accumulates decoded SSE chunks until a predicate matches, and migrates the single-read() sites and the two pre-existing inline readWithTimeout closures onto it. No production code is touched, so the absent changeset is correct.
Prior review rounds
Earlier runs of this review left four nit-level inline comments: (1) an inaccurate priming-event comment on the GET-stream test, (2) the surviving inline readWithTimeout closures, (3) a stale doc note on readSSEEvent, and (4) the unmigrated should handle pre-parsed batch messages single-read site. I verified in the current checkout (HEAD = 3988a19, the PR's latest commit, pushed after those comments) that all four are fixed: the comment now says the event "may arrive split across multiple chunks", both closures are folded into readSSEUntil, the doc note now points to readSSEUntil, and the pre-parsed batch test uses the helper. This run's bug hunting pass found no new issues.
Security risks
None. This is test code only; no auth, transport, or protocol logic changes. The helper is confined to the test file and not exported.
Level of scrutiny
Low-to-moderate: test-only change with no public API or runtime impact. The main risk in test-harness changes is masking real failures, and I checked for that: on timeout the helper cancels the reader, the loop exits on done, and the accumulated text is returned so the subsequent assertions still fail loudly — the helper cannot silently pass a test whose events never arrive. The replay test's disconnect simulation still works because the helper releases the reader lock before the test calls sseResponse.body!.cancel().
Other factors
The fix pattern matches what the neighboring replay tests already used, the approach originates from the reproduced issue report (#2661), and the author reports the full package test suite (99 tests) passing plus lint/typecheck clean. Remaining single-read sites in the file each await exactly one server-side write before reading, so they do not have the multi-write chunking bug this PR targets.
1 participant
Requested by Felix Weinberger · Slack thread
Fixes #2661.
Problem
Two tests in
packages/middleware/node/test/streamableHttp.test.tscallreader.read()once and then assert that multiple SSE events are present in that single chunk:should handle batch request messages with SSE stream for responses— asserts bothreq-1andreq-2responses after one read.should store and replay MCP server tool notifications— asserts a notification after one read, twice (initial stream and the Last-Event-ID reconnect), even though the server writes a priming event (id:/retry:frame,packages/server/src/server/streamableHttp.ts) and the notification as separate writes.fetchmakes no guarantee that separately-written SSE events share a chunk. On Node 26.7 they arrive in separate reads, so both tests fail while the middleware itself behaves correctly (reported with a repro in #2661). On earlier Node versions the writes usually coalesce, which is why the tests have been green.Fix (test-only)
Adds a
readSSEUntil(response, predicate, timeoutMs = 2000)helper that accumulates decoded chunks until the predicate is satisfied, the stream ends, or the timeout cancels the reader — the same read-until pattern the neighboringshould store and replay multiple notifications sent while client is disconnectedtest already uses — and applies it to the three single-read sites in the two affected tests. The helper releases the reader's lock on return so the disconnect-simulationcancel()in the replay test still works.No production code is touched, so no changeset is included.
Verification
read().pnpm --filter @modelcontextprotocol/node test -- test/streamableHttp.test.ts: 4 files, 99 tests pass (Node 22.22.2 — a Node 26.x runtime wasn't available in this environment; the loop is chunking-agnostic by construction, accumulating across reads until the expected text appears).Credit
The failure report, repro steps, and the read-until-predicate approach are from @jstar0 (#2661), who prepared an equivalent branch but hit a permissions error opening a PR. This PR independently implements that approach against
main.Generated by Claude Code