fix(protocol): progress-notification race; preserve pre-dispatch abort reason

[felixweinberger]

Progress-notification race

// a tool that reports progress right before returning:
async ({ items }, ctx) => {
  for (const i of items) ctx.mcpReq.notify({ method: 'notifications/progress', ... });
  return { content: [...] };
}
// before: the client logged "Received a progress notification for an unknown token"
//         because the response dispatched synchronously while notifications were
//         microtask-deferred; the response retired the handler first.
// after:  notifications dispatch synchronously in arrival order; tokens retired by a
//         response/cancel/timeout are tombstoned (cap 64) so genuinely-late progress
//         is dropped silently (or via the new ProtocolOptions.onLateProgress hook).
//         A token that was never issued still raises onerror.

Pre-dispatch abort reason

const ac = new AbortController();
ac.abort(new Error('user cancelled'));
await client.callTool({...}, { signal: ac.signal });
// before: rejected with bare SdkError(REQUEST_TIMEOUT), reason lost
// after:  SdkError(REQUEST_TIMEOUT) with error.data.reason === the abort reason

How Has This Been Tested?

core-internal 1298, client 698. Five new tests in protocol.test.ts.

Breaking Changes

None.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

[changeset-bot]

🦋 Changeset detected

Latest commit: d483df1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@modelcontextprotocol/core-internal	Patch

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

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2378

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2378

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2378

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2378

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2378

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2378

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2378

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2378

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2378

commit: d483df1

[claude]

🔴 The new public ProtocolOptions.onLateProgress hook has no concrete consumer in this PR — only the new tests use it — and the core fix (tombstoning retired tokens + silently dropping late progress) works without it; per the repo's minimalism conventions, consider shipping the silent-drop fix without the hook or justifying it with a real consumer. If the hook (and the new silent-drop semantics) is kept, prose docs should be updated as well — docs/client.md's "Tracking progress" section doesn't mention either; the PR adds only JSDoc and a changeset.

Extended reasoning...

What this is. The PR adds a new public option, ProtocolOptions.onLateProgress, alongside the actual fix for the progress-notification race. The fix itself has two independent parts: (1) notification handlers now dispatch synchronously in arrival order, and (2) progress tokens retired by a response/cancel/timeout/send-failure are tombstoned so a genuinely-late notifications/progress is dropped instead of surfacing as a spurious unknown-token onerror. Neither part requires the hook — it is purely an observability escape hatch for the dropped notifications.

No concrete consumer. A grep across the repo shows the only callers of onLateProgress are the new tests in protocol.test.ts; there is no production callsite, example, or cookbook usage anywhere in the diff. The repository's review conventions are explicit on this: burden of proof is on addition, every new export is intentional, and new abstractions have at least one concrete callsite in the PR. The hook fails all three as it stands.

Why this matters more than a typical option. ProtocolOptions is not internal-only surface: it is re-exported from packages/core-internal/src/exports/public/index.ts and both ClientOptions (packages/client/src/client/client.ts:160) and ServerOptions (packages/server/src/server/server.ts:72) extend it. So onLateProgress becomes user-facing public API on the Client and Server constructors the moment this merges, and removing it later is a breaking change. That asymmetry — easy to add now, hard to remove later — is exactly what the minimalism convention is guarding against.

The strongest argument for keeping it, and why it doesn't carry on its own. Before this PR, late progress surfaced via onerror, so the hook arguably preserves observability that the silent-drop change removes. But the PR's own framing (and the changeset) treats that onerror as spurious noise — that's the bug being fixed — and no real consumer asking to observe these dropped notifications is cited. A caller receiving the bare ProgressNotification couldn't act on it meaningfully anyway (the request has already settled). If a maintainer or downstream user has a concrete need, that justification belongs in the PR; absent one, the simpler shape is to ship the tombstone + silent drop without the hook.

Concrete walk-through. A user constructs new Client({...}, { onLateProgress: ... }) — this works today only because ClientOptions extends ProtocolOptions. Once published in a release, any later decision that the hook was unnecessary requires a deprecation cycle on the public client/server option surface, even though nothing in the SDK or its examples ever exercised it. Compare with shipping just the fix: the only externally visible change is that the spurious onerror stops firing for tombstoned tokens, which is the behavior the PR description advertises.

Documentation gap (if the hook stays). docs/client.md's "Tracking progress" section (~line 424) documents onprogress, resetTimeoutOnProgress, and maxTotalTimeout, and docs/server.md has a "Progress" section — neither mentions the new late-progress drop semantics or onLateProgress, and the PR adds only JSDoc and a changeset. The repo conventions ask for prose documentation (not just JSDoc) for new user-visible features. To be fair, no existing prose described the old unknown-token onerror behavior, so nothing in the docs is now incorrect — this is a missing-new-docs point, and it is contingent on the hook surviving the design question above. If the hook is dropped, a one-line note about the silent-drop behavior change is optional.

Suggested resolution. Either remove onLateProgress and ship the tombstone/silent-drop fix on its own (preferred per the conventions), or keep it with (a) a concrete consumer or stated real-world need and (b) a paragraph in the "Tracking progress" docs covering the new late-progress semantics and the hook.