feat(server): BackchannelCompat; Server.buildContext outbound via ctx.mcpReq.send#2059
feat(server): BackchannelCompat; Server.buildContext outbound via ctx.mcpReq.send#2059felixweinberger wants to merge 3 commits into
Conversation
felixweinberger
added
the
v2-stateless
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/r3-handlehttp
branch
from
7758dfc to
b0d286d
Compare
felixweinberger
force-pushed
the
fweinberger/r4-backchannel
branch
from
3eebc7f to
2358111
Compare
|
@claude review |
There was a problem hiding this comment.
Thanks — 2358111 resolves the createMessage cap-check regression I flagged; what's left from this pass is two non-blocking nits. Not auto-approving only because this adds a new public class (BackchannelCompat) with stateful request correlation and transport-layer wiring, which is worth a maintainer's design sign-off.
Extended reasoning...
Overview
R4 of the handleHttp decomposition series. Adds BackchannelCompat (new public export, ~120 lines) — a per-session {requestId → resolver} map that lets a tool handler's ctx.mcpReq.send write elicitation/sampling requests onto the open POST's SSE stream and await the client's POSTed-back response. shttpHandler is wired to route inbound responses through it and to call closeSession on session close (guarded by a module-level WeakMap<SessionCompat, WeakSet<BackchannelCompat>> to avoid listener accumulation). Server.buildContext now routes elicitInput/requestSampling/log through ctx.mcpReq.send/notify via new _createMessageVia/_elicitInputVia shared bodies, so the instance methods and the per-request path share validation. Conformance harness updated; new vitest suite covers resolve/reject/timeout/abort/closed-stream/per-session isolation.
Change since last review
My prior 🔴 (unconditional base-sampling cap check breaking createMessage() v1 parity) was addressed in 2358111 — _createMessageVia now only enforces the sampling.tools sub-capability inline, with a comment explaining the base check stays gated on enforceStrictCapabilities. That restores the pre-PR contract for the public instance method. The two findings from this pass (missing reason on notifications/cancelled, and onprogress/resetTimeoutOnProgress/maxTotalTimeout silently dropped) are both parity/diagnostics nits on a path that previously rejected outright with NotConnected, so neither is a regression.
Security risks
None identified. No auth, crypto, or permission logic touched. Response correlation is keyed by (sessionId, numericId) with an explicit typeof response.id !== 'number' guard, so a string "0" cannot claim numeric pending id 0. The backchannel is only attached when both session and backchannel are configured and sessionId is defined, so stateless requests can't reach another session's resolver map.
Level of scrutiny
Medium-high. This is production transport-layer code with new public API surface and exactly the async-lifecycle shape (abort listeners, deferred timers, cleanup ordering, close-path teardown) the repo's Recurring Catches section flags. The implementation looks careful — cleanup() removes the abort listener and clears the timer on every settle path, pre-aborted signals short-circuit before allocating an id, writeSSE returning false rejects immediately rather than hanging — and the test file exercises each of those branches. But "every new export is intentional" and "burden of proof is on addition" are first-order principles here, and the module-level wiredBackchannels WeakMap plus the documented _clientCapabilities singleton caveat are design choices a maintainer should ratify.
Other factors
Dual conformance is reported 40/40 on both targets and the expected-failures baseline is now empty, which is good evidence the legacy-client path works end-to-end. No changeset is included; given a new public export this is likely intentional for the decomposition series but worth confirming. The PR is part of a stacked sequence (depends on R0–R3, F1 follow-up planned for per-request caps), so a human reviewer with that series context is better placed to approve than I am.
| const onAbort = () => { | ||
| // Tell the client to stop processing, then reject locally. | ||
| writeSSE({ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: id } }); | ||
| settle.reject(opts!.signal!.reason instanceof Error ? opts!.signal!.reason : new Error(String(opts!.signal!.reason))); |
There was a problem hiding this comment.
🟡 nit: onAbort sends notifications/cancelled with only { requestId: id }, dropping the abort reason even though opts!.signal!.reason is read on the very next line to build the local rejection. The connected-transport path (StreamDriver.request, streamDriver.ts:187-188) includes reason: String(reason) on the same notification — adding reason: String(opts!.signal!.reason) here would give the client the same diagnostic context (e.g. "Client closed SSE stream") and keep the two env.send providers in parity. Spec-optional, so non-blocking.
Extended reasoning...
What the gap is
BackchannelCompat.makeEnvSend's onAbort handler (backchannelCompat.ts:56-59) writes:
writeSSE({ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: id } });
settle.reject(opts!.signal!.reason instanceof Error ? opts!.signal!.reason : new Error(String(opts!.signal!.reason)));The wire notification carries only requestId. The abort reason is in scope — it's dereferenced on the immediately following line to construct the local rejection — but it never reaches the client.
The sibling path includes it
BackchannelCompat is one of two env.send providers. The other, StreamDriver.request (packages/core/src/shared/streamDriver.ts:184-191), handles the same abort-→-cancel case for the connected-transport path and sends:
{ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: messageId, reason: String(reason) } }So a server-initiated request cancelled via StreamDriver tells the client why; the same request cancelled via BackchannelCompat does not. Same protocol message, same semantic event, two different payloads depending on which transport adapter the server is mounted through.
Why nothing prevents it
CancelledNotification.params.reason is z.string().optional() in the spec schema, so the message validates either way. There's no shared helper that builds this notification — each path constructs the literal independently — so nothing forces them to agree on shape.
Step-by-step
- Server is mounted via
handleHttp(mcp, { session, backchannel }); a tool handler callsctx.mcpReq.elicitInput(params, { signal }). ctx.mcpReq.sendresolves tobackchannel.makeEnvSend(sessionId, writeSSE). The elicitation request is written to the POST's SSE stream with id0;signal.addEventListener('abort', onAbort)is registered.- The inbound request that spawned this handler is cancelled (e.g. the client disconnects and
shttpHandler'scancel()runsctrl.abort(new Error('Client closed SSE stream')), which propagates to the handler'sctx.mcpReq.signalif the handler wired it through). onAbortfires. It writes{ method: 'notifications/cancelled', params: { requestId: 0 } }to the SSE stream — noreasonfield. Then it rejects the local promise withError('Client closed SSE stream').- The client receives a bare cancellation. It knows request
0was cancelled but not whether the server timed out, the user aborted, or the stream broke. Had the same flow gone throughStreamDriver, the client would have receivedreason: "Error: Client closed SSE stream".
Impact
Not a protocol violation — reason is optional — so this is a consistency/diagnostics nit, not a correctness bug. The client loses human-readable cancel context on the handleHttp backchannel path only, and the two env.send implementations diverge on wire shape for no stated reason.
Fix
One-token addition:
writeSSE({ jsonrpc: '2.0', method: 'notifications/cancelled', params: { requestId: id, reason: String(opts!.signal!.reason) } });This matches StreamDriver exactly and reuses the value already being read on the next line.
| const timeoutMs = opts?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC; | ||
| timer = setTimeout( | ||
| () => settle.reject(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout: timeoutMs })), | ||
| timeoutMs | ||
| ); | ||
|
|
||
| opts?.signal?.addEventListener('abort', onAbort, { once: true }); | ||
|
|
||
| const wire: JSONRPCRequest = { jsonrpc: '2.0', id, method: req.method, params: req.params }; | ||
| if (!writeSSE(wire)) { |
There was a problem hiding this comment.
🟡 nit: makeEnvSend reads only opts.signal and opts.timeout, so onprogress / resetTimeoutOnProgress / maxTotalTimeout are silently dropped on this path — the same ctx.mcpReq.requestSampling(params, { onprogress, resetTimeoutOnProgress: true }) call that gets progress callbacks and timeout extension via StreamDriver.request on the connect(transport) path gets neither here. Wiring progress fully would require routing inbound notifications/progress through the backchannel (out of scope for this shim), so consider just adding a sentence to the JSDoc next to the existing "not persisted to EventStore" caveat noting these options aren't honoured on the legacy backchannel path.
Extended reasoning...
What the gap is
RequestOptions exposes onprogress, resetTimeoutOnProgress, and maxTotalTimeout (context.ts:70,89,96), and the env.send contract type is (Request, RequestOptions?) => Promise<Result> (context.ts:250), so callers see the full RequestOptions surface on ctx.mcpReq.send. There are two env.send providers:
StreamDriver.request(theconnect(transport)path) honours all three: whenopts.onprogressis set it injectsparams._meta.progressTokenand registers a progress handler (streamDriver.ts:172-178), and it wiresmaxTotalTimeout/resetTimeoutOnProgressinto the timeout machinery (streamDriver.ts:217-219).BackchannelCompat.makeEnvSend(this PR) reads onlyopts.signal(lines 45, 59, 65, 85) andopts.timeout(line 79). The wire request at line 87 is built as{jsonrpc, id, method, params: req.params}verbatim — no_meta.progressTokenis added — and no progress-handler map exists.
Why the dispatcher doesn't cover this
One might expect the shared ctx.mcpReq.send wrapper in dispatcher.ts to handle progress, but it doesn't: the wrapper at dispatcher.ts:168-185 just forwards {...options, signal} to env.send. Setting _meta.progressToken and registering progress handlers is the env.send sink's responsibility — StreamDriver.request does it, makeEnvSend does not. So the asymmetry is real and lives at exactly this layer.
Step-by-step proof
- A tool handler running under
handleHttp+{session, backchannel}callsctx.mcpReq.requestSampling(params, { onprogress: cb, resetTimeoutOnProgress: true })for a long-running LLM sampling call. Server.buildContext→_createMessageVia(ctx.mcpReq.send, params, sendOpts(options))(server.ts:170) —sendOptsspreads the caller's options, soonprogressandresetTimeoutOnProgressare still present.ctx.mcpReq.send(dispatcher.ts) callsenv.send(r, {...options, signal})— still present.env.sendisbackchannel.makeEnvSend(sessionId, writeSSE)(shttpHandler.ts:391). InsidemakeEnvSend:opts.onprogressis never read → no_meta.progressTokenon the wire request → the client never knows to sendnotifications/progress.- Even if the client did send progress,
shttpHandlerhas no path that routes inboundnotifications/progressto the backchannel. opts.resetTimeoutOnProgress/opts.maxTotalTimeoutare never read → the singlesetTimeoutat line 80 fires atopts.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSECregardless.
- Result:
cbis never invoked, and the request times out at the default 60s even though the same call on theconnect(transport)path would have extended on each progress notification. No error is raised about the dropped options.
Impact and why it's a nit
The behavioural divergence is real but narrow: server→client sampling-with-progress over the legacy SSE backchannel is an uncommon path, and before this PR ctx.mcpReq.send on handleHttp rejected with NotConnected entirely, so this is strictly an improvement. The class JSDoc already frames BackchannelCompat as "one removable file once MRTR (SEP-2322) is the protocol floor", and implementing progress properly would require shttpHandler to route inbound notifications/progress POSTs to a per-backchannel handler map plus timeout-reset machinery — substantial scope creep for a temporary compat shim, and counter to the repo's Minimalism principle.
Suggested fix
Add one sentence to the makeEnvSend JSDoc, alongside the existing "Backchannel writes are not persisted to the EventStore…" caveat, e.g.:
RequestOptions.onprogress,resetTimeoutOnProgress, andmaxTotalTimeoutare not honoured on this path; use a fixedtimeoutfor long-running requests.
That documents the limitation for users of this public class without expanding the shim's scope.
felixweinberger
force-pushed
the
fweinberger/r3-handlehttp
branch
from
b0d286d to
f15bcb8
Compare
felixweinberger
force-pushed
the
fweinberger/r4-backchannel
branch
from
2358111 to
8493aac
Compare
|
@claude review |
|
|
||
| test('per-session correlation: same id on different sessions does not collide', async () => { | ||
| const sendA = bc.makeEnvSend('A', writeSSE); | ||
| const sendB = bc.makeEnvSend('B', writeSSE); | ||
| const pA = sendA({ method: 'x' }); | ||
| const pB = sendB({ method: 'x' }); | ||
| const wA = written[0] as JSONRPCRequest; | ||
| const wB = written[1] as JSONRPCRequest; | ||
|
|
||
| bc.handleResponse('B', { jsonrpc: '2.0', id: wB.id, result: { from: 'B' } }); | ||
| await expect(pB).resolves.toEqual({ from: 'B' }); | ||
| bc.handleResponse('A', { jsonrpc: '2.0', id: wA.id, result: { from: 'A' } }); | ||
| await expect(pA).resolves.toEqual({ from: 'A' }); |
There was a problem hiding this comment.
🟡 nit: the test name says "same id on different sessions does not collide", but _nextId is an instance-wide counter (backchannelCompat.ts:27), so sendA mints id=0 and sendB mints id=1 — the ids are never the same. If _pending were flattened to a single Map<number, settle> with no session dimension this test would still pass, so it doesn't actually cover per-session keying. Adding expect(bc.handleResponse('A', { jsonrpc: '2.0', id: wB.id, result: {} })).toBe(false) (response for B's id arriving on session A must not be claimed) would make the test fail if session keying were ever removed — or just rename the test to reflect what it checks.
Extended reasoning...
What the test claims vs. what it does
The test at backchannelCompat.test.ts:51-63 is titled 'per-session correlation: same id on different sessions does not collide'. The implication is that two pending requests share a numeric request id and the per-session map keeps them apart. But BackchannelCompat._nextId is a single instance-level counter:
private _nextId = 0;
// ...
const id = this._nextId++;It is incremented inside makeEnvSend's returned closure regardless of which sessionId the closure was bound to. So in the test, sendA({method:'x'}) mints id=0 and sendB({method:'x'}) mints id=1. wA.id !== wB.id — the "same id on different sessions" scenario the title describes never occurs.
Why this is false coverage
The reason _pending is Map<string, Map<number, settle>> rather than Map<number, settle> is so that a response POSTed back on session B cannot resolve a request that session A is awaiting. This test does not exercise that property. Counterfactual: imagine _pending were flattened to a single Map<number, settle> and handleResponse ignored its sessionId argument entirely. Then:
sendA({method:'x'})→pending.set(0, settleA);sendB({method:'x'})→pending.set(1, settleB).bc.handleResponse('B', {id: wB.id /* 1 */, result:{from:'B'}})→ findssettleB, resolvespB. ✅bc.handleResponse('A', {id: wA.id /* 0 */, result:{from:'A'}})→ findssettleA, resolvespA. ✅
Both assertions pass. The earlier test at line 48-49 (handleResponse('unknown', {id:99}) === false) doesn't catch it either since id 99 was never minted. So nothing in this file would fail if the per-session dimension of _pending were removed.
Step-by-step proof on the actual code
bc = new BackchannelCompat()→_nextId = 0.sendA = bc.makeEnvSend('A', writeSSE);sendB = bc.makeEnvSend('B', writeSSE). Both closures capture the samethis.pA = sendA({method:'x'})→ executesconst id = this._nextId++→id = 0,_nextId = 1.written[0].id === 0.pB = sendB({method:'x'})→id = 1,_nextId = 2.written[1].id === 1.wA.id = 0,wB.id = 1— distinct. The test then resolves each by its own id on its own session, which would work with or without session keying.
Impact
Production code is correct — this is purely test-quality. The test gives false confidence that per-session isolation is covered: a future refactor that drops the outer Map<string, …> (or that accidentally looks up across sessions) would not be caught here. Given _nextId is instance-global, the literal scenario in the test name cannot even occur within a single BackchannelCompat instance, so the name is misleading on its face.
Suggested fix
Either rename the test to what it actually verifies (e.g. "responses route to the correct session"), or — better — add one assertion that would fail under a flat map:
// Response for B's id arriving on session A must not be claimed.
expect(bc.handleResponse('A', { jsonrpc: '2.0', id: wB.id, result: {} })).toBe(false);placed before the handleResponse('B', …) call. With a flat Map<number, settle> this would return true (and resolve pB with the wrong payload), so the test would actually guard the invariant its name advertises.
….mcpReq.send BackchannelCompat: opt-in 2025-11 server-to-client backchannel for handleHttp. A handler's elicitInput/requestSampling writes onto the open POST's SSE stream and the client's POSTed-back response resolves the await. Server.buildContext factors elicitInput/requestSampling into _createMessageVia/_elicitInputVia so instance methods and ctx.mcpReq share the same validation and capability check (capability check reads instance _clientCapabilities; per-request flow in F1).
…check (v1 parity; was gated on enforceStrictCapabilities)
felixweinberger
force-pushed
the
fweinberger/r4-backchannel
branch
from
8493aac to
716039a
Compare
|
@claude review |
1 participant
Adds
BackchannelCompat— the legacy server→client request channel forhandleHttp(writes elicitation/sampling onto the open POST's SSE stream and correlates the client's POSTed-back response).Server.buildContextfactorselicitInput/requestSamplingthrough_createMessageVia/_elicitInputViaso the instance methods and thectx.mcpReq.sendpath share the same validation/capability checks.Motivation and Context
Completes
handleHttp's legacy-client story: a pre-2026-06 client connecting to ahandleHttpserver can answer elicitation/sampling. The_*Viafactor keeps validation in one place regardless of which path (createMessage()vsctx.mcpReq.requestSampling()) is used.How Has This Been Tested?
Dual conformance both 40/40, client conformance 289/289. New
backchannelCompat.test.ts+ a real elicitation round-trip viatools/call test_elicitation.Breaking Changes
None.
Types of changes
Checklist
Additional context
R4 of decomposition; depends on R0–R3.
_*Viareads the instance_clientCapabilities(same asmain); F1 makes it per-request via_meta. Review guide: https://gist.github.com/felixweinberger/5a48e0f14d5aced39ed6a91b61940711.