fix(mcp): re-establish the bridge session instead of reusing a dead one#1881
Conversation
e502fe4 to
ffa38fb
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e502fe4f4e
ℹ️ 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".
| case .cannotConnectToHost, .cannotFindHost, .networkConnectionLost, .notConnectedToInternet: | ||
| return .unreachable(detail: urlError.localizedDescription) |
There was a problem hiding this comment.
Avoid replaying after mid-flight connection loss
When URLSession reports .networkConnectionLost, the request may already have reached TablePro and executed before the response connection dropped; this mapping turns it into .unreachable, which recoveryReason treats as recoverable and recoverAndReplay resends the original JSON-RPC message. For mutating MCP calls such as tools/call with execute_query or confirm_destructive_operation, that can run the same SQL twice after a lost response. Limit transparent replay to failures where no request could have been processed, or only replay known-idempotent methods.
Useful? React with 👍 / 👎.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Problem
In an agent session (Claude Code, Cursor), MCP tool calls stop working after the server sits idle for ~15 minutes, or after the MCP server is toggled off and on in Settings. Every call then fails until the user restarts the client. The screenshot in the report shows
execute_queryfailing, thenconnectfailing three times, which is the tell: it is a transport-level failure hitting every tool, not a database error.Root cause
TablePro's stdio bridge is the HTTP client for every agent, and it violates a MUST in the MCP Streamable HTTP spec:
MCPStreamableHttpClientTransportcached the session id once and never cleared it; on the server's 404 it converted the error to JSON-RPC and forwarded it to the agent, then reused the dead id on every later call. AndBridgeMainhardcodedserverInitiatedStream: false, so an idle bridge put zero traffic on the wire and the 900s server idle timeout was guaranteed to fire.The server side is spec-correct (404 + JSON-RPC
-32001) and is unchanged.No upstream client rescues this: Claude Code does not auto-reconnect stdio servers (documented), and the request to re-initialize on 404 was closed as not planned (claude-code#60949). The same bug shipped in DataGrip (IJPL-235013, fixed) and still affects Figma desktop.
What changed
The transport now owns its upstream session lifecycle:
initialize+notifications/initialized. On a 404 for a request carrying a session id, it single-flights a re-initialize (via the existingOnceTask) and replays the original request once under its original JSON-RPC id. The recovery handshake runs on a separate, non-yielding path with a bridge-internal id, so the agent never sees a duplicate response.ping(the server'sPingHandleralready refreshes the idle clock) keeps an attached session warm, so recovery is the cold path rather than a 15-minute drumbeat.MCP-Protocol-Versionis now sent on every post-initialize request, using the negotiated version.MCPAuthPolicy.clearSession, which was dead code leaking session approvals for the life of the app process.Concurrency is guarded with a session generation counter (the pattern
ConnectionAttemptRegistryuses): concurrent 404s collapse to one re-initialize, and the session id is only ever assigned from a server response so no request goes out without a header.Deliberately unchanged: the server's 404 (spec-correct) and the 900s idle timeout (with keepalive it correctly means "no client attached", and raising it only delays the wedge while leaking sessions longer).
Behavior note
A recovered session is genuinely a new session, so a connection set to "ask each time" re-prompts once after recovery. This is correct on security grounds but is a visible change.
Tests
40 MCP tests pass,
swiftlint --strictclean, both the app andmcp-servertargets build.MCPStreamableHttpClientTransportTests: transparent re-init + replay, concurrent-404 single-flight,notifications/initializedreplayed, recovery-failure surfaces an actionable error, credential refresh on 401/unreachable, keepalive stays invisible to the host, protocol-version header, DELETE on close.MCPBridgeIntegrationTests.testIdleSessionEvictionRecoversTransparently: drives the real server through a real idle eviction and asserts the follow-up call recovers with a new session and a full replayed handshake. This replaces the old test that asserted the broken behavior (a permanently dead session).MCPAuthPolicyTests:clearSessiondrops approvals, no-ops for unknown sessions, and re-prompts a previously approvedaskEachTimeconnection.Known issue (not from this change)
The recovery regression test was first written through the stdio-pipe harness and reliably killed the test host. Traced as far as the environment allows (
SWIFT_BACKTRACE=enable=no, crash reports suppressed): recovery writes nothing extra to the pipes, and the same recovery against the real server without pipes passes. The harnessshutdown()closes all four pipe file descriptors synchronously while its detached reader is still blocked inFileHandle.bytes; the extra round trips shift timing onto that race. It looks like a pre-existing harness teardown bug and is worth fixing separately. The regression test drives the real server directly instead, which is the stronger test.Docs
docs/external-api/mcp-clients.mdxupdated: the 404/401 troubleshooting entries no longer claim clients recover automatically, and a note explains that the bridge recovers on its own.