fix(server): restore onclose after modern exchanges - #2778
felixweinberger merged 7 commits into
Conversation
🦋 Changeset detectedLatest commit: 67f14d2 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: |
|
Added the missing Changesets entry for the |
Signed-off-by: vjymisal0 <misalvijay153@gmail.com>
6baf9e9 to
37260aa
Compare
claude
Bot
added
the
v2
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
packages/server/src/server/createMcpHandler.ts— Clients of a reused server whose request overlaps a notification get 499 instead of their result, and the notification's own transport is never closed. The microtask at createMcpHandler.ts:887 calls server.close(), which closes whichever transport is current (protocol.ts:1228); on a reused instance a request that connected after the notification owns that slot. Fix: the notification path must close the transport that invoke created for this exchange, not the server's current transport, so reuse-tolerant behaviour (which the changeset now advertises) holds for notifications too.Why this was flagged
Trigger: a factory returns the same instance (this PR's changeset explicitly targets that population), a notification N is served, and a request R connects on the same instance before N's queued microtask runs (both handle() continuations can be in the microtask queue in the same tick under load). N's invoke resolves with 202 and createMcpHandler.ts:887 queues server.close(). R's invoke.ts:66 connect sets _transport to R's transport (protocol.ts:786). The microtask then runs Protocol.close() at protocol.ts:1227-1228, which closes R's transport: perRequestTransport.ts:365-367 rejects R's deferred response with ConnectionClosed and createMcpHandler.ts:891-894 turns that into a 499 for R's client. N's transport is never closed, so N's wrapper from :870 only unwinds later through R's chain. The dismissal is correct that the line is unchanged from base, but the changeset in this diff (.changeset/curvy-rivers-restore.md:5) now tells consumers reused servers are handled per exchange, so the population reaching this path grows. Remedy: have invoke return or expose the per-request transport and…
Verification: pre-existing — the mechanism is real but the defective line (createMcpHandler.ts:887) is untouched by this diff and the base branch fails identically by the same route; the diff only changes the onclose wrapper at line 872, which does not alter which transport
server.close()tears down. Triggering condition: the factory returns the same instance and a request R'sserver.connect()lands on…
| inflight.add(server); | ||
| server.onclose = () => { | ||
| inflight.delete(server); | ||
| server.onclose = previousOnClose; |
There was a problem hiding this comment.
🟡 nit (optional): A consumer who reassigns a reused server's onclose during an exchange (chaining to the previous handler) silently loses that handler once the exchange ends. The restore at createMcpHandler.ts:872 writes previousOnClose unconditionally, so it overwrites whatever is currently installed, not just the SDK wrapper. Fix: restore only when the wrapper is still the installed handler, e.g. if (server.onclose === wrapper) server.onclose = previousOnClose;, the same guarded pattern used in packages/client/src/client/versionNegotiation.ts:307. On the base branch the consumer's handler survives (the wrapper chain just grows).
Why this was flagged
Trigger: a factory returns the same Server/McpServer across modern requests (the case this PR targets), and consumer code running inside a tool handler during the exchange does const prev = server.onclose; server.onclose = () => { mine(); prev?.(); }, a chaining pattern the SDK itself documents for transports (docs/serving/stdio.md:84 sets server.server.onclose). At transport close, Protocol._onclose (packages/core-internal/src/shared/protocol.ts:846) invokes the consumer's function, which calls the SDK wrapper installed at packages/server/src/server/createMcpHandler.ts:870. The wrapper then executes server.onclose = previousOnClose at createMcpHandler.ts:872 regardless of what is currently installed, replacing the consumer's chained handler with the value captured at createMcpHandler.ts:868. After the exchange the reused instance no longer carries the consumer's handler; on the next close only the original runs. On the base branch the wrapper never touched server.onclose, so the consumer's handler persisted (at the cost of the growing chain the PR fixes). No safeguard checks…
Verification: nit. Triggering condition: consumer code running during a modern exchange (e.g. inside a tool handler) on a factory-reused server reassigns server.onclose by chaining to the currently installed handler (the SDK wrapper). Mechanism verified at /home/claude/typescript-sdk/packages/server/src/server/createMcpHandler.ts:870-874: the wrapper does `inflight.delete(server); server.onclose =… | nit —…
| it('restores a reused server onclose handler after each modern exchange', async () => { | ||
| const reused = new McpServer({ name: 'entry-test-server', version: '1.0.0' }); | ||
| reused.registerTool('echo', { inputSchema: z.object({ text: z.string() }) }, async ({ text }) => ({ | ||
| content: [{ type: 'text', text }] | ||
| })); | ||
|
|
||
| const originalOnClose = vi.fn(); | ||
| reused.server.onclose = originalOnClose; | ||
|
|
||
| const handler = createMcpHandler(() => reused); | ||
|
|
||
| for (let i = 0; i < 3; i++) { | ||
| const response = await handler.fetch(postRequest(modernToolsCall('echo', { text: `hello-${i}` }))); | ||
| expect(response.status).toBe(200); | ||
| await response.text(); | ||
| expect(reused.server.onclose).toBe(originalOnClose); | ||
| } | ||
|
|
||
| expect(originalOnClose).toHaveBeenCalledTimes(3); | ||
| }); |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get no test that a reused server's onclose is restored on the failure or shutdown paths, only on the happy path. The new test at packages/server/test/server/createMcpHandler.test.ts:277-296 drives three successful tools/call exchanges; the restore also has to hold when the exchange fails internally (createMcpHandler.ts:900 calls server.close()) and when handler.close() aborts an in-flight reused instance (createMcpHandler.ts:1005). Fix: add coverage for both error/teardown paths with a reused McpServer, asserting reused.server.onclose is the original function afterwards and that it ran once per exchange. [also at: packages/server/test/server/createMcpHandler.test.ts:277 - nit: REVIEW.md asks that new behavior have vitest coverage including error paths: the new test 'restores a reused server onclose handler after each modern exchange' only exercises three successful tools/call exchanges.]
Why this was flagged
REVIEW.md asks that new behavior have vitest coverage including error paths. The behavior this diff adds is the server.onclose = previousOnClose; restore at packages/server/src/server/createMcpHandler.ts:872, which is reached through three routes: the transport's auto-close after a terminal response, the explicit server.close() on the internal-failure path at createMcpHandler.ts:900, and handler.close() at createMcpHandler.ts:1005. The added test at packages/server/test/server/createMcpHandler.test.ts:277-296 only exercises the first route with three successful tools/call exchanges and asserts originalOnClose was called 3 times. The existing failure-path test at createMcpHandler.test.ts:298-325 uses a fresh per-request instance from testFactory() and never inspects onclose, so a regression that leaves the wrapper installed after a failed or aborted exchange on a reused server (the original bug this PR fixes, on a sibling path) would pass CI.…
Verification: nit. The REVIEW.md block above contains, verbatim under "Tests & docs", "- New behavior has vitest coverage including error paths". The new behavior in this diff is the single added line server.onclose = previousOnClose; at /home/claude/typescript-sdk/packages/server/src/server/createMcpHandler.ts:872, inside the wrapper installed at lines 870-874. That wrapper is reached on three routes:…
| inflight.add(server); | ||
| server.onclose = () => { | ||
| inflight.delete(server); | ||
| server.onclose = previousOnClose; |
There was a problem hiding this comment.
🟡 nit (optional): CLAUDE.md asks for 2-space indentation: the added line server.onclose = previousOnClose; (and the new test block in packages/server/test/server/createMcpHandler.test.ts:277-296) is indented with 4 spaces. The repo's committed .prettierrc.json sets tabWidth: 4, so re-indenting to 2 spaces would fail pnpm lint:all; the instruction and the enforced formatter disagree. Fix: keep the code as prettier formats it and update the stale CLAUDE.md formatting bullet to say 4-space indentation, so future diffs are not measured against a rule the linter contradicts. Same instruction at 2 sites (packages/server/src/server/createMcpHandler.ts:872, packages/server/test/server/createMcpHandler.test.ts:277).
Why this was flagged
Nothing fails at runtime. The CLAUDE.md line literally requires 2-space indentation, and every added line in this diff uses 4 spaces. However, the base commit's .prettierrc.json enforces tabWidth: 4 and the whole file already uses 4 spaces, so the diff matches the formatter the repo actually runs in pnpm lint:all; the only practical consequence is that the CLAUDE.md formatting bullet is stale and contradicts the tooling. Purely a wording/consistency matter for the maintainers to decide (most likely by correcting the CLAUDE.md line).
Verification: Base CLAUDE.md (git show 7f7a94c:CLAUDE.md, "Code Style Guidelines") contains verbatim "- Formatting: 2-space indentation, semicolons required, single quotes preferred". The diff's added line in packages/server/src/server/createMcpHandler.ts:872 ( server.onclose = previousOnClose;) is indented at 12 spaces, i.e. 4-space levels, and every added line of the new test block in…
2 participants
Summary
Fixes #2607.
When a
createMcpHandlerfactory returns the sameMcpServerinstance for multiple modern requests, the handler temporarily wrapsserver.onclosefor in-flight tracking. The wrapper removed the server from the in-flight set but did not restore the previousonclose, so reused servers accumulated a nested wrapper chain across requests.This restores the previous
onclosebefore invoking it, so each completed exchange leaves a reused server with its original close callback instead of another wrapper layer.Tests
pnpm --filter @modelcontextprotocol/server exec vitest run test/server/createMcpHandler.test.ts -t "restores a reused server onclose"pnpm --filter @modelcontextprotocol/server exec vitest run test/server/createMcpHandler.test.tspnpm exec prettier --check packages/server/src/server/createMcpHandler.ts packages/server/test/server/createMcpHandler.test.tspnpm --dir packages/server exec eslint src/server/createMcpHandler.tspnpm --filter @modelcontextprotocol/server run typecheckNote:
pnpm --filter @modelcontextprotocol/server run checkcurrently reaches Prettier warnings across many existing package files unrelated to this patch, so I also ran targeted Prettier/ESLint on the touched files.