feat: add timeout for client/server -> transport calling option - #50
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis PR threads configurable per-operation timeouts through client and server flows. A default timeout constant and a parse option were added; client and server states gain a timeout field. Operation structs require a timeout supplied by callers. All transport send_message APIs were changed to accept options (including timeout) and forward opts[:timeout] into GenServer.call. Call sites across initialize, requests, notifications, sampling, and logging were updated to pass operation.timeout or state.timeout. Tests and test-support mocks/stubs were updated for the new send_message arity. Sequence Diagram(s)sequenceDiagram
participant C as Client.Base
participant S as Client.State
participant O as Operation
participant SV as Server.Base
participant T as Transport (SSE/STDIO/WS/HTTP)
participant G as GenServer/Network
C->>S: init(opts{timeout: T_default})
S-->>C: state{timeout: T_default}
C->>O: Operation.new(method, timeout: T_op)
O-->>C: operation{timeout: T_op}
C->>T: send_message(message, opts{timeout: O.timeout || S.timeout})
note right of T #E8F0FF: Transport.send_message/3 forwards\nopts[:timeout] into GenServer.call
T->>G: GenServer.call(..., timeout: provided_timeout)
alt success
G-->>T: response
T-->>C: :ok | {:ok, resp}
else timeout/error
G-->>T: {:error, :timeout} | {:error, reason}
T-->>C: {:error, reason}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (5)test/**/*.exs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ex,exs}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*_test.exs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
lib/anubis/client/**/*.ex📄 CodeRabbit inference engine (CLAUDE.md)
Files:
lib/**/*.ex📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-10-07T19:46:22.924ZApplied to files:
📚 Learning: 2025-10-07T19:46:22.924ZApplied to files:
📚 Learning: 2025-10-07T19:46:22.924ZApplied to files:
🧬 Code graph analysis (2)test/anubis/client/state_test.exs (2)
lib/anubis/client/base.ex (4)
🔇 Additional comments (13)
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
test/anubis/server/transport/stdio_test.exs (1)
26-42: Test updated correctly, but describe block title outdated (P3).The test properly uses the new
send_message/3API with the timeout option. However, thedescribe "send_message/2"block should be updated todescribe "send_message/3"to reflect the new arity.- describe "send_message/2" do + describe "send_message/3" dolib/anubis/server/transport/streamable_http.ex (3)
450-452: [P0] Timer not canceled (key mismatch) → use :task_timeoutMap.has_key? checks :timeout_ref but the stored key is :task_timeout. Timer never canceled => stray :task_timeout messages and avoidable mailbox churn.
Apply:
- if Map.has_key?(task_info, :timeout_ref) do - Process.cancel_timer(task_info.task_timeout) - end + if Map.has_key?(task_info, :task_timeout) do + Process.cancel_timer(task_info.task_timeout) + end
106-119: [P2] Update docs/spec for new arity and optionsDocs still list only transport/message. Add opts (currently :timeout) with default behavior; include @SPEC.
Apply:
@@ - ## Parameters - * `transport` - The transport process - * `message` - The message to send + ## Parameters + * `transport` - The transport process + * `message` - The message to send (binary) + * `opts` - Keyword options + - `:timeout` (ms, default: #{@default_send_timeout}) – GenServer.call timeout @@ - @impl Transport - def send_message(transport, message, opts) when is_binary(message) do + @impl Transport + @spec send_message(GenServer.server(), binary(), keyword()) :: :ok | {:error, term()} + def send_message(transport, message, opts) when is_binary(message) doAs per coding guidelines.
Also applies to: 120-123
85-92: [P3] Define module default(s) as attributesConsider moving other literal timeouts to module attributes (e.g., 5_000 in register_sse_handler/2) for consistency.
test/anubis/transport/sse_test.exs (1)
69-69: [P3] Update describe title to new arityChange “send_message/2” → “send_message/3” for accuracy.
- describe "send_message/2" do + describe "send_message/3" dolib/anubis/server/transport/sse.ex (1)
120-132: P0: Guard against nil timeout and update API docs/spec.Passing
opts[:timeout]directly can benil, which will crashGenServer.call/3. Use a sane default and document/spec the new opts.Apply this change to the function:
- def send_message(transport, message, opts) when is_binary(message) do - GenServer.call(transport, {:send_message, message}, opts[:timeout]) - end + def send_message(transport, message, opts) when is_binary(message) do + timeout = Keyword.get(opts, :timeout, @default_call_timeout) + GenServer.call(transport, {:send_message, message}, timeout) + endAnd add (outside this hunk):
# near the top, with other constants @default_call_timeout 5_000 # just above send_message/3 @spec send_message(GenServer.server(), binary(), keyword()) :: :ok | {:error, term()}Also extend the @doc to include:
- opts: [:timeout] – timeout in ms for the call; defaults to 5_000.
As per coding guidelines.Also applies to: 134-136
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
lib/anubis/client/base.ex(10 hunks)lib/anubis/client/operation.ex(1 hunks)lib/anubis/client/state.ex(3 hunks)lib/anubis/server/base.ex(7 hunks)lib/anubis/server/transport/sse.ex(1 hunks)lib/anubis/server/transport/stdio.ex(2 hunks)lib/anubis/server/transport/streamable_http.ex(1 hunks)lib/anubis/transport/behaviour.ex(1 hunks)lib/anubis/transport/sse.ex(1 hunks)lib/anubis/transport/stdio.ex(1 hunks)lib/anubis/transport/streamable_http.ex(1 hunks)lib/anubis/transport/websocket.ex(1 hunks)test/anubis/client/base_test.exs(40 hunks)test/anubis/client/state_test.exs(3 hunks)test/anubis/server/transport/sse_test.exs(1 hunks)test/anubis/server/transport/stdio_test.exs(1 hunks)test/anubis/server/transport/streamable_http_test.exs(1 hunks)test/anubis/transport/sse_test.exs(6 hunks)test/anubis/transport/stdio_test.exs(2 hunks)test/anubis/transport/streamable_http_test.exs(10 hunks)test/anubis/transport/websocket_test.exs(1 hunks)test/support/mock_transport.ex(1 hunks)test/support/stub_transport.ex(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
lib/anubis/transport/**/*.ex
📄 CodeRabbit inference engine (CLAUDE.md)
All transports implement Anubis.Transport.Behaviour with required callbacks start_link/1, send_message/2, shutdown/1 (e.g., Anubis.Transport.{STDIO,SSE,WebSocket,StreamableHTTP})
Files:
lib/anubis/transport/sse.exlib/anubis/transport/streamable_http.exlib/anubis/transport/websocket.exlib/anubis/transport/behaviour.exlib/anubis/transport/stdio.ex
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Only add code comments if strictly necessary; avoid comments generally
Formatting must follow .formatter.exs rules with Peri imports; run mix format
Files:
lib/anubis/transport/sse.exlib/anubis/client/operation.exlib/anubis/server/transport/stdio.exlib/anubis/transport/streamable_http.extest/anubis/server/transport/stdio_test.exslib/anubis/transport/websocket.extest/support/mock_transport.exlib/anubis/server/transport/streamable_http.extest/anubis/server/transport/sse_test.exstest/support/stub_transport.exlib/anubis/server/base.exlib/anubis/server/transport/sse.exlib/anubis/transport/behaviour.extest/anubis/client/state_test.exstest/anubis/transport/sse_test.exstest/anubis/server/transport/streamable_http_test.exslib/anubis/client/state.extest/anubis/transport/streamable_http_test.exslib/anubis/transport/stdio.extest/anubis/client/base_test.exslib/anubis/client/base.extest/anubis/transport/websocket_test.exstest/anubis/transport/stdio_test.exs
lib/**/*.ex
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.ex: Use @type/@SPEC for all public functions
Use snake_case for function names and PascalCase for modules
Group imports at the top, organized by category (Elixir stdlib, deps, project modules)
Include @moduledoc for modules and @doc with examples for public functions
Prefer pattern matching on {:ok, } and {:error, reason} for error handling
Define defaults as module attributes (e.g., @default*)
Module structure order: moduledoc, types, constants, public API, GenServer callbacks, private helpers
Files:
lib/anubis/transport/sse.exlib/anubis/client/operation.exlib/anubis/server/transport/stdio.exlib/anubis/transport/streamable_http.exlib/anubis/transport/websocket.exlib/anubis/server/transport/streamable_http.exlib/anubis/server/base.exlib/anubis/server/transport/sse.exlib/anubis/transport/behaviour.exlib/anubis/client/state.exlib/anubis/transport/stdio.exlib/anubis/client/base.ex
lib/anubis/client/**/*.ex
📄 CodeRabbit inference engine (CLAUDE.md)
lib/anubis/client/**/*.ex: In Anubis.Client, use transport via state configuration and send via transport.layer.send_message(transport.name, data)
Use Anubis.Client.State for client state operations (State.new/1, add/remove request, progress callbacks, capability validation/merge)
Use Anubis.Client.Operation to configure requests (progress, timeouts, method/params)
Use Anubis.Client.Request for request lifecycle tracking (ID generation, timing, caller refs)
Files:
lib/anubis/client/operation.exlib/anubis/client/state.exlib/anubis/client/base.ex
lib/anubis/server/**/*.ex
📄 CodeRabbit inference engine (CLAUDE.md)
lib/anubis/server/**/*.ex: Build servers on Anubis.Server.Base and implement Anubis.Server.Behaviour callbacks (init/1, handle_request/2, handle_notification/2, server_info/0; optional server_capabilities/0)
Configure server transport via options (transport: [layer: TransportModule, name: process_name]) and use Anubis.Server.Transport.{STDIO,StreamableHTTP}
Files:
lib/anubis/server/transport/stdio.exlib/anubis/server/transport/streamable_http.exlib/anubis/server/base.exlib/anubis/server/transport/sse.ex
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.exs: Use Anubis.MCP.Case for MCP protocol testing with MockTransport (avoid real transports)
Use Anubis.MCP.Builders for message construction in tests (init_request/1, ping_request/0, tools_list_request/1, build_request/2, build_response/2, build_notification/2)
Use provided setup helpers in tests (setup_client/2, initialize_client/2, initialized_client/2, setup_server/2, initialize_server/2, initialized_server/2, server_with_mock_transport/2)
Use MCP-specific assertions (assert_mcp_response/2, assert_mcp_error/3, assert_mcp_notification/2, assert_success/2, assert_resources/2, assert_tools/2)
Files:
test/anubis/server/transport/stdio_test.exstest/anubis/server/transport/sse_test.exstest/anubis/client/state_test.exstest/anubis/transport/sse_test.exstest/anubis/server/transport/streamable_http_test.exstest/anubis/transport/streamable_http_test.exstest/anubis/client/base_test.exstest/anubis/transport/websocket_test.exstest/anubis/transport/stdio_test.exs
test/**/*_test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Write descriptive test blocks (e.g., clear describe/it contexts)
Files:
test/anubis/server/transport/stdio_test.exstest/anubis/server/transport/sse_test.exstest/anubis/client/state_test.exstest/anubis/transport/sse_test.exstest/anubis/server/transport/streamable_http_test.exstest/anubis/transport/streamable_http_test.exstest/anubis/client/base_test.exstest/anubis/transport/websocket_test.exstest/anubis/transport/stdio_test.exs
🧠 Learnings (6)
📚 Learning: 2025-10-07T19:46:22.924Z
Learnt from: CR
PR: zoedsoupe/anubis-mcp#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-07T19:46:22.924Z
Learning: Applies to lib/anubis/server/**/*.ex : Configure server transport via options (transport: [layer: TransportModule, name: process_name]) and use Anubis.Server.Transport.{STDIO,StreamableHTTP}
Applied to files:
lib/anubis/server/transport/stdio.exlib/anubis/server/transport/streamable_http.ex
📚 Learning: 2025-10-07T19:46:22.924Z
Learnt from: CR
PR: zoedsoupe/anubis-mcp#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-07T19:46:22.924Z
Learning: Applies to lib/anubis/client/**/*.ex : In Anubis.Client, use transport via state configuration and send via transport.layer.send_message(transport.name, data)
Applied to files:
lib/anubis/server/transport/stdio.exlib/anubis/server/transport/streamable_http.extest/support/stub_transport.exlib/anubis/server/base.exlib/anubis/transport/behaviour.extest/anubis/client/base_test.exslib/anubis/client/base.ex
📚 Learning: 2025-10-07T19:46:22.924Z
Learnt from: CR
PR: zoedsoupe/anubis-mcp#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-07T19:46:22.924Z
Learning: Applies to lib/anubis/transport/**/*.ex : All transports implement Anubis.Transport.Behaviour with required callbacks start_link/1, send_message/2, shutdown/1 (e.g., Anubis.Transport.{STDIO,SSE,WebSocket,StreamableHTTP})
Applied to files:
lib/anubis/server/transport/stdio.extest/support/mock_transport.exlib/anubis/server/transport/streamable_http.exlib/anubis/transport/behaviour.extest/anubis/server/transport/streamable_http_test.exstest/anubis/client/base_test.exs
📚 Learning: 2025-10-07T19:46:22.924Z
Learnt from: CR
PR: zoedsoupe/anubis-mcp#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-07T19:46:22.924Z
Learning: Applies to lib/anubis/client/**/*.ex : Use Anubis.Client.State for client state operations (State.new/1, add/remove request, progress callbacks, capability validation/merge)
Applied to files:
lib/anubis/client/state.ex
📚 Learning: 2025-10-07T19:46:22.924Z
Learnt from: CR
PR: zoedsoupe/anubis-mcp#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-07T19:46:22.924Z
Learning: Applies to test/**/*.exs : Use Anubis.MCP.Case for MCP protocol testing with MockTransport (avoid real transports)
Applied to files:
test/anubis/client/base_test.exs
📚 Learning: 2025-10-07T19:46:22.924Z
Learnt from: CR
PR: zoedsoupe/anubis-mcp#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-07T19:46:22.924Z
Learning: Applies to lib/anubis/client/**/*.ex : Use Anubis.Client.Operation to configure requests (progress, timeouts, method/params)
Applied to files:
lib/anubis/client/base.ex
🧬 Code graph analysis (18)
lib/anubis/transport/sse.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/server/transport/stdio.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/transport/streamable_http.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
test/anubis/server/transport/stdio_test.exs (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/transport/websocket.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
test/support/mock_transport.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/server/transport/streamable_http.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
test/anubis/server/transport/sse_test.exs (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/server/base.ex (2)
lib/anubis/client/base.ex (1)
send_to_transport(1499-1503)test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/server/transport/sse.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/transport/behaviour.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
test/anubis/transport/sse_test.exs (1)
test/support/stub_transport.ex (1)
send_message(83-85)
test/anubis/server/transport/streamable_http_test.exs (1)
test/support/stub_transport.ex (1)
send_message(83-85)
test/anubis/transport/streamable_http_test.exs (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/transport/stdio.ex (1)
test/support/stub_transport.ex (1)
send_message(83-85)
lib/anubis/client/base.ex (3)
lib/anubis/server/base.ex (3)
send_to_transport(679-681)send_to_transport(683-687)encode_notification(673-677)test/support/stub_transport.ex (1)
send_message(83-85)lib/anubis/mcp/message.ex (2)
encode_notification(455-457)encode_notification(469-473)
test/anubis/transport/websocket_test.exs (1)
test/support/stub_transport.ex (1)
send_message(83-85)
test/anubis/transport/stdio_test.exs (1)
test/support/stub_transport.ex (1)
send_message(83-85)
🔇 Additional comments (20)
test/support/stub_transport.ex (1)
83-85: LGTM! Stub transport signature updated correctly.The arity change to
send_message/3with an unused_optsparameter aligns perfectly with the new transport behavior. For a test stub, ignoring the options is acceptable and keeps the implementation simple.lib/anubis/client/state.ex (3)
17-17: Type spec looks good! 👍The
timeout: pos_integer()field addition is correctly typed and positioned within the state struct definition.
32-32: Struct field added correctly.The
:timeoutfield is properly declared in the defstruct, making it a required field that must be provided during initialization.
48-49: No issues found — all callers provide timeout (P2 resolved) 😎Verification shows both callers of
State.new/1supplytimeout:
- Test suite (state_test.exs):
timeout: 30_000- Production code (base.ex:785):
timeout: opts.timeoutNo
KeyErrorrisk exists. The original concern is satisfied.lib/anubis/server/base.ex (5)
63-64: Timeout option configured correctly.The
:timeoutoption with a 30-second default is a reasonable choice for server transport operations. The schema correctly defaults the value.
94-95: State initialization looks good.The
timeoutfield is correctly initialized from the parsed options and will be available for all transport operations.
679-687: send_to_transport/3 properly updated.The helper function correctly threads the
optsparameter through to the transport layer'ssend_message/3callback. Error handling is preserved.
728-728: Timeout correctly passed for sampling request.The
send_to_transportcall properly includes the timeout option from server state.
859-859: Roots request timeout handling is correct.Both
send_to_transportcalls for roots requests properly include the timeout option from server state.Also applies to: 895-895
test/anubis/transport/stdio_test.exs (1)
60-61: Test updates look solid! ✓The tests correctly use the new 3-arity
send_message/3API with explicit timeout values. The 5-second timeout is appropriate for test scenarios.Also applies to: 83-83
test/anubis/server/transport/streamable_http_test.exs (1)
128-129: [P3] LGTM on send_message/3 usageCall correctly provides timeout option. ✅
test/anubis/transport/websocket_test.exs (1)
115-116: [P3] LGTM on new API usagesend_message/3 invoked with timeout as intended. 👍
test/anubis/transport/sse_test.exs (1)
134-135: [P3] LGTM on send_message/3 callsAll updated calls pass timeout; assertions look good. 🚀
Also applies to: 171-172, 383-384, 423-424, 497-498
test/support/mock_transport.ex (1)
9-9: [P3] LGTM: mock updated to 3-arity with default optsMatches behavior callback; keeps tests simple.
test/anubis/server/transport/sse_test.exs (1)
129-129: LGTM: uses new send_message/3 API correctly.Call shape and opts usage are good. 😎
lib/anubis/client/base.ex (4)
139-141: Nice! Timeout configuration properly threaded through 🎯The timeout option is correctly added to the client initialization schema with a sensible default from
Operation.default_timeout(). This allows global configuration while still permitting per-operation overrides.Pattern observed:
- Client-level default:
state.timeout(from this option)- Per-operation override:
operation.timeout(when specified)
789-791: Clean state initialization with timeout 👌The timeout is correctly propagated into the client state, making it available for all transport operations that don't have explicit per-operation timeouts.
1499-1503: Solid helper update – timeout propagation looks good ✅The
send_to_transporthelper correctly:
- Accepts opts parameter
- Forwards opts to
transport.layer.send_message/3- Wraps transport errors consistently
All callsites reviewed (lines 832, 890, 984, 1023, 1049, 1507) correctly pass
timeout: operation.timeoutortimeout: state.timeoutdepending on context. The pattern is consistent and correct:
- Use
operation.timeoutfor client-initiated operations- Use
state.timeoutfor notifications and server-request responses
1598-1598: P3: Minor inconsistency – direct transport call bypasses error wrapping 🤔Lines 1598 and 1610 call
transport.layer.send_messagedirectly instead of using thesend_to_transport/3helper. This is fine but bypasses the error wrapping logic that converts transport errors toError.transport(:send_failure, ...).These are in the sampling callback execution path where errors might be handled differently, but for consistency, consider:
-:ok = transport.layer.send_message(transport.name, response, timeout: state.timeout) +send_to_transport(state.transport, response, timeout: state.timeout)Not critical since these are in fire-and-forget Task contexts, but worth noting for maintainability.
Also applies to: 1610-1610
⛔ Skipped due to learnings
Learnt from: CR PR: zoedsoupe/anubis-mcp#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-07T19:46:22.924Z Learning: Applies to lib/anubis/client/**/*.ex : In Anubis.Client, use transport via state configuration and send via transport.layer.send_message(transport.name, data)test/anubis/client/base_test.exs (1)
21-21: LGTM – Consistent arity update across all mocks 👍All
send_messageexpectations updated from arity-2 to arity-3 to match the new transport behavior. The mechanical changes are consistent throughout the file.Optional enhancement: Consider adding at least one test case that validates the timeout option is correctly propagated to the transport layer. For example:
test "passes timeout option to transport", %{client: client} do expect(Anubis.MockTransport, :send_message, fn _, message, opts -> assert opts[:timeout] > 0 :ok end) Anubis.Client.Base.ping(client, timeout: 1000) endThis would verify the integration behavior matches the PR objectives.
Also applies to: 49-49, 72-72
⛔ Skipped due to learnings
Learnt from: CR PR: zoedsoupe/anubis-mcp#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-07T19:46:22.924Z Learning: Applies to lib/anubis/client/**/*.ex : In Anubis.Client, use transport via state configuration and send via transport.layer.send_message(transport.name, data)
|
@coderabbitai ignore |
✅ Actions performedReviews paused. |
1 participant
Problem
Client functions like
call_tool/4already had thetimeoutoption passed to the internalOperationstruct, which is handled by the GenServer, however thesend_message/2callback of all transports weren't accepting a timeout option, which fallbacks to 5sSolution
Remove
send_message/2and implementsend_message/3that accepts a kw as options (for now only timeout) to control this internal timeout flow.This option can be passed on the start_link parameters of both
clientandserverprocessesRationale
It probably correctly solves #45 without relying on
:infinity(need testing)Summary by CodeRabbit
New Features
Breaking Changes
Tests