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: feat(container-runner): self-sleep on repeated actor start

Overall this is a well-documented, carefully-scoped change (opt-in via RIVET_REJECT_SECOND_START, off by default). The backward-compat handling for ActorState (flatten + #[serde(default)], with a dedicated CBOR round-trip regression test) is exactly the right pattern for extending persisted actor state. A few things worth confirming/discussing before merge:

1. Once started_once is set, the actor can never run again -- please confirm this is intended

Walking through the combined idle-timeout + reject-second-start flow (container-runner/src/actor.rs):

• With RIVET_IDLE_TIMEOUT_SECS set, started_once is recorded on the first request the actor serves (note_request -> mark_started_once), not on start.
• If that actor later idle-sleeps (no traffic within the window) and is woken again for a new request, on_start now sees ctx.state().started_once == true, sets reject_start, spawns no child, and run() immediately calls ctx.sleep() again.
• The request that triggered the wake reaches on_fetch/on_websocket, finds self.child still None, and fails with "no running child for actor {id}".

Net effect: an idle-timeout actor that has served even one request becomes permanently inert after its first idle-sleep -- every future wake attempt (including a legitimate reconnect to what should be an ongoing session) errors out and immediately re-sleeps, forever. The same is true without idle-timeout too: mark_started_once fires right after the child is confirmed ready, so if the child later crashes (run() returns Err on a non-zero exit) and something restarts the actor, that restart is rejected the same way -- crash-recovery is permanently disabled once started_once is set.

If the intent is genuinely "this actor gets exactly one lifetime, ever" (e.g. a billing/abuse guard for single-match game servers), that is a reasonable feature, but it is worth:

• calling out this consequence explicitly in the module/function doc comments (right now the comments explain the mechanics but not the "permanently wedged after any wake/crash" end state), and
• giving callers something more actionable than a generic "no running child" error, e.g. a distinguishable error/response so a client can tell "actor was rejected as a repeat start" apart from other transient proxy failures.

Since this is opt-in and off by default, the blast radius is contained, but it would be good to get an explicit confirmation this is the desired semantic rather than an edge case that slipped through.

2. No test coverage for the actor-level guard itself

container-runner/tests/inline/input.rs covers the CBOR round-trip and legacy-decode compatibility well, but nothing exercises the actual on_start/run() interaction added in actor.rs (second-start rejection, the idle-timeout-deferred mark_started_once, or the reject_start -> ctx.sleep() path). There is no existing actor-level test harness in this crate to hook into (the only precedent is unit tests on input.rs), so this may not be practical today. Flagging in case there is a lighter-weight way to unit-test the guard logic in isolation, e.g. by extracting the decision as a pure function of (reject_second_start, idle_timeout, started_once) -> Action that can be tested without a full Ctx.

Minor

• container-runner/src/main.rs: the REJECT_SECOND_START closure body (.map(|value| matches!(...))) is a fairly long single line; might be worth breaking up for consistency with the rest of the file, if agent-format.mjs / rustfmt does not already normalize it.
• mark_started_once's read-then-write of started_once is not atomic across concurrent on_fetch/on_websocket calls (both could observe false and both call request_save()), but since the operation is idempotent and the end state is the same either way, this looks harmless. Just noting it was considered, not blocking.

Nice attention to the CBOR/backward-compat details in input.rs, that part looks solid.