fix(client): preserve pre-set transport handlers across the version-negotiation probe window - #2455
Conversation
Sorry, something went wrong.
…egotiation probe window The probe window took raw ownership of the transport by installing its own onmessage/onerror/onclose and detaching to undefined — wiping any handlers the caller attached before connect(). A plain connect chains those handlers through Protocol.connect(); a negotiating connect silently lost them for the life of the connection (e.g. an onerror watching for session-expiry auth failures never fired again). The window now saves pre-set handlers on open, forwards error and close events to them while the probe is in flight, and restores them on detach so Protocol.connect() chains them exactly as on a plain connect. A mid-window close is forwarded exactly once — the restored handler is cleared so cleanup close() calls cannot re-deliver it — and a start() failure detaches before rethrowing instead of leaking the window's handlers onto the transport. Inbound messages are deliberately not forwarded (the window's drop-guard is a module invariant; the probe reply has no plain-connect counterpart); a pre-set onmessage is restored at detach like the others.
🦋 Changeset detectedLatest commit: 6ea4ce5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Sorry, something went wrong.
|
@modelcontextprotocol/client
npm i https://pkg.pr.new/@modelcontextprotocol/client@2455
npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2455
npm i https://pkg.pr.new/@modelcontextprotocol/core@2455
npm i https://pkg.pr.new/@modelcontextprotocol/server@2455
npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2455
npm i https://pkg.pr.new/@modelcontextprotocol/express@2455
npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2455
npm i https://pkg.pr.new/@modelcontextprotocol/hono@2455
npm i https://pkg.pr.new/@modelcontextprotocol/node@2455 commit: 6ea4ce5 |
Sorry, something went wrong.
There was a problem hiding this comment.
I didn't find any bugs — the handler save/restore matches how Protocol.connect() chains pre-set handlers on a plain connect, and the tests cover the modern, fallback, detach, and mid-probe close/error paths — but this is subtle connect-lifecycle behavior with a couple of judgment calls (clearing onclose after a mid-window close, deliberately not forwarding onmessage), so it's worth a human maintainer's look.
Extended reasoning...Overview
This PR touches only the client's version-negotiation probe window (packages/client/src/client/versionNegotiation.ts), plus a changeset and new tests. The ProbeWindow now snapshots any onmessage/onerror/onclose the caller set on the transport before connect(), forwards error/close events to them while the probe is in flight, and restores them on detach() so Protocol.connect() chains them exactly as it does on a plain (non-negotiating) connect. It also detaches on transport.start() failure inside open() instead of leaking the window's handlers.
Correctness assessment
I cross-checked Protocol.connect() in packages/core-internal/src/shared/protocol.ts — it chains whatever handlers are on the transport at connect time, so restoring the saved handlers before the handover is the right mechanism, and the previous behavior (setting them to undefined) did silently drop pre-set observers. The _closeDelivered guard prevents double delivery when the client's cleanup path calls transport.close() after a mid-window close, matching the plain-connect once-only semantics; the failed-negotiation path in client.ts (transport.close() after negotiateEra throws) is covered by the pin-mode test. I did not find a correctness issue.
Security risks
None. No auth, crypto, or input-handling changes; the drop-guard for non-probe inbound messages is unchanged (pre-set onmessage is intentionally not invoked during the window).
Level of scrutiny
This is client connect-path lifecycle code — the kind of async/handler-ordering logic the repo's review conventions specifically flag as bug-prone. While the change is small (~30 source lines) and well-tested (five new scenarios plus the existing suite), it embeds behavioral decisions (once-only close forwarding, clearing the restored onclose after a mid-window close, not forwarding mid-probe messages) that a maintainer should confirm are the intended semantics rather than something to auto-approve.
Other factors
Changeset is a patch bump for @modelcontextprotocol/client and its prose matches the implementation. No prior human review exists on this PR, and the bug-hunting pass found nothing.
Sorry, something went wrong.
Summary
Connecting with versionNegotiation silently discarded transport handlers the caller set before connect(). The probe window installed its own onmessage/onerror/onclose and detached them to undefined, so Protocol.connect() — which chains pre-set handlers on a plain connect — found nothing to chain. A caller's transport.onerror (a common pattern for detecting session-expiry auth failures for the life of a connection) never fired again once negotiation was enabled.
Behavior
Tests
Five scenarios in versionNegotiation.test.ts: handlers restored after modern negotiation and after legacy fallback (reachable through the Protocol chain), mid-probe errors forwarded to the pre-set handler, the detach path (pin-mode failure) restoring handlers with close delivered exactly once, and a mid-probe close never double-delivering.
Full client suite: 718 passing. Also driven end-to-end against a live HTTP server with a proxy-shaped 400 to the probe: legacy fallback negotiates and the pre-set onerror observes both the mid-probe transport error and post-connect errors.