fix(examples): resume streams whose IDs contain underscores - #2567
fix(examples): resume streams whose IDs contain underscores#2567patrickswedish wants to merge 2 commits into
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
b7894fe to
657390d
Compare
knoal
left a comment
There was a problem hiding this comment.
Reviewing via MCE A/B pilot 9 (sophia@hermes.local).
Summary
Fixes stream ID extraction for event IDs containing underscores. The original code eventId.split('_')[0] was incorrect for event IDs like stream_with_underscore_event123 — it would extract stream instead of the full stream ID. The fix uses the events Map to look up the actual stream ID. 45/-6 LOC, 2 files.
What's good
- Real bug fix — the split-and-take-first approach is brittle; the Map lookup is the correct approach.
- Also fixes the ordering — replaces explicit sort with Map iteration order (insertion order is the event chronology). O(n) vs O(n log n).
- The test that exposed the bug is in the diff — likely a failing test that was added in the same PR.
APPROVE — clean bug fix.
— sophia
Resolve stream IDs from stored event metadata and replay events in insertion order so standalone GET streams resume correctly. Signed-off-by: Patrick Ribbsaeter <patrickswedish@gmail.com>
657390d to
e7e2c38
Compare
|
CI note: the Node 20 job failed only because the Cloudflare compatibility worker did not become ready within 60 seconds (last probe: ECONNREFUSED; no worker stderr/stdout). The job completed 370 tests successfully before that timeout, and the Node 22 and Node 24 matrix jobs passed. The full main workflow also passed on the preceding PR revision. I do not have permission to rerun upstream Actions; could a maintainer please rerun the failed Node 20 job? |
knoal
left a comment
There was a problem hiding this comment.
Re-checking — the test (20) workflow is FAILING. This is a Node 20-specific test failure (the equivalent test (22) and test (24) both pass). The other 18 tests pass.
Recommend:
- Check the
test (20)logs for the specific failure - Node 20 is the LTS version that's still widely used; the fix likely needs to handle a Node 20-specific behavior
- May be related to a Node 20 + Vitest 3.x compatibility issue (vitest 3 dropped Node 18 but Node 20 should work)
HOLD until CI is green on all Node versions.
— sophia
Signed-off-by: Patrick Ribbsaeter <hello@glasheldereindhoven.nl>
Summary
Fixes #2560.
Mapinsertion order, which is their actual chronology._GET_streamthat verifies the returned stream ID and replay order.Why
Splitting an event ID on the first underscore returns an empty stream ID for
_GET_stream. That preventsLast-Event-IDresume from reconnecting the standalone stream correctly. Parsing is also inherently ambiguous for any stream ID containing underscores.Validation
pnpm --filter @mcp-examples/shared run checkpnpm --filter @mcp-examples/shared run test— 2 test files, 3 tests passedNo changeset is included because
@mcp-examples/sharedis a private examples workspace package.