Repository navigation
fix: deliver progress notifications that share a read with their response - #3023
Steeve-Crypto wants to merge 6 commits into
Conversation
…onse _onnotification deferred handlers via Promise.resolve(), while _onresponse runs synchronously and deletes the progress handler. When both messages arrive in one turn, the response won the race and the progress was dropped. Dispatch notification handlers synchronously (async rejections still go to _onerror) and add a regression test for the same-turn case. Fixes modelcontextprotocol#3018
🦋 Changeset detectedLatest commit: c6543cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
The sync notification dispatch in modelcontextprotocol#3023 delivers progress that shares a read with its response, so the SSE progress scenarios should pass.
|
Verified the fix locally. On main, applying only the new regression test fails with 1525 passed and 1 failed in packages/core-internal—the progress callback is never called. On this branch the full suite passes with 1526 passed. The synchronous dispatch in packages/core-internal/src/shared/protocol.ts:933 closes the race by handling the notification before _onresponse deletes the progress handler. The Promise.resolve() deferral was the culprit. |
claude
Bot
added
the
v2
karaaslanz
left a comment
karaaslanz
left a comment
There was a problem hiding this comment.
Independent exact-source review of PR HEAD 9af4566b1010d1496cd8c3633e622bc27d0cc7d1. I compared Protocol._onnotification() in base 9bc40e903fde15534ec06ff4c1347d47000dbe16 (packages/core-internal/src/shared/protocol.ts blob 9d60be8efeb4801c7ab48ef9757e36bc71239b34) against this head (blob c0edd7322207b81d28be3c8a00461cb60d4b2f9d).
I extracted the actual method bodies, stripped TypeScript using Node 24, and executed them in deterministic isolated protocol harnesses with a registered notification callback and captured error reports.
Results: 8/8 checks passed across base and patched methods.
- Base: a notification handler is deferred after the caller returns (
after-notification | progress), leaving a same-read response able to retire its handler before it executes. Patched: handler runs in the inbound call (progress | after-notification), closing that dispatch-order window. - Additionally, in each source version I exercised three error/Promise behaviors: a synchronous handler throw causes exactly one error report, an async rejected handler Promise causes exactly one error report, and an async resolved handler Promise causes no error. These checks specifically cover the exception-handling behavior surrounding the new synchronous invocation and Promise-like branch; all six assertions passed.
This supports the targeted fix and suggests the synchronous/asynchronous error routing remains intact in the tested cases. The PR's pinned GitHub CI workflows report success. Scope: isolated source-extracted dispatch method simulation with mocked codec/registry/error callbacks, not the complete progress-token response lifecycle or real SSE/network transport, and not a full monorepo test. I found no issue in the paths tested, and made no repository source edits.
atomicdjt
left a comment
atomicdjt
left a comment
There was a problem hiding this comment.
Exact-head review of 9af4566b1 (source: packages/core-internal/src/shared/protocol.ts).
The progress callback ordering fix is sound for the reproduced same-read case. One compatibility question before choosing this implementation over #2967: the changed dispatch path applies to every registered notification handler and the fallback handler, not just notifications/progress. Previously their invocation was queued on a microtask; now it happens inside the transport's onmessage stack.
That difference is immediately observable without a race: calling transport.onmessage(customNotification) followed by a synchronous state read or log previously occurred before the registered callback; with this PR the callback runs first. This is potentially relevant to consumers whose handlers close the connection, register new handlers, or issue requests (reentrant dispatch while the transport is still processing its current message). This is a compatibility consideration, not a demonstrated failure in this branch.
Could we either:
- Prefer the narrower alternative in #2967, which defers progress-handler cleanup while preserving existing microtask dispatch for other notifications; or
- Include explicit regression coverage for an extension/fallback notification handler that synchronously closes the connection or issues another protocol operation, and document the wider ordering change?
The existing same-read progress regression and async-throw/rejection checks are useful; they do not by themselves establish compatibility for non-progress notification callbacks.
This is a source-based semantic review, not a claim to have run the complete MCP suite.
Keep microtask dispatch for every other notification and the fallback handler so their callbacks do not run inside the transport's onmessage stack. Adds regression coverage for an extension notification handler that closes the connection.
|
@atomicdjt Good catch, thanks. I narrowed it: only notifications/progress is dispatched synchronously now, and every other handler (including the fallback) keeps the original microtask dispatch, so nothing else runs inside the transport's onmessage stack. I added regression tests for an extension notification handler, one checking ordering and one where the handler closes the connection. Both fail on the previous head and pass now, and the full core-internal suite is green. |
Summary
Fixes #3018.
_onnotificationdeferred handlers withPromise.resolve(), while_onresponseruns synchronously and deletes the matching progress handler. When a progress notification and its response arrive in the same read/turn, the response cleared the handler before the deferred notification ran, so progress was dropped andonerrorreported an unknown token.This dispatches notification handlers synchronously (same posture as
_onresponse). Synchronous throws and async rejections still go to_onerror.Test plan
pnpm testinpackages/core-internal(1526 passed, including the new case)AI disclosure
Used AI assistance to locate the race in
_onnotificationvs_onresponse, draft the sync dispatch change and regression test, and write this PR description. I reviewed the diff and ran the tests above myself.