Skip to content

feat(container-runner): sleep on startup idle timeout - #5586

Open
abcxff wants to merge 1 commit into
stack/chore-container-runner-tighten-comments-zrxxyxlsfrom
stack/feat-container-runner-sleep-on-startup-idle-timeout-ontuyypk
Open

feat(container-runner): sleep on startup idle timeout#5586
abcxff wants to merge 1 commit into
stack/chore-container-runner-tighten-comments-zrxxyxlsfrom
stack/feat-container-runner-sleep-on-startup-idle-timeout-ontuyypk

Conversation

@abcxff

@abcxff abcxff commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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

```rust
async fn on_sleep(self: Arc, ctx: Ctx) -> Result<()> {
if idle_timeout().is_some() {
self.stop_child(ctx.actor_id(), "actor sleeping (idle)").await;
} else {
self.drain_then_stop_child(ctx.actor_id(), "actor sleeping").await;
}
Ok(())
}
```

idle_timeout() only reflects whether RIVET_IDLE_TIMEOUT_SECS is configured for the deployment, it says nothing about why this particular on_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, every on_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:

```rust
async fn on_sleep(self: Arc, ctx: Ctx) -> Result<()> {
if idle_timeout().is_some() && !self.got_request.load(Ordering::Relaxed) {
self.stop_child(ctx.actor_id(), "actor sleeping (idle)").await;
} else {
self.drain_then_stop_child(ctx.actor_id(), "actor sleeping").await;
}
Ok(())
}
```

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant