Repository navigation
feat(session_store): supervise Redis store subtree to make restarts race-free - #242
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughProblemRedis session store restarts under the SolutionReworked RationaleBy making the pool and operational server children of the store subtree, OTP can synchronously terminate everything before restart, preventing WalkthroughThe Redis session store now runs as a supervisor containing Redix pool children and an internal 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
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/store/redis.ex (1)
146-170: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winP3 — Hibernate after initialization.
Return
{:ok, state, :hibernate}from both initialization-success paths.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26186f26-bdc2-49e9-b91e-96c038826125
📒 Files selected for processing (2)
lib/anubis/server/session/store/redis.extest/anubis/server/session/store/redis_test.exs
faisalnazir7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
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/store/redis.ex (1)
131-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPool-name derivation is duplicated between
init/1andServer.get_connection/1.The
:"anubis_#{conn_name}_#{i}"pattern here is re-derived independently inget_connection/1(line 350) as:"anubis_#{state.conn_name}_#{index}". They happen to agree today, but nothing enforces that — a future edit to one naming scheme without the other silently produces:noprocerrors for every Redis call. Extract a single private helper (e.g.pool_worker_name(conn_name, index)) shared by both sites.♻️ Proposed shared helper
+ defp pool_worker_name(conn_name, index), do: :"anubis_#{conn_name}_#{index}" + def init(opts) do ... pool_children = for i <- 1..pool_size do - child_id = :"anubis_#{conn_name}_#{i}" + child_id = pool_worker_name(conn_name, i) ...defp get_connection(state) when is_struct(state, State) do index = rem(:erlang.unique_integer([:positive]), state.pool_size) + 1 - :"anubis_#{state.conn_name}_#{index}" + Anubis.Server.Session.Store.Redis.pool_worker_name(state.conn_name, index) end
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c6c1541-2cc4-42b7-9c60-4b8f00d7e722
📒 Files selected for processing (2)
lib/anubis/server/session/store/redis.extest/anubis/server/session/store/redis_test.exs
…, dedupe cascade test setup
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/store/redis.ex (1)
131-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winP2 — Reject non-positive pool sizes before building children.
pool_size: 0enumerates1..0but stores0in state; the first Redis operation then crashes atrem(_, 0). Require a positive integer via the initialization schema before constructing the pool. 😎As per coding guidelines, “Use
import Perianddefschemafor all validation with schema definitions as module attributes.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bc04232b-5b15-4b5e-a161-72ba4afc8404
📒 Files selected for processing (2)
lib/anubis/server/session/store/redis.extest/anubis/server/session/store/redis_test.exs
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/store/redis.ex (1)
211-256: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winP2 — Validate
:pool_sizebefore starting the subtree.
pool_size: 0builds1..0, then every Redis-backed call crashes inrem(_, 0)insideget_connection/1. Parse and validate initialization options with a Peri schema, requiring a positive pool size before constructing children. 😎As per coding guidelines, use
import Peri/defschemafor validation and theparse_options!/1initialization pattern.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc608a19-669a-47e8-8700-41ac485d9558
📒 Files selected for processing (2)
lib/anubis/server/session/store/redis.extest/anubis/server/session/store/redis_test.exs
zoedsoupe
changed the title
🚀 Want to release this? --- ## [1.11.0](v1.10.0...v1.11.0) (2026-07-29) ### Features * **session_store:** supervise Redis store subtree to make restarts race-free ([#242](#242)) ([48c8c1a](48c8c1a)) ### Bug Fixes * **prompts:** wrap prompt content objects and map system_message to user role ([#234](#234)) ([2451bb7](2451bb7)) * **server:** make title option compile in use Anubis.Server.Component ([b04feed](b04feed)) * **server:** use restart :temporary for session processes ([#240](#240)) ([30bc4f5](30bc4f5)) * **sse:** buffer partial events across Finch chunks ([#245](#245)) ([beea2f6](beea2f6)) * Stream the client SSE GET instead of buffering it (server push never delivered) ([#231](#231)) ([a722c1b](a722c1b)) * **telemetry:** expose tool call success/failure in tool_call span metadata ([#246](#246)) ([ace5ecb](ace5ecb)) * **telemetry:** include client_info in initialize response event metadata ([#248](#248)) ([74ed457](74ed457)) * **telemetry:** namespace tool_call span under :anubis_mcp ([#244](#244)) ([561a96b](561a96b)) ### Tests * **server:** fix stale tool_call event name and function_exported? loading races ([7247d2c](7247d2c)) * **transport:** synchronize held SSE plug with Bypass teardown ([93c5a34](93c5a34)) ### Continuous Integration * fix dialyzer plt caching ([8424439](8424439)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
Closes #241
Problem
Anubis.Server.Session.Store.Redisfails to restart under the server's:one_for_allsupervisor, crashing with{:error, {:already_started, pid}}.The store registered two fixed names — the store GenServer (
name: __MODULE__)and a Redix pool supervisor started inside
init/1(:"anubis_#{conn_name}_ supervisor"). That inner supervisor was linked to the store but was not asupervised child, so its teardown ran asynchronously relative to the parent's
restart. Since
Anubis.Server.Supervisorruns:one_for_alland mounts thestore as a direct child, any sibling crash (transport / registry /
session-supervisor) restarted the store and re-ran the racy start against
still-registered names — either
{:already_started, pid}on the store name orthe
{:stop, error}"Failed to start Redis session store" path on the innersupervisor name. Under sustained restarts the endpoint flapped or failed to
recover until the node restarted.
Solution
Make the store a proper supervised subtree instead of patching the race:
Anubis.Server.Session.Store.Redisis now aSupervisor(type: :supervisorchild spec) whose children are the Redix connection pool plus an internal
Redis.ServerGenServer that answers theSession.Storebehaviour calls.Supervisor.start_link-inside-init/1orphan is gone; the pool lives inthe supervision tree.
:one_for_allparent restart, OTP shuts the whole store subtree downsynchronously (supervisor children wait
:infinity), releasing the store nameand every
:"anubis_#{conn_name}_#{i}"connection name before the storeis restarted. Restarts are race-free by construction — no
{:already_started},no process adoption.
The Redis wire behaviour (SETEX/GET/DEL/EXPIRE/SCAN, JSON, namespace, TTL, key
format, monotonic pool selection) is unchanged, and the public API signatures are
unchanged, so no caller (
session.ex,plug.ex) is touched.Rationale
The first pass rescued
{:already_started, pid}on both start paths (commit68733c9). Review correctly flagged that returning that pid to the supervisor isan OTP contract violation —
GenServer.start_linkdoes not link on{:already_started}, so the parent would hold an unlinked child it cannotmonitor. Rather than paper over the race, commit
cc2cabfremoves its source: asupervised subtree lets OTP coordinate start/stop and free names synchronously.
This resolves the ownership and pool-reuse concerns structurally rather than
defensively.