feat(cp): graceful shutdown — SIGTERM/SIGHUP handling and connection drain by Reese-max · Pull Request #1555 · openabdev/openab

Reese-max · 2026-09-30T18:03:01Z

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1532377620241449040 (control-plane discussion; this issue is the #1469 round-4 F12 follow-up)

openab-cp previously handled only ctrl_c (SIGINT). Under docker stop/ECS/k8s, SIGTERM is sent first and escalates to SIGKILL after the grace period, so the control plane never ran its shutdown path: registered sockets got a TCP reset instead of a WebSocket Close frame, and in-flight delegations were silently dropped.

What changed

main.rs waits on SIGINT/SIGTERM/SIGHUP (via tokio::signal::unix; non-Unix platforms keep ctrl_c()) and passes the signal into server::graceful_shutdown, which drains in place of axum::serve+ctrl_c.
AppState owns a process-wide CloseWatch broadcast channel; every WebSocket task subscribes immediately after upgrade — before cp/register — so even pre-registration sockets receive the Close frame.
Router::fail_all synthesizes terminals for every in-flight delegation (cp/delegate_result with target_disconnected to initiators, cp/cancel to serving runtimes), emits observer events, releases reserved sessions, and latches the router so a delegation admitted mid-drain is refused with SATURATED instead of being orphaned.
Connection tasks flush queued outbound frames (synthesized terminals/cancels first) and then send Close with code 1012 (service restart) and reason control plane shutting down, within shutdown_drain_secs (new config, default 5, 0 = immediate) plus at most one write_timeout_secs for a stalled close write.
After the drain completes or expires, the sweeper is aborted and the process exits cleanly — the sweeper no longer stops first while WS tasks are still running.
send_bounded polls the outbound write first (tokio::select! biased) so a frame already dequeued isn't dropped when the shutdown signal lands mid-send.
Final close frames go through send_final_frame — raced only against write_timeout, never the close watch — so a second latch mid-write (e.g. a lease sweep landing during the drain) cannot eat the close frame a peer was promised (132d42ef).
A second shutdown signal during the drain force-exits with the conventional 128 + signum status (132d42ef).
shutdown_drain_secs documented in cp.toml.example, docs/control-plane.md, and the ADR shutdown section.

Also on this branch (drift fixes needed for a green suite under current toolchains, kept as separate commits):

91432ca4 — resolve pre-existing clippy --all-targets warnings under rust 1.98 (all test-code).
6ca7417f — normalize workspace formatting under rustfmt 1.98 (the pinned base predates it; whitespace-only).
92c367b5 — make run_inline_script_timeout resilient to transient spawn errors (observed flaking in a detached replay run).

Review Contract

Goal

Under docker stop/ECS/k8s (SIGTERM first, SIGKILL after grace), openab-cp must run an ordered shutdown: synthesize terminal results for in-flight delegations where possible, send WebSocket Close frames (1012 + reason) to every connected socket including pre-registration ones, drain briefly, then stop the sweeper and exit — instead of leaving runtimes with a TCP reset and dropped delegations.

Non-goals

No rolling reconnect or session migration; close code 1012 signals clients to reconnect to a restarted instance.
No change to the delegation protocol surface (methods, params, error codes) — the synthesized terminal reuses the existing target_disconnected code.
Windows graceful-signal parity beyond the existing ctrl_c fallback (Windows service stop signals are out of scope).
Per-request admission once shutdown starts mid-drain: refused SATURATED is terminal for the attempt — no graceful retry-with-later-drain queue.

Accepted Residual Risks

A connection task that ignores close ordering (stuck write past write_timeout_secs, or a peer that never ACKs the close) is force-dropped at the drain deadline — bounded by shutdown_drain_secs + write_timeout_secs.
In a µs-window race, a per-connection policy close (e.g. lease expiry) latching between fail_all's enqueues and the process-wide begin_shutdown sends code 1008 and drops that peer's queued synthesized terminals — the peer was already being evicted; a valid Close frame is still delivered.
shutdown_drain_secs = 0 means "signal and exit": close frames are sent but the process does not wait for the outbound flush.
Baseline flake risk accepted upstream: run_inline_script_timeout transiently flakes under parallel load on the base revision; this PR carries the retry fix as a separate test-only commit.

Acceptance Criteria

SIGTERM and SIGHUP each drive the full drain and a clean exit: sigterm_and_sighup_run_the_drain_and_exit_cleanly spawns the real binary and asserts both signals close sockets with 1012 + shutdown reason and the process exits successfully.
Registered initiator receives synthesized cp/delegate_result (target_disconnected) and the serving runtime receives cp/cancel before Close: shutdown_synthesizes_terminals_then_closes_with_restart.
The same, against a real process, with a delegation in flight — the ordering that only a process can prove, because the frames must be on the wire before main returns: sigterm_resolves_in_flight_delegations_before_the_process_exits (new).
A connected socket that never sent cp/register still receives 1012 + shutdown reason: shutdown_closes_sockets_that_never_registered, plus a deterministic in-process check for a socket that upgrades after the drain latched: a_socket_upgraded_after_the_drain_latched_is_never_registered (new).
A delegation submitted after fail_all drained the table is refused SATURATED (not orphaned): delegate_after_fail_all_is_refused_instead_of_orphaned.
shutdown_drain_secs defaults to 5 and bounds the flush, the close write and the drain; 0 is rejected (it would emit observer terminals and then exit before any frame reached a wire).
A second signal latching while a close frame is in flight still delivers the 1012 close, never a bare reset: a_second_signal_latching_mid_close_still_sends_a_close_frame.
Nothing a peer does can pin or truncate the shutdown: a half-sent HTTP request cannot hold the process past the budget (a_stalled_http_request_cannot_pin_the_shutdown, new), a second signal still force-exits 143 (a_second_signal_exits_immediately_while_the_drain_is_pending, new), the drain budget is not the process lifetime (the_drain_budget_bounds_the_shutdown_not_the_process_lifetime, new), and an undeliverable terminal is logged rather than dropped silently (shutdown_survives_an_initiator_whose_queue_is_already_full, new).
cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace, and cargo check -p openab-cp --all-targets --target x86_64-pc-windows-gnu all pass; detached clean-replay verification passed end to end.

Pre-landing review

Five read-only review passes; every blocker and should-fix finding was fixed with a
regression test that fails against the code it replaced. The two that mattered
most, both found by reproducing the binary:

The drain ran after axum::serve's graceful shutdown resolved. That wait
ends when the last in-flight HTTP request finishes, so a peer that connected
and then stopped sending pinned it — no synthesized terminals, no Close
frames, just a process waiting to be SIGKILLed. The listener is now its own
task, the signal is handled in main, and the drain runs immediately.
A single ctrl-c could force-exit without draining. On unix
tokio::signal::ctrl_c() is signal(SignalKind::interrupt()), so a
registered SIGINT stream plus a ctrl-c arm meant one listener set saw one
ctrl-c and the escape-hatch task read it as the second signal: measured 8
force-exits out of 10, each one a TCP reset. One listener per signal now;
verified 8/8 graceful exits and 3/3 forced 130s afterwards.

Also fixed from review: the shutdown bound was armed at startup rather than at
the signal (the process exited ~6s after boot with the default config); the
per-connection close writes are remainders of one latched deadline so
shutdown_drain_secs really is the ceiling; an undeliverable terminal is logged;
a mid-drain undeliverable result no longer blames backpressure; docs, the field
rustdoc and cp.toml.example state the same bounds the code keeps.

Toolchain normalization

6ca7417f / 91432ca4 reformat the workspace and fix test-only clippy lints: the
fixed base is not cargo fmt --check/clippy-clean under rustc 1.98, so the
project's own gates fail there. A token-level comparison against the base
confirms those commits carry no production semantics — formatting only.

Follow-ups

If a deployment needs per-connection close ACKs before exit, a drain loop keyed on transport confirmation can be added behind a new config flag.
The lease-expiry-vs-drain µs-race could be eliminated by draining rx whenever frames are pending rather than keying on which close signal latched.
Broader hardening: expose drain progress (connections remaining) as a metric/log during shutdown.

Generated with Devin

Fixes pre-existing test-code lints that `cargo clippy --workspace --all-targets -- -D warnings` reports on current stable: literal-bool assert_eq, manual str::repeat, unwrap_or on literal Some/None, get().is_some()/is_none() on maps, an unused cfg(test) helper, and one intentional ENV_LOCK hold across .await (env-serialization in tests). (cherry picked from commit ae0e563) Refs openabdev#1475 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

…drain docker stop, ECS and k8s send SIGTERM before SIGKILL, but openab-cp only handled ctrl-c: the default disposition killed the process, so connections saw TCP resets, in-flight delegations vanished without terminal frames, and the sweeper was aborted while WS tasks were still mid-write. Now all three signals run one drain path: every in-flight delegation is resolved first (initiators get a synthesized target_disconnected terminal, serving runtimes a cp/cancel, both stamped "control plane shutting down"), then every connection — including pre-registration sockets the registry cannot reach — flushes its outbound queue and closes with code 1012 (service restart) inside shutdown_drain_secs, and only then is the sweeper stopped and the process exits. A shutting_down latch on Router, checked under the same inflight lock that fail_all drains, refuses admissions racing the drain with SATURATED — no delegation can be accepted after the last terminal synthesis. Refs openabdev#1475 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Whitespace-only `cargo fmt --all` pass over files outside the feature change: the workspace predates the current stable rustfmt, so `cargo fmt --all -- --check` fails on ~36 unrelated files and blocks the fmt gate for any change. Refs openabdev#1475 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Under heavy parallel load (e.g. full workspace `cargo test` on a busy box), child spawn/wait can transiently fail with a non-timeout error before the 1s timeout path is reached — the previous exact-message assertion then flaked (observed in a detached replay run). Retry up to 3 attempts and require at least one to hit the timeout path; a real regression that never reaches the timeout arm still fails.

Pre-landing review of the shutdown drain found the last frame raced the close watch: a second latch landing mid-write (a lease sweep during the drain, or the process signal during a per-connection close) aborted the close frame entirely — the peer saw a bare TCP reset on precisely the path that exists to prevent one. Final frames now go through send_final_frame, bounded only by write_timeout since the connection is already committed to ending. A second shutdown signal during the drain is also now honored as a force-quit: exit immediately with the signal's conventional status (128 + signum), instead of waiting out the drain budget. Refs openabdev#1475 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

The in-process regressions cover the drain's individual steps, but the part that actually breaks in a container is a race between the drain and process exit: the synthesized terminals and the 1012 close frames must be on the wire before `main` returns, or the runtime sees the same TCP reset the shutdown path exists to prevent. Only a real process sent a real SIGTERM reproduces that ordering. The test therefore spawns the binary on an ephemeral port, holds one delegation in flight, and sends SIGTERM: the initiator must receive its `target_disconnected` terminal naming the admission, the serving runtime its `cp/cancel`, both sockets a 1012 close carrying the reason, and the process exit 0 rather than dying by signal. A second test races fresh upgrades against the drain and asserts none is ever acked — a CP that is leaving must not admit a registration it cannot serve. Refs openabdev#1475

Pre-landing review reproduced two defects in the shutdown sequence, both of which end with the orchestrator's SIGKILL doing the closing the CP was supposed to do: - The drain ran AFTER `axum::serve`'s graceful shutdown resolved. That wait ends when the last in-flight HTTP request finishes, so a peer that opened a socket and then stopped sending — a half-sent request, a stalled upgrade — pinned it indefinitely: no `fail_all`, no synthesized terminals, no close frames, just a process that sat there until it was killed. The shutdown sequence is now spawned at the signal, and the listener's own wait is bounded by the same budget as the drain. - The second-signal force-quit was armed only after that wait, so the operator's "stop waiting" never fired in exactly the case it exists for. It is armed at the signal and left armed until the process is gone, so it covers the drain and the listener's bookkeeping alike. Also tightened, from the same review: - Every budget in the drain is now a remainder of ONE deadline, latched with the close signal. Each connection previously started a fresh `shutdown_drain_secs` when it left its main loop, so a late one could be granted a longer life than the drain it is part of — and the close write, bounded by `write_timeout_secs`, could push the process exit past the documented ceiling. `shutdown_drain_secs` is now the single bound, and the docs say so. - `shutdown_drain_secs = 0` is refused: it emitted the observer-side `delegation_completed` events and then exited before any frame reached a wire, so the two surfaces disagreed by construction. - A terminal that the drain cannot queue (full peer queue) is now logged at warn level naming the agent, instead of being dropped silently — the drain is that delegation's last chance, and the surviving outcome is the initiator's own deadline reconciliation. - A `cp/delegate_result` that arrives during the drain no longer blames backpressure: the queue was closed deliberately, not overflowed, and the connection is already closing with the shutdown reason. Regression tests: `a_stalled_http_request_cannot_pin_the_shutdown` and `a_second_signal_exits_immediately_while_the_drain_is_pending` drive the real binary with a half-sent HTTP request and a SIGTERM pair (both fail against the previous `main`), and `shutdown_survives_an_initiator_whose_queue_is_already_full` pins the refusal path. The mid-close test now asserts the actual 1012 close instead of accepting any close frame, and the spawned CP's stdio is no longer inherited into the harness log. Refs openabdev#1475

Second review round. The previous commit armed `timeout(hard_bound, …)` in `main` before the signal ever arrived, and `axum::serve` is lazy — so the "bound" started ticking at startup and became the process's LIFETIME: with the default `shutdown_drain_secs = 5` the CP exited 0 about six seconds after boot, having served nobody, and no signal path ever ran. Trading "the CP dies mid-delegation" for "the CP vanishes on its own" is not a fix. The bound now applies where it belongs: the listener runs as its own task, the signal is awaited in `main` and the drain happens inline (bounded, as before), and only the listener's remaining graceful wait gets a one-second slack — a deliberate allowance, not a reasoned budget, since `/health` answers immediately and `/cp` finishes its response as soon as the handshake is done: anything still pending is a peer that connected and stopped talking. Also from the same round: - The second-signal escape hatch reused freshly registered streams, so a repeat arriving between the first signal resolving and that registration was discarded by tokio's registry — no listener, no delivery. `ShutdownSignals` now registers once and is awaited twice: the first wait consumes the signal that starts the drain, the second (in the force-quit task) waits for the next one, with no gap and no risk of the first signal being consumed twice. - The early-return close paths — a socket caught mid-upgrade by the drain, a pre-registration close, a refused register frame, the observer cap — still wrote with the full `write_timeout_secs`, so a task could outlive the drain by 30s while the docs claimed `shutdown_drain_secs` was the ceiling. They now spend what is left of the drain (`final_frame_budget`). - A drain that exhausts its budget still exits 0 (it always did); the docs now say so, since a non-zero status would read as a crash. - `spawn_cp_process` removes its temp config through an RAII guard, so a panicking assert no longer leaves one per run in /tmp. Regression test: `the_drain_budget_bounds_the_shutdown_not_the_process_lifetime` starts a CP with a one-second drain, waits three, and asserts it still registers a runtime — it fails against the previous `main`, which had already exited. Refs openabdev#1475

…able Third review round (verdict: pass, nits addressed). - SIGINT now goes through the same boot-registered `Signal` machinery as SIGTERM and SIGHUP instead of `tokio::signal::ctrl_c()`, which re-registers its handler on every call: a second ctrl-c arriving between one wait returning and the next being created was dropped by tokio's registry, so the escape hatch could miss exactly the signal it exists for. ctrl-c stays as an arm for hosts where the SIGINT stream cannot be registered. - `CfgGuard` and `spawn_cp_process` in the lifecycle tests are now both `#[cfg(unix)]` again (a doc comment had captured the attribute), so a Windows `--all-targets` check compiles. - `Cp::connect` reports the child's status when its retries run out. A CP that exited during startup and a CP that is slow are indistinguishable from the client side, and the port-grab-then-release race is exactly where a one-in-a-hundred flake lands; the status makes the next occurrence actionable instead of guesswork. - The mid-drain registration race waits for the shutdown latch before racing it, since a `cp/register` that completes before the latch is legitimately acked and asserting otherwise would be asserting something untrue. - Docs: the listener's one second of slack after the drain is now stated, and the ADR no longer describes a spawn ordering the code stopped using. Refs openabdev#1475

Review round four caught a regression I introduced in the previous commit, and measured it: on unix `tokio::signal::ctrl_c()` IS `signal(SignalKind::interrupt())`, so selecting on a registered SIGINT stream AND ctrl-c handed the SAME ctrl-c to two listeners. The force-quit task then read the first signal's notification as a second one and called exit(130) immediately — one ctrl-c skipped the drain in 8 runs out of 10, which is the TCP-reset failure this whole issue exists to remove. Exactly one listener per signal now: the registered stream when it exists, ctrl-c only where it could not be registered (where it is also the only thing that can work). Verified against the built binary: a single SIGINT now drains and exits 0 with the 1012 close in 8/8 runs, and a double SIGINT still force-exits 130 in 3/3. Also from the same round: the mid-drain registration test slept 250 ms before racing the latch, which made it vacuous — the CP exits well inside that window, so every attempt took the "connection refused" arm and the test asserted nothing. Replaced with a deterministic in-process test that latches the drain directly (`AppState::begin_shutdown`, now public) and asserts a socket that upgrades afterwards gets the shutdown close instead of a registration. Refs openabdev#1475

Final review round (verdict: pass, nits closed). `shutdown_drain_secs` bounds the flush, the close write and the drain; after it, the CP allows the HTTP listener one further second for its own bookkeeping before dropping it. `docs/control-plane.md` already said so, the ADR and `cp.toml.example` did not — the example claimed the process exit itself fit inside the budget. Also: where the SIGINT stream could not be registered, `ctrl_c()` resolves immediately with an error, which `recv` was naming as a SIGINT — a shutdown signal that never arrived. It now waits instead, and says the CP will not stop on ctrl-c. Refs openabdev#1475

`shutdown_drain_secs` bounds the flush, the close write and the drain; the process then allows the HTTP listener one further second for its own graceful wait. The ADR, `docs/control-plane.md` and `cp.toml.example` now say so — the rustdoc on the field itself did not, and a reader of `config.rs` would conclude the exit is hard-bounded at the budget. Also: the run-on ADR clause that referred forward to "the listener slack below", and the warning text for a failed ctrl-c registration, which claimed the CP "will not stop on ctrl-c" — the OS default still kills it; only the graceful path is gone. Refs openabdev#1475

The previous commit appended the listener-slack paragraph to this rustdoc but left the earlier sentence claiming the budget bounds "the flush, the close write and the process exit" — one paragraph contradicting the next, and the last remaining place in the repo asserting a bound the code does not keep. The field bounds the drain; the exit allows the listener one further second after it, and an operator sizing a grace period needs to know that. Refs openabdev#1475

github-actions · 2026-10-04T17:21:26Z

Caution

This PR has been waiting on the author for more than 2 days (labeled pending-contributor since 2026-10-02).
It will be automatically closed in 24 hours if there is no update.

@Reese-max — You must add a new comment on this PR to remove the closing-soon label and keep it open. Pushing commits alone is not sufficient. Feel free to reopen a new PR later if it gets closed and you want to pick it back up.

github-actions · 2026-10-05T23:30:05Z

🔒 Auto-closing: this PR has had the closing-soon label for more than 1 days without activity from the author.

If you'd like to continue working on this, please submit a new PR and link to this one if necessary.

devin-ai-integration Bot added 4 commits October 1, 2026 01:36

Reese-max requested a review from thepagent as a code owner September 30, 2026 18:03

openab-app Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Sep 30, 2026

openab-app Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Sep 30, 2026

github-actions Bot added the pending-contributor label Sep 30, 2026

worker-w15 added 8 commits October 1, 2026 21:40

github-actions Bot added pending-maintainer pending-contributor needs-rebase closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. and removed pending-contributor pending-maintainer labels Oct 1, 2026

github-actions Bot closed this Oct 5, 2026

Navigation Menu

Repository navigation

feat(cp): graceful shutdown — SIGTERM/SIGHUP handling and connection drain - #1555

feat(cp): graceful shutdown — SIGTERM/SIGHUP handling and connection drain#1555
Reese-max wants to merge 13 commits into
openabdev:mainfrom
Reese-max:devin/issue-1475

Reese-max commented Sep 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 4, 2026

Uh oh!

github-actions Bot commented Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Reese-max commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Review Contract

Goal

Non-goals

Accepted Residual Risks

Acceptance Criteria

Pre-landing review

Toolchain normalization

Follow-ups

Uh oh!

github-actions Bot commented Oct 4, 2026

Uh oh!

github-actions Bot commented Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Reese-max commented Sep 30, 2026 •

edited

Loading