Guard registry lookups so draining nodes fail loudly, not wrongly

[brainlid]

Problem

During a rolling deploy a node receives SIGTERM, OTP shuts Sagents.Supervisor down, and the BEAM keeps running for the platform's whole grace period (commonly 30 to 60 seconds). The load balancer may still route requests to that node for all of it.

Every agent lookup in that window raised:

** (ArgumentError) errors were found at the given arguments:
  * 1st argument: the table identifier does not refer to an existing ETS table
    :ets.lookup(:"keys_Elixir.Sagents.Registry", {:agent_server, "conversation-..."})
    (horde) lib/horde/registry.ex:251: Horde.Registry.lookup/2
    (horde) lib/horde/registry.ex:393: Horde.Registry.whereis_name/2
    (elixir) lib/gen_server.ex:1355: GenServer.whereis/1
    (sagents) lib/sagents/session.ex:94: Sagents.Session.start/3

Horde.Registry.lookup/2 derives its ETS table name arithmetically (:"keys_#{registry}") and reads it with no liveness check. Named ETS tables die with their owning process, so the precondition for any read is "the local Horde.RegistryImpl is alive" — and nothing verifies it. Elixir's Registry has the same hole via Registry.key_info!/1. Neither backend can report the condition, so it escaped to callers as an :ets error.

This is not a narrow race. It is a stable failure mode for the entire drain period, and no client-side or in-process retry can recover on that node.

Investigating it surfaced a second, independent defect. A registry crash left running AgentSupervisors and AgentServers alive but unregistered — their :via names are established once at start and nothing re-registers them. Lookups then answered "not running", and the next request started a second AgentServer for a conversation that already had one. Both held and persisted state for it, with nothing reporting the conflict. In a multi-node cluster Horde repairs this from a peer's CRDT within ~200ms; on a single node nothing does.

Solution

Model "registry unavailable" as a value, and never let it collapse into "not registered."

Sagents.ProcessRegistry gains available?/0, which checks the ETS table each backend actually reads via :ets.whereis/1 — that answers :undefined instead of raising, and it tests exactly the precondition the read requires. On top of it:

• fetch/1 returns three distinct outcomes: {:ok, pid}, {:error, :not_registered}, {:error, :registry_unavailable}.
• lookup/1, select/1, count/0 and keys/1 cannot express the condition in their return values, so they raise the new Sagents.RegistryUnavailableError rather than answering [] or 0.

The split is deliberate and is the whole point of the change. A caller reading "nothing is registered" responds by starting an agent; on a draining node that manufactures the duplicate described above. fetch/1 and guarded/2 re-check available?/0 inside their rescue so a genuine ArgumentError still surfaces as itself, closing the microsecond TOCTOU window without swallowing unrelated bugs.

AgentServer.fetch_pid/1 is the new request-path lookup. AgentServer's internal safe_call/3 now resolves through it instead of handing GenServer.call a :via tuple, which puts the whole lifecycle API (execute/1, cancel/1, resume/2, add_message/3, reset/1, dismiss_interrupt/1) behind the guard in one place. Session.start/3 refuses to start an agent it cannot rule out. (GenServer.cast on a :via tuple was already safe — Elixir wraps name resolution in catch _, _ — so only the call and whereis paths needed changing.)

Stop receiving traffic before you stop being able to serve it. Sagents.ready?/0 exposes the signal for a readiness check, and docs/deployment.md documents the ordering that has to hold on SIGTERM: mark unready → LB stops routing → in-flight drains → tree stops. Previously only the last step happened.

Close the orphan/duplicate defect. Sagents.Supervisor is now :rest_for_one — the registry genuinely is a dependency of the dynamic supervisors after it, and :one_for_one asserted an independence that does not hold.

That change alone was not sufficient, which the tests caught. Horde.Registry.start_link/3 starts a supervisor, and the process registered as Sagents.Registry is its child:

Sagents.Supervisor
└── Sagents.Horde.RegistryImpl   <- the child Sagents.Supervisor sees (a supervisor)
    ├── Horde.RegistryImpl       <- the process registered as Sagents.Registry
    └── Sagents.Registry.Crdt

When the registered process crashes, Horde restarts it internally with fresh empty ETS tables and Sagents.Supervisor never observes a failed child, so the restart chain never fires. Elixir's Registry is shaped identically. Sagents.RegistryWatcher closes that gap: listed immediately after the registry, it monitors the registered process directly and stops when it dies, which is a child failure :rest_for_one acts on. Verified directly — after a kill, the agents supervisor moves and count_agents/0 is 0 rather than 2.

The trade is explicit: a registry crash now costs running agents instead of leaving invisible duplicates. Agent state is durable, so a restart is recoverable; a silent duplicate is not.

Changes

Core

• lib/sagents/process_registry.ex — Added available?/0, fetch/1, ensure_available!/1; routed lookup/1, select/1, count/0, keys/1 through a guarded/2 helper that raises rather than returning a plausible default.
• lib/sagents/registry_unavailable_error.ex — New. Named exception carrying :operation and :registry, with a message that explains the lifecycle cause and points at the readiness fix.
• lib/sagents/registry_watcher.ex — New. Monitors the registered registry process and stops when it dies, so a backend-internal restart reaches :rest_for_one. Polls (100ms) when the name is unclaimed so a mid-restart backend cannot become a restart storm. Stops with {:shutdown, {:registry_down, reason}} so it restarts without being reported as a crash.
• lib/sagents/supervisor.ex — Strategy changed :one_for_one → :rest_for_one; RegistryWatcher inserted between the registry and its dependents.
• lib/sagents.ex — Added Sagents.ready?/0.

Call sites threaded through the new API

• lib/sagents/agent_server.ex — Added fetch_pid/1; get_pid/1 now raises instead of answering nil; safe_call/3 resolves via fetch_pid/1; added call!/3 for calls with no room for an error tuple; queue_message folds the condition into its existing :no_server fallback, since raising out of a tool body is what that guard exists to prevent.
• lib/sagents/session.ex — start/3, stop/2 and session_info/2 use fetch_pid/1 and propagate {:error, :registry_unavailable}; running?/2 raises, since a boolean has no room for "cannot tell".
• lib/sagents/agent_supervisor.ex — get_pid/1 distinguishes :not_found from :registry_unavailable; do_wait_for_agent_ready/4 fails fast rather than burning its timeout on a node that cannot recover.
• lib/sagents/agents_dynamic_supervisor.ex — stop_agent/2 error union widened; start-wait fails fast on :registry_unavailable.

Documentation

• docs/deployment.md — New guide (261 lines). The failure explained concretely, the required shutdown sequence, readiness vs liveness (a draining node is not unhealthy — restarting it is the wrong response), supervision tree ordering, Fly.io and Kubernetes examples, mapping the error to a retryable 503, and how to verify it.
• docs/clustering.md — Added a "Taking a node out of the cluster" section and Sagents.ready?/0 to the inspection snippets, cross-linking the new guide. It previously had no shutdown or drain guidance at all.
• lib/sagents/supervisor.ex (@moduledoc) — Now states explicitly that Sagents.Supervisor must precede the Endpoint and why (reverse shutdown order), plus a section documenting the :rest_for_one rationale.
• lib/sagents/process_registry.ex (@moduledoc) — New "Availability" section covering why neither backend reports the condition and why :registry_unavailable must never be collapsed into "not registered".
• mix.exs — docs/deployment.md added to the docs extras.

Testing

• test/sagents/horde/rolling_deploy_test.exs — New, 7 tests, tagged :cluster and :slow, using LocalCluster with real Erlang nodes and members: :participation. Runs in ~24s and passed 4 of 4 repeat runs at varying seeds. Covers:
  • Exact reproduction — stop Sagents.Supervisor (what OTP does on SIGTERM) while leaving the node up, and assert the raise carries the reported message and the reported frames in order.
  • The window is the whole drain period — a probe spawned outside the supervision tree (the position an Endpoint occupies) polls continuously; the tail of the result sequence contains nothing but raises. This is what proves retry cannot recover.
  • Blast radius is one node — 3 nodes, agent placed and replicated, probes on all three; node1 raises on every request while node2 and node3 never raise once. This is the framing that makes readiness (not clustering) the fix.
  • Guarded API on a draining node — ready?/0 reports false and the API returns :registry_unavailable instead of raising, and an agent that is running is never reported as merely not running.
  • CRDT self-healing is real — a surviving peer repairs both membership and registrations after a registry crash, pointing at the same still-running process. Kept as a passing test deliberately: it documents the property and stops a disproven theory being re-litigated.
  • Single node takes the agent down rather than orphaning it — the regression test for the :rest_for_one + watcher fix.
• test/sagents/process_registry_availability_test.exs — New, 15 tests. Asserts the ETS table available?/0 checks is the one the backend actually reads, that fetch/1 keeps the two error cases distinct, and case-by-case that each raising function raises rather than answering nil, [] or 0.
• test/sagents/registry_watcher_test.exs — New, 5 tests. Stays up while the watched process lives, stops when it dies, waits rather than failing when the name is unclaimed, ignores unrelated DOWN messages, and asserts Sagents.Supervisor actually wires the chain :rest_for_one.
• test/support/sagents/cluster_test_helper.ex — Reusable pieces: stop_supervisor/1, kill_registry/0, probe_once/1 (captures a raise instead of propagating it), start_probe/2 (out-of-tree caller that keeps polling while the tree comes down), registry_table_present?/0, ready?/0, classify/1, member_nodes/1, start_agent/1.
• test/sagents/session_test.exs — Updated Mimic stubs from get_pid/1 to fetch_pid/1.

mix precommit already runs test --include cluster --include slow, so the deploy path stays covered.

Migration

No config, schema, or data changes. Existing code compiles unchanged.

• Two error unions were widened (AgentSupervisor.get_pid/1, AgentsDynamicSupervisor.stop_agent/2) — a case that matched them exhaustively needs a catch-all clause.
• AgentServer.get_pid/1 and Session.running?/2 now raise a Sagents exception where they previously raised an :ets one, so nothing that handled the old behaviour correctly changes meaning.
• Behavioural change worth calling out: a registry crash now restarts running agents rather than leaving them orphaned.

The real upgrade work is not in the code — it is wiring Sagents.ready?/0 into your readiness check per docs/deployment.md. Without that, the guarded API turns 500s into 503s but the requests still land on a node that cannot serve them.

Known gaps, deliberately out of scope

• The FileSystem lookups (FileSystemServer.whereis/1, FileSystemSupervisor.get_filesystem/1, SubAgentServer.whereis/1, SubAgentsDynamicSupervisor.whereis/1) were not converted to fetch/1. They now raise Sagents.RegistryUnavailableError instead of an :ets ArgumentError — already an improvement, and never a silent lie — but their error unions were left alone. Widening them touches more call sites than this bug justifies. Worth a follow-up for consistency.
• list_agents/0 derives ids from ProcessRegistry.keys/1 and silently drops unregistered children while count_agents/0 counts them. With :rest_for_one + the watcher in place the orphan cannot arise through this path, so surfacing them was not implemented, but the inconsistency is still there and worth a separate cleanup.
• test/sagents/horde/node_transfer_test.exs:166 ("agent process is redistributed on graceful shutdown") fails 100% of the time on this machine, in isolation and in a full run. This is pre-existing — the only shared code touched here is additive functions on ClusterTestHelper. An independent probe of the same scenario showed redistribution working correctly (both keys resolve on the surviving node within 1s, count_agents/0 goes to 1), so the discrepancy looks like the test's own wait/teardown logic. It needs its own investigation, since graceful shutdown is the rolling-deploy path and this test is the only coverage of it.
• :local backend: killing Sagents.Registry under config :sagents, :distribution, :local brought the whole tree down and left it down (no peer, no parent supervisor in the test). The precise mechanism was not isolated and is recorded as an observation only; the follow-up should be done against the :rest_for_one change rather than before it.