feat(container-runner): drain engine sleeps unless the idle timer fired

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

_{change ktlrlsuw}

[claude]

Code Review

Reviewed container-runner/src/actor.rs, the only file in this PR (idle-timer sleep now records IDLE_SLEEPING so on_sleep can skip the drain for idle-timeout sleeps).

Potential bug: race window still lets a real request get dropped without a drain

arm_idle_timeout CASes idle_state from IDLE_ARMED to IDLE_SLEEPING (actor.rs:116-122) and then calls ctx.sleep(), which is fire-and-forget — it only requests sleep; the actual on_sleep call happens later, after an engine round trip that drives the actor through SleepGrace.

Per rivetkit-core's accepting_dispatch() (rivetkit-rust/packages/rivetkit-core/src/actor/task.rs:1868-1873), dispatch (i.e. on_fetch/on_websocket) is still accepted in Started, SleepGrace, and DestroyGrace. So between the CAS to IDLE_SLEEPING and on_sleep actually running, a genuine request can still land and be proxied to the child via note_request → on_fetch/on_websocket.

The problem: note_request (actor.rs:133-140) only CASes IDLE_ARMED -> IDLE_REQUESTED. Since the state is already IDLE_SLEEPING at that point, the CAS silently no-ops and the flag is never corrected back to "this generation actually served a request." When on_sleep eventually runs (actor.rs:378-384), it reads idle_state == IDLE_SLEEPING and calls stop_child directly instead of drain_then_stop_child, abruptly killing the child mid-request — the exact failure mode this PR is trying to fix, just narrowed to a smaller window instead of eliminated.

Suggested fix direction: have note_request distinguish "raced a sleeping generation" from "still armed," e.g. by also CASing IDLE_SLEEPING -> IDLE_REQUESTED (or checking the resulting state and forcing a drain path) so a request that arrives after the idle timer fired but before on_sleep runs still gets drained. Since on_sleep reads the flag once and picks a code path, the state machine needs a way to represent "sleep was armed idle, but a request arrived during grace" distinctly from "no request ever arrived."

Other notes

• The updated doc comments (lines 28-31, 96-98, 130-132, 374-377) are clear and match the new three-state design's intended behavior; consider adding a note about the request-during-grace race once the fix above lands, since it's the kind of subtlety a future reader will want called out.
• Ordering::SeqCst for the idle-state CAS/load is a reasonable, conservative choice for a rare one-shot transition; no complaints there.
• No test coverage included for the new IDLE_SLEEPING behavior or the race above. Given this is exactly the drain-vs-no-drain distinction the PR is about, a test that forces a request to land after the idle timer fires but before sleep completes would be valuable, if the test harness can simulate that timing.

Overall the change is a good incremental improvement (idle sleeps with genuinely no requests now avoid an unnecessary drain wait), but it doesn't fully close the race it targets — worth confirming whether the narrowed window is considered acceptable risk before merging, or whether it should be closed in this PR.