fix(protocol): ignore late progress for recently completed tokens

[qiushui7]

Summary

• keep a bounded FIFO of the 64 most recently completed progress tokens
• retire registered progress handlers consistently after normal responses, cancellation, timeouts, and terminal task cleanup
• ignore late progress notifications only when their token belonged to a recently finished request
• preserve the existing protocol error for genuinely unknown or never-registered tokens
• add deterministic in-memory transport coverage for notification ordering and cache eviction

Context

Several request lifecycle paths remove a registered progress handler before an already-written progress notification can be dispatched. _onprogress then treats the previously valid token as unknown and calls onerror.

This change remembers a bounded set of recently finished progress tokens whenever a registered handler is retired. It covers normal responses, caller aborts, request timeouts, maxTotalTimeout, and terminal task cleanup without delivering stale progress. Tokens that were never registered still take the existing error path.

Addresses #2580.

Validation

• npm run check
• npx vitest run test/shared/protocol.test.ts — 140 tests passed
• npx vitest run --exclude test/client/stdio.test.ts --exclude test/e2e/scenarios/stdio.test.ts — 2,748 tests passed
• npx tsc -p tsconfig.prod.json
• npx tsc -p tsconfig.cjs.json

The two excluded stdio suites depend on Unix process and signal behavior that is not available in the local Windows environment. The upstream Linux CI matrix runs them on every pushed commit.

[changeset-bot]

🦋 Changeset detected

Latest commit: 1e7b8b6

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

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

npm i https://pkg.pr.new/@modelcontextprotocol/sdk@2586

commit: 1e7b8b6

[sweetrb]

Validated — and the core fix does what it should, but there's a second door onto the same race that this doesn't close yet.

I built from the PR head (072d5f3) rather than the pkg.pr.new tarball, and drove Protocol directly against an in-memory transport, per the deterministic approach discussed above. Your four new tests pass. My control case — late progress arriving after a normal response — passes too, so the completed-token set is working as designed.

The gap: cancel() doesn't remember the token

_rememberCompletedProgressToken is only called from _onresponse. Three other sites delete a progress handler without it, and one is squarely the same race:

// Protocol.request()
options?.signal?.addEventListener('abort', () => cancel(options?.signal?.reason));
const timeoutHandler = () => cancel(McpError.fromError(ErrorCode.RequestTimeout, 'Request timed out', { timeout }));

const cancel = (reason: unknown) => {
    this._responseHandlers.delete(messageId);
    this._progressHandlers.delete(messageId);   // <-- token never remembered
    this._cleanupTimeout(messageId);
    ...
};

Both caller-abort and request-timeout route through cancel(). A progress notification already in flight at that moment lands in _onprogress, finds no handler, isn't in _recentlyCompletedProgressTokens, and raises the same Received a progress notification for an unknown token error this PR is removing.

This is arguably the more likely path in practice: a request times out precisely because the server is still working — and therefore still emitting progress — whereas the completion race needs the response and the notification to be dispatched in the same tick.

Reproduction

Three cases against the PR head, same harness style as your tests:

✓ CONTROL — late progress after a normal response is ignored   (your fix works)
✗ ABORT   — Received a progress notification for an unknown token: {"progressToken":0,...}
✗ TIMEOUT — Received a progress notification for an unknown token: {"progressToken":0,...}

test/shared/late-progress-cancel.test.ts

test('ABORT — late progress after caller abort should also be ignored', async () => {
    const onErrorMock = vi.fn();
    protocol.onerror = onErrorMock;
    await protocol.connect(transport);

    const ac = new AbortController();
    const { requestPromise, sent } = startRequest({ signal: ac.signal });
    requestPromise.catch(() => {});
    ac.abort(new Error('caller aborted'));
    await Promise.resolve();

    await sendProgress(sent.params._meta.progressToken);
    expect(onErrorMock).not.toHaveBeenCalled();
});

test('TIMEOUT — late progress after a request timeout should also be ignored', async () => {
    vi.useFakeTimers();
    try {
        const onErrorMock = vi.fn();
        protocol.onerror = onErrorMock;
        await protocol.connect(transport);

        const { requestPromise, sent } = startRequest({ timeout: 10 });
        requestPromise.catch(() => {});
        await vi.advanceTimersByTimeAsync(20);   // fire timeout -> cancel()

        await sendProgress(sent.params._meta.progressToken);
        expect(onErrorMock).not.toHaveBeenCalled();
    } finally {
        vi.useRealTimers();
    }
});

Happy for you to lift these verbatim if useful.

Suggested fix

Same one-liner you already have, at the other deletion sites — guarding on the delete's return value so unknown tokens still error:

const cancel = (reason: unknown) => {
    this._responseHandlers.delete(messageId);
    if (this._progressHandlers.delete(messageId)) {
        this._rememberCompletedProgressToken(messageId);
    }
    this._cleanupTimeout(messageId);
    ...
};

Two narrower sites worth the same treatment:

• maxTotalTimeout cleanup inside _onprogress — deletes the handler then calls responseHandler(error); a second in-flight notification for that id hits the identical error.
• _cleanupTaskProgressHandler — and note _onresponse deliberately skips remembering when isTaskResponse, so the task surface has no protection at all. That may well be intentional given tasks keep progress alive across responses, but it's worth a comment either way so the asymmetry is deliberate rather than incidental.

One note on the bound: eviction relies on Set preserving insertion order, which is spec-guaranteed, and progress tokens here are monotonic request ids so a re-add can't disturb ordering. That's sound. Adding the cancel path raises the fill rate a little — every timed-out request now consumes a slot — but 64 still looks comfortable for a stdio client.

I have not run this against the original apple-mail-mcp stdio server that surfaced the bug; the deterministic harness pins the behaviour more precisely than the loaded-CI-runner race I originally reported it from. Say the word if you'd still like the end-to-end run as well.

[qiushui7]

Thanks — great catch. I’ve pushed a follow-up in 1e7b8b6 that applies the same invariant whenever a registered progress handler is retired, rather than only after a normal response.

The update:

• routes caller aborts and request timeouts through the completed-token cache
• covers maxTotalTimeout cleanup
• remembers the token when terminal task cleanup removes its progress handler
• centralizes the delete-and-remember guard so tokens are recorded only when a registered handler actually existed
• adds deterministic abort and timeout regressions, and extends the existing maxTotalTimeout and terminal-task tests to assert that onerror is not called

The focused protocol suite now passes 140/140 tests, the broader non-Windows-stdio suite passes 2,748/2,748 tests, and the full upstream CI matrix is green on the new commit.

If you’re able to rerun your deterministic control/abort/timeout harness against 1e7b8b6, that confirmation would be very helpful. The original apple-mail-mcp end-to-end run would be welcome too, but I don’t think it needs to block the deterministic validation.

[sweetrb]

Confirmed on 1e7b8b6 — all three cases pass now.

Same harness as before, unchanged, rebuilt from the new PR head (verified git rev-parse HEAD == the PR's head.sha, 1e7b8b641825c6c7fd570d692eb7703f56cba0ae):

                    072d5f3        1e7b8b6
CONTROL  (response)  ✓ pass    →    ✓ pass
ABORT    (signal)    ✗ FAIL    →    ✓ pass
TIMEOUT  (request)   ✗ FAIL    →    ✓ pass

I also reproduced your focused-suite number independently rather than taking it on trust: vitest run test/shared/protocol.test.ts → 140/140 passed here.

On the implementation

Extracting _cleanupProgressHandler() is a better fix than the per-site patch I suggested — the invariant now lives in one place instead of being replicated four times:

private _cleanupProgressHandler(progressToken: number): void {
    if (this._progressHandlers.delete(progressToken)) {
        this._rememberCompletedProgressToken(progressToken);
    }
}

Worth noting for future readers: after this commit there is exactly one raw _progressHandlers.delete left in protocol.ts, and it is inside that helper. That is the property that makes the fix durable — a new retirement path added later has an obvious correct thing to call, and grep makes a regression visible in one line.

You also got the task nuance right, which was the part I was least sure about when I raised it. _onresponse still skips remembering under isTaskResponse — correct, since progress legitimately outlives a task's response — while _cleanupTaskProgressHandler now remembers at terminal cleanup. That closes the hole without breaking task semantics, rather than papering over it by remembering on every response.

Coverage now

Between the two commits the original issue is closed for: normal completion, caller abort, request timeout, maxTotalTimeout exhaustion, and terminal task cleanup — where the first commit covered only normal completion. That is the full set of paths that retire a registered progress handler, as far as I can see from the current file.

The 64-entry bound still looks comfortable. Adding the abort/timeout paths does raise the fill rate — every cancelled request now consumes a slot where before only completions did — but for a stdio client the in-flight window is nowhere near that.

Nothing outstanding from my side; happy to see it marked ready for review. The apple-mail-mcp end-to-end run is still available if you'd like it as belt-and-braces, but I agree it shouldn't block — the deterministic harness pins the behaviour more precisely than the original loaded-CI-runner race I reported from, and it now covers strictly more paths than that repro did.

[sweetrb]

Correction to my own comments, before it settles into the record: I misattributed the original repro to apple-mail-mcp, and that was wrong.

I introduced the name in my first comment here; it isn't in #2580, which describes a generic server ("Server emits N progress notifications then the result", tool named export) and states only "Observed with @modelcontextprotocol/sdk 1.29.0 over stdio". I've since checked apple-mail-mcp and it emits no progress notifications at all — no progressToken, no notifications/progress anywhere in its source — so it demonstrably cannot be the server that produced this race. Apologies for the noise; you then repeated the name in good faith because I'd asserted it.

Two consequences:

1. The "apple-mail-mcp end-to-end run" I offered doesn't exist and never did. Please disregard that offer — running it would prove nothing, since that server never emits the terminal progress frame this bug needs.
2. Nothing about the diagnosis or the validation changes. The issue's own reproduction is server-agnostic by construction, and the deterministic harness never depended on any particular server — it drives Protocol directly against an in-memory transport. The control/abort/timeout results on 1e7b8b6, and the 140/140 protocol-suite run, all stand exactly as reported.

If a real-server confirmation is still wanted before this goes ready-for-review, the honest version is a purpose-built stdio server that emits N progress notifications immediately followed by the result — essentially the repro from #2580 — rather than any existing server of mine. Happy to build that if it's useful, though I'd still argue the deterministic harness is the stronger evidence for exactly the reason raised earlier in this thread: it removes the timing dependence instead of relying on losing a race on a loaded machine.

[qiushui7]

Thanks for the thorough validation and for correcting the server attribution. Agreed that the deterministic in-memory coverage is the stronger validation here, so I won’t treat a real-server reproduction as a blocker. With the request lifecycle paths covered and CI green, this is ready for maintainer review from my side.