Repository navigation
Conversation
🦋 Changeset detectedLatest commit: eb54a1d 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 |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
karaaslanz
left a comment
There was a problem hiding this comment.
Exact-source review of draft HEAD fdd1ffb9baef3d93c996224d164d5b2ebaf4db7f. Found a reproducible post-close write race introduced by async outbound interception.
Location: packages/server/src/server/serveStdio.ts, StdioConnectionChannel.send (approx. lines 184?197). It checks this._closed before await this._outboundIntercept?.(message), but not again afterwards. modernOutboundIntercept is now async, introducing an await boundary even for ordinary responses where interception returns undefined. If the channel is closed/discarded during the await, the continuation still forwards the message to the shared wire after close() finished.
Exact-source reproduction: fetched serveStdio.ts from the PR head (blob 4f6d5fc7928b93269c149d25691428022e82c153), transformed the actual StdioConnectionChannel class with Node 24 TypeScript-strip, instantiated it using a deferred async interceptor and a counting fake wire. Sequence:
- Start
channel.send({jsonrpc:'2.0', id:7, result:{ok:true}}); interceptor awaits. await channel.close().- Resolve the interceptor with
undefined; then await the pending send. - Observed
WIRE_WRITES_AFTER_CLOSE = 1.
No repo files changed. This is a deterministic exact-source isolated test, not a full stdio integration suite.
Narrow fix and regression gate: check this._closed again after awaiting interception, before this._wire.send. Add a deferred-interceptor test that closes the channel between the two steps and verifies zero writes to the shared wire. This prevents discarded connection instances from emitting late messages. I did not review the broader admission-completion design end to end.
|
The post-close stdio write race was fixed in
The PR description contains the current validation results, completion contract, and review status. |
Retain processing promises through OpenTelemetry scopes so admission completion still follows owned work and cleanup. Cover ended spans, cancellation, close, asynchronous validation and immediate request-ID reuse. Clarify the completion contract in the documentation and changeset.
|
The final fixes are in The proposal adds a local completion signal for reusable transport admission. Completion follows SDK-owned processing, response writes, cache invalidation and cleanup; cancellation or connection closure alone does not release unfinished work. The metadata is never serialized. The PR description records the final scope, exact validation results and local environment limitations. This is ready for maintainer review and ownership. Maintainer edits are enabled. Maintainers are welcome to adjust the API or implementation, split or cherry-pick the changes, and merge through the repository's normal review process. No further sign-off from me is needed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2020035c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
Transport adapters cannot safely return reusable admission credits from a response or cancellation alone: handlers, response writes and SDK cleanup may still be running. This adds optional, local
MessageExtraInfo.onMessageProcessedmetadata and preserves it through the official stdio entry. Related: #3020.The public interface remains
onMessageProcessed?: () => void, withsupportsInboundMessageCompletionexported from@modelcontextprotocol/server/stdio. Metadata is never serialized. Completion covers asynchronous result validation, opening/error paths, and subscription teardown with its pending writes. It does not attest remote delivery or application work outside returned promises.The branch includes
mainat1e608cae, preserving OpenTelemetry context and outcomes. A span may end before a response write settles or when cancellation starts; admission completion still waits for owned work. Request IDs can be reused immediately from the completion callback after controller cleanup. Official stdio sends settle with their chunk callback, and closed channels cannot start a late shared-wire write after outbound interception.The final fixes preserve acknowledgement-first subscription ordering, let unrelated cancellation proceed while a retired subscription's writes finish, and include asynchronous client cache eviction in completion. Notification handlers still dispatch immediately. Acknowledgement tracking is registered before sending so synchronous transport closure cannot lose completion.
Maintainer edits are enabled. Maintainers may adjust the API or implementation, split or cherry-pick the changes, and merge through the normal review process without further sign-off from me. Public API acceptance remains a maintainer decision.
Evidence
Validated head:
eb54a1da45a49b5a65280c68ef88e41edf123e8e.All 14 fresh GitHub checks pass on this exact head: checks/build, Node 20/22/24 tests and end-to-end tests, and Bun/Deno integration, examples, client/server conformance, and Publish Any Commit, including its Continuous Releases check.
Before: six regression cases fail on
c2020035: a routed change precedes its acknowledgement; a stalled subscription write blocks unrelated cancellation with and without a completion observer; and three client notification paths complete before asynchronous cache eviction finishes. After: all pass. An additional regression verifies completion after synchronous transport closure during acknowledgement transmission.All 146 focused stdio, client-cache and protocol/instrumentation tests pass; the optional duration test is skipped. Existing cancellation, asynchronous validation, failed-write, subscription-teardown, post-close-send and immediate request-ID reuse coverage is retained.
Local
pnpm run check:all(including documentation),pnpm run build:all, and ESM/CommonJS distribution type smoke checks pass with Node 24.22.0 and pinned pnpm 10.26.1. Required pre-push typecheck, lint and build hooks pass.The fresh broad local run includes 1,864 core-internal, 692 server and 1,078 client test passes. It is not wholly green: three client fixtures require unavailable
/usr/bin/tee; two example tests lack the SQLite native binding; the Workers fixture cannot install@opentelemetry/api@1.10.0from the registry; and two SSE max-total-timeout tests fail. These match the previous local run; the SSE failures were also reproduced against unchanged upstreammainduring the earlier validation pass.The optional ten-minute lifecycle soak was not rerun. Source and CI evidence do not establish remote-effect settlement, released SDK support or downstream production qualification.
Merge Danger
Door: Two-way.
Blast Radius: SDK.
This changes local processing lifecycle accounting and stdio write-settlement timing. Consumers should adopt it through a reviewed SDK release; API acceptance and downstream validation remain separate from these checks.