refactor(phase-3)!: server re-implementation and simplification - #96
Merged
Merged
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (9)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can generate a title for your PR based on the changes.Add WalkthroughThis PR performs a comprehensive architectural refactoring of the MCP server implementation, shifting from a Sequence Diagram(s)sequenceDiagram
participant Client
participant Transport
participant Session
participant ServerModule
participant Frame
Note over Transport,Session: OLD FLOW (Server.Base)
Client->>Transport: MCP Request
Transport->>ServerModule: Forward Request<br/>(via registry lookup)
ServerModule->>ServerModule: handle_call + Frame
ServerModule->>Transport: Send Response
Transport->>Client: Response
Note over Transport,Session: NEW FLOW (Session-driven)
Client->>Transport: MCP Request
Transport->>Session: {:mcp_request, msg, context}
Session->>Session: Prepare Frame +<br/>Context
Session->>ServerModule: Optional handle_call
ServerModule->>Session: Return response
Session->>Transport: Encode + Send
Transport->>Client: Response
sequenceDiagram
participant ServerModule
participant Session
participant Transport
participant Client
Note over ServerModule,Client: OLD: Notification via Frame
ServerModule->>ServerModule: send_resource_updated(frame, uri)
ServerModule->>Transport: queue_notification(frame, ...)
Transport->>Client: Notification
Note over ServerModule,Client: NEW: Notification via self()
ServerModule->>Session: send(self(), {:send_notification, ...})
Session->>Session: handle_cast<br/>Process notification
Session->>Transport: encode_notification()
Transport->>Client: Notification
sequenceDiagram
participant Registry
participant Supervisor
participant Session
participant StoreAdapter
Note over Registry,StoreAdapter: Session Lifecycle (New)
Supervisor->>Session: start_link(opts)
Session->>Session: init() — parse options
Session->>Session: Initialize server info<br/>& capabilities
Session->>StoreAdapter: maybe_persist_session()
Session->>Session: Schedule expiry timer
Note over Session: Ready for requests
Supervisor->>Registry: register_session(name,<br/>session_id, pid)
Registry->>Registry: Store in ETS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: This is a multi-faceted architectural refactor touching 15+ core files with heterogeneous changes (Frame internals, Session lifecycle, Registry pattern, transport routing, notification dispatch). While large portions are internally consistent (e.g., all transports updated the same way), each area requires separate reasoning. The removal of Possibly related PRs
🎯 Key observations & concerns (P0–P3)P0: Control-flow verification
P1: Frame & Context consistency
P2: Registry & deterministic naming
P3: Test coverage
Minor cosmetics & actionables 🔧
💪 Strengths
Overall: A well-executed architectural shift with clear intent and comprehensive test coverage. Main risk is control-flow edge cases around session resolution in transports; verify all 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Owner
Author
|
yeah, credo is claiming errors, will tackle that another time. lets know if code rabbit will "like" this giant pr haha |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/anubis/server/transport/streamable_http/plug_persistence_test.exs (1)
118-140:⚠️ Potential issue | 🟡 MinorP2
⚠️ : Test"initialize request triggers session persistence"has a placeholder assertion.Line 139 just asserts
truewithout verifying that the initialize request actually triggered persistence. This test doesn't validate any behavior.💡 Consider completing the test or marking as pending
- assert true + # TODO: Complete this test by calling the Plug and verifying MockSessionStore.load + # Or mark as `@tag` :pending if not readyOr implement the full flow:
conn = Plug.call(conn, plug_opts) assert {:ok, _} = MockSessionStore.load(conn.assigns[:session_id], [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/anubis/server/transport/streamable_http/plug_persistence_test.exs` around lines 118 - 140, The test "initialize request triggers session persistence" currently contains a placeholder assertion and must verify persistence: call the plug under test (e.g., invoke Plug.call(conn, plug_opts) or the module plug being tested) with the built conn, then assert the session was stored by checking the session store/mock (e.g., call MockSessionStore.load(conn.assigns[:session_id], []) or the real store API) and assert the returned value matches expected session data; ensure you reference the conn built in the test, the plug_opts used when calling the plug, and the conn.assigns[:session_id] or whichever key the plug sets to locate the persisted session.lib/anubis/server/transport/sse.ex (1)
416-436: 🧹 Nitpick | 🔵 TrivialP2 ✨: Consider catching
:noprocexplicitly for clearer error semantics.The
catch :exit, reasonblock handles server unavailability, but specific:noproc/{:noproc, _}exits could produce a more meaningful:no_sessionerror rather than the generic:server_unavailable.💡 Optional refinement
catch - :exit, reason -> + :exit, {:noproc, _} -> + Logging.transport_event("session_not_found", %{}, level: :warning) + {:error, :no_session} + + :exit, reason -> Logging.transport_event("server_call_failed", %{reason: reason}, level: :error) {:error, :server_unavailable} end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/anubis/server/transport/sse.ex` around lines 416 - 436, The catch-all exit handler in forward_request_to_session currently maps any :exit reason to {:error, :server_unavailable}; update forward_request_to_session to explicitly detect :noproc (and {:noproc, _}) in the catch clause and return {:error, :no_session} (and log a distinct transport_event like "no_session" or include the reason) while leaving other exits mapped to {:error, :server_unavailable}; this change should be applied in the catch block of the forward_request_to_session function so the GenServer unregistered case is differentiated from other server failures.lib/anubis/server/transport/stdio.ex (1)
290-321:⚠️ Potential issue | 🟡 MinorP1 (Must Fix): Credo nesting depth violation — refactor
process_message/2🔧The static analysis and CI pipeline flag this function for nesting too deep (max depth = 2, was 3). The nested
if+if+casestructure exceeds Credo's threshold.♻️ Proposed fix: Extract helper functions to flatten nesting
- defp process_message(message, %{server: server_module} = state) do - session_pid = Registry.stdio_session_name(server_module) - timeout = state.request_timeout - - context = %{ - type: :stdio, - env: System.get_env(), - pid: System.pid() - } - - if Process.whereis(session_pid) do - if Message.is_notification(message) do - GenServer.cast(session_pid, {:mcp_notification, message, context}) - else - case GenServer.call(session_pid, {:mcp_request, message, context}, timeout) do - {:ok, response} when is_binary(response) -> - IO.write(response <> "\n") - - {:ok, nil} -> - :ok - - {:error, reason} -> - Logging.transport_event("session_error", %{reason: reason}, level: :error) - end - end - else - Logging.transport_event("no_session", %{server: server_module}, level: :error) - end - catch - :exit, reason -> - Logging.transport_event("session_call_failed", %{reason: reason}, level: :error) - end + defp process_message(message, %{server: server_module} = state) do + session_pid = Registry.stdio_session_name(server_module) + + case Process.whereis(session_pid) do + nil -> + Logging.transport_event("no_session", %{server: server_module}, level: :error) + + _pid -> + route_to_session(session_pid, message, state) + end + catch + :exit, reason -> + Logging.transport_event("session_call_failed", %{reason: reason}, level: :error) + end + + defp route_to_session(session_pid, message, state) do + context = %{ + type: :stdio, + env: System.get_env(), + pid: System.pid() + } + + if Message.is_notification(message) do + GenServer.cast(session_pid, {:mcp_notification, message, context}) + else + handle_request_response(session_pid, message, context, state.request_timeout) + end + end + + defp handle_request_response(session_pid, message, context, timeout) do + case GenServer.call(session_pid, {:mcp_request, message, context}, timeout) do + {:ok, response} when is_binary(response) -> + IO.write(response <> "\n") + + {:ok, nil} -> + :ok + + {:error, reason} -> + Logging.transport_event("session_error", %{reason: reason}, level: :error) + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/anubis/server/transport/stdio.ex` around lines 290 - 321, process_message/2 has excessive nesting (if -> if -> case) causing a Credo depth violation; refactor by extracting helper functions to flatten control flow: compute session_pid and context as now, then early-return when Process.whereis(session_pid) is nil by calling a new helper (e.g., handle_no_session/2) that logs via Logging.transport_event("no_session", ...); for the present-session path extract two helpers such as handle_notification(session_pid, message, context) to perform Message.is_notification + GenServer.cast and handle_request(session_pid, message, context, timeout) to perform the GenServer.call and the case branches (writing response, noop for nil, logging session_error), and keep the try/catch (or rescue) wrapper around the simplified top-level flow so nesting depth is reduced in process_message/2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/anubis/server.ex`:
- Around line 546-550: The send_resources_list_changed/0 helper currently uses
send(self(), ...) which silently delivers messages to whichever process calls
it; add an optional runtime assertion guarded by a config flag (e.g.
Application.get_env(:anubis, :debug_notifications, false)) that checks for the
Session marker (e.g. Process.get(:__anubis_session__)) and raises a clear error
like "send_resources_list_changed must be called from a Session process" when
the check fails, while keeping the original send(self(), {:send_notification,
"notifications/resources/list_changed", %{}}) and returning :ok in
normal/non-debug runs.
In `@lib/anubis/server/frame.ex`:
- Around line 270-276: get_component/2 currently does an O(n) fallback lookup by
name using Enum.find over frame.resources; to fix, add and use a reverse index
(e.g. frame.resource_name_index :: %{name => uri}) so get_component will resolve
resources by uri lookup instead of scanning: change get_component to check
frame.resource_name_index[name] and return frame.resources[uri] when present,
and ensure every place that mutates frame.resources (functions that add, update
or remove resources) also updates resource_name_index accordingly (add mappings
on insert/update, remove on delete) to keep the index consistent.
In `@lib/anubis/server/registry.ex`:
- Around line 19-22: Mark callbacks that adapters may omit by adding
`@optional_callbacks` to the behaviour; update the module to include something
like `@optional_callbacks` child_spec: 1 (or list all four if you want full
flexibility: child_spec: 1, register_session: 3, lookup_session: 2,
unregister_session: 2) so implementations like Registry.None can omit the no-op
functions while keeping the typespecs for the behaviour (referencing the
existing callback names child_spec/1, register_session/3, lookup_session/2,
unregister_session/2).
In `@lib/anubis/server/registry/local.ex`:
- Around line 36-48: lookup_session/2 repeatedly calls table_name(name) causing
repeated atom lookups; cache the computed table name once in the GenServer state
during init (store it under a key like :table or :table_name) and change
lookup_session/2 to read the cached table from state (or add a hot-path function
like do_lookup(table, session_id) and a public lookup_session_by_table/2 that
accepts the cached table). Alternatively, for extreme hot paths consider storing
the table in persistent_term but prefer keeping the table in the GenServer state
accessed by lookup_session/2; update any callers accordingly.
- Around line 74-83: The handle_call clause for {:unregister, session_id}
removes monitor entries from state.monitors but never calls Process.demonitor/2;
update the code in handle_call({:unregister, session_id}, _from, state) to
iterate the monitor entries that are being removed, call Process.demonitor(ref,
[:flush]) for each removed monitor ref before building the new monitors map, and
then return {:reply, :ok, %{state | monitors: monitors}} so no monitor refs
remain dangling (refer to state.monitors, session_id, and Process.demonitor/2).
In `@lib/anubis/server/session.ex`:
- Around line 680-714: handle_sampling_request_send and
handle_roots_request_send duplicate the same "create timer → store request_info
→ validate capability → encode → send → on error cancel timer + cleanup" flow;
extract that logic into a shared helper (e.g. send_server_request/5) that
accepts request_id, method (like "sampling/createMessage"), params, state and
timeout, performs Process.send_after, inserts into state.server_requests, calls
validate_client_capability/2, encode_request/3 and send_to_transport/3, and on
any {:error, _} cancels the timer and removes the request from
state.server_requests before returning {:noreply, state}; then replace the
bodies of handle_sampling_request_send and handle_roots_request_send to call
this helper and emit the appropriate Logging.server_event messages.
- Around line 771-776: The error handler handle_server_request_error currently
assumes Map.pop returned request_info and calls
Process.cancel_timer(request_info.timer_ref) which can crash if request_info is
nil; update handle_server_request_error to safely handle a missing request by
pattern-matching or using a case/if on request_info (from
Map.pop(state.server_requests, request_id)) before calling Process.cancel_timer
and before updating state.server_requests, e.g., only cancel the timer when
request_info is not nil and proceed to return the state unchanged or with
updated server_requests otherwise; reference the function
handle_server_request_error, Map.pop(state.server_requests, request_id), and
request_info.timer_ref when making the guard.
- Around line 753-758: The function handle_server_request_response currently
assumes Map.pop returned request_info and calls
Process.cancel_timer(request_info.timer_ref), which will crash if request_info
is nil; update the function (handle_server_request_response) to defensively
handle a nil request_info by pattern matching the Map.pop result or adding an
explicit guard: if request_info is nil, skip Process.cancel_timer and proceed to
return the updated state (or log a warning), otherwise cancel the timer using
request_info.timer_ref and update state.server_requests as before; ensure you
reference the Map.pop(state.server_requests, request_id) result and the
request_info.timer_ref access when making this change.
In `@lib/anubis/server/supervisor.ex`:
- Around line 45-69: The stop_session/3 function currently discards the return
value of DynamicSupervisor.terminate_child/2; update stop_session to
pattern-match the result of
DynamicSupervisor.terminate_child(Registry.session_supervisor_name(server), pid)
and return :ok on success or propagate {:error, :not_found} (or the returned
error) so the caller is aware of the TOCTOU case; locate stop_session, the
registry_mod.lookup_session call, and the DynamicSupervisor.terminate_child
invocation and add a case/match that handles {:ok, _}, :ok, and {:error,
:not_found} (or other error tuples) and returns them appropriately.
In `@lib/anubis/server/transport/sse.ex`:
- Around line 338-356: Replace the dead is_nil check on
state.registry.session_name(state.server, session_id) with a real session lookup
and flatten nested conditionals: call
Anubis.Server.Registry.lookup_session(state.server, session_id) and
pattern-match on {:ok, session_pid} vs {:error, :not_found} (or, alternately,
skip the lookup and rely on forward_request_to_session/4 which already returns
{:error, :server_unavailable} for missing processes); then use an early return
or cond to branch: if Message.is_notification(message) ->
GenServer.cast(session_pid, {:mcp_notification, message, context}) and reply
{:ok, nil}; else call forward_request_to_session(session_pid, message, context,
timeout) and handle {:ok, response} by calling maybe_send_through_sse(response,
session_id, state) or {:error, reason} -> {:reply, {:error, reason}, state}.
In `@lib/anubis/server/transport/streamable_http/plug.ex`:
- Around line 357-367: The session start branch in start_new_session/2 currently
only returns results from ServerSupervisor.start_session/2; add observability by
emitting a telemetry event or logging on both success and failure: call
telemetry:execute or process_logger (matching the project's existing pattern)
after the {:ok, pid} path (right after
registry_mod.register_session(registry_name, session_id, pid)) to record
session_id, pid and registry_name, and likewise emit a failure event in the
{:error, reason} clause (and for {:error, {:already_started, pid}} if desired)
including session_id and reason; reference the ServerSupervisor.start_session/2
call and registry_mod.register_session usage so you update the exact branches
shown.
- Around line 246-258: The nested ifs in the else branch of the block handling
handler_pid violate Credo's max nesting depth; extract the inner conditional
into a small private helper (e.g., defp maybe_unregister_handler(transport,
session_id, handler_pid)) that calls
StreamableHTTP.unregister_sse_handler(transport, session_id) only when
handler_pid is truthy, then replace the inner if handler_pid do ... end with a
call to maybe_unregister_handler(transport, session_id, handler_pid) and leave
the call to establish_sse_for_request(conn, response, session_id, opts) in the
else branch.
In `@test/anubis/server/component/tool_annotations_test.exs`:
- Around line 184-213: The two near-identical setup blocks should be refactored
into a shared helper to remove duplication; create a helper function (e.g., in
test/support/session_helpers.ex) that encapsulates the session start logic used
in ServerWithOutputSchemaTools tests — use Registry.transport_name/2,
Registry.task_supervisor_name/1, Registry.session_name/2, start_supervised! for
StubTransport and Task.Supervisor, start_supervised!({Session, ...}, id: ...),
call GenServer.call(session, {:mcp_request, init_request(...), %{}}), and
GenServer.cast(session, {:mcp_notification, build_notification(...), %{}}) and
return %{server: session, session_id: session_id}; then replace both setup
blocks to call this helper to keep tests identical but DRY.
In `@test/anubis/server/session_test.exs`:
- Around line 99-115: The test for send_notification/3 currently sends a message
and returns :ok without verifying the transport received it; update the "sends
notification to transport" test (which calls Anubis.Server.send_log_message and
uses send(session, {:send_notification, ...})) to assert delivery by either
calling StubTransport.get_last_message(test_pid_or_server) and comparing its
payload to %{"level" => :info, "message" => "hello"} or by using assert_receive
with the expected {:send_notification, "notifications/log/message", payload}
tuple (depending on how test_pid is configured); ensure the assertion references
the same session/transport used in the setup so the test actually fails when
notifications are not emitted.
- Around line 37-57: The test titled "rejects requests when not initialized" is
inconsistent with its assertion which expects success from
GenServer.call(session, {:mcp_request, request, %{}}); fix by making intent
consistent: either rename the test to "accepts requests when not initialized"
(update the test description string) if Session should accept requests pre-init,
or change the assertion to expect an error tuple (e.g., assert {:error, _} =
GenServer.call(...)) if Session is supposed to reject requests; adjust only the
test description or the assertion around the GenServer.call in the test that
uses Session, build_request, and request so name and behavior match.
In `@test/support/mock_custom_registry.ex`:
- Around line 48-58: The two GenServer callback clauses missing implementation
annotations are the handle_call clauses for lookup and unregister; add `@impl`
GenServer immediately above the functions handling handle_call({:lookup,
session_id}, _from, state) and handle_call({:unregister, session_id}, _from,
state) so both callback definitions include the `@impl` annotation for consistency
and documentation.
- Around line 33-35: start_link/1 currently accepts opts but always hardcodes
name: __MODULE__ when calling GenServer.start_link, preventing multiple
instances; update start_link/1 to honor the :name option by extracting name =
Keyword.get(opts, :name, __MODULE__) (or similar) and pass the full opts to
GenServer.start_link(__MODULE__, opts, name: name) (or pass opts directly if it
already contains :name) so callers can override the registered name; reference
start_link/1, GenServer.start_link, __MODULE__, and opts.
---
Outside diff comments:
In `@lib/anubis/server/transport/sse.ex`:
- Around line 416-436: The catch-all exit handler in forward_request_to_session
currently maps any :exit reason to {:error, :server_unavailable}; update
forward_request_to_session to explicitly detect :noproc (and {:noproc, _}) in
the catch clause and return {:error, :no_session} (and log a distinct
transport_event like "no_session" or include the reason) while leaving other
exits mapped to {:error, :server_unavailable}; this change should be applied in
the catch block of the forward_request_to_session function so the GenServer
unregistered case is differentiated from other server failures.
In `@lib/anubis/server/transport/stdio.ex`:
- Around line 290-321: process_message/2 has excessive nesting (if -> if ->
case) causing a Credo depth violation; refactor by extracting helper functions
to flatten control flow: compute session_pid and context as now, then
early-return when Process.whereis(session_pid) is nil by calling a new helper
(e.g., handle_no_session/2) that logs via Logging.transport_event("no_session",
...); for the present-session path extract two helpers such as
handle_notification(session_pid, message, context) to perform
Message.is_notification + GenServer.cast and handle_request(session_pid,
message, context, timeout) to perform the GenServer.call and the case branches
(writing response, noop for nil, logging session_error), and keep the try/catch
(or rescue) wrapper around the simplified top-level flow so nesting depth is
reduced in process_message/2.
In `@test/anubis/server/transport/streamable_http/plug_persistence_test.exs`:
- Around line 118-140: The test "initialize request triggers session
persistence" currently contains a placeholder assertion and must verify
persistence: call the plug under test (e.g., invoke Plug.call(conn, plug_opts)
or the module plug being tested) with the built conn, then assert the session
was stored by checking the session store/mock (e.g., call
MockSessionStore.load(conn.assigns[:session_id], []) or the real store API) and
assert the returned value matches expected session data; ensure you reference
the conn built in the test, the plug_opts used when calling the plug, and the
conn.assigns[:session_id] or whichever key the plug sets to locate the persisted
session.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (34)
lib/anubis/mcp/error.exlib/anubis/server.exlib/anubis/server/base.exlib/anubis/server/context.exlib/anubis/server/frame.exlib/anubis/server/handlers.exlib/anubis/server/handlers/prompts.exlib/anubis/server/handlers/resources.exlib/anubis/server/handlers/tools.exlib/anubis/server/registry.exlib/anubis/server/registry/adapter.exlib/anubis/server/registry/local.exlib/anubis/server/registry/none.exlib/anubis/server/session.exlib/anubis/server/session/supervisor.exlib/anubis/server/supervisor.exlib/anubis/server/transport/sse.exlib/anubis/server/transport/sse/plug.exlib/anubis/server/transport/stdio.exlib/anubis/server/transport/streamable_http.exlib/anubis/server/transport/streamable_http/plug.extest/anubis/server/base_test.exstest/anubis/server/component/tool_annotations_test.exstest/anubis/server/session/store_test.exstest/anubis/server/session_test.exstest/anubis/server/transport/sse/plug_test.exstest/anubis/server/transport/sse_test.exstest/anubis/server/transport/streamable_http/plug_persistence_test.exstest/anubis/server/transport/streamable_http/plug_test.exstest/anubis/server/transport/streamable_http_test.exstest/support/mcp/assertions.extest/support/mcp/setup.extest/support/mock_custom_registry.extest/support/stub_transport.ex
💤 Files with no reviewable changes (4)
- test/anubis/server/base_test.exs
- lib/anubis/server/base.ex
- lib/anubis/server/registry/adapter.ex
- lib/anubis/server/session/supervisor.ex
Comment on lines
+546
to
550
| @spec send_resources_list_changed :: :ok | ||
| def send_resources_list_changed do | ||
| send(self(), {:send_notification, "notifications/resources/list_changed", %{}}) | ||
| :ok | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P3: Consider adding runtime assertion or clearer error for wrong-process calls
Since send(self(), ...) silently delivers to whatever process calls it, users calling send_tools_list_changed/0 from a Task or external process won't get an error — the message just goes to the wrong place. A debug-mode assertion or prominent warning could help catch this mistake.
💡 Optional: Add debug assertion
def send_tools_list_changed do
# Optional: assert we're in a Session process during dev/test
if Application.get_env(:anubis, :debug_notifications, false) do
unless Process.get(:__anubis_session__), do: raise "Must call from Session process"
end
send(self(), {:send_notification, "notifications/tools/list_changed", %{}})
:ok
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/anubis/server.ex` around lines 546 - 550, The
send_resources_list_changed/0 helper currently uses send(self(), ...) which
silently delivers messages to whichever process calls it; add an optional
runtime assertion guarded by a config flag (e.g. Application.get_env(:anubis,
:debug_notifications, false)) that checks for the Session marker (e.g.
Process.get(:__anubis_session__)) and raises a clear error like
"send_resources_list_changed must be called from a Session process" when the
check fails, while keeping the original send(self(), {:send_notification,
"notifications/resources/list_changed", %{}}) and returning :ok in
normal/non-debug runs.
Comment on lines
+270
to
276
| @spec get_component(t(), name :: String.t()) :: server_component_t() | nil | ||
| def get_component(%__MODULE__{} = frame, name) do | ||
| frame | ||
| |> get_components() | ||
| |> Enum.find(&(&1.name == name)) | ||
| frame.tools[name] || | ||
| frame.prompts[name] || | ||
| frame.resource_templates[name] || | ||
| Enum.find(Map.values(frame.resources), &(&1.name == name)) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P3: O(n) resource lookup by name — acceptable but worth noting
get_component/2 falls back to Enum.find over resources because they're keyed by URI, not name. This is O(n) per call. For servers with many resources, this could matter — but typical MCP servers have few resources, so this is likely fine.
💡 Optional: Add name→uri index if performance becomes an issue
# Alternative: maintain a reverse index in frame
# resource_name_index: %{name => uri}
def get_component(%__MODULE__{} = frame, name) do
frame.tools[name] ||
frame.prompts[name] ||
frame.resource_templates[name] ||
(uri = frame.resource_name_index[name]) && frame.resources[uri]
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/anubis/server/frame.ex` around lines 270 - 276, get_component/2 currently
does an O(n) fallback lookup by name using Enum.find over frame.resources; to
fix, add and use a reverse index (e.g. frame.resource_name_index :: %{name =>
uri}) so get_component will resolve resources by uri lookup instead of scanning:
change get_component to check frame.resource_name_index[name] and return
frame.resources[uri] when present, and ensure every place that mutates
frame.resources (functions that add, update or remove resources) also updates
resource_name_index accordingly (add mappings on insert/update, remove on
delete) to keep the index consistent.
Comment on lines
+19
to
+22
| @callback child_spec(keyword()) :: Supervisor.child_spec() | :ignore | ||
| @callback register_session(name :: term(), session_id(), pid()) :: :ok | {:error, term()} | ||
| @callback lookup_session(name :: term(), session_id()) :: {:ok, pid()} | {:error, :not_found} | ||
| @callback unregister_session(name :: term(), session_id()) :: :ok |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P3: Consider adding @optional_callbacks for flexibility
Currently all four callbacks are required. If child_spec/1 returning :ignore is the pattern for "no-op" registries (like Registry.None), this is fine. Just noting that if some adapters don't need certain callbacks, marking them optional could be helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/anubis/server/registry.ex` around lines 19 - 22, Mark callbacks that
adapters may omit by adding `@optional_callbacks` to the behaviour; update the
module to include something like `@optional_callbacks` child_spec: 1 (or list all
four if you want full flexibility: child_spec: 1, register_session: 3,
lookup_session: 2, unregister_session: 2) so implementations like Registry.None
can omit the no-op functions while keeping the typespecs for the behaviour
(referencing the existing callback names child_spec/1, register_session/3,
lookup_session/2, unregister_session/2).
Comment on lines
+36
to
+48
| def lookup_session(name, session_id) do | ||
| table = table_name(name) | ||
|
|
||
| case :ets.lookup(table, session_id) do | ||
| [{^session_id, pid}] when is_pid(pid) -> | ||
| if Process.alive?(pid), do: {:ok, pid}, else: {:error, :not_found} | ||
|
|
||
| [] -> | ||
| {:error, :not_found} | ||
| end | ||
| rescue | ||
| ArgumentError -> {:error, :not_found} | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P3 💡: Consider caching the table name to avoid atom generation on every lookup.
table_name/1 is called on every lookup_session/2. While atom creation from existing atoms is fast, you could store the table name in the GenServer state during init to avoid repeated computation.
💭 Alternative: pass table directly
# Could expose a lookup that takes the table directly for hot paths
def lookup_session(name, session_id) do
table = table_name(name)
do_lookup(table, session_id)
endOr memoize in a persistent_term for extremely hot paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/anubis/server/registry/local.ex` around lines 36 - 48, lookup_session/2
repeatedly calls table_name(name) causing repeated atom lookups; cache the
computed table name once in the GenServer state during init (store it under a
key like :table or :table_name) and change lookup_session/2 to read the cached
table from state (or add a hot-path function like do_lookup(table, session_id)
and a public lookup_session_by_table/2 that accepts the cached table).
Alternatively, for extreme hot paths consider storing the table in
persistent_term but prefer keeping the table in the GenServer state accessed by
lookup_session/2; update any callers accordingly.
Comment on lines
+74
to
+83
| def handle_call({:unregister, session_id}, _from, state) do | ||
| :ets.delete(state.table, session_id) | ||
|
|
||
| monitors = | ||
| state.monitors | ||
| |> Enum.reject(fn {_ref, sid} -> sid == session_id end) | ||
| |> Map.new() | ||
|
|
||
| {:reply, :ok, %{state | monitors: monitors}} | ||
| end |
There was a problem hiding this comment.
P1 🐛: Missing Process.demonitor/2 call leaves monitors dangling.
When unregistering a session, the code removes the ref from state.monitors but never calls Process.demonitor(ref, [:flush]). This causes:
- Resource leak: The monitor remains active in the ERTS
- Spurious :DOWN handling: When the process eventually terminates,
handle_info({:DOWN, ...})fires but the session was already removed - Potential race: If the process crashes between unregister and the monitor firing, you may double-delete
🔧 Proposed fix
def handle_call({:unregister, session_id}, _from, state) do
:ets.delete(state.table, session_id)
- monitors =
- state.monitors
- |> Enum.reject(fn {_ref, sid} -> sid == session_id end)
- |> Map.new()
+ {monitors, ref_to_demonitor} =
+ Enum.reduce(state.monitors, {%{}, nil}, fn {ref, sid}, {acc, found_ref} ->
+ if sid == session_id do
+ {acc, ref}
+ else
+ {Map.put(acc, ref, sid), found_ref}
+ end
+ end)
+
+ if ref_to_demonitor, do: Process.demonitor(ref_to_demonitor, [:flush])
{:reply, :ok, %{state | monitors: monitors}}
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/anubis/server/registry/local.ex` around lines 74 - 83, The handle_call
clause for {:unregister, session_id} removes monitor entries from state.monitors
but never calls Process.demonitor/2; update the code in
handle_call({:unregister, session_id}, _from, state) to iterate the monitor
entries that are being removed, call Process.demonitor(ref, [:flush]) for each
removed monitor ref before building the new monitors map, and then return
{:reply, :ok, %{state | monitors: monitors}} so no monitor refs remain dangling
(refer to state.monitors, session_id, and Process.demonitor/2).
Comment on lines
184
to
+213
| setup do | ||
| start_supervised!(Anubis.Server.Registry) | ||
| transport = start_supervised!(StubTransport) | ||
| transport_name = Registry.transport_name(ServerWithOutputSchemaTools, StubTransport) | ||
| start_supervised!({StubTransport, name: transport_name}) | ||
|
|
||
| # Start session supervisor | ||
| start_supervised!( | ||
| {Anubis.Server.Session.Supervisor, server: ServerWithOutputSchemaTools, registry: Anubis.Server.Registry} | ||
| ) | ||
| task_sup = Registry.task_supervisor_name(ServerWithOutputSchemaTools) | ||
| start_supervised!({Task.Supervisor, name: task_sup}) | ||
|
|
||
| server_opts = [ | ||
| module: ServerWithOutputSchemaTools, | ||
| name: :test_output_server, | ||
| registry: Anubis.Server.Registry, | ||
| transport: [layer: StubTransport, name: transport] | ||
| ] | ||
|
|
||
| server = start_supervised!({Anubis.Server.Base, server_opts}) | ||
|
|
||
| # Initialize the server | ||
| session_id = "test-session-output" | ||
| session_name = Registry.session_name(ServerWithOutputSchemaTools, session_id) | ||
|
|
||
| session = | ||
| start_supervised!( | ||
| {Session, | ||
| session_id: session_id, | ||
| server_module: ServerWithOutputSchemaTools, | ||
| name: session_name, | ||
| transport: [layer: StubTransport, name: transport_name], | ||
| task_supervisor: task_sup}, | ||
| id: :output_session | ||
| ) | ||
|
|
||
| request = | ||
| init_request("2025-03-26", %{"name" => "TestClient", "version" => "1.0.0"}) | ||
|
|
||
| assert {:ok, _} = GenServer.call(server, {:request, request, session_id, %{}}) | ||
| assert {:ok, _} = GenServer.call(session, {:mcp_request, request, %{}}) | ||
| notification = build_notification("notifications/initialized", %{}) | ||
| assert :ok = GenServer.cast(session, {:mcp_notification, notification, %{}}) | ||
| Process.sleep(30) | ||
|
|
||
| assert :ok = | ||
| GenServer.cast(server, {:notification, notification, session_id, %{}}) | ||
|
|
||
| %{server: server, session_id: session_id} | ||
| %{server: session, session_id: session_id} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P3 (Nitpick): Second setup block mirrors the first — consider extracting a shared helper 🔧
Both setup blocks (lines 63-92 and 184-214) follow nearly identical patterns. While not blocking, you could extract a shared helper in test/support/ to reduce duplication across test modules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/anubis/server/component/tool_annotations_test.exs` around lines 184 -
213, The two near-identical setup blocks should be refactored into a shared
helper to remove duplication; create a helper function (e.g., in
test/support/session_helpers.ex) that encapsulates the session start logic used
in ServerWithOutputSchemaTools tests — use Registry.transport_name/2,
Registry.task_supervisor_name/1, Registry.session_name/2, start_supervised! for
StubTransport and Task.Supervisor, start_supervised!({Session, ...}, id: ...),
call GenServer.call(session, {:mcp_request, init_request(...), %{}}), and
GenServer.cast(session, {:mcp_notification, build_notification(...), %{}}) and
return %{server: session, session_id: session_id}; then replace both setup
blocks to call this helper to keep tests identical but DRY.
Comment on lines
+37
to
+57
| test "rejects requests when not initialized" do | ||
| transport_name = Registry.transport_name(StubServer, StubTransport) | ||
| task_sup = Registry.task_supervisor_name(StubServer) | ||
| session_name = Registry.session_name(StubServer, "not_initialized") | ||
|
|
||
| session = | ||
| start_supervised!( | ||
| {Session, | ||
| session_id: "not_initialized", | ||
| server_module: StubServer, | ||
| name: session_name, | ||
| transport: [layer: StubTransport, name: transport_name], | ||
| task_supervisor: task_sup}, | ||
| id: :uninit_session | ||
| ) | ||
|
|
||
| request = build_request("tools/list", 123) | ||
|
|
||
| assert {:ok, _} = | ||
| GenServer.call(session, {:mcp_request, request, %{}}) | ||
| end |
There was a problem hiding this comment.
P2 🤔: Test name says "rejects" but assertion expects {:ok, _}.
The test is titled "rejects requests when not initialized" but Line 55-56 asserts success:
assert {:ok, _} =
GenServer.call(session, {:mcp_request, request, %{}})Either the test name is misleading (and it should be renamed to reflect that requests are accepted before init), or the assertion should expect an error tuple if non-initialized sessions should indeed reject requests.
📝 Suggested rename if behavior is intentional
- test "rejects requests when not initialized" do
+ test "accepts requests even when not initialized" do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/anubis/server/session_test.exs` around lines 37 - 57, The test titled
"rejects requests when not initialized" is inconsistent with its assertion which
expects success from GenServer.call(session, {:mcp_request, request, %{}}); fix
by making intent consistent: either rename the test to "accepts requests when
not initialized" (update the test description string) if Session should accept
requests pre-init, or change the assertion to expect an error tuple (e.g.,
assert {:error, _} = GenServer.call(...)) if Session is supposed to reject
requests; adjust only the test description or the assertion around the
GenServer.call in the test that uses Session, build_request, and request so name
and behavior match.
Comment on lines
+99
to
+115
| describe "send_notification/3" do | ||
| setup :initialized_server | ||
|
|
||
| test "sends notification to transport", %{server: session} do | ||
| assert :ok = | ||
| :info | ||
| |> Anubis.Server.send_log_message("hello") | ||
| |> then(fn _ -> | ||
| send( | ||
| session, | ||
| {:send_notification, "notifications/log/message", %{"level" => :info, "message" => "hello"}} | ||
| ) | ||
|
|
||
| :ok | ||
| end) | ||
| end | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P3 🧹: Test "sends notification to transport" doesn't verify the notification was actually sent.
The test sends a notification via send/2 and returns :ok, but there's no assertion that the transport received the message. Consider asserting with StubTransport.get_last_message/1 or using assert_receive if test_pid is configured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/anubis/server/session_test.exs` around lines 99 - 115, The test for
send_notification/3 currently sends a message and returns :ok without verifying
the transport received it; update the "sends notification to transport" test
(which calls Anubis.Server.send_log_message and uses send(session,
{:send_notification, ...})) to assert delivery by either calling
StubTransport.get_last_message(test_pid_or_server) and comparing its payload to
%{"level" => :info, "message" => "hello"} or by using assert_receive with the
expected {:send_notification, "notifications/log/message", payload} tuple
(depending on how test_pid is configured); ensure the assertion references the
same session/transport used in the setup so the test actually fails when
notifications are not emitted.
Comment on lines
+33
to
35
| def start_link(opts \\ []) do | ||
| GenServer.start_link(__MODULE__, opts, name: __MODULE__) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P2 (Recommended): start_link/1 ignores opts and hardcodes name 🤔
The function accepts opts but ignores them, always using __MODULE__ as the process name. This limits flexibility if tests need multiple instances.
Consider honoring the :name option like Registry.Local does:
♻️ Respect opts[:name] for flexibility
- def start_link(opts \\ []) do
- GenServer.start_link(__MODULE__, opts, name: __MODULE__)
+ def start_link(opts \\ []) do
+ name = Keyword.get(opts, :name, __MODULE__)
+ GenServer.start_link(__MODULE__, opts, name: name)
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/support/mock_custom_registry.ex` around lines 33 - 35, start_link/1
currently accepts opts but always hardcodes name: __MODULE__ when calling
GenServer.start_link, preventing multiple instances; update start_link/1 to
honor the :name option by extracting name = Keyword.get(opts, :name, __MODULE__)
(or similar) and pass the full opts to GenServer.start_link(__MODULE__, opts,
name: name) (or pass opts directly if it already contains :name) so callers can
override the registered name; reference start_link/1, GenServer.start_link,
__MODULE__, and opts.
Comment on lines
+48
to
58
| def handle_call({:lookup, session_id}, _from, state) do | ||
| case Map.get(state.sessions, session_id) do | ||
| nil -> {:reply, {:error, :not_found}, state} | ||
| pid -> {:reply, {:ok, pid}, state} | ||
| end | ||
| end | ||
|
|
||
| def start_link(opts \\ []) do | ||
| GenServer.start_link(__MODULE__, opts, name: __MODULE__) | ||
| end | ||
|
|
||
| def init(_opts) do | ||
| {:ok, %{}} | ||
| def handle_call({:unregister, session_id}, _from, state) do | ||
| sessions = Map.delete(state.sessions, session_id) | ||
| {:reply, :ok, %{state | sessions: sessions}} | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
P3 (Nitpick): Missing @impl GenServer annotations on handle_call clauses
Lines 48 and 55 define additional handle_call/3 clauses but lack @impl GenServer. While Elixir doesn't strictly require it for pattern-match continuations, adding them improves consistency and documentation.
♻️ Add `@impl` annotations
+ `@impl` GenServer
def handle_call({:lookup, session_id}, _from, state) do
case Map.get(state.sessions, session_id) do
nil -> {:reply, {:error, :not_found}, state}
pid -> {:reply, {:ok, pid}, state}
end
end
+ `@impl` GenServer
def handle_call({:unregister, session_id}, _from, state) do
sessions = Map.delete(state.sessions, session_id)
{:reply, :ok, %{state | sessions: sessions}}
end📝 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.
Suggested change
| def handle_call({:lookup, session_id}, _from, state) do | |
| case Map.get(state.sessions, session_id) do | |
| nil -> {:reply, {:error, :not_found}, state} | |
| pid -> {:reply, {:ok, pid}, state} | |
| end | |
| end | |
| def start_link(opts \\ []) do | |
| GenServer.start_link(__MODULE__, opts, name: __MODULE__) | |
| end | |
| def init(_opts) do | |
| {:ok, %{}} | |
| def handle_call({:unregister, session_id}, _from, state) do | |
| sessions = Map.delete(state.sessions, session_id) | |
| {:reply, :ok, %{state | sessions: sessions}} | |
| end | |
| `@impl` GenServer | |
| def handle_call({:lookup, session_id}, _from, state) do | |
| case Map.get(state.sessions, session_id) do | |
| nil -> {:reply, {:error, :not_found}, state} | |
| pid -> {:reply, {:ok, pid}, state} | |
| end | |
| end | |
| `@impl` GenServer | |
| def handle_call({:unregister, session_id}, _from, state) do | |
| sessions = Map.delete(state.sessions, session_id) | |
| {:reply, :ok, %{state | sessions: sessions}} | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/support/mock_custom_registry.ex` around lines 48 - 58, The two GenServer
callback clauses missing implementation annotations are the handle_call clauses
for lookup and unregister; add `@impl` GenServer immediately above the functions
handling handle_call({:lookup, session_id}, _from, state) and
handle_call({:unregister, session_id}, _from, state) so both callback
definitions include the `@impl` annotation for consistency and documentation.
Merged
zoedsoupe
added a commit
that referenced
this pull request
Mar 16, 2026
🚀 Want to release this? --- ## [1.0.0](v0.17.1...v1.0.0) (2026-03-16) ### ⚠ BREAKING CHANGES * remove client base module and client macro ([#110](#110)) * **phase-3:** server re-implementation and simplification ([#96](#96)) ### Features * add _meta support to Tool struct and JSON encoder ([#108](#108)) ([6ac49d1](6ac49d1)) ### Bug Fixes * **phase-5:** remove dead code and update docs ([#104](#104)) ([eea86af](eea86af)) * regression for input/output server schema ([85f8ebb](85f8ebb)) * remove client base module and client macro ([#110](#110)) ([1f9f13c](1f9f13c)) * server examples and sse server transport ([944bafb](944bafb)) * session serializion errors ([#112](#112)) ([cb8c0e3](cb8c0e3)), closes [#60](#60) * Start SSE keepalive when first handler is registered ([#83](#83)) ([c3c01e9](c3c01e9)) * stdio server transport working ([#111](#111)) ([b331281](b331281)) ### Code Refactoring * **phase-3:** server re-implementation and simplification ([#96](#96)) ([badb0f0](badb0f0)) * **phase-4:** client extraction of handlers ([#100](#100)) ([08b98c0](08b98c0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
zoedsoupe
added a commit
that referenced
this pull request
Jul 16, 2026
## Problem The server architecture had a single-GenServer bottleneck (`Server.Base`) that serialized all requests across all sessions through one mailbox. The `Frame` struct accumulated internal state (`transport`, `request`, `private`, `initialized`) that leaked implementation details to users and created transport-dependent branching in user code. Related: https://github.com/cloudwalk/hermes-mcp/issues/179, https://github.com/cloudwalk/hermes-mcp/issues/#141, RFC #24 ## Solution **Phase 3 — Session-Centric Architecture** - Each MCP session is now an independent GenServer process under `Session.Supervisor` (DynamicSupervisor) - `Server.Base` becomes a coordinator: session lifecycle management, expiry timers, and delegation — no longer processes MCP requests directly - Transport modules (`StreamableHTTP.Plug`, `SSE.Plug`, `STDIO`) route requests to the correct Session process via `Registry` - `Registry` is pluggable (`Registry.Adapter` behaviour) with `Registry.Local` (ETS) as default, supporting future distributed implementations (Horde, Phoenix.Tracker) - Heavy tool execution offloaded to `Task.Supervisor` per session, keeping the session mailbox responsive **Phase 3.5 — Context Refactor** - Introduced `Anubis.Server.Context` — a minimal 4-field struct (`session_id`, `client_info`, `headers`, `remote_ip`) added as a read-only field on `Frame` - Removed `Frame.transport`, `Frame.request`, and `Frame.initialized` fields — these were internal state that polluted the user-facing struct - Transport-agnostic design: STDIO naturally has `headers: %{}, remote_ip: nil`, no type branching needed - Added `Frame.to_saved/1` and `Frame.from_saved/1` for `Session.Store` integration — context is omitted (request-scoped), only user state persists - Session recovery flow: `Registry.lookup` → miss → `Store.load` → `DynamicSupervisor.start_child` → `Registry.register` - No process dictionary usage — context is a value passed through Frame, works naturally with Tasks and async code ## Rationale **Session-per-process over shared GenServer**: the old design forced all sessions through one mailbox — a slow tool call in session A blocked unrelated requests in session B. OTP's model is one process per concurrent unit of work, and an MCP session is exactly that. **Context as a Frame field over process dictionary**: `Process.put` doesn't propagate to `Task.async` spawns, creating silent failures. A struct field on Frame is explicit, testable, and works everywhere Frame is passed. **Minimal Context (4 fields) over full request mirror**: the original plan had 7+ fields including `protocol_version`, `initialized`, `request.id`, `request.method`, `request.params`, and transport type unions. Most duplicated data already available through callback arguments or enforced by Session lifecycle. Users should never branch on transport type — that defeats transport-agnostic design. **Registry and Store as orthogonal concerns**: Registry answers "where is the process?" (ephemeral, dies with process). Store answers "what was the state?" (durable, survives restarts). Both are optional and pluggable independently. --- **Next steps**: - Phase 4 — Client refactor: decompose `Client.Base` (1634 LOC) into `Client.Handlers` + `Client.Sampling` - Phase 5 — Dead code removal, deduplication audit, and documentation fixes (including `CONTRIBUTING.md` license mismatch, outdated API examples in hexdocs pages, missing `pages/home.md`) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Error responses now follow JSON‑RPC 2.0 envelope with a top-level "jsonrpc": "2.0". * **Refactor** * Notification API simplified — sending notifications no longer requires passing per-frame state. * Sessions moved to a session-centric model with session-based routing for transports. * Registry and naming model modernized for deterministic transport/session names and pluggable registry options. * Frame model streamlined to public component maps and a read-only per-request context. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
zoedsoupe
added a commit
that referenced
this pull request
Jul 16, 2026
🚀 Want to release this? --- ## [1.0.0](v0.17.1...v1.0.0) (2026-03-16) ### ⚠ BREAKING CHANGES * remove client base module and client macro ([#110](#110)) * **phase-3:** server re-implementation and simplification ([#96](#96)) ### Features * add _meta support to Tool struct and JSON encoder ([#108](#108)) ([3b26a1c](3b26a1c)) ### Bug Fixes * **phase-5:** remove dead code and update docs ([#104](#104)) ([2a33dfc](2a33dfc)) * regression for input/output server schema ([fbf138b](fbf138b)) * remove client base module and client macro ([#110](#110)) ([a8ec690](a8ec690)) * server examples and sse server transport ([f4d3097](f4d3097)) * session serializion errors ([#112](#112)) ([8416cef](8416cef)), closes [#60](#60) * Start SSE keepalive when first handler is registered ([#83](#83)) ([67d93bb](67d93bb)) * stdio server transport working ([#111](#111)) ([b3d42a8](b3d42a8)) ### Code Refactoring * **phase-3:** server re-implementation and simplification ([#96](#96)) ([14c4e4f](14c4e4f)) * **phase-4:** client extraction of handlers ([#100](#100)) ([dc0ea85](dc0ea85)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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
The server architecture had a single-GenServer bottleneck (
Server.Base) that serialized all requests across all sessions through one mailbox. TheFramestruct accumulated internal state (transport,request,private,initialized) that leaked implementation details to users and created transport-dependent branching in user code.Related: https://github.com/cloudwalk/hermes-mcp/issues/179, https://github.com/cloudwalk/hermes-mcp/issues/#141, RFC #24
Solution
Phase 3 — Session-Centric Architecture
Session.Supervisor(DynamicSupervisor)Server.Basebecomes a coordinator: session lifecycle management, expiry timers, and delegation — no longer processes MCP requests directlyStreamableHTTP.Plug,SSE.Plug,STDIO) route requests to the correct Session process viaRegistryRegistryis pluggable (Registry.Adapterbehaviour) withRegistry.Local(ETS) as default, supporting future distributed implementations (Horde, Phoenix.Tracker)Task.Supervisorper session, keeping the session mailbox responsivePhase 3.5 — Context Refactor
Anubis.Server.Context— a minimal 4-field struct (session_id,client_info,headers,remote_ip) added as a read-only field onFrameFrame.transport,Frame.request, andFrame.initializedfields — these were internal state that polluted the user-facing structheaders: %{}, remote_ip: nil, no type branching neededFrame.to_saved/1andFrame.from_saved/1forSession.Storeintegration — context is omitted (request-scoped), only user state persistsRegistry.lookup→ miss →Store.load→DynamicSupervisor.start_child→Registry.registerRationale
Session-per-process over shared GenServer: the old design forced all sessions through one mailbox — a slow tool call in session A blocked unrelated requests in session B. OTP's model is one process per concurrent unit of work, and an MCP session is exactly that.
Context as a Frame field over process dictionary:
Process.putdoesn't propagate toTask.asyncspawns, creating silent failures. A struct field on Frame is explicit, testable, and works everywhere Frame is passed.Minimal Context (4 fields) over full request mirror: the original plan had 7+ fields including
protocol_version,initialized,request.id,request.method,request.params, and transport type unions. Most duplicated data already available through callback arguments or enforced by Session lifecycle. Users should never branch on transport type — that defeats transport-agnostic design.Registry and Store as orthogonal concerns: Registry answers "where is the process?" (ephemeral, dies with process). Store answers "what was the state?" (durable, survives restarts). Both are optional and pluggable independently.
Next steps:
Client.Base(1634 LOC) intoClient.Handlers+Client.SamplingCONTRIBUTING.mdlicense mismatch, outdated API examples in hexdocs pages, missingpages/home.md)Summary by CodeRabbit