fix: prevent stack overflow from re-entrant close() in StreamableHTTP transport

[ctonneslan]

Summary

• Adds _isClosing re-entrancy guard to WebStandardStreamableHTTPServerTransport.close() to prevent RangeError: Maximum call stack size exceeded
• Snapshots and clears _streamMapping before iterating cleanup functions, preventing re-entrant deletes from cancel callbacks
• Wraps individual cleanup calls in try/catch to suppress errors from already-closed controllers
• NodeStreamableHTTPServerTransport delegates to the same close() method, so both transports are covered

Fixes #1699

Related PRs:

• Supersedes fix: prevent stack overflow on concurrent transport close #1700 (closed — subset of this PR)
• Complementary to fix: call onerror callback before throwing transport errors #1704 (onerror callbacks on send methods — separate concern)

Test plan

• Existing tests pass (pnpm --filter @modelcontextprotocol/server test)
• Calling close() multiple times concurrently does not throw RangeError
• Calling close() after already closed is a no-op

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5fcb221

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@1705

@modelcontextprotocol/server

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@1705

@modelcontextprotocol/express

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@1705

@modelcontextprotocol/hono

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@1705

@modelcontextprotocol/node

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@1705

commit: 5fcb221

[travisbreaks]

This is the most comprehensive of the three related PRs (#1700, #1704, #1705). Good to see the snapshot-before-clear pattern and individual try/catch on cleanup calls.

A few findings:

1. onclose not protected by finally
In close(), onclose?.() is called after the try/finally block:

try {
    const entries = [...this._streamMapping.values()];
    this._streamMapping.clear();
    for (const { cleanup } of entries) { ... }
    this._requestResponseMap.clear();
} finally {
    // Don't reset _isClosing
}
this.onclose?.();

If _requestResponseMap.clear() throws (or any future code added to the try block), onclose never fires. This should be inside the finally:

} finally {
    this.onclose?.();
}

2. Overlap with #1700 and #1704
This PR subsumes both:

• #1700: same _isClosing guard on close()
• #1704: same onerror callback pattern on send() methods

If this PR lands, the other two should be closed or rebased. Worth noting in the PR description to help maintainers triage.

3. onerror before reject is good
The pattern of calling this.onerror?.(err) before reject(err) in send() is correct: it lets error handlers run before the promise rejection propagates. Consistent across all three transports.

4. Suppressed cleanup errors
The empty catch {} blocks in cleanup are fine for shutdown, but a debug-level log would help operators diagnose issues in production. Not a blocker.

Solid PR. The main actionable item is moving onclose into the finally block.

[ctonneslan]

Thanks for the thorough review!

Re: point 1 — onclose is already inside the finally block in the current diff. Might have been a misread of the code structure.

Re: point 2 — closed #1700 since this PR fully covers it. Note that #1704 (onerror callbacks on send methods) is actually complementary, not redundant — this PR doesn't touch the send() paths. I'll keep #1704 open and address your defensive wrapping suggestion there.

Re: point 4 — agreed, suppressed cleanup errors are fine for now. Happy to add debug logging in a follow-up if maintainers want it.

[travisbreaks]

Solid fix for the re-entrant close() problem. The _isClosing guard and snapshot-before-clear pattern are both correct approaches for this class of bug. A few observations:

1. Guard placement is good. The early return on _isClosing prevents the stack overflow, and snapshotting _streamMapping.values() before calling .clear() prevents the cancel callbacks from mutating the map mid-iteration. Clean solution.

2. Consider resetting _isClosing on error. Right now, if one of the cleanup() calls throws something unexpected that somehow bypasses the inner catch (e.g., a non-Error thrown value in some edge runtime), the _isClosing flag stays true permanently. Since onclose?.() is in the finally block, the transport signals closure either way, so this may be acceptable. But if there is any scenario where close() could be retried after a partial failure, the flag would block it. Might be worth a brief comment noting that _isClosing is intentionally permanent (one-shot).

3. The inner catch {} silently swallows errors. This is fine for "already-closed controllers" as the comment says, but consider whether logging at debug level would help during development. Silent catches in transport code can make debugging painful when something unexpected slips through.

4. Missing the same fix on the Node transport? This patch only touches WebStandardStreamableHTTPServerTransport. Does the Node-specific StreamableHTTPServerTransport (if it exists in this codebase) have the same re-entrant close vulnerability? Worth checking for consistency.

Overall this is a clean, well-scoped fix. Nice work identifying the root cause.

[ctonneslan]

Thanks for the thorough review!

Re: resetting _isClosing on error — good point. The flag is intentionally permanent (one-shot) since close() should only succeed once. If cleanup partially fails, the transport is in an indeterminate state anyway, and retrying close() on a half-torn-down transport would likely cause more issues. I'll add a comment noting this is intentional.

Re: silent catch — agreed, a debug log would help. The catch is specifically for already-closed ReadableStream controllers, but I can see how silent swallowing would be frustrating to debug in other cases.

Re: Node transport — checked this. The Node-specific StreamableHTTPServerTransport in packages/middleware/node/ delegates directly to the web standard transport via this._webStandardTransport.close(), so it's already protected by the same guard. No separate fix needed.