feat(container-runner): log unexpected platform SIGTERM as an error

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5556 fix(container-runner): read cgroup v1 memory usage under gvi …
• #5548 feat(container-runner): log version and git sha at actor sta …
• #5549 feat(container-runner): setup instance memory and cpu usage …
• #5546 fix(rivetkit): keep actor event loop alive through shutdown …
• #5539 feat(container-runner): report actors as crashed on unexpect …
• #5536 feat(container-runner): log unexpected platform SIGTERM as a … 👈
• #5530 fix(rivetkit-core): cancel driver alarm before sqlite teardo …
• #5531 feat(container-runner): keep instance warm instead of self-e …
• #5533 chore(container-runner): default stop grace to 10s
• #5528 fix(rivetkit): log and surface actor startup failures
• #5558 fix(sdks): drop rivet-util metrics dep from protocol crates …
• main

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

_{change zsrurkrz}

[claude]

Review

This bundles six commits (rivet-util dep removal from the versioned protocol crates, rivetkit startup-failure surfacing, container-runner stop-grace default, container-runner keep-warm-instead-of-self-exit, rivetkit-core alarm-cancel reorder, container-runner SIGTERM error logging). Reviewed the full current diff against 99da275 (PR head).

Issues

1. Blanket tracing::error! on every platform SIGTERM misclassifies expected reclaims (container-runner/src/main.rs:437-448). The module doc comment added by this same PR a few lines up (main.rs:15-17) states the instance is reaped either by the engine draining the /start connection once the request lifespan elapses, or by the platform sending a SIGTERM - i.e. a bare SIGTERM is documented as one of the two expected ways a warm, idle instance gets reclaimed (scale-down, redeploy), not inherently an anomaly. The signal handler now unconditionally logs error!("unexpected platform SIGTERM received, likely hitting OOM or running longer than 60 minutes"), including in the actor_ids.is_empty() branch - exactly the idle-instance-reclaim case the doc comment describes as normal. This will produce false-positive error-level logs (and likely alerts) for routine reclaims of idle warm instances. Consider only escalating to error! when actors were actively running at signal time (the non-empty branch, where an abrupt kill is more likely to indicate a real problem) and keeping the idle/no-actor case at info!/warn!, or reconcile the doc comment if a bare SIGTERM really is meant to be always-anomalous now.

2. PLATFORM_RECLAIM is written but never read (container-runner/src/main.rs:81, :427). The doc comment says this flag "distinguishes a reclaim from a manual stop for downstream shutdown handling," but nothing in the crate loads it - grep only turns up the declaration and the single .store(...) call. This is a dead atomic that costs a store on every SIGTERM and documents an intent nothing consumes yet. Per this repo's convention against half-finished implementations, either wire it into the downstream shutdown path it's meant to inform, or drop it until that consumer exists.

3. retain_async used for a pure read (container-runner/src/main.rs:431-436). The new block calls CHILDREN.retain_async with a closure that only pushes into actor_ids and always returns true, so nothing is ever removed. retain_async takes the map's mutation-capable locking path unnecessarily; scc::HashMap::iter_async is the right tool for a read-only walk and is already used for this pattern elsewhere (e.g. rivetkit-rust/packages/client/src/connection.rs:286-291). stop_all_children a few lines away legitimately needs retain_async for its drain (main.rs:400), but this block doesn't need mutation semantics.

Worth double-checking

• Observability regression from the rivet-util removal (engine/sdks/rust/data, depot-protocol, envoy-protocol, epoxy-protocol, ups-protocol). Swapping rivet_util::serde::bare_to_vec!/bare_from_slice! for raw serde_bare::to_vec/from_slice is the right move per this repo's vbare convention (these are versioned OwnedVersionedData impls, not raw persisted encodings), and unlinking rocksdb from these protocol crates is a reasonable dependency-hygiene goal. But the removed macros also recorded SERIALIZE_SIZE/DESERIALIZE_SIZE histograms and OBSERVATION_DURATION timing per call site - this PR silently drops that instrumentation for all (de)serialization in these five crates. Worth confirming no dashboard/alert relies on those series before merging.
• stop_grace_secs default drops from 25s to 10s (container-runner/src/main.rs:254). This shortens the SIGTERM to SIGKILL grace for engine-initiated child stops (not just the signal path), and correspondingly halves the default sleep_grace_period computed in main.rs (stop_grace + 10s, so 35s to 20s). Likely intentional, but worth confirming game servers with slower graceful-shutdown/save routines were considered.

Looks good

• rivetkit-rust/packages/rivetkit/src/start.rs: wrapping the startup phase (state creation, create, on_create, on_start) in one fallible block and forwarding the error through startup_ready via RivetError::extract is a solid fix - previously a startup failure silently dropped the handshake reply and only surfaced as a generic closed-channel error. Good that it's paired with a regression test (run_actor_invalid_input_fails_to_start).
• rivetkit-rust/packages/rivetkit-core/src/actor/task.rs: moving cancel_driver_alarm_logged() before wait_for_pending_alarm_writes().await on the Destroy path is correct - it ensures the persist spawned by the cancel is awaited before SQLite teardown instead of racing it. No new regression test was added for the reorder itself, but this is a narrow, well-explained fix.
• container-runner/src/actor.rs and child.rs: the keep-warm behavior change (no longer self-exiting on last-actor-stop or failed start) is applied consistently across stop_child and the on_start error path, and the doc comments were updated to match in both README.md and main.rs. The Cloud Run to generic "platform" wording cleanup is a reasonable consistency pass.
• No stray rivet_util references remain in the five touched protocol crates after the dependency removal (verified via grep), so the swap is clean.

Nits

• The hardcoded "likely hitting OOM or running longer than 60 minutes" string (main.rs:439, :445) has no comment tying the 60-minute figure to whatever default it's referencing, so it can silently drift if that default changes elsewhere.

No test coverage was added for the container-runner signal-handling/keep-warm changes, but that's a hard thing to unit test in a PID-1 binary; the rivetkit-rust side did add a targeted test for its behavior change, which is the right call there.