You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review: feat(container-runner): sleep on startup idle timeout
Reviewed container-runner/src/actor.rs and container-runner/src/main.rs. Overall the design (a one-shot startup idle timer that self-triggers ctx.sleep()) is reasonable, but there's a correctness bug in how on_sleep decides whether to drain.
Bug: on_sleep skips draining for any sleep once the feature is enabled, not just the idle-timer-triggered one
idle_timeout() only reflects whether RIVET_IDLE_TIMEOUT_SECS is configured for the deployment, it says nothing about why this particularon_sleep call fired. Per the existing comment above on_sleep, the engine can call on_sleep for reasons other than the startup timer (dashboard-triggered sleep, crash policy, eviction), and no_sleep: true only blocks core's own idle-sleep evaluation, not these engine-driven sleeps (confirmed in can_arm_sleep_timer in rivetkit-core/src/actor/sleep.rs, which returns CanSleep::NoSleep before even checking connection/request state).
So once an operator sets RIVET_IDLE_TIMEOUT_SECS at all, everyon_sleep call, including one triggered on an actor with live players hours into a game, via dashboard or eviction, will skip the drain window and forcibly kill the child immediately. Before this PR, on_sleep always drained. This looks like an unintended regression: the drain-skip should be scoped to "this actor never received a request," not "the idle-timeout feature happens to be turned on."
Suggested fix, key off the actual signal already available (got_request) instead of the global config:
This preserves the intended "no request ever arrived, nothing to drain" fast path while restoring the graceful drain for any sleep on an actor that has actually served traffic.
Minor notes
Race window (low severity, likely acceptable):arm_idle_timeout's check-then-ctx.sleep() isn't atomic with got_request. If a request arrives in the same instant the timer fires, the timer task could still call ctx.sleep() after the request handler already started (or vice versa). Combined with the bug above, an in-flight request could get killed instead of drained. Fixing the bug above narrows this to a genuinely tiny window and probably doesn't need further hardening, but worth being aware of.
Test coverage: No tests were added for the new idle-timeout arm/disarm logic or the on_sleep drain-vs-stop branching. Given this changes shutdown behavior, at least a test exercising "no request within timeout leads to sleep being called" and "request before timeout means sleep never fires" would help guard against regressions (and would have caught the bug above if it asserted drain behavior).
Docs:RIVET_IDLE_TIMEOUT_SECS is a new user-facing env var; worth double-checking whether it needs a mention in runner/env-var docs per the docs-sync conventions.
Nothing else stood out. The AtomicBool/Ordering::Relaxed usage is appropriate here (single flag, no ordering dependency with other memory), the spawned task is properly scoped to the actor's abort_signal() for cleanup, and the main.rs env parsing follows the existing DRAIN_GRACE/SIGTERM_BUDGET pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.