feat(container-runner): drain child before SIGTERM on engine pause

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5594 feat(container-runner): drain engine sleeps unless the idle …
• #5595 feat(container-runner): jitter idle timeout to avoid teardow …
• #5585 feat(container-runner): self-sleep on repeated actor start
• #5586 feat(container-runner): sleep on startup idle timeout
• #5589 chore(container-runner): tighten comments
• #5587 feat(container-runner): exit process when the last child sto …
• #5588 feat(container-runner): drain child before SIGTERM on engine … 👈
• #5584 feat(container-runner): drain children and engine concurrent …
• #5565 fix(envoy-client): ack terminating stop commands so pegboard …
• main

Get stack: forklift get 5588
Push local edits: forklift submit
Merge when ready: forklift merge 5588

_{change yloxstwo}

[claude]

Review: drain child before SIGTERM on engine pause

Re-checked against the current commit (86bc582, unchanged since the last review). I independently re-verified the main finding against rivetkit-core's FSM and rivetkit-rust's start.rs; it still holds. Summary below (same substance as the prior pass, since nothing changed).

1. The drain window reopens a registry race with run()'s watchdog, tripping a debug_assert on the happy path

stop_child (container-runner/src/actor.rs:39-49) always removes the actor from children()/ACTOR_CTXS before touching the child, so a stop-induced exit could never race the run() watchdog's arbitration (children().remove_async in actor.rs:237, which decides "was this exit deliberate or unexpected").

The new drain_then_stop_child (actor.rs:62-81), used from on_sleep, instead waits on child.wait_exit() (racing the drain timer and exit_token()) for up to DRAIN_GRACE (15 min default) without touching the registry, only calling stop_child (which does the removal) after the select! resolves.

Meanwhile run() is a separately spawned task (spawn_run_task in rivetkit-rust/packages/rivetkit/src/start.rs:248-264) blocked on that same child.wait_exit() for the actor's whole lifetime. Verified it is not cancelled while on_sleep runs: stop_run_task (start.rs:293-306) only cancels run_cancel after the event loop returns, i.e. after actor.on_sleep(ctx).await (start.rs:425) completes, so during the entire drain window both run() and drain_then_stop_child are racing on the same watch-channel change when the child exits naturally.

run() has fewer steps to reach children().remove_async(&actor_id) (actor.rs:237) than drain_then_stop_child -> stop_child, so it plausibly wins most of the time, routing a clean exit into the "unexpected exit" branch, which calls ctx.destroy() (actor.rs:249).

Confirmed this doesn't fully mis-fire in practice: since the generation is already in SleepGrace, the conflicting Stop from ctx.destroy() hits the current_reason != Some(reason) branch in rivetkit-core/src/actor/task.rs (e.g. lines 611-619, 707-718), which trips debug_assert!(false, "engine actor2 sends one Stop per actor instance"), logs "conflicting Stop during grace, ignoring", and drops it, so the generation still finalizes as slept, not destroyed. So the blast radius is smaller than it first looks, but it's still worth fixing:

• It panics in any debug build (debug_assert!) on a path that isn't actually the engine violating its Stop-once invariant, but a real, reachable race introduced by this runner.
• It triggers a spurious ctx.destroy()/stop round-trip on every drain-induced clean exit.
• A genuine crash (nonzero exit) during the drain window races the same way and error-bails from run() down a different branch, worth checking too.

Root cause: drain_then_stop_child doesn't mark the actor as deliberately-stopping before it starts waiting, unlike stop_child. Suggest having the drain path and run()'s watchdog agree up front (e.g. remove from children() / mark "sleeping" before entering the select!, mirroring stop_child) so the race can't happen at all, rather than relying on the FSM's conflict-suppression as a safety net.

2. effective_stop_grace() drops the previously-configurable engine-initiated stop grace, and some docs/comments are now stale

--stop-grace-secs / RIVET_STOP_GRACE_SECS is removed entirely, and effective_stop_grace() (main.rs) now always returns *SIGTERM_BUDGET regardless of whether the stop is engine-initiated (on_destroy) or a platform-signal shutdown. Previously an operator could configure a longer, independent grace (default 10s) for a normal on_destroy teardown, decoupled from the ~9s platform SIGTERM to SIGKILL budget. That's now collapsed to one short budget everywhere, with no flag to change it. That may be intentional given the new drain window replaces its purpose, but a few things suggest the change wasn't fully carried through:

• The comment on SIGNAL_SHUTDOWN still says "engine-initiated stops keep the full configured grace (their budget is the pool's drain grace period instead)", no longer true once effective_stop_grace() is unconditional.
• docs/content/docs/container-runner.mdx still documents --stop-grace-secs / RIVET_STOP_GRACE_SECS as a supported flag. If a deployed runner passes --stop-grace-secs as an actual CLI arg (not just env var), clap will now fail to parse and the container won't start, worth a deliberate deprecation callout since this is user-facing, documented config.
• container-runner/examples/unity-demo/Dockerfile also references the now-dead RIVET_STOP_GRACE_SECS.
• Consider documenting the new RIVET_DRAIN_GRACE_SECS (and RIVET_SIGTERM_BUDGET_SECS, already undocumented before this PR but now more central to behavior) in container-runner.mdx's configuration table.

3. No test coverage for the new drain/race behavior

drain_then_stop_child / effective_stop_grace are timing- and concurrency-sensitive, and per finding #1, easy to get subtly wrong even reasoning carefully. container-runner/tests/ today only covers pure functions (boot_id.rs, input.rs). Since ChildProcess already spawns a real subprocess (no mocking needed, consistent with the repo's "test against real infra" policy), an integration test that spawns a short-lived test binary, triggers on_sleep, and exits the child mid-drain would pin down the watchdog interaction and catch regressions here.

Minor

• drain_grace() / exit_token() accessors are clean, thin wrappers consistent with the rest of the file.
• The sleep_grace_period math (DRAIN_GRACE + SIGTERM_BUDGET + 5s) correctly outlives the worst-case drain_then_stop_child duration, with the caveat that it assumes the post-SIGKILL reap in ChildProcess::stop is near-instant (no explicit timeout on that final wait_exit()), which is a reasonable assumption but not a documented guarantee.