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

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #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

Overall the split between "unsignaled drain window" (DRAIN_GRACE) and "SIGTERM→SIGKILL window" (SIGTERM_BUDGET) is a clean idea, and the doc comments explaining the reasoning (e.g. why sleep_grace_period must outlast both) are genuinely helpful. Two issues worth addressing before merge:

1. Drain window reopens the exact race the registry-removal ordering was built to prevent (container-runner/src/actor.rs:39-83, 229-251)

stop_child relies on this invariant: "Remove from the registry FIRST so the watchdog treats the exit as deliberate, then stop." That only works because, previously, the registry removal (children().remove_async) happened synchronously before anything that could let the child exit. run()'s watchdog (container-runner/src/actor.rs:229) treats "I won the remove_async race" as "this exit was unexpected" and either bails an error or calls ctx.destroy().

drain_then_stop_child (added at container-runner/src/actor.rs:65) now waits up to DRAIN_GRACE (15 min default) for the child to exit before calling stop_child, i.e. before the registry entry is ever removed. If the child exits naturally during that window — which is the entire point of the feature, not an edge case — run()'s watchdog (also blocked on child.wait_exit()) wakes up at the same time and now races stop_child for the children().remove_async call:

• If run() wins: it treats the drain-triggered exit as "unexpected," and either calls ctx.destroy() (exit code 0) or anyhow::bail!s an error (nonzero exit), while on_sleep concurrently also completes and reports the sleep as successful. That's a real behavioral regression: an actor that was supposed to go to sleep (preserving its persisted launch spec for a later wake) can instead get destroyed or reported as crashed, depending on scheduling.
• As a side effect, both paths then call release_child_port for the same port (run() at line 238, stop_child at line 48). RESERVED_PORTS is an un-versioned HashSet, so a second release can free a port that a different, concurrently-starting actor on the same container instance has since reserved. The pre-bind TcpStream::connect check in ChildProcess::spawn will usually catch the resulting collision and fail the new start with a clear error, but that's still a spurious start failure caused by this race.

Suggested fix: move the registry deregistration (children().remove_async / ACTOR_CTXS.remove_async) to the top of drain_then_stop_child, before the tokio::select!, so the watchdog can never win the race no matter how long the drain takes. stop_child can keep its own removal calls (they'd just become idempotent no-ops when called via the drain path).

2. Stale docs for the removed RIVET_STOP_GRACE_SECS env var (container-runner/examples/unity-demo/Dockerfile:32)

RIVET_STOP_GRACE_SECS / --stop-grace-secs is removed from main.rs (previously backing RunnerConfig::stop_grace), but container-runner/examples/unity-demo/Dockerfile still documents it:

#   RIVET_STOP_GRACE_SECS    SIGTERM->SIGKILL grace for the child. (default 25)

This env var is now silently ignored — worth updating that comment block to document RIVET_SIGTERM_BUDGET_SECS (default 9) and RIVET_DRAIN_GRACE_SECS (default 900) instead. Also worth a callout that removing the CLI flag entirely (rather than deprecating it) means any existing deploy that passes --stop-grace-secs explicitly will now fail to start (clap errors on an unknown argument), not just silently ignore it.

Minor

• No test coverage was added for the new drain/select logic. Given it's timing- and concurrency-sensitive, even a narrow test around ChildProcess/registry interaction (if the existing test harness supports spawning a fake child) would help guard against the race in point 1.

Nice find splitting the "let it finish work" window from the "kill it" window conceptually — just want to make sure the registry-ordering invariant survives the split.