refactor(core): extract connection-scoped state into a private Connection owner#2470
Draft
felixweinberger wants to merge 4 commits into
Draft
Conversation
…il structurally after dispose
…ction disposal notifications/cancelled aborts a handler while its connection stays open; the structural disposed-connection check alone let a cancelled handler's per-request senders (notify/send, elicitInput/requestSampling) reach the wire. Restore the abort-signal check alongside the connection check at the sender gates, preserving the previous error shapes: notifications resolve as no-ops, requests reject with the typed ConnectionClosed error. Also clear the pending negotiated-version seed once a connection completes its own negotiation, so a post-close read cannot resurrect a stale pre-connect seed over the connection-negotiated value. Tests: pin cancellation-on-a-live-connection behavior (senders inert, nothing reaches the wire, the connection still serves fresh requests), unit-test the Connection owner's structural mechanisms (disposed send rejection, idempotent dispose, request-funnel early reject), and cover the seed lifecycle and the outbound _meta envelope attach seam.
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Stacked on #2421 (
fweinberger/stateless-reuse-guard) — only the top four commits are new here. This is a feasibility proof for maintainer review of the structural direction, not a merge candidate: if the shape looks right, the plan is to fold it into (or rebase it after) the lifecycle PR.Extracts everything scoped to a single transport connection out of
Protocol's instance fields into a privateConnectionowner: the transport with its wrapped callbacks, the response/progress/timeout registries, the in-flight handler abort controllers, pending debounced notifications, and the negotiated protocol version.Motivation and Context
The five lifecycle bugs reviewed on #2421 (failed-start unwind, wedged close on a silent transport, debounced sends crossing connections, handler senders outliving their connection, stale negotiated version) share one root cause: per-connection state is smeared across ~7 instance fields with four separate teardown paths that each reset a different subset. Every fix on that PR is a guard patching one observable symptom of the same structural gap.
Giving the connection a single owner object makes the wrong states unrepresentable instead of individually guarded:
Connection.open()installs callbacks, starts the transport, and fully unwinds on failure; the failed-start unwind guard is deleted.ConnectionClosed) once it is gone; the five call-site abort gates are deleted.The deleted guards' tests from #2421 pass unmodified against the structural version — including the pin for handlers cancelled on a still-live connection.
Commit structure (intended to be read in order):
Connectionwith zero behavior change (suite passes untouched).dispose(), sends fail structurally after dispose, the expected-failing pin flips to plain.connection.disposed) covers connection loss but notnotifications/cancelledon a live connection; the sender gates now check both, with a pin that nothing from a cancelled handler reaches the wire while the connection keeps serving fresh requests.How Has This Been Tested?
protocol.tsbranch coverage 89.22% vs 87.05% on the base branch.notifya no-op,send/elicitInputrejectingConnectionClosed, nothing on the wire, and the same connection serving the next request; on the pre-fix commit the leak reproduces (notification delivered, server→client ping answered, elicitation prompt shown).typecheck:all,lint:all(ESLint + Prettier + snippet sync) clean.Breaking Changes
None. No public API change: the only surface touch is the additive optional
Transport.protocolVersion, which the base PR already introduces for session resumption; this branch builds on it.Connectionis module-private. Error shapes and messages of the replaced guards are preserved exactly.Types of changes
Checklist
Additional context
connect()land on a pending seed the next connection starts from (the serving-entry binding pattern), and a connection completing its own negotiation supersedes the seed so a post-close read cannot resurrect a stale value.mainand untouched here:notifications/cancelledwithrequestId: 0is dropped by the falsy guard in_oncancel, so the first request of a session (ids start at 0) cannot be cancelled. Worth a separate fix.