fix(session): return encodable JSON-RPC errors when init/2 fails - #211
Conversation
📝 WalkthroughProblemSession initialization and recovery failures could return raw tuples that failed JSON encoding, causing HTTP 500 responses. SolutionAdded RationaleEnsure all failures produce valid JSON-RPC responses while preserving structured MCP errors. WalkthroughAdds Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
✨ 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. 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 870d8d54-fe87-4fde-b016-c1b12b44f7cb
📒 Files selected for processing (7)
lib/anubis/mcp/error.exlib/anubis/server/session.exlib/anubis/server/transport/streamable_http/plug.extest/anubis/mcp/error_test.exstest/anubis/server/session_expiry_test.exstest/anubis/server/transport/streamable_http/plug_test.exstest/support/stub_session_recovery_server.ex
Address CodeRabbit review on PR zoedsoupe#211.
|
Addressed the CodeRabbit doc feedback in 0091443 — added |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/anubis/mcp/error.ex (1)
305-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: handle bare atoms in
stringify_reason/1for cleaner error messages.When
init/2returns{:error, :timeout}, the current catch-all produces"error: :timeout"(note the leading colon frominspect/1). Adding an atom clause would yield the cleaner"error: timeout".✨ Optional refactor
defp stringify_reason(reason) when is_binary(reason), do: reason +defp stringify_reason(reason) when is_atom(reason), do: Atom.to_string(reason) + defp stringify_reason({tag, inner}) when is_atom(tag) do "#{tag}: #{stringify_reason(inner)}" end
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68d0657c-282a-47b3-b90a-e2ae5d789ed4
📒 Files selected for processing (1)
lib/anubis/mcp/error.ex
|
could you please address the test errors? the doctest one seems to be a escape issue on the second one i think its related to the merge i did with |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07516de1-daaa-4f22-8130-4b8569c75700
📒 Files selected for processing (5)
lib/anubis/server/session.exlib/anubis/server/transport/streamable_http/plug.extest/anubis/server/session_expiry_test.exstest/anubis/server/transport/streamable_http/plug_test.exstest/support/stub_session_recovery_server.ex
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07516de1-daaa-4f22-8130-4b8569c75700
📒 Files selected for processing (5)
lib/anubis/server/session.exlib/anubis/server/transport/streamable_http/plug.extest/anubis/server/session_expiry_test.exstest/anubis/server/transport/streamable_http/plug_test.exstest/support/stub_session_recovery_server.ex
🛑 Comments failed to post (2)
lib/anubis/server/session.ex (2)
142-146: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent timeouts during session recovery by threading the configured timeout (P2). 😎
GenServer.call/2defaults to a 5-second timeout. Becauseauto_initializetriggers the server module'sinit/2callback (which might perform heavier logic or DB I/O), it can easily exceed this limit, causing a crash that bubbles up as{:session_unavailable, {:timeout, ...}}. Exposing and passing the plug's configured timeout resolves this.
lib/anubis/server/session.ex#L142-L146: Add an optionaltimeout \\ 5000parameter toauto_initializeand pass it toGenServer.call/3. (Merge the 1-arity and 2-arity function definitions to avoid default argument conflicts).lib/anubis/server/transport/streamable_http/plug.ex#L381-L384: Passopts.timeouttoSession.auto_initialize/3so the plug's request timeout (default 30s) is respected during auto-recovery.🛠️ Proposed fixes
lib/anubis/server/session.ex:- `@spec` auto_initialize(GenServer.server()) :: :ok | {:error, term()} - def auto_initialize(session), do: auto_initialize(session, nil) - - `@spec` auto_initialize(GenServer.server(), map() | nil) :: :ok | {:error, term()} - def auto_initialize(session, transport_context) do - GenServer.call(session, {:auto_initialize, transport_context}) + `@spec` auto_initialize(GenServer.server(), map() | nil, timeout()) :: :ok | {:error, term()} + def auto_initialize(session, transport_context \\ nil, timeout \\ 5000) do + GenServer.call(session, {:auto_initialize, transport_context}, timeout)
lib/anubis/server/transport/streamable_http/plug.ex:defp start_and_auto_initialize_session(opts, session_id, context) do case start_new_session(opts, session_id) do {:ok, pid} -> - case Session.auto_initialize(pid, context) do + case Session.auto_initialize(pid, context, opts.timeout) do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@spec` auto_initialize(GenServer.server(), map() | nil, timeout()) :: :ok | {:error, term()} def auto_initialize(session, transport_context \\ nil, timeout \\ 5000) do GenServer.call(session, {:auto_initialize, transport_context}, timeout)📍 Affects 2 files
lib/anubis/server/session.ex#L142-L146(this comment)lib/anubis/server/transport/streamable_http/plug.ex#L381-L384
501-504: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log unexpected linked process exits (P3). 😎
Silently ignoring
{:EXIT, pid, reason}can hide critical failures from your observability stack, such as an attached process crashing unexpectedly. Let's add a quick debug log so these ghosts don't haunt us later!🛠️ Proposed fix to log the exit
- def handle_info({:EXIT, _pid, _reason}, state) do + def handle_info({:EXIT, pid, reason}, state) do + Logging.server_event("linked_process_exited", %{pid: inspect(pid), reason: inspect(reason)}, level: :debug) {:noreply, state} end
Address CodeRabbit review on PR zoedsoupe#211.
4103f2e to
dddedc1
Compare
Normalize init/2 and session-recovery failures to %Anubis.MCP.Error{}
via Error.wrap_reason/1 instead of returning raw {:init_failed, reason}
tuples that crash JSON.encode! in the StreamableHTTP transport.
Add transport-side wrap_reason/1 fallback and regression tests covering
plain and structured init/2 rejections.
Fixes zoedsoupe#205
Address CodeRabbit review on PR zoedsoupe#211.
dddedc1 to
14a8bff
Compare
|
hey, i've messed with history and fixed for you. thanks for the contribution and sorry. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d24037e-5db8-4c8b-be09-4c589982b81c
📒 Files selected for processing (8)
lib/anubis/mcp/error.exlib/anubis/server.exlib/anubis/server/session.exlib/anubis/server/transport/streamable_http/plug.extest/anubis/mcp/error_test.exstest/anubis/server/session_expiry_test.exstest/anubis/server/transport/streamable_http/plug_test.exstest/support/stub_session_recovery_server.ex
| request = %{ | ||
| "jsonrpc" => "2.0", | ||
| "id" => "req-init-fail", | ||
| "method" => "tools/list", | ||
| "params" => %{} | ||
| } | ||
|
|
||
| body = JSON.encode!(request) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
P3 — Use test builders for message construction 😎
Both of these test payloads construct raw JSON-RPC maps and manually stringify them. As per coding guidelines, use Anubis.MCP.Builders and Message.encode_request/2 to keep test payloads consistent with the rest of the codebase (and the earlier tests in this exact file!).
test/anubis/server/transport/streamable_http/plug_test.exs#L642-L649: replace the raw map andJSON.encode!withbuild_request/2andMessage.encode_request/2.test/anubis/server/transport/streamable_http/plug_test.exs#L670-L677: apply the same builder pattern here.
♻️ Example Refactor (for L642-649)
- request = %{
- "jsonrpc" => "2.0",
- "id" => "req-init-fail",
- "method" => "tools/list",
- "params" => %{}
- }
-
- body = JSON.encode!(request)
+ request = build_request("tools/list", %{})
+ {:ok, body} = Message.encode_request(request, "req-init-fail")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| request = %{ | |
| "jsonrpc" => "2.0", | |
| "id" => "req-init-fail", | |
| "method" => "tools/list", | |
| "params" => %{} | |
| } | |
| body = JSON.encode!(request) | |
| request = build_request("tools/list", %{}) | |
| {:ok, body} = Message.encode_request(request, "req-init-fail") |
📍 Affects 1 file
test/anubis/server/transport/streamable_http/plug_test.exs#L642-L649(this comment)test/anubis/server/transport/streamable_http/plug_test.exs#L670-L677
Source: Coding guidelines
🚀 Want to release this? --- ## [1.9.0](v1.8.0...v1.9.0) (2026-07-16) ### Features * **streamable_http:** add spec resumability (Last-Event-ID replay) ([#216](#216)) ([78e33b4](78e33b4)) * support pre_initialized sessions for cross-pod restore ([#187](#187)) ([13be0d7](13be0d7)) ### Bug Fixes * **session:** return encodable JSON-RPC errors when init/2 fails ([#211](#211)) ([f9af7cc](f9af7cc)) * **streamable_http:** emit telemetry on SSE handler registration ([#217](#217)) ([4a4c528](4a4c528)) * **streamable_http:** restore session from store on notif/resp registry miss ([#221](#221)) ([d757b39](d757b39)) * **streamable_http:** return correct JSON-RPC error codes for parse failures ([#222](#222)) ([1867994](1867994)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
Summary
When
Server.init/2(or session recovery) rejects a session, normalize the failure to an encodable%Anubis.MCP.Error{}instead of returning raw{:init_failed, reason}/{:recovery_rejected, reason}tuples that crashJSON.encode!/1in the StreamableHTTP transport.Motivation
Fixes the HTTP 500 +
Protocol.UndefinedErrorreported in #205 when a server legitimately rejects a session ininit/2. Clients now receive a well-formed JSON-RPC error they can act on.Changes
Error.wrap_reason/1to pass through%Error{}and stringify other failure terms for JSON encodinginit/2andhandle_session_expired/2rejection paths inSessionError.wrap_reason/1in StreamableHTTP transport error handling (defense in depth)init/2rejections via HTTP and session auto-initTests
mix test test/anubis/mcp/error_test.exs test/anubis/server/session_expiry_test.exs test/anubis/server/transport/streamable_http/plug_test.exsAll 57 tests pass.
Notes
notifications/initializedpath still assumesinit/2returns{:ok, frame}— separate from the auto-init path covered here.Fixes #205