fix(server): install createMcpHandler's onclose hook once per instance

[hamzashah-dev]

Fixes #2607

Root cause

createMcpHandler keeps a Set<Server> of modern instances with an exchange in flight so close() can tear them down, and it kept that set current by wrapping server.onclose — once per request:

const previousOnClose = server.onclose;
inflight.add(server);
server.onclose = () => {
    inflight.delete(server);
    previousOnClose?.();
};

That is fine when the factory honours its contract and returns a fresh instance per request. When the factory returns the same instance every time (createMcpHandler(() => sharedServer)), each request wraps the previous wrapper. The chain grows one layer per request, retains every closure for the life of the process, and when it finally runs it recurses one frame per layer — RangeError: Maximum call stack size exceeded. Because the throw happens on the async close path it lands after handler.close() has already resolved, so the caller can't catch it; the process just dies.

The fix

Guard the wrap with a WeakSet<Server> so the hook is installed at most once per instance. The inflight set is already keyed by instance, so re-adding a server that is already tracked is idempotent, and the single existing wrapper keeps doing the bookkeeping for every later exchange.

The per-request-instance path — the intended one — is byte-for-byte unchanged: one add, one wrap, one delete.

Scope

This makes instance reuse degrade rather than crash; it does not make reuse a supported pattern. setNegotiatedProtocolVersion, installModernOnlyHandlers and seedClientIdentityFromEnvelope still re-run against the shared instance, and one exchange's teardown still closes it out from under the next. A fresh instance per request remains the contract. But an easy-to-make factory mistake shouldn't take the process down 20k requests later with an uncatchable error.

Testing

Two unit tests in packages/server/test/server/createMcpHandler.test.ts:

• installs the in-flight onclose hook at most once per instance — 21 requests through a shared instance, then asserts server.onclose is the same function object it was after the first request. Fails on main (expected [Function] to be [Function]), passes with the fix.
• preserves the instance's own onclose behind the hook — a consumer-supplied onclose set before the first request still fires exactly once on close, so the wrap isn't swallowing it.

I also ran the reporter's 25k-request loop against both trees:

	result
main	14,988 × RangeError: Maximum call stack size exceeded, exit 1
this branch	25,000 requests all 200, close() clean, exit 0

pnpm --filter @modelcontextprotocol/server test → 470 passed. pnpm typecheck:all and pnpm lint:all clean (the pre-push hook's build/lint/typecheck basket also passed).

Changeset included as a patch to @modelcontextprotocol/server.

[changeset-bot]

🦋 Changeset detected

Latest commit: 2baca96

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

This PR includes changesets to release 6 packages

Name	Type
@modelcontextprotocol/server	Patch
@modelcontextprotocol/core	Patch
@modelcontextprotocol/client	Patch
@modelcontextprotocol/server-legacy	Patch
@modelcontextprotocol/codemod	Patch
@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@2610

@modelcontextprotocol/codemod

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

@modelcontextprotocol/core

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

@modelcontextprotocol/server

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

@modelcontextprotocol/server-legacy

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

@modelcontextprotocol/express

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

@modelcontextprotocol/fastify

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

@modelcontextprotocol/hono

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

@modelcontextprotocol/node

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

commit: 2baca96

[felixweinberger]

@claude review

[claude]

LGTM — a minimal, well-tested defensive fix for the onclose-wrapper stacking crash (#2607).

What was reviewed

• The WeakSet guard in serveModern: the intended fresh-instance-per-request path is behaviorally unchanged (every new instance misses the WeakSet, gets exactly one wrap), and the shared-instance misuse path now installs one wrapper whose inflight.delete stays correct across repeated adds since the Set is instance-keyed.
• The error/teardown paths around the hook (catch-path server.close() + explicit inflight.delete, notification microtask close, handler.close()) — all still converge on the single wrapper.
• One residual edge: a consumer overwriting server.onclose on a shared instance after the first request now permanently displaces the bookkeeping wrapper (old code would re-wrap on the next request), leaving a stale inflight entry until handler.close(). That's a doubly-unsupported pattern with a bounded consequence, consistent with the PR's stated degrade-don't-crash scope.
• Changeset text matches the implementation; tests pin both the once-per-instance install and preservation of a consumer-supplied onclose.

Extended reasoning...

Overview

The PR changes one code path in packages/server/src/server/createMcpHandler.ts: the per-request onclose wrap that keeps the modern-leg inflight set current is now guarded by a module-scoped-per-handler WeakSet<Server> so it installs at most once per server instance. Previously, a factory that (incorrectly) returned the same instance for every request stacked one closure per request, retaining all of them and eventually crashing the process with an uncatchable RangeError: Maximum call stack size exceeded on the async close path. The PR adds two focused unit tests and a patch changeset. No public API surface changes.

Security risks

None. The change is internal lifecycle bookkeeping — no auth, parsing, header handling, or trust-boundary code is touched. If anything it removes a denial-of-service-shaped failure mode (unbounded closure accumulation + process crash) triggered by a consumer misconfiguration.

Level of scrutiny

createMcpHandler is a production-critical serving entry, so I read the change against every path that interacts with the hook rather than treating it as a one-liner. Verified: (1) the intended fresh-instance path is byte-identical in behavior — a fresh instance always misses the WeakSet; (2) on the shared-instance path, inflight.add is idempotent and the single wrapper's inflight.delete remains correct for later exchanges, including reuse after a close (the wrapper persists on the instance); (3) the catch path in serveModern still closes and deletes explicitly, and handler.close() clears the set regardless; (4) previousOnClose is captured once, which for a fresh instance is exactly the consumer's factory-set onclose — pinned by the second test; (5) WeakSet imposes no retention on per-request instances. The one behavioral delta I found (consumer overwriting onclose on a shared instance after the first request loses the bookkeeping wrapper, where the old code would re-wrap next request) is a bounded leak of one Set entry under a doubly-unsupported pattern, and matches the PR's explicit degrade-not-support scope statement.

Other factors

This aligns with the repo's recurring-catch guidance on close-path hygiene (wrapped onclose chains). The PR description is accurate against the diff, including the changeset prose. Tests directly pin the fixed behavior (same function object after 21 requests through a shared instance) and the author reports the reproducer's 25k-request loop passing plus the full server test suite, typecheck, and lint clean. Small, self-contained, follows the existing code's style, no design decisions requiring maintainer judgment.