Skip to content

feat(container-runner): drain children and engine concurrently on SIGTERM #5584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abcxff wants to merge 1 commit into
base: stack/fix-envoy-client-ack-terminating-stop-commands-so-pegboard-envoy-stops-replaying-them-zrklppqy
Choose a base branch
from
Open

feat(container-runner): drain children and engine concurrently on SIGTERM #5584

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 29 additions & 32 deletions container-runner/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use std::time::Duration;

use anyhow::Result;
use clap::Parser;
use futures_util::future::join_all;
use rivetkit::serverless_http::{self, ListenerConfig};
use rivetkit::{ActorConfig, EngineSpawnMode, Registry, ServeConfig};
use tokio_util::sync::CancellationToken;
Expand Down Expand Up @@ -103,30 +104,19 @@ static SIGNAL_SHUTDOWN: AtomicBool = AtomicBool::new(false);
static PLATFORM_RECLAIM: AtomicBool = AtomicBool::new(false);

/// How long the platform gives this container between SIGTERM and SIGKILL.
/// Defaults to 10 seconds; keep this in sync with the platform's actual budget
/// via RIVET_SIGTERM_BUDGET_SECS.
/// The signal-path teardown splits the budget: ~60% for the engine drain
/// (whose per-actor stops SIGTERM children with a grace capped at ~40%), 1s
/// for the straggler sweep, and the rest as margin.
/// Defaults to 9s, one second under the common ~10s platform budget so the whole
/// teardown lands before SIGKILL; keep it in sync with the platform's actual
/// budget via RIVET_SIGTERM_BUDGET_SECS. On the signal path the engine drain and
/// the child kills run concurrently, each bounded by this full budget.
static SIGTERM_BUDGET: LazyLock<Duration> = LazyLock::new(|| {
let secs = std::env::var("RIVET_SIGTERM_BUDGET_SECS")
.ok()
.and_then(|value| value.parse::<u64>().ok())
.unwrap_or(10)
.unwrap_or(9)
.max(3);
Duration::from_secs(secs)
});

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


fn signal_drain_timeout() -> Duration {
SIGTERM_BUDGET.mul_f64(0.6)
}

fn signal_child_stop_grace() -> Duration {
SIGTERM_BUDGET.mul_f64(0.4)
}

const SIGNAL_SWEEP_GRACE: Duration = Duration::from_secs(1);

pub fn runner_config() -> Arc<RunnerConfig> {
RUNNER_CONFIG
.get()
Expand Down Expand Up @@ -203,7 +193,7 @@ pub async fn release_child_port(port: u16) {
pub fn effective_stop_grace() -> Duration {
let grace = runner_config().stop_grace;
if SIGNAL_SHUTDOWN.load(Ordering::Acquire) {
grace.min(signal_child_stop_grace())
grace.min(*SIGTERM_BUDGET)
} else {
grace
}
Expand Down Expand Up @@ -395,11 +385,13 @@ async fn async_main() -> Result<()> {
// currently unreachable and kept only as a fallback for a future
// actor-driven exit.
//
// Signal (platform is reclaiming the instance): tell the engine FIRST so
// it can start re-placing actors immediately. Its per-actor stops run our
// on_destroy hooks, which SIGTERM children with the capped signal grace.
// The drain is bounded so an unreachable engine cannot eat the whole
// platform budget; the sweep then catches any child whose hooks never ran.
// Signal (platform is reclaiming the instance): kill our children AND
// notify the engine at the SAME time, each bounded by the full SIGTERM
// budget. The direct sweep is what guarantees children die within budget
// rather than waiting on an engine round-trip to run our on_destroy hooks;
// notifying the engine in parallel just lets it start re-placing actors
// immediately. Bounding the drain means an unreachable engine cannot eat
// the budget the children need.
//
// Fallback actor-driven exit (unreachable today): no platform deadline.
// Children are already reaped by the hooks (the sweep is a no-op backstop),
Expand All @@ -417,13 +409,15 @@ async fn async_main() -> Result<()> {
)
.await;
}
if tokio::time::timeout(signal_drain_timeout(), runtime.shutdown())
.await
.is_err()
{
tracing::warn!("engine drain exceeded the signal budget, sweeping children directly");
}
stop_all_children(SIGNAL_SWEEP_GRACE).await;
let drain = async {
if tokio::time::timeout(*SIGTERM_BUDGET, runtime.shutdown())

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

.await
.is_err()
{
tracing::warn!("engine drain exceeded the signal budget");
}
};
tokio::join!(drain, stop_all_children(*SIGTERM_BUDGET));

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

} else {
stop_all_children(stop_grace).await;
runtime.shutdown().await;
Expand All @@ -442,7 +436,9 @@ async fn async_main() -> Result<()> {

/// Stop every child still in the registry. Actor `on_destroy` normally reaps
/// its own child first; this is the belt-and-suspenders sweep for the signal

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cleanup] Doc comment now misdescribes the actual ordering (CONFIRMED)

This comment says on_destroy normally reaps its own child first and calls this a belt-and-suspenders sweep for stragglers - true when the sweep ran strictly after runtime.shutdown() finished/timed out. Now the function is invoked concurrently with the engine drain (which triggers on_destroy) via tokio::join!, so the sweep races the normal per-actor path on every signal shutdown rather than only catching leftovers. A future maintainer relying on this comments sequential framing (e.g. gating new logic in stop_child on the CHILDREN entry still being present) could introduce a real bug, since that gate would now silently almost never fire.

/// path so children are never orphaned.
/// path so children are never orphaned. Children are stopped concurrently so
/// each gets the full `grace` within the SIGTERM budget instead of queueing
/// behind the others.
async fn stop_all_children(grace: Duration) {
let mut children: Vec<Arc<ChildProcess>> = Vec::new();
CHILDREN

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] Resource-monitor attribution goes empty before children are actually dead (CONFIRMED)

active_actor_ids() (main.rs:133-143) reports actor ids by scanning CHILDREN, and gates the resource monitors per-actor logging loop (monitor.rs:231-234). This retain_async sweep removes every entry from CHILDREN in one synchronous pass before join_all (below) has actually stopped any of the collected children. During a signal shutdown, active_actor_ids() reports zero active actors almost immediately even though the children are still alive and consuming resources for up to the full grace period while being SIGTERM/SIGKILLed - so resource samples during that window get dropped or misattributed to no actor.

Expand All @@ -455,10 +451,11 @@ async fn stop_all_children(grace: Duration) {
return;
}
println!("runner: shutdown, stopping {} child(ren)", children.len());
for child in children {
join_all(children.into_iter().map(|child| async move {

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

child.stop(grace).await;

@claude claude Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cleanup] stop+release_child_port pair duplicated across three call sites (CONFIRMED)

The exact pair child.stop(grace).await; release_child_port(child.child_port).await; (in that order) now appears independently in three places: here in stop_all_childrens join_all closure, in GameServer::stop_child (actor.rs:47-48), and in on_starts defensive registration-failure branch (actor.rs:187-188). A shared helper (e.g. a ChildProcess::stop_and_release() method) would keep this two-step invariant in one place instead of three independently-maintained copies that can silently drift (e.g. if the release-order or added logging/metrics is updated in one call site and forgotten in the others).

release_child_port(child.child_port).await;
}
}))
.await;
}

fn env_u16(key: &str) -> Option<u16> {
Expand Down
Loading