feat(container-runner): self-sleep on repeated actor start

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

_{change qspsskoy}

[claude]

Review: feat(container-runner): self-sleep on repeated actor start

Overall this is a clean, well-scoped feature guarded behind an off-by-default env var (RIVET_REJECT_SECOND_START), with good doc comments explaining the idle-mode vs non-idle-mode timing of when started_once gets recorded, and a solid CBOR round-trip test proving the #[serde(flatten)] state migration decodes legacy (pre-field) persisted state correctly.

Correctness

mark_started_once uses a non-immediate request_save(), leaving a window where the guard can silently fail on the exact crash-and-restart scenario it's meant to prevent (container-runner/src/actor.rs:381-389).

Ctx::request_save() defaults to RequestSaveOpts { immediate: false, .. }, which schedules a throttled/debounced save rather than persisting immediately (see compute_save_deadline in rivetkit-core/src/actor/state.rs:313-324, and the doc comment on Ctx::request_save itself: "If save-request delivery must be observed, use the error-aware request_save_and_wait path").

For the non-idle path, mark_started_once is called right after the child is already spawned and registered (actor.rs:279-283), i.e. the child is already live. If the process crashes (OOM, platform SIGTERM, etc.) before the debounced save flushes, and the engine reschedules the actor on a new container, the new instance's on_start will see started_once == false and spawn a second child — the exact duplicate-start this feature exists to prevent.

Since mark_started_once's callers (on_start, note_request) are already async fns, consider awaiting ctx.request_save_and_wait(RequestSaveOpts { immediate: true, .. }) (or at minimum passing immediate: true via request_save_with_opts) so the flag is durably persisted before the actor is considered "really started."

Behavior worth confirming with the author

Once started_once is set (idle mode: after the first request; non-idle mode: right after spawn), any later start of that actor id — including a legitimate sleep→wake cycle from a dashboard action, eviction, or engine-driven pause, not just a duplicate/racy start — will hit the guard and self-sleep. That looks intentional given the framing ("match state lives in the child and cannot outlive the container" in stop_child's doc comment), i.e. these are meant to be one-shot, non-resumable actors, but it's worth double-checking that's the desired semantics for every caller of this flag, since it's a fairly sharp edge (an actor that served exactly one request can never be woken again while this flag is on).

Test coverage

The new tests (container-runner/tests/inline/input.rs) only cover the ActorState/ActorInput CBOR (de)serialization shape. There's no test exercising the actual guard logic in actor.rs: env var parsing (reject_second_start()), or that a second on_start with started_once == true skips spawning and results in run() calling ctx.sleep(). Given the project's "no mocking, real infra" testing policy this may be harder to cover with a unit test, but at least the env-var truthy/falsy parsing in main.rs (REJECT_SECOND_START) looks straightforward to unit test and currently has none.

Minor

• main.rs:139-143: the .map(...) closure line is quite long; not a blocker since agent-format.mjs/rustfmt will likely wrap it, but flagging in case formatting wasn't run before pushing.
• Adding ciborium as a dev-dependencies-only entry in container-runner/Cargo.toml (rather than a full dependency) is a nice touch, it keeps the CBOR round-trip test honest without adding runtime weight.

Security / Performance

No concerns. The env var is process-local config (no new trust boundary), and the atomic/state-lock usage in note_request/mark_started_once looks correctly scoped (read guard dropped before the write, no lock held across .await).

[claude]

Review

Reviewed the RIVET_REJECT_SECOND_START self-sleep guard added on top of the idle-timeout feature. Overall the implementation is careful (feature is fully gated behind an env var and is zero-cost when disabled, the grace-period task is cancelled via abort_signal(), and the CBOR compat handling for the state shape change is tested). One design question stands out as worth resolving before this ships.

Potential design issue: the guard looks like it would also block legitimate wake-from-sleep, not just runaway restarts

started_once is committed to persisted actor state (ctx.request_save()) and never cleared. Once it's true, every future on_start for that actor id — in this container or a brand new one, minutes or days later — hits the reject_second_start() && ctx.state().started_once branch in on_start (container-runner/src/actor.rs:190) and self-sleeps without spawning a child.

But this file's own docs describe actors as expected to sleep and wake repeatedly:

• stop_child's doc comment: "a later wake respawns an equivalent child from the persisted launch spec."
• on_sleep's doc comment: "the engine can still sleep an actor (dashboard, crash policy, eviction)... otherwise it drains for in-flight work first," implying the actor is expected to come back.

If RIVET_REJECT_SECOND_START and normal sleep are ever enabled together (e.g. RIVET_IDLE_TIMEOUT_SECS, or an engine-initiated sleep), the first successful start permanently locks the actor out of ever running again. Reconnect-after-sleep would just get silently re-slept forever, with no automatic way to recover short of clearing/recreating actor state. That seems like a much bigger behavioral change than "self-sleep on a repeated/duplicate start," and it isn't mentioned in the PR description (which is empty) or the README.

Could you confirm:

1. Is this meant only for "single-life" actors that should never legitimately restart once they've truly started (so any second start is by definition a bug/duplicate)? If so, it'd be worth saying that explicitly in the doc comment on REJECT_SECOND_START and/or the README, since it's a fairly sharp footgun for anyone who also enables idle-timeout sleep on the same actor.
2. If legitimate wake-after-sleep should still be allowed, the guard needs a way to distinguish "this actor already completed its lifecycle and shouldn't restart" from "this actor slept and is being legitimately woken" — e.g. resetting/not setting started_once on a deliberate on_sleep, or only guarding against restarts within some bounded window of the previous start rather than for the actor's entire remaining lifetime.

Minor

• Test coverage: the new tests (container-runner/tests/inline/input.rs) only cover CBOR round-tripping of ActorState/legacy ActorInput decoding. There's no test exercising the actual guard behavior in actor.rs (reject path skipping spawn + run() sleeping via reject_start, arm_second_start_mark committing after grace, note_request's idle-mode deferral). Given the design question above, a test that pins down the intended semantics (e.g. "second start after grace is rejected", "start within grace after a crash is not committed") would help lock in the intended behavior.
• container-runner/src/main.rs: the REJECT_SECOND_START closure line (.map(|value| matches!(...))) is quite long, probably worth letting the formatter wrap it (node scripts/format/agent-format.mjs) rather than a single long line, for consistency with the rest of the file.
• Enabling the guard is a fairly consequential, hard-to-reverse state per actor (it silently disables all future starts), but there's no metric/counter for how often actors hit the reject path. Could be useful for operators to notice if this fires unexpectedly in production.

What looks solid

• The env-var wiring (reject_second_start(), second_start_grace()) follows the existing pattern in the file closely (mirrors idle_timeout()/drain_grace()), and is fully off by default.
• arm_second_start_mark's grace window correctly runs only after the child is confirmed spawned and registered, and is cancelled on shutdown via the same abort_signal() pattern already used by arm_idle_timeout.
• mark_started_once's read-then-write on ctx.state() correctly relies on the temporary in the if condition being dropped before the block executes, so there's no double-borrow with the following ctx.state_mut().
• The #[serde(flatten)] migration for ActorState wrapping the old bare ActorInput persisted state is a sound backward-compat approach, and is directly tested for both the new and legacy CBOR shapes.
• Idle-mode deferral of started_once to the first real request (rather than at spawn time) is a sensible way to avoid punishing an idle-timeout actor that never served a request.