Trap exits in Session so terminate/2 runs on supervisor-initiated stop - #210
Conversation
…stop
Session did not trap exits, so a supervisor :shutdown (the path an explicit
client DELETE takes via stop_session -> DynamicSupervisor.terminate_child)
killed the process without running terminate/2 or its
[:anubis_mcp, :server, :terminate] telemetry. Only callback-initiated
{:stop, ...} returns (e.g. idle expiry) ran terminate/2.
Set Process.flag(:trap_exit, true) in init/1 so :shutdown is delivered
through the gen_server loop and terminate/2 (+ telemetry) runs on every stop
path, consistent with the transports which already trap exits. Tasks use
Task.Supervisor.async_nolink so trapping introduces no stray {:EXIT}; an
explicit handle_info({:EXIT, ...}) clause guards against an exit signal from
a process a host links to the session pid leaking into the host's
handle_info/2.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 030a4440-6da3-447b-b5a8-cde61cc424a9
📒 Files selected for processing (3)
lib/anubis/server/session.extest/anubis/server/session_test.exstest/support/stub_terminate_server.ex
…isor stop Tighten the supervisor-stop assertion from a wildcard reason to :shutdown, pinning that supervisor-initiated termination runs terminate/2 with the graceful :shutdown reason (not :killed). Guards against a regression where the session fails to stop promptly on a supervisor :shutdown.
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 (2)
test/anubis/server/session_test.exs (2)
536-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winP2: Compose this setup through the MCP test helpers.
start_supervised_session/2manually rebuilds transport and server setup instead of composing the prescribedAnubis.MCP.Casesetup functions. Keep the DynamicSupervisor-specific portion, but reuse the shared setup helpers where applicable so lifecycle tests do not drift from the rest of the suite.Source: Coding guidelines
592-611: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winP2: Assert the library telemetry reason too.
The session termination telemetry includes both
session_idandreason, but this test only verifies the session ID. It can therefore pass if the library event reports an incorrect termination reason.✅ Suggested assertion
- assert_receive {:lib_terminate, %{session_id: ^session_id}}, 500 + assert_receive {:lib_terminate, %{reason: :shutdown, session_id: ^session_id}}, 500
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebcb9b44-d332-489d-8338-647ee08aedb6
📒 Files selected for processing (1)
test/anubis/server/session_test.exs
|
Overlaps with #209. |
#210) Addresses behaviour 4 in #204. ### Problem `Anubis.Server.Session` is `use GenServer` and its `init/1` returns `{:ok, state, :hibernate}` without `Process.flag(:trap_exit, true)`. The library already traps exits on its transport processes (`server/transport/streamable_http.ex`, `server/transport/stdio.ex`), just not on `Session`. Because of that, when a session is stopped via the supervisor — `Supervisor.stop_session/3` -> `DynamicSupervisor.terminate_child/2`, a `:shutdown` exit — the non-trapping process dies immediately without running `terminate/2` (standard OTP: a non-trapping process receiving `:shutdown` terminates without invoking `terminate/2`). This is the path a spec-compliant client `DELETE /mcp` takes (`handle_delete -> delete_session_from_store -> stop_session_process -> Supervisor.stop_session`). On it: - the library's own `[:anubis_mcp, :server, :terminate]` telemetry (emitted before the `exported?(module, :terminate, 2)` gate) never fires; - the server module's optional `terminate/2` never fires — so any cleanup/observability a host hangs off `terminate/2` is silently skipped on explicit disconnect. The idle-expiry path already works, because `handle_info(:session_expired, ...)` returns `{:stop, {:shutdown, :session_expired}, ...}`, and a callback-initiated `{:stop, …}` always runs `terminate/2`. The gap is specifically the explicit-stop (DELETE) path — arguably the more common close for short-lived tool sessions. ### Change - `Process.flag(:trap_exit, true)` in `Session.init/1`. With trapping, the supervisor's `:shutdown` is delivered through the `gen_server` loop and `terminate/2` (+ the `[:anubis_mcp, :server, :terminate]` telemetry) runs on every stop path — explicit DELETE, idle expiry, and supervisor shutdown alike — consistent with how the transports already behave. - An explicit `handle_info({:EXIT, _pid, _reason}, state)` clause that ignores the signal. Session's general `handle_info` catch-all forwards unmatched messages to the host's `module.handle_info/2`; this clause keeps a stray `{:EXIT, …}` (from a process a host links to the session pid) from leaking into the host callback. ### Trapping rationale Trapping was deemed safe due to: - Tasks are spawned with `Task.Supervisor.async_nolink`, so they are not linked to the session — task crashes deliver `{:DOWN, …}` (already handled) / `{ref, result}`, never `{:EXIT, …}`. Trapping changes nothing for tasks. - The session links only to its supervisor parent; a parent exit signal to a trapping GenServer is intercepted by the `gen_server` loop and triggers normal termination (which runs `terminate/2`) rather than being dispatched to `handle_info`. That interception is exactly what makes the fix work. - So in normal operation the session receives no `{:EXIT, …}` at `handle_info`; the explicit clause is defense-in-depth for the exotic host-link case. ### Tests - Host `terminate/2` fires when the session is stopped via the supervisor (`DynamicSupervisor.terminate_child`, the mechanism `Supervisor.stop_session/3` uses), asserted via telemetry; the process is confirmed dead. - The library `[:anubis_mcp, :server, :terminate]` telemetry fires on the same path, with the session id in metadata. Both fail before the change (the session dies on `:shutdown` without running `terminate/2`) and pass after. `mix lint` (format + credo --strict + dialyzer) and `mix test` pass. --------- Co-authored-by: zoey <zoey.spessanha@zeetech.io>
2 participants
Addresses behaviour 4 in #204.
Problem
Anubis.Server.Sessionisuse GenServerand itsinit/1returns{:ok, state, :hibernate}withoutProcess.flag(:trap_exit, true). The libraryalready traps exits on its transport processes
(
server/transport/streamable_http.ex,server/transport/stdio.ex), just not onSession.Because of that, when a session is stopped via the supervisor —
Supervisor.stop_session/3->DynamicSupervisor.terminate_child/2, a:shutdownexit — the non-trapping process dies immediately without running
terminate/2(standard OTP: a non-trapping process receiving:shutdownterminates without invoking
terminate/2).This is the path a spec-compliant client
DELETE /mcptakes(
handle_delete -> delete_session_from_store -> stop_session_process -> Supervisor.stop_session). On it:[:anubis_mcp, :server, :terminate]telemetry (emitted beforethe
exported?(module, :terminate, 2)gate) never fires;terminate/2never fires — so anycleanup/observability a host hangs off
terminate/2is silently skipped onexplicit disconnect.
The idle-expiry path already works, because
handle_info(:session_expired, ...)returns
{:stop, {:shutdown, :session_expired}, ...}, and a callback-initiated{:stop, …}always runsterminate/2. The gap is specifically theexplicit-stop (DELETE) path — arguably the more common close for short-lived tool
sessions.
Change
Process.flag(:trap_exit, true)inSession.init/1. With trapping, thesupervisor's
:shutdownis delivered through thegen_serverloop andterminate/2(+ the[:anubis_mcp, :server, :terminate]telemetry) runs onevery stop path — explicit DELETE, idle expiry, and supervisor shutdown alike —
consistent with how the transports already behave.
handle_info({:EXIT, _pid, _reason}, state)clause that ignores thesignal. Session's general
handle_infocatch-all forwards unmatched messages tothe host's
module.handle_info/2; this clause keeps a stray{:EXIT, …}(froma process a host links to the session pid) from leaking into the host callback.
Trapping rationale
Trapping was deemed safe due to:
Task.Supervisor.async_nolink, so they are not linked tothe session — task crashes deliver
{:DOWN, …}(already handled) /{ref, result}, never{:EXIT, …}. Trapping changes nothing for tasks.trapping GenServer is intercepted by the
gen_serverloop and triggers normaltermination (which runs
terminate/2) rather than being dispatched tohandle_info. That interception is exactly what makes the fix work.{:EXIT, …}athandle_info; theexplicit clause is defense-in-depth for the exotic host-link case.
Tests
terminate/2fires when the session is stopped via the supervisor(
DynamicSupervisor.terminate_child, the mechanismSupervisor.stop_session/3uses), asserted via telemetry; the process is confirmed dead.
[:anubis_mcp, :server, :terminate]telemetry fires on the samepath, with the session id in metadata.
Both fail before the change (the session dies on
:shutdownwithout runningterminate/2) and pass after.mix lint(format + credo --strict + dialyzer)and
mix testpass.