Repository navigation
fix: scope POST-with-SSE response to originating conn - #144
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches✨ Simplify code
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. Review rate limit: 0/1 reviews remaining, refill in 26 minutes and 20 seconds.Comment |
Merged
zoedsoupe
added a commit
that referenced
this pull request
May 8, 2026
## Problem `Anubis.Server.Session` runs tool handlers (and every other `module.handle_request/2`) inline inside `handle_call`, blocking the GenServer mailbox until the handler returns. Consequences: - `notifications/cancelled` cannot be processed while a slow tool runs — the spec's only abort mechanism is broken for long-running tools. - Sampling/roots/elicitation responses, `notifications/initialized`, `tools/list_changed`, and user-defined `handle_info`/`handle_cast` are all delayed by an in-flight tool. [duics raised this](#143 (comment)) on #143 after the routing fix in #144 landed: _"one long-running tool call would cause other parallel tool calls to time out since they run sequentially"_. duics later [confirmed Claude does not actually fire parallel tool calls per session](#143 (comment)), so true parallel execution is unmotivated for now — but the mailbox-blocking problem affects every Anubis user with a slow tool, regardless of client. ## Solution Run every `module.handle_request/2` in a `Task.Supervisor.async_nolink` task on the per-server `task_supervisor` (already in the supervision tree at [lib/anubis/server/supervisor.ex#L176](lib/anubis/server/supervisor.ex#L176), previously unused). Session keeps single-in-flight semantics via a `:queue`. Frame mutations stay single-writer (the Session GenServer applies them in `handle_info`). - `state.in_flight` tracks the running task (`ref`, `pid`, `request_id`, `from`, `started_at`, `method`). - `state.request_queue` holds `{request, ctx, from}` tuples while a task is in flight; drained on completion in arrival order. - `state.deferred_callbacks` holds `{:cast, msg}` / `{:info, msg}` for competing frame writers (sampling/roots/elicitation responses, non-cancellation client notifications, user `handle_cast`/`handle_info`); applied in arrival order after the in-flight task completes. - `notifications/cancelled` is _not_ deferred — it processes inline, calling `Task.Supervisor.terminate_child` on the in-flight task pid (or removing from the queue) and replying `Request cancelled` so the Plug doesn't time out. - `terminate/2` replies `internal_error` to in-flight + queued callers so Plug returns immediately on shutdown. - `[:anubis_mcp, :server, :tool_call]` (defined but unemitted previously) now emits via `:telemetry.span/3` around the handler. No public API change. `GenServer.call(session_pid, {:mcp_request, ...}, timeout)` from the Plug still receives `{:ok, encoded}` synchronously — just after the task completes instead of inline. ## Rationale `async_nolink` over `async`: tool crashes don't kill the Session; `{:DOWN, ref, ...}` arrives as a normal message and the queue drains. Single-in-flight queue over parallel exec: concurrent frame mutations would race, breaking the existing `Anubis.Server.Behaviour` contract. Per duics's empirical follow-up, no known client issues parallel `tools/call` per session today, so the multi-tool feature is deferred until a real consumer demands it. Per-server local `Task.Supervisor`: always co-located with the Session, so distribution-safe — pluggable Registry (`Registry.Local`, Horde, etc.) and pluggable session store (Redis at [lib/anubis/server/session/store/redis.ex](lib/anubis/server/session/store/redis.ex)) are unaffected. `to_serializable/1` continues to exclude transient fields.
zoedsoupe
added a commit
that referenced
this pull request
Jul 16, 2026
zoedsoupe
added a commit
that referenced
this pull request
Jul 16, 2026
## Problem `Anubis.Server.Session` runs tool handlers (and every other `module.handle_request/2`) inline inside `handle_call`, blocking the GenServer mailbox until the handler returns. Consequences: - `notifications/cancelled` cannot be processed while a slow tool runs — the spec's only abort mechanism is broken for long-running tools. - Sampling/roots/elicitation responses, `notifications/initialized`, `tools/list_changed`, and user-defined `handle_info`/`handle_cast` are all delayed by an in-flight tool. [duics raised this](#143 (comment)) on #143 after the routing fix in #144 landed: _"one long-running tool call would cause other parallel tool calls to time out since they run sequentially"_. duics later [confirmed Claude does not actually fire parallel tool calls per session](#143 (comment)), so true parallel execution is unmotivated for now — but the mailbox-blocking problem affects every Anubis user with a slow tool, regardless of client. ## Solution Run every `module.handle_request/2` in a `Task.Supervisor.async_nolink` task on the per-server `task_supervisor` (already in the supervision tree at [lib/anubis/server/supervisor.ex#L176](lib/anubis/server/supervisor.ex#L176), previously unused). Session keeps single-in-flight semantics via a `:queue`. Frame mutations stay single-writer (the Session GenServer applies them in `handle_info`). - `state.in_flight` tracks the running task (`ref`, `pid`, `request_id`, `from`, `started_at`, `method`). - `state.request_queue` holds `{request, ctx, from}` tuples while a task is in flight; drained on completion in arrival order. - `state.deferred_callbacks` holds `{:cast, msg}` / `{:info, msg}` for competing frame writers (sampling/roots/elicitation responses, non-cancellation client notifications, user `handle_cast`/`handle_info`); applied in arrival order after the in-flight task completes. - `notifications/cancelled` is _not_ deferred — it processes inline, calling `Task.Supervisor.terminate_child` on the in-flight task pid (or removing from the queue) and replying `Request cancelled` so the Plug doesn't time out. - `terminate/2` replies `internal_error` to in-flight + queued callers so Plug returns immediately on shutdown. - `[:anubis_mcp, :server, :tool_call]` (defined but unemitted previously) now emits via `:telemetry.span/3` around the handler. No public API change. `GenServer.call(session_pid, {:mcp_request, ...}, timeout)` from the Plug still receives `{:ok, encoded}` synchronously — just after the task completes instead of inline. ## Rationale `async_nolink` over `async`: tool crashes don't kill the Session; `{:DOWN, ref, ...}` arrives as a normal message and the queue drains. Single-in-flight queue over parallel exec: concurrent frame mutations would race, breaking the existing `Anubis.Server.Behaviour` contract. Per duics's empirical follow-up, no known client issues parallel `tools/call` per session today, so the multi-tool feature is deferred until a real consumer demands it. Per-server local `Task.Supervisor`: always co-located with the Session, so distribution-safe — pluggable Registry (`Registry.Local`, Horde, etc.) and pluggable session store (Redis at [lib/anubis/server/session/store/redis.ex](lib/anubis/server/session/store/redis.ex)) are unaffected. `to_serializable/1` continues to exclude transient fields.
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
Closes #143. Concurrent
tools/callPOSTs withAccept: text/event-streamon the same session bleed responses across HTTP connections. POST_B's response can be written on POST_A's chunked conn while POST_B's own conn returns an empty 202 ack. Reporter symptom: "sometimes you might get a result of the previous tool call instead of what you expected".Race is NOT in tool execution —
Anubis.Server.Sessionhandle_call({:mcp_request, ...})serializes correctly. Race is in response routing.Anubis.Server.Transport.StreamableHTTPkeepssse_handlers :: %{session_id => {pid, ref}}— single handler slot per session. The first POST-with-SSE registers its own pid into that slot; later POSTs reuse the same slot viaroute_sse_responseand forward their response binary to whoever is registered there.Solution
POST-initiated SSE streams are scoped to the POST's own connection, never multiplexed through the session-wide handler.
handle_sse_request/6now calls a newstream_response_on_conn/4that writes the response chunk on the currentPlug.Connand lets Plug finalize the chunked response.route_sse_response/4andestablish_sse_for_request/4.StreamableHTTP.sse_handlersmap remains for the GET-initiated server-push channel and broadcast notifications.parallel POST-with-SSE responses do not bleed across HTTP connectionsinplug_test.exs(adapted from duics' reproducer branch).Rationale
Per MCP 2025-06-18 Streamable HTTP, a POST that opts into SSE response gets its own stream on its own HTTP connection, scoped to that single request. The GET-initiated SSE channel is the session-wide server-push channel. These are distinct and the implementation now reflects that separation.
The user's intuition that wrapping each tool call in a
Taskwould be the wrong fix is correct: tool execution is already serialized by the session GenServer, tasking would not change response routing, and it would introduce state-mutation hazards across concurrent tools. Parallel tool execution remains an orthogonal feature for a separate PR.Full test suite: 711 tests, 0 failures. Credo + format clean.