feat(container-runner): exit process when the last child stops

[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 5587
Push local edits: forklift submit
Merge when ready: forklift merge 5587

_{change ssuvmoso}

[claude]

Review

Same underlying commit as the last pass (42e69ff), so this re-checks whether prior feedback still applies and looks for anything new.

stop_child (shared by on_sleep and on_destroy) now calls request_exit once children() is empty, so a container instance self-terminates after its last actor stops instead of staying warm forever. Overall a clean, well-commented change; two things worth addressing before merge.

Stale module docs — still unresolved from the last review

The new self-exit behavior contradicts doc comments in three places this PR doesn't touch:

• container-runner/src/actor.rs:7 (module doc): "on_destroy stops the child while the instance stays warm for the next placement."
• container-runner/src/main.rs:15-17 (module doc): "The instance stays warm after its last actor stops and never self-exits; the engine reaps it by draining the /start connection once the request lifespan elapses, or the platform sends a SIGTERM."
• container-runner/README.md:11-12: "The instance stays warm after its last actor stops; the engine reaps it by draining the /start connection after the request lifespan."

docs/content/docs/container-runner.mdx:68 already describes the new behavior ("...exits the process once no actors remain"), so the public docs and these in-repo comments are out of sync with each other and with the code. Worth updating all three in this change.

is_empty() race window isn't always the full SIGTERM budget

stop_child in container-runner/src/actor.rs:39-65:

children().remove_async(actor_id).await;
...
if let Some(child) = child {
    child.stop(effective_stop_grace()).await;
    release_child_port(child.child_port).await;
}
if children().is_empty() {
    request_exit(actor_id, reason);
}

The gap between removing this actor from children() and checking is_empty() is meant to give a concurrently-landing placement on the same warm instance time to register itself first. But ChildProcess::stop (container-runner/src/child.rs:206-211) returns immediately if has_exited() is already true:

pub async fn stop(&self, grace: Duration) {
    if self.has_exited() {
        return;
    }
    ...
}

So when the child already exited on its own before stop_child runs (the common "child shut down gracefully" case, or the run() watchdog → ctx.destroy() → on_destroy re-entry path), stop() is a no-op and the window between removal and the is_empty() check collapses to near zero. Meanwhile a sibling actor's on_start only inserts into children() after ChildProcess::spawn(...) succeeds (actor.rs:199-223), which can take up to readiness_timeout (default 30s). If a new actor is mid-spawn on the same instance when the last existing actor's child happens to have already exited, children() can read as empty and trigger request_exit, tearing the whole process (and its HTTP front door) down while the sibling's /start is still in flight.

This only matters for pools with per-instance concurrency > 1 (the recommended game-server setup pins concurrency to 1, where two actors can never land on the same instance and this can't happen), and it's self-healing at the engine level (a failed placement gets retried elsewhere), so I'd call this a real but narrow edge case rather than a blocker. Worth either an explicit callout in the comment above the is_empty() check (the current comment implies the check is race-free, which isn't quite true), or a small pending-start guard (e.g. an in-flight-starts counter alongside children()) if concurrency > 1 is an expected configuration.

Test coverage

No test exercises the new self-exit condition (last-child-stops → EXIT cancelled → async_main takes the actor-driven branch, vs. still-other-children-running → no exit). container-runner's only automated Rust coverage today is the inline boot_id test, so a full integration test may be out of scope here, but this directly affects instance billing/reuse behavior and would benefit from at least a follow-up note.

Everything else checks out

• Concurrent stops on a multi-actor instance can't cause a premature exit in the normal (non-race) case: each stop_child call removes only its own actor id before checking is_empty().
• on_sleep's reuse of stop_child via drain_then_stop_child is consistent with the function's existing rationale ("the engine considers [the instance] vacated"), so self-exiting on sleep-driven teardown is intentional, not scope creep from the destroy path.
• request_exit/EXIT.cancel() is idempotent, so double-triggering it from multiple actors racing to be "last" is harmless.

[claude]

Race: last-child self-exit can kill a concurrently-placed sibling actor.

stop_child exits the whole process once children() is empty, but nothing serializes that decision against the engine placing a NEW actor on the same multi-actor instance in the same window. Traced through pegboard-envoy/src/ws_to_tunnel_task.rs (a connection is only excluded from future placement after it receives ToRivetStopping, which fires only after the local envoy loop processes Shutdown, i.e. after request_exit already ran) and engine-runner envoy-client/src/commands.rs CommandStartActor handler (no shutting_down check before creating/inserting the actor). If a new actor C is placed on this instance in that RTT window, its CommandStartActor can land after Shutdown was already processed; the graceful-shutdown snapshot wont include C, so the eventual Stop branch clears ctx.actors, dropping Cs just-created entry. Cs /start SSE observes is_stopped() == true almost immediately and reports the freshly-placed actor as stopped.

Since this container-runner is explicitly designed to host as many concurrent actors as the engine places on it (see the module doc in main.rs), this is reachable whenever pool concurrency is greater than 1, not just a theoretical edge case.

[claude]

Double release of the child port.

run() (the watchdog) releases the child_port on any exit (line 246: release_child_port(child.child_port).await) but never clears self.child. When the framework later invokes on_destroy for this same actor generation (via ctx.destroy() on a clean exit, or via stop_with_error -> engine round-trip on a crash), on_destroy calls stop_child, which does self.child.lock().await.take() and finds the same Arc still present, then calls release_child_port(child.child_port) a SECOND time.

If a different actor successfully reserves that same port number in between (reserve_child_port only checks the RESERVED_PORTS set, which was already freed once), the second release call frees that other actors live reservation out from under it while its child is still bound to the port, letting a third actor grab the same port and collide. This is pre-existing in stop_child/run but is directly inside the function this PR modifies.

[claude]

Removed warning about log-agent drain time, with no replacement mitigation.

The deleted comment explicitly warned that keeping the instance warm avoids a fast self-exit that could otherwise lose the log agents chance to drain stderr. The new code intentionally performs exactly that fast self-exit (stop_all_children then runtime.shutdown() then return, with no added delay/flush) once the last actor stops. If the departing child crashed, the final stderr lines (or the runners own shutdown log lines) may not be scraped by the platforms external log agent before the container disappears. Worth confirming this tradeoff was intentional, and whether a short drain delay is needed before final process exit on the actor-driven path.

[claude]

Correctness (high): last-child self-exit races with a concurrently placed sibling actor on the same multi-actor instance.

stop_child decides to call request_exit purely from children().is_empty(). But the process only actually stops accepting new work once async_main teardown reaches runtime.shutdown(), which is what sets rivetkit-core serverless runtime shutting_down flag (checked in ensure_envoy). Between EXIT.cancel() firing here and that flag actually being set, the engine can place a brand new actor C on this same warm instance (module doc: runner hosts as many concurrent actors as the engine places on it). C on_start can run, reserve a port, and even spawn its child before it is inserted into children(), and even after insertion shutting_down may still read false. When async_main resumes, it runs stop_all_children and runtime.shutdown() and exits the process, tearing down C freshly placed, healthy actor.

This is exactly the case the new else branch comment (a multi-actor instance does not tear down siblings still hosting a child) is trying to protect against, but it only accounts for siblings already registered at check time, not ones concurrently starting.

[claude]

Stale module doc: still says the instance never self-exits.

container-runner/src/main.rs lines 12-17 (module doc) still say: "The instance stays warm after its last actor stops and never self-exits; the engine reaps it by draining the /start connection..." and container-runner/src/actor.rs line 7 says on_destroy stops the child while the instance stays warm for the next placement. Both are now false: this same PR makes stop_child call request_exit and terminate the process once the last child on the instance stops. Worth updating both doc comments so future readers do not rely on the old invariant.

[claude]

Correctness (medium-high, pre-existing but sits in this modified function): child port double release when a crashed or naturally exited child later gets torn down again via on_destroy.

The watchdog run() removes the actor from children() and calls release_child_port(child.child_port) on a child exit, but never clears the actor own self.child field. On a clean exit it calls ctx.destroy(), and on a crash it calls stop_with_error via report_run_error; both round trip through the engine and eventually invoke on_destroy, which calls this stop_child. stop_child takes self.child (still Some, since run never cleared it), calls child.stop() again (harmless, stop is idempotent) and then calls release_child_port(child.child_port) here a second time.

If another actor reserved that same port number in the window between the two releases (reserve_child_port probes for the first free port, so a freed port is immediately reusable), this second release removes a live reservation belonging to a different, currently running child, letting a third actor bind the same port and collide with an active child.

This predates this PR, run() and stop_child both existed before, but it sits in a function this diff modifies, and the new request_exit call means a stray double release now also has a path to interact with a real process teardown rather than just staying latent on a warm instance.

[claude]

Removed-behavior (medium): the deleted comment on this branch explicitly warned that a fast self-exit could lose the log agent chance to drain container stderr, and that risk is not addressed anywhere in this diff.

The old stop_child comment said: the instance stays warm... reaped by the platforms own shutdown signal, not by self-exit... a fast self-exit could otherwise lose [output because] the log agent [needs time] to drain its stderr. This PR intentionally replaces that with a fast self-exit (request_exit -> EXIT.cancel() -> stop_all_children -> runtime.shutdown() -> process return), but I do not see any compensating delay, flush hand-off, or drain window added anywhere in the new exit path (see async_main in main.rs) to cover the exact risk the deleted comment called out. If the platform log agent has not scraped the runner/child stderr written just before the last actor stops, that tail of output can be lost when the process exits immediately afterward.

[claude]

Conventions/cleanup (low-medium): stale doc comments elsewhere still describe the old never-self-exits behavior this PR replaces.

This hunk updates the doc comment on request_exit correctly, but two other comments in this crate were not updated and now directly contradict the new self-exit behavior:

• main.rs module doc (near the top of the file): The instance stays warm after its last actor stops and never self-exits.
• actor.rs module doc: on_destroy stops the child while the instance stays warm for the next placement.

Both are now only true when a sibling actor is still running; a solo actor on an instance now causes the whole process to exit. Worth updating both so a future reader does not build an incorrect mental model of the lifecycle from the module docs.