Repository navigation
fix(mcp): serve the facade stateless so a stale session never 404s - #1578
Conversation
The facade used rmcp's LocalSessionManager with default config, which retires a session after 5 idle minutes (and all of them on restart). The client's next call then got `404 Session not found`. Claude Code recovers by retrying the call on a fresh session, but its own background reconnect closes that connection about a second later: the retried call runs downstream (a write lands) while the agent is told `MCP server "oab-facade" session expired`. Retrying by hand then duplicates non-idempotent writes. Nothing in the facade needs a protocol-level session: identity is the per-request `Authorization` bearer (rmcp still injects the HTTP Parts in stateless mode), the two tools are plain request/response, and it never pushes notifications or sends server->client requests. Serve it stateless (NeverSessionManager + stateful_mode = false): no Mcp-Session-Id is issued, a stale one is ignored instead of 404'd, GET/DELETE return 405. This also matches the MCP 2026-07-28 revision, which removes protocol-level sessions. - e2e session-token test: drop the initialize -> session-id step; a bare tools/call with or without the bearer is the whole exchange now. - New regression test: initialize issues no session id, a stale session id is still served (200, not 404), GET is 405. - docs/oab-mcp-facade.md: document the stateless transport and why. Agent-Bot: alpha Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- F1: gmail adapter doc no longer claims to mirror the facade transport; it states that gmail is still stateful (same idle/restart 404 exposure) and what must be shared before it can go stateless. Also correct "one instance per serve process" for HTTP mode. - F2: scope the 2026-07-28 claim to the removal of protocol-level sessions (the facade still speaks the 2025 initialize handshake), in the build_router doc and docs/oab-mcp-facade.md. - F3: ADR 6.2 gains a "Stateless transport" bullet (gmail remains stateful); the operator guide states that a stale Mcp-Session-Id is ignored rather than rejected, and the Trust model section now says "agent session" for the bearer-backed session so it is not confused with the MCP protocol session; the e2e test doc says "session-token (bearer) mechanism". - F4: the regression test checks the initialize body (result + protocolVersion, no error) and asserts DELETE /mcp -> 405 alongside GET, both with Allow: POST. - From the earlier non-blocking notes: document that future facade tools must not rely on server->client requests or the GET SSE stream, and note why NeverSessionManager is intentionally redundant with stateful_mode = false. Agent-Bot: alpha Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
|
Thanks for the review — round 1 is addressed in 174e566:
|
This comment has been minimized.
This comment has been minimized.
- F1: finish the "session" disambiguation: ADR 6.2 Security posture and the
operator guide's Trust model now say "agent session" / "per-agent-session
bearer" / "agent-session-free" wherever they mean the bearer-backed session.
- Non-blocking notes taken:
- move the misplaced serve_http_with description back above serve_http_with
(it had run into the build_router rustdoc);
- trim the build_router rustdoc to the architectural reason and point to the
operator guide for the client-side failure narrative (fixes the "and all of
them on restart" wording on the way);
- extend the future-tool constraint: a tool that needs client cancellation
must wire the request cancellation token, since stateless mode drops
notifications/cancelled (code doc + guide);
- regression test: a notification-only POST is 202, and the tools/call half
asserts "id":1 and no "isError":true.
Agent-Bot: alpha
Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
…ion test Peer review of the round-2 changes: - The future-tool constraint said a tool needing client cancellation "must wire RequestContext.ct". That cannot work stateless: rmcp drops notifications/cancelled on the stateless POST path and each POST runs its own one-shot service, so the token never fires on a client cancel. State that cancellation is unavailable and that needing it (like server->client requests or the GET stream) means revisiting the choice (code doc + guide). - Regression test: assert "isError":false positively instead of the absence of "isError":true; bound the SSE body reads with a 10 s timeout so a stream that never ends fails instead of hanging; doc comment lists all checks. Agent-Bot: alpha Co-authored-by: rockexe0000 <rockexe0000@gmail.com>
|
Round 2 addressed in e8b1575 + c2741f3:
|
|
Note LGTM ✅ - The round-2 wording finding is fixed at every flagged location, the round-3 delta is docs, rustdoc, and test hardening only, and no runtime, auth, or performance issue was found. Two merge preconditions remain: upstream CI must run green at this head, and a maintainer must run one kiro-cli smoke call against the facade. What This PR DoesThe OAB MCP Facade served Streamable HTTP with rmcp's Round 3 reviews How It Works
Findings
Finding Details🟢 F1: Wording disambiguation completeAll five locations requested in round 2 were changed:
🟢 F2: Rustdoc placement and de-duplicationThe 🟢 F3: Cancellation constraint verifiedIn rmcp 1.8.0 🟢 F4: Regression test hardening
🟢 F5: Auth boundary unchanged
🟢 F6: Operational profile
🟢 F7: Gmail kept stateful with an accurate prerequisite
Merge preconditions (not code findings)
Non-blocking suggestions
Follow-ups (out of scope for this PR)
5. Three Reasons We Might Not Need This PR
Net: the PR is necessary, minimal, and well-documented. It can merge once the two preconditions above are met. Addressing Earlier Feedbackchaodu-agent (round 2,
|
What problem does this solve?
The OAB MCP Facade serves Streamable HTTP with rmcp's
LocalSessionManager::default(), whoseSessionConfig::DEFAULT_KEEP_ALIVEretires a session after 5 idle minutes (and every session on restart). The first facade call after an idle gap therefore carries a retiredMcp-Session-Idand gets404 Not Found: Session not found— that request is not executed.Claude Code (observed on 2.1.296) then runs two recovery paths at once: (a) its transport closes and starts a background auto-reconnect, and (b) the tool-call layer retries the call once on a fresh session. The retry does run downstream, but (a)'s next reconnect attempt (scheduled ~1000 ms later) closes the connection the retry is using, so the in-flight call fails with
Connection closed, is surfaced asMcpSessionExpiredError, and the agent seesMCP server "oab-facade" session expired.Net effect: any facade call whose downstream work takes longer than ~1 s (most GitHub writes) lands, but is reported as failed. Agents that retry then duplicate non-idempotent writes (e.g.
add_issue_comment), or wrongly conclude nothing happened. We hit it on 4/4 first-calls-after-idle in one deployment (idle gaps 43 min – 19 h; every error at 1.05–1.07 s after the call started).Closes # (no upstream issue — searched issues/PRs for session expired / Session not found / stateless / Mcp-Session-Id / facade idle; none found)
Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1564158355285745737
Review Contract
Goal
A facade call must never be answered with
404 Session not foundbecause of idle time or a facade restart, so MCP clients never enter their session-recovery path for it.Non-goals
server/discover, per-request_metacapabilities) — that depends on rmcp support.Accepted Residual Risks
Mcp-Session-Idfrominitialize;GET/DELETE /mcp→405) are all explicitly allowed by the Streamable HTTP spec, so compliant clients must accept them. A client that requires a session id or treatsGET 405as fatal would break. Verified with Claude Code (against a mock server) and end-to-end with Antigravity CLIagy1.2.13 (real facade, PR preview image); rmcp-based clients accept it by default (allow_stateless: true,GET 405→ServerDoesNotSupportSse); Kiro and others should get a smoke test (connect + one call).Last-Event-IDresume). On a loopback listener this only happens on process restart, where stateful sessions were lost anyway; the 2026-07-28 spec removes resumability too.notifications/cancelledis now dropped by rmcp instead of cancelling the request's token. No behavior change in practice: the facade'scall_toolnever observed that token, so a client cancel did not stop the downstream call before either; per-server timeouts in the runtime still apply.Acceptance Criteria
cargo clippy --workspace -- -D warningsandcargo test --workspacepass.http_stateless_issues_no_session_and_never_404s_a_stale_one:initializereturns noMcp-Session-Id; atools/callcarrying a stale session id returns 200 with a result;GET /mcpreturns 405.http_e2e_session_token_gates_source_visibilitystill proves the per-request bearer gates session-bound sources (now without an initialize/session step).Follow-ups
server/discover,subscriptions/listen) once rmcp supports it.At a Glance
Prior Art & Industry Research
Not applicable for OpenClaw / Hermes Agent — this is a transport-mode bug fix inside openab's own facade, not an architectural change. Relevant references instead:
Mcp-Session-Idheader from the Streamable HTTP transport" — https://modelcontextprotocol.io/specification/2026-07-28/changelog405toGET, and may return405toDELETE.StreamableHttpServicedocs:NeverSessionManager"disables sessions entirely (stateless mode)";SessionConfig::DEFAULT_KEEP_ALIVE = 300s.docs/adr/oab-mcp-adapter.md§6.2 (identity is the per-request bearer from feat(acp): browser control via MCP-over-ACP + stdio bridge (Phase 2) #1447/feat(facade): session-aware in-process capability sources #1454, not the MCP session);docs/mcp-over-acp-tunnel-contract.md§4 anddocs/adr/acp-server-websocket-reverse-mcp.mdalready note the spec is retiringMcp-Session-Id.Proposed Solution
build_routernow buildsStreamableHttpServicewithNeverSessionManagerandStreamableHttpServerConfig::default().with_stateful_mode(false). Responses stay SSE (json_responseunchanged) to keep the change to session behavior only. rmcp's stateless path still injects the HTTPPartsinto request extensions, sosession_ctx_from_extensions(theAuthorization: Beareridentity) works unchanged. No config keys, public function signatures, or tool contracts change.Tests: the e2e session-token test drops the initialize → session-id step (a bare
tools/callis the whole exchange); a POST helpermcp_postis shared; a new regression test covers no-session-issued / stale-id-served / GET 405.docs/oab-mcp-facade.mdgains a "Stateless transport" section.Why this approach?
The facade needs nothing a session provides: identity is per-request, the two tools are plain request/response, it advertises no
listChanged, sends no progress, and never issues server→client requests (sampling/elicitation/roots). The session only added a state that can expire. Stateless removes the trigger for both idle expiry and restarts, in a two-line change, and aligns with where the spec is going.Alternatives Considered
SessionConfig.keep_alive(e.g. 24 h, or configurable): same size, but still 404s after a facade restart or a longer idle gap, and leaks sessions of clients that exit withoutDELETE.SessionStore(transparent restore): rmcp deletes a stored session only on a clientDELETE, not on idle expiry, so with a store an idle-retired session id is restored in place (rmcp replaysinitialize) and the request is served instead of 404'd. This is the zero-wire-change option: session ids, the standalone GET stream andDELETEkeep working, so even a client that requiresMcp-Session-Idis unaffected. Costs: a smallSessionStoreimpl with a size/TTL bound (clients often exit withoutDELETE), and an in-memory store does not survive a facade restart (fine when the facade and the CLI restart together, as in a pod; a standalone facade would need a persistent store). If maintainers prefer no wire change, this PR can switch to this approach.Validation
cargo check/cargo clippy --workspace -- -D warnings(+ unified) /cargo test --workspace— green in this PR's CI (CI,CI openab-agent); not run locally (the authoring environment has no Rust toolchain). Both Docker Smoke Test workflows are green as well.Parts, returns 405 for GET/DELETE;NeverSessionManager+with_stateful_modeexist and are public; on the stateful path anotifications/cancelledonly cancels the request token (the handler is not aborted).session expired(1043 / 1047 ms).server/discoverprobe got -32601 and the client fell back to the 2025 protocol; GET 405 handled silently.agy1.2.13) on this PR’s preview image (pr1-antigravity) against the real facade + downstream: discovery and a read call OK; after an 11-minute idle gap a ~3 s call returned normally — nosession expired, no MCP reconnect.🤖 Generated with Claude Code