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]

Review

Small, focused change to container-runner/src/actor.rs: it fixes the prior bug where on_sleep skipped the drain for any sleep as long as idle-timeout mode was configured (dashboard/crash-policy/eviction sleeps included), not just idle-timer-triggered sleeps. Adding IDLE_SLEEPING as an explicit third state and gating the drain-skip on that specific state is the right fix, and the updated doc comments accurately describe the new behavior.

Correctness

• The CAS-based state transition is a genuine improvement over the old plain load()-then-sleep() check — the old code had a TOCTOU gap between reading idle_state and calling ctx.sleep(); the new compare_exchange(IDLE_ARMED, IDLE_SLEEPING, ...) makes the "did a request race the timer" decision atomic.
• Residual race worth being aware of (not introduced by this PR, but not fully closed either): if a request arrives after the idle timer's CAS has already flipped ARMED → SLEEPING and called ctx.sleep(), but before the engine actually delivers on_sleep, note_request's CAS (ARMED → REQUESTED) will fail silently and the state stays SLEEPING. When on_sleep eventually fires, it will still skip the drain even though a request came in during that window. This is a narrow timing window and the one-shot atomic can't represent "committed to sleep, but now there's also a pending request," so there's no cheap fix within this design — just flagging it as a known edge case rather than a blocker, since it's strictly better than the previous behavior (which skipped the drain unconditionally).
• Ordering::SeqCst is used for both CAS's, which is safe/conservative for a rarely-hit lifecycle transition; no performance concern here.

Test coverage

• No unit/integration test exercises the new IDLE_SLEEPING transition or on_sleep's branch on it (container-runner/tests/ only has boot_id.rs and input.rs). Given this is a state-machine correctness fix for a real production bug (dropping in-flight drains), a test that arms the idle timer, lets it fire, and asserts on_sleep skips the drain — plus a companion test asserting a non-idle sleep still drains — would give good regression coverage. Understand container-runner may not have much existing test scaffolding for actor lifecycle, so this may be consistent with the surrounding code, but worth considering.

Style

• Comments follow the repo's "complete sentences, no dash fragments" convention. No _ => fallthrough issues (no enum match involved). Doc comments were updated in sync with the code, which is appreciated.

Overall this is a solid, minimal bug fix. No blocking issues found; the residual race and missing test coverage are the two things worth a second look before merging.

[claude]

Review

Small, well-scoped fix to container-runner/src/actor.rs: on_sleep previously treated any engine-initiated sleep as an idle sleep whenever idle_timeout() was configured, even for an actively-serving actor put to sleep by the dashboard/crash-policy/eviction. The three-state atomic (IDLE_ARMED / IDLE_REQUESTED / IDLE_SLEEPING) now distinguishes "the idle timer itself fired" from "idle timeout is merely configured," which is a real correctness improvement: an active actor forced to sleep by the engine now correctly drains instead of stopping the child immediately.

Code quality

• The doc comments accurately describe the state machine and its rationale (why the CAS, why on_sleep reads it). Good adherence to the repo's comment style.
• Reusing a single AtomicU8 for a one-shot lifecycle plus a CAS-based transition is a clean, minimal design; no new locks or data structures introduced.

Potential edge case worth a look

ctx.sleep() (rivetkit-rust/packages/rivetkit-core/src/actor/context.rs:498) requests sleep from the envoy unconditionally. It does not wait for active_http_request_count/CanSleep::Yes the way the core's own automatic idle-sleep arming does (can_arm_sleep_timer, gated by no_sleep, which container-runner sets to true in main.rs:401). That means there is a real window between arm_idle_timeout's CAS to IDLE_SLEEPING and the engine actually delivering on_sleep: a request that lands in that window calls note_request, but its CAS(ARMED to REQUESTED) fails because the state is already SLEEPING, so idle_state stays SLEEPING. When on_sleep fires shortly after, it takes the no-drain path and stops the child immediately even though a fresh request/connection was just proxied in.

This is a narrower window than the previous behavior, which had this problem for the entire idle-configured lifetime rather than just this brief race, so the PR is a strict improvement. But the race is not fully closed. Worth confirming whether this is an accepted trade-off (the window is just the envoy round trip for the sleep request) or whether note_request should also handle the SLEEPING case, for example by having the timer task double-check for a pending request right before calling ctx.sleep().

Minor / nit

• The idle_state CAS/load calls all moved from Ordering::Relaxed to Ordering::SeqCst. Since idle_state does not guard any other non-atomic memory (it is a standalone one-shot flag), Ordering::AcqRel/Relaxed would likely suffice and avoid the small, infrequent SeqCst fence cost. Not a correctness issue, just stronger than needed.

Test coverage

No tests exercise on_sleep/idle-timeout behavior in container-runner (the tests/ directory only has inline, nothing idle-timer related). This concurrency-sensitive state machine, especially the race above, would benefit from a test that simulates a request racing the idle timer, but there does not appear to be an existing test harness in this crate for driving on_start/on_sleep directly, so this may be a larger lift than fits this PR.

Security

No concerns; this is actor-local lifecycle logic operating on already-trusted engine-originated sleep signals.

Overall: solid, well-documented fix for a real bug (forced/engine sleeps skipping drain). The residual race on the self-initiated idle path is worth a conscious decision (accept vs. close), but not a blocker.