fix(client): reconnect backends automatically after they lose their session - #414
Open
Quentin-M wants to merge 2 commits into
Open
fix(client): reconnect backends automatically after they lose their session#414Quentin-M wants to merge 2 commits into
Quentin-M wants to merge 2 commits into
Conversation
…ession When a downstream MCP server restarts, its in-memory Streamable HTTP / SSE session store resets, invalidating any session ID 1MCP was holding for it. The client's onerror handler only logged the resulting error and never recovered, so every subsequent request to that backend — including tools/list and tools/call — kept failing with "Session not found" until the whole 1MCP process was restarted by hand. onerror now detects this error class and reconnects the backend through a freshly recreated transport with the stale sessionId dropped, so it performs a full initialize handshake and gets a new session instead of replaying the dead one. TransportRecreator gains an opt-in `preserveSessionId: false` mode (and a `recreateForSessionLoss` convenience wrapper) for this; OAuth-retry recreation is unaffected and keeps preserving the session ID by default.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClientManager now detects lost Streamable HTTP/SSE sessions, recreates transports without stale session IDs, handles recreation failures, and reconnects active clients. Tests cover supported and unrelated errors, transport replacement, connected status, and unsupported transports. ChangesSession loss recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientManager
participant TransportRecreator
Client->>ClientManager: onerror(session lost)
ClientManager->>TransportRecreator: recreateForSessionLoss(transport, serverName)
TransportRecreator-->>ClientManager: fresh session-less transport
ClientManager->>ClientManager: createSingleClient(fresh transport)
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/client/clientManager.test.ts`:
- Around line 381-405: The SSE recovery test should exercise the actual SSE
transport and verify successful reconnection, not merely invocation of
recreateForSessionLoss. In the test around createSingleClient, construct the
fixture with SSEClientTransport, then await the transport replacement and assert
the client reaches ClientStatus.Connected while preserving the existing
session-loss error trigger.
In `@src/core/client/clientManager.ts`:
- Around line 205-208: Restrict the session-loss recovery flow in the client
manager around recreateForSessionLoss to HTTP/SSE transports before invoking it,
so unsupported AuthProviderTransport instances cannot throw from client.onerror;
alternatively catch recreation failures within that handler before
createSingleClient is scheduled. Add a regression test covering a non-HTTP
transport and confirming the error is safely handled without escaping the client
error path.
In `@src/core/client/transportRecreator.test.ts`:
- Around line 64-69: Replace manually constructed transport fixtures with the
established factories from test/unit-utils/MockFactories.ts. Update the stale
Streamable HTTP and session-loss fixtures in
src/core/client/transportRecreator.test.ts lines 64-69 and 79-84, and the
original/fresh, SSE recovery, and unrelated-error fixtures in
src/core/client/clientManager.test.ts lines 339-349, 384-388, and 410-414,
preserving each fixture’s required transport type and state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 050c8294-e73a-4a26-83a7-a8e392e0796b
📒 Files selected for processing (4)
src/core/client/clientManager.test.tssrc/core/client/clientManager.tssrc/core/client/transportRecreator.test.tssrc/core/client/transportRecreator.ts
- Guard recoverFromSessionLoss's transport recreation with a try/catch: recreateForSessionLoss throws for non-HTTP/SSE transports, and that throw was previously outside the createSingleClient(...).catch() path, so it could escape client.onerror uncaught for any backend whose transport isn't HTTP/SSE. Added a regression test with a stdio-style transport. - Fix the SSE recovery test: it was prototyping its fixture as StreamableHTTPClientTransport instead of SSEClientTransport, and only asserted that recreateForSessionLoss was invoked rather than exercising the real SSE recreation path and confirming the client actually reaches ClientStatus.Connected on a replaced transport.
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.
Problem
When a downstream MCP server restarts, its in-memory Streamable HTTP / SSE session store resets, invalidating any session ID 1MCP was holding for it.
ClientManager'sclient.onerrorhandler only logs the error:It never recovers. Every subsequent request to that backend —
tools/listduring capability aggregation andtools/call— keeps failing with the server's "Session not found" response until the whole 1MCP process is restarted by hand. In practice this means a single backend restart silently breaks that backend for every client of the proxy until someone notices and bounces 1MCP.Reproduced this against a real deployment (1MCP fronting several backends, one Streamable HTTP, one SSE): restarting a backend container reliably produced repeating
Client <name> error: ... "Session not found"(Streamable HTTP) /... Could not find session ID '...'(SSE) log lines, with the backend's tools absent fromtools/listand its tool calls failing, indefinitely, until 1MCP itself was restarted.Fix
onerrornow recognizes this error class (isSessionLostError, matching both message shapes above) and reconnects the backend through a freshly recreated transport with the stalesessionIddropped, so the new transport performs a fullinitializehandshake and is issued a new session instead of replaying the dead one.TransportRecreatoralready hadrecreateHttpTransport, used for OAuth-retry recreation, which always carries the oldsessionIdforward (correct there — the session itself is still valid, only auth needs refreshing). This PR adds an opt-in{ preserveSessionId: false }option plus arecreateForSessionLossconvenience wrapper for the session-loss case, without changing the existing OAuth-retry behavior/callers.The existing
client.client !== erroredClientguard pattern (already used inonclose) prevents a stale/superseded client instance from triggering a duplicate recovery, andcreateSingleClient's existing per-name semaphore dedupes any concurrent recovery attempts.Testing
transportRecreator.test.tscoveringpreserveSessionId: falseandrecreateForSessionLoss.clientManager.test.tscovering: recovery on the Streamable HTTP "Session not found" message, recovery on the SSE "Could not find session ID" message, and a negative case ensuring unrelatedonerrorerrors don't trigger recovery.pnpm test:unit(viavitest run) — full suite passes except 4 pre-existing failures unrelated to this change (verified identical onmainwithout this diff): tworelease-notes-range.test.tsfailures fromgit tagneeding a configured commit identity in this sandbox, and twocommonFormatters.test.tsdate-formatting failures that are timezone-dependent.tsc --noEmitandeslintclean.Summary by CodeRabbit