Carry request context (assigns + headers/auth) into StreamableHTTP session auto-recovery - #208
Conversation
auto_initialize/1 ran the recovered init/2 and handle_session_expired/2 with empty frame.assigns and context, so a server that authenticates in a Plug (setting conn.assigns) saw an unauthenticated frame on the recovery path even when the request carried valid credentials. Add auto_initialize/2 taking the triggering request's transport context; apply its assigns to the recovered frame (replace, not merge: the session store round-trips assigns as string keys while live conn.assigns are atoms, so merging would leave stale keys) and thread the context into prepare_frame so headers/remote_ip/auth are populated. auto_initialize/1 is preserved.
Thread the request's transport context down the plug's unknown-session recovery branch (find_or_create_session -> start_and_auto_initialize_session) into Session.auto_initialize/2, so recovered sessions receive the triggering request's assigns and context. Only the request recovery branch is affected; initialize and existing-session branches are unchanged.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughProblemStreamableHTTP session recovery could lose the triggering request’s assigns and transport context. SolutionAdded RationaleRecovered sessions now use current request data instead of stale stored assigns, without changing normal initialization or existing-session behavior. WalkthroughSession auto-initialization now accepts transport context while retaining the one-argument compatibility API. Streamable HTTP request handling forwards context through session lookup and creation. Recovery applies non-empty request assigns to recovered frames and prepares them with transport context. Tests cover recovered assigns, request metadata, precedence over stored assigns, store-only recovery, and compatibility behavior. 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}
|
thanks for the contribution 💜 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/anubis/server/session.ex (1)
142-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winP2: Document the new
auto_initialize/2API.The new public arity has a spec but no
@docor usage example. Document thetransport_contextcontract and its backward-compatible relationship withauto_initialize/1.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6b1bf11-d7fa-4993-9642-85b611de364a
📒 Files selected for processing (1)
lib/anubis/server/session.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/anubis/server/session.ex (1)
142-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winP2: Document the new
auto_initialize/2API.The new public arity has a spec but no
@docor usage example. Document thetransport_contextcontract and its backward-compatible relationship withauto_initialize/1.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6b1bf11-d7fa-4993-9642-85b611de364a
📒 Files selected for processing (1)
lib/anubis/server/session.ex
🛑 Comments failed to post (1)
lib/anubis/server/session.ex (1)
678-679: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
P1: Do not mark normal initialization complete before
notifications/initialized.Setting
initialized: truein the initialize request allows subsequent requests before the required initialization notification, becauseis_server_initialized/2uses this flag as the protocol gate. Leave this flag false here; the existingnotifications/initializedhandler should remain responsible for setting it.🔧 Proposed fix
state = %{ state | protocol_version: protocol_version, protocol_module: protocol_module, client_info: client_info, - client_capabilities: client_capabilities, - initialized: true + client_capabilities: client_capabilities }📝 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.client_capabilities: client_capabilities
) ## Summary Enable `Process.flag(:trap_exit, true)` in `Session.init/1` so explicit supervisor shutdown (e.g. StreamableHTTP client `DELETE`) runs `terminate/2` and emits `[:anubis_mcp, :server, :terminate]` telemetry. ## Motivation `Anubis.Server.Session` did not trap exits while the transport processes (`streamable_http`, `sse`, `stdio`) already do. When a session is stopped via `DynamicSupervisor.terminate_child/2` (the DELETE path), OTP delivers a `:shutdown` exit to a non-trapping GenServer, which terminates immediately **without** calling `terminate/2`. That silently skips: - library `[:anubis_mcp, :server, :terminate]` telemetry - optional server-module `terminate/2` cleanup hooks Idle-expiry shutdown already worked because it uses callback-initiated `{:stop, ...}`. Fixes the outstanding part 4 of #204 (session idle expiry and assigns recovery are tracked separately in #208). ## Changes - `lib/anubis/server/session.ex`: set `trap_exit` at the top of `init/1` - `test/anubis/server/session_test.exs`: regression test stopping a session via `DynamicSupervisor.terminate_child/2` and asserting terminate telemetry ## Tests - `mix test test/anubis/server/session_test.exs` — 21 tests, 0 failures - `mix test test/anubis/server/session_test.exs:537` — new regression test passes ## Notes Session task work already uses `Task.Supervisor.async_nolink/2` + monitors, matching the existing transport `trap_exit` pattern. No new `handle_info` for `{:EXIT, ...}` is required. --------- Co-authored-by: syf2211 <syf2211@users.noreply.github.com> Co-authored-by: zoey <zoey.spessanha@zeetech.io>
…ssion auto-recovery (#208) **Addresses behaviour 1 in #204.** **Follows up #125**, which added transparent auto-recovery so a client caching a stale session id keeps working instead of erroring out. This completes that path for servers that authenticate per-request. ### Problem On the StreamableHTTP transport, when a non-`initialize` request arrives for an expired/unknown session, the recovery path (`find_or_create_session` -> `start_and_auto_initialize_session` -> `Session.auto_initialize/1`) runs the recovered `init/2` (and the optional `handle_session_expired/2`) with an **empty `frame.assigns` and empty `frame.context`**. `auto_initialize/1` takes no transport context, so the triggering request's `conn.assigns`, headers, remote IP, and auth claims are dropped. `Anubis.Server.Frame`'s moduledoc says assigns inherit from `Plug.Conn.assigns` for HTTP transports. That holds on the normal request path but not on recovery: a server that authenticates in a Plug (setting `conn.assigns.current_user`) and reads it back in `init/2` sees an *unauthenticated* frame on any request that triggers recovery, even though that request carried valid credentials. ### Change - Add `Session.auto_initialize/2 (session, transport_context)`; keep `auto_initialize/1` delegating with `nil` (backwards-compatible). - Thread the request's transport context down the plug's unknown-session recovery branch into `auto_initialize/2`. Only that branch is affected; `initialize` and existing-session branches are unchanged. The notification/response paths (which already return 404 for unknown sessions) are untouched. - In the recovery handler, apply the live request's assigns to the recovered frame and pass the context to `prepare_frame/2` so `frame.context` (headers/remote_ip/auth) is populated. Both `handle_session_expired/2` and `init/2` now see the populated frame, consistent with the normal request path. ### Rationale for replace assigns instead of merging On the recovery path the live request's assigns **replace** the frame's assigns (only when the request actually carries assigns. An empty/absent set leaves any store-restored assigns untouched). This is deliberate rather than a merge: The session store round-trips assigns through serialization - `Frame.to_saved`/ `from_saved` do no key normalization - so a JSON-backed store returns assigns with **string keys** (`"current_user"`) while live `conn.assigns` are **atom keys** (`:current_user`). A key-wise merge would leave *both* present: a host reading the atom key gets the live value, but a stale string-keyed value silently survives alongside it, leaving a latent auth bug. Atom-normalizing untrusted stored strings is an atom-table-exhaustion risk, so the recovery path treats the live per-request assigns as authoritative and replaces rather than reconciling. ### Tests - Recovered frame carries live request assigns (no store). - Recovered `frame.context` carries headers / remote_ip / auth. - Live assigns replace stale store assigns (the string-vs-atom key case above); the stale key does not survive. - Store-only recovery preserved when the request carries no assigns. - `auto_initialize/1` backwards-compat unchanged. Verified end-to-end by driving a `conn` with `conn.assigns` set through the plug to the recovery branch and confirming the recovered session's frame carries them. `mix lint` (format + credo --strict + dialyzer) and `mix test` pass. --------- Co-authored-by: zoey <zoey.spessanha@zeetech.io>
) ## Summary Enable `Process.flag(:trap_exit, true)` in `Session.init/1` so explicit supervisor shutdown (e.g. StreamableHTTP client `DELETE`) runs `terminate/2` and emits `[:anubis_mcp, :server, :terminate]` telemetry. ## Motivation `Anubis.Server.Session` did not trap exits while the transport processes (`streamable_http`, `sse`, `stdio`) already do. When a session is stopped via `DynamicSupervisor.terminate_child/2` (the DELETE path), OTP delivers a `:shutdown` exit to a non-trapping GenServer, which terminates immediately **without** calling `terminate/2`. That silently skips: - library `[:anubis_mcp, :server, :terminate]` telemetry - optional server-module `terminate/2` cleanup hooks Idle-expiry shutdown already worked because it uses callback-initiated `{:stop, ...}`. Fixes the outstanding part 4 of #204 (session idle expiry and assigns recovery are tracked separately in #208). ## Changes - `lib/anubis/server/session.ex`: set `trap_exit` at the top of `init/1` - `test/anubis/server/session_test.exs`: regression test stopping a session via `DynamicSupervisor.terminate_child/2` and asserting terminate telemetry ## Tests - `mix test test/anubis/server/session_test.exs` — 21 tests, 0 failures - `mix test test/anubis/server/session_test.exs:537` — new regression test passes ## Notes Session task work already uses `Task.Supervisor.async_nolink/2` + monitors, matching the existing transport `trap_exit` pattern. No new `handle_info` for `{:EXIT, ...}` is required. --------- Co-authored-by: syf2211 <syf2211@users.noreply.github.com> Co-authored-by: zoey <zoey.spessanha@zeetech.io>
2 participants
Addresses behaviour 1 in #204. Follows up #125, which added transparent
auto-recovery so a client caching a stale session id keeps working instead of
erroring out. This completes that path for servers that authenticate per-request.
Problem
On the StreamableHTTP transport, when a non-
initializerequest arrives for anexpired/unknown session, the recovery path (
find_or_create_session->start_and_auto_initialize_session->Session.auto_initialize/1) runs therecovered
init/2(and the optionalhandle_session_expired/2) with an emptyframe.assignsand emptyframe.context.auto_initialize/1takes notransport context, so the triggering request's
conn.assigns, headers, remoteIP, and auth claims are dropped.
Anubis.Server.Frame's moduledoc says assigns inherit fromPlug.Conn.assignsfor HTTP transports. That holds on the normal request path but not on recovery:
a server that authenticates in a Plug (setting
conn.assigns.current_user) andreads it back in
init/2sees an unauthenticated frame on any request thattriggers recovery, even though that request carried valid credentials.
Change
Session.auto_initialize/2 (session, transport_context); keepauto_initialize/1delegating withnil(backwards-compatible).recovery branch into
auto_initialize/2. Only that branch is affected;initializeand existing-session branches are unchanged. Thenotification/response paths (which already return 404 for unknown sessions) are
untouched.
frame and pass the context to
prepare_frame/2soframe.context(headers/remote_ip/auth) is populated. Both
handle_session_expired/2andinit/2now see the populated frame, consistent with the normal request path.Rationale for replace assigns instead of merging
On the recovery path the live request's assigns replace the frame's assigns
(only when the request actually carries assigns. An empty/absent set leaves any
store-restored assigns untouched). This is deliberate rather than a merge:
The session store round-trips assigns through serialization -
Frame.to_saved/from_saveddo no key normalization - so a JSON-backed store returns assignswith string keys (
"current_user") while liveconn.assignsare atomkeys (
:current_user). A key-wise merge would leave both present: a hostreading the atom key gets the live value, but a stale string-keyed value silently
survives alongside it, leaving a latent auth bug. Atom-normalizing untrusted stored
strings is an atom-table-exhaustion risk, so the recovery path treats the live
per-request assigns as authoritative and replaces rather than reconciling.
Tests
frame.contextcarries headers / remote_ip / auth.the stale key does not survive.
auto_initialize/1backwards-compat unchanged.Verified end-to-end by driving a
connwithconn.assignsset through the plugto the recovery branch and confirming the recovered session's frame carries them.
mix lint(format + credo --strict + dialyzer) andmix testpass.