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 container-runner self-sleep-on-repeated-start feature (RIVET_REJECT_SECOND_START). Overall the implementation is careful: the guard is placed after the existing actor-already-running idempotency check (so live retries aren't misclassified), it reuses the existing ctx.sleep() -> on_sleep -> stop_child machinery instead of adding a new shutdown path, and the ActorInput -> ActorState migration via #[serde(flatten)] + #[serde(default)] correctly handles decoding state persisted by older binaries. A few things worth a look:

Worth confirming (semantics). mark_started_once permanently latches started_once = true after the first real start. In non-idle mode (idle_timeout().is_none()) this happens unconditionally right after the child spawns successfully, not just for a duplicate-start-message race. That means any later wake of this actor generation-lineage (e.g. an engine/dashboard-initiated sleep for eviction, or crash-policy restart) will hit the guard and immediately self-sleep instead of respawning the child, since the flag survives sleep. Given on_sleep's own comment (the engine can still sleep an actor for dashboard, crash policy, eviction) implies these are meant to be resumable pauses, it would be good to confirm this is the intended one-shot-match semantics (a woken match cannot resume state anyway) rather than an accidental foreclosure of legitimate engine-driven wake-after-sleep for this runner. If intentional, a short doc note on RIVET_REJECT_SECOND_START clarifying that actors using this flag are treated as single-lifetime, even across engine-initiated sleeps, would save the next reader from re-deriving this from the code.

Minor: formatting. container-runner/src/main.rs's new REJECT_SECOND_START closure line (the .map(...matches!(...)) chain) looks like it exceeds the default 100-col rustfmt width (project uses hard_tabs = true with default max_width), and the identical boolean-env pattern in monitor.rs::monitor_enabled wraps the matches! macro across multiple lines. Worth running the formatter on this file to match and avoid a CI fmt-check failure.

Test coverage. The new tests (actor_state_cbor_round_trips, legacy_bare_input_state_decodes_into_actor_state) solidly cover the serde/CBOR migration shape, which is the main this-will-silently-break-on-a-rollback risk. The actual reject-second-start control flow in actor.rs (on_start's guard, run's self-sleep branch, note_request's idle-mode deferral) has no test coverage, though actor.rs has no unit/integration tests at all today (only input.rs and boot_id.rs are covered under tests/inline/), so this may be consistent with how the rest of the actor lifecycle is tested elsewhere (e2e/driver tests) rather than a gap introduced by this PR specifically.

Nit. note_request's mark_started_once read-then-write isn't atomic across concurrent on_fetch/on_websocket calls in idle mode, but since it's idempotent (same end state, at most a redundant request_save()), this looks fine.

No security or performance concerns: the env var is process-local config, ciborium is a dev-dependency only, and everything else runs on existing hot paths already covered by the design constraints in CLAUDE.md.