feat(container-runner): drain children and engine concurrently on SIGTERM - #5584
Conversation
|
Stack for rivet-dev/actors
Get stack: change tpptkxqq |
|
🚅 Deployed to the actors-pr-5584 environment in rivet-frontend
|
cbecbab to
203b9d3
Compare
729e8f8 to
8b1ec5f
Compare
|
Review Small, focused change: Potential issues
Other notes
Overall this looks like a solid, well-reasoned tightening of the shutdown budget; the two points above are worth a quick sanity check but are not blockers. |
203b9d3 to
3411262
Compare
8b1ec5f to
b062fac
Compare
b062fac to
e0fd0b9
Compare
3411262 to
346d4c7
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| tracing::warn!("engine drain exceeded the signal budget"); | ||
| } | ||
| }; | ||
| tokio::join!(drain, stop_all_children(*SIGTERM_BUDGET)); |
There was a problem hiding this comment.
Duplicate concurrent stop of the same child (broken "backstop" invariant).
stop_all_children(*SIGTERM_BUDGET) now runs concurrently with the engine drain via tokio::join!, instead of only after the drain completes/times out. stop_all_children's retain_async grabs its own Arc<ChildProcess> from CHILDREN and calls child.stop(...), while each actor's own on_destroy → GameServer::stop_child (actor.rs:39-49) independently grabs its own Arc (from self.child) and also calls .stop(...) on the very same underlying ChildProcess. Neither side checks whether the other already claimed the child.
Because ChildProcess::stop() only early-returns via has_exited() (child.rs:209), two concurrent calls both pass that check, both send SIGTERM, and (if the process does not exit within grace) both send SIGKILL and print duplicate "sending SIGTERM"/"sending SIGKILL"/"child stopped/killed" log lines for a single pid on effectively every signal shutdown — not just the rare straggler case the doc comment still describes ("belt-and-suspenders sweep... so children are never orphaned"). This is now guaranteed duplicate work rather than a true backstop.
| tracing::warn!("engine drain exceeded the signal budget"); | ||
| } | ||
| }; | ||
| tokio::join!(drain, stop_all_children(*SIGTERM_BUDGET)); |
There was a problem hiding this comment.
The sweep bypasses effective_stop_grace(), so it ignores a shorter configured --stop-grace-secs in exactly the fallback case it exists for.
effective_stop_grace() (line 193-200) is documented as "the configured --stop-grace-secs normally, capped to the platform budget" and is what on_destroy uses via stop_child. But stop_all_children on the signal path is called directly with the raw *SIGTERM_BUDGET (line 420), not effective_stop_grace().
In the common case this is masked because both stop paths race on the same pid and whichever sends SIGKILL first wins (see the sibling comment on this line). But in the exact scenario this sweep exists for — the engine drain hangs/never invokes on_destroy for an actor — the sweep is the only path stopping that child, and it will wait up to the full SIGTERM_BUDGET (9s default) before SIGKILL, silently overriding an operator's explicit shorter --stop-grace-secs. Consider threading effective_stop_grace() (or stop_grace.min(*SIGTERM_BUDGET)) into the sweep's grace instead of the raw budget.
| .unwrap_or(9) | ||
| .max(3); | ||
| Duration::from_secs(secs) | ||
| }); |
There was a problem hiding this comment.
The new full-budget-concurrent model leaves almost no margin for the still-unbounded shutdown tail.
Previously the signal path was bounded to ~61% of SIGTERM_BUDGET (60% drain + 1s sweep) inside a 10s budget, leaving roughly 3s of slack. Now drain and stop_all_children each individually run up to the full SIGTERM_BUDGET (default dropped 10s→9s) concurrently via tokio::join!, so the bounded phase alone can consume ~9 of the ~10s platform deadline.
After that join, async_main still does serve_shutdown.cancel(); serve.await (main.rs:425-431), which waits on serverless_http::serve's graceful shutdown — unbounded by any timeout, and dependent on in-flight websocket/SSE connections (including the engine's own long-lived /start request) draining. If any such connection is still open when the join finishes, this tail step has roughly 1s of margin instead of the previous ~3s before the platform's real SIGKILL lands, making a hard kill mid-exit more likely under load. Consider bounding serve.await with its own timeout, or keeping some of the previous margin unconsumed by the bounded phase.
| } | ||
| println!("runner: shutdown, stopping {} child(ren)", children.len()); | ||
| for child in children { | ||
| join_all(children.into_iter().map(|child| async move { |
There was a problem hiding this comment.
The sweep can now swallow a real child crash as a "deliberate stop" during a plain SIGINT.
stop_all_children's retain_async (main.rs:444-449) removes every actor's entry from the global CHILDREN map essentially as soon as this function starts running inside tokio::join! (line 420) — well before the child has actually exited, and well before on_destroy's own paced per-actor teardown would reach it. GameServer::run()'s watchdog (actor.rs:204-229) decides "was this exit deliberate" by racing children().remove_async(&actor_id) (actor.rs:212) against the child's actual exit: if the entry is already gone by the time the child exits, run() treats it as a deliberate stop and takes no action, even if the child actually crashed independently.
Since a local SIGINT (developer Ctrl-C) also sets SIGNAL_SHUTDOWN and takes this same tokio::join! branch (line 400) without calling crash_all_actors (that's gated on PLATFORM_RECLAIM, which SIGINT never sets, line 406), a child that happens to crash independently around the same time as a Ctrl-C shutdown will have its CHILDREN entry vacuumed by the sweep before run() observes the exit, so the crash is silently reported as a clean/deliberate stop instead of an error. In the old sequential code the sweep only ran after the drain completed or timed out, so it could not race ahead of a genuine crash like this.
| tracing::warn!("engine drain exceeded the signal budget"); | ||
| } | ||
| }; | ||
| tokio::join!(drain, stop_all_children(*SIGTERM_BUDGET)); |
There was a problem hiding this comment.
[correctness] Duplicate concurrent stop on the same child (CONFIRMED)
stop_all_children (this tokio::join!) and GameServer::stop_child (called from on_destroy/on_sleep in actor.rs:39-57) each hold an independent Arc<ChildProcess> for the same child: one via the global CHILDREN map, one via GameServer.childs TokioMutex. Previously the drain ran to completion (or timed out) before the sweep, so in the common case on_destroy already stopped and removed the child and the sweep was a true no-op backstop.
Now that drain and stop_all_children(*SIGTERM_BUDGET) run concurrently, both code paths can call child.stop() on the same ChildProcess at the same time on every ordinary signal shutdown, not just as a rare straggler case. ChildProcess::stop() (child.rs:206-230) has no synchronization between them, so this produces duplicate SIGTERM/SIGKILL sends and duplicate release_child_port calls, plus a narrow TOCTOU window between the two callers unsynchronized has_exited() checks (a stray signal could hit a recycled pid).
Failure scenario: SIGTERM arrives with actor A running. stop_all_childrens retain_async grabs As child and starts stop(). Concurrently, the engine drain reaches As on_destroy -> stop_child, which independently takes self.child and also calls stop(). Both send SIGTERM/SIGKILL to the same pid and log independently: duplicated work on every shutdown, contradicting the belt-and-suspenders framing in the surviving doc comment on stop_all_children.
| tracing::warn!("engine drain exceeded the signal budget"); | ||
| } | ||
| }; | ||
| tokio::join!(drain, stop_all_children(*SIGTERM_BUDGET)); |
There was a problem hiding this comment.
[correctness] Sweep defeats the watchdogs deliberate-vs-unexpected-exit detection (CONFIRMED)
stop_all_childrens retain_async sweep (lines 442-449) removes every actors entry from the global CHILDREN map essentially immediately when this join starts polling, well before each actors own on_destroy (paced by the engine drain) would normally reach it and remove it itself. GameServer::run()s watchdog (actor.rs:199-230) decides deliberate vs unexpected exit by racing children().remove_async(actor_id) against the childs actual exit: whichever caller removes the entry first wins.
Since the sweep now wins that race for essentially every actor almost immediately (not just stragglers), if a child crashes independently around the time of a SIGINT (developer Ctrl-C - note this path does NOT call crash_all_actors, since that is gated on PLATFORM_RECLAIM which is only set for an actual platform SIGTERM), run() will find the CHILDREN entry already gone and silently treat the crash as a deliberate stop instead of reporting an errored/crashed actor.
Failure scenario: a developer hits Ctrl-C while a game-server child is independently crash-looping. The sweep evacuates CHILDREN within microseconds of the signal handler firing. The childs subsequent unexpected exit is picked up by run(), but remove_async returns None (already removed by the sweep), so no anyhow::bail! and no ctx.destroy() - the crash is silently absorbed.
| } | ||
| stop_all_children(SIGNAL_SWEEP_GRACE).await; | ||
| let drain = async { | ||
| if tokio::time::timeout(*SIGTERM_BUDGET, runtime.shutdown()) |
There was a problem hiding this comment.
[correctness] Shutdown safety margin shrunk while unbounded steps sit outside the timeout (CONFIRMED)
The tokio::join! bounds drain and stop_all_children each to *SIGTERM_BUDGET (9s by default, down from a 10s budget that was explicitly split 60/40/1s with a documented margin). But crash_all_actors (lines 406-411, before the join) and serve_shutdown.cancel(); serve.await (lines 425-431, after the join) are not bounded by any timeout tied to SIGTERM_BUDGET. serve.await ultimately awaits axum::serve(...).with_graceful_shutdown(...), which waits unboundedly for in-flight connections (proxied websockets, the engines long-lived /start SSE request) to close.
The new doc comment on SIGTERM_BUDGET only says "9s, one second under the common ~10s budget" without re-deriving how two now-fully-concurrent, full-budget waits plus this unbounded post-join tail interact with that 1s margin.
Failure scenario: a child takes close to the full 9s to die (SIGTERM ignored, grace elapses, SIGKILL sent, exit reaped near t=9s). The join returns near 9.x s. serve.await then still needs to drain an in-flight websocket/SSE connection, which has no timeout - pushing total wall-clock past the platforms real ~10s SIGKILL deadline and getting the process killed mid-shutdown rather than exiting cleanly.
| } | ||
| println!("runner: shutdown, stopping {} child(ren)", children.len()); | ||
| for child in children { | ||
| join_all(children.into_iter().map(|child| async move { |
There was a problem hiding this comment.
[correctness] PID-recycling TOCTOU from the concurrent double-stop (PLAUSIBLE)
ChildProcess::stop() (child.rs:206-230) checks has_exited() and returns early, otherwise sends SIGTERM, waits, and possibly sends SIGKILL, with no locking against a second concurrent caller. Given the double-stop race above (stop_all_children and on_destroys stop_child can both call stop() on the same ChildProcess concurrently), theres a narrow window where both callers pass the has_exited() check before either sends a signal. If the process exits and its pid is recycled by the OS between one callers stale check and the others signal::kill(), the stray second signal could be delivered to an unrelated process holding the recycled pid.
This requires fast pid recycling within the shutdown window to actually misfire, so its realistic but not certain from the code alone - a real TOCTOU class of bug for pid-based signaling that the previous sequential (drain-then-sweep) design avoided by construction.
| tracing::warn!("engine drain exceeded the signal budget"); | ||
| } | ||
| }; | ||
| tokio::join!(drain, stop_all_children(*SIGTERM_BUDGET)); |
There was a problem hiding this comment.
[correctness] Grace-value mismatch between the two concurrent stop paths (CONFIRMED)
effective_stop_grace() returns grace.min(*SIGTERM_BUDGET) and is used by GameServer::stop_child (actor.rs:47, the on_destroy/on_sleep path). But this join calls stop_all_children(*SIGTERM_BUDGET) directly, not through effective_stop_grace(). If --stop-grace-secs/RIVET_STOP_GRACE_SECS is configured below SIGTERM_BUDGET (e.g. 3s vs the 9s default budget), the per-actor on_destroy path uses a 3s grace while the concurrently-racing sweep uses the full 9s grace for potentially the same child - an unintended divergence from the operators configured stop_grace intent.
1 participant
No description provided.