fix(rivetkit-core): report actor crashes as a sleep intent instead of a stop intent

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/actors

• #5670 fix(rivetkit): surface native runtime load failures instead …
• #5667 fix(envoy-client): fail sent vfs sqlite requests on disconne …
• #5666 fix(rivetkit-core): report actor crashes as a sleep intent i … 👈
• #5668 fix(envoy-client): ack start commands immediately instead of …
• main

Get stack: forklift get 5666
Push local edits: forklift submit
Merge when ready: forklift merge 5666

_{change krtqytxz}

[claude]

Code Review

Reviewed the crash-vs-stop intent rework in rivetkit-core's ActorContext. The core idea (crashes report as a sleep intent with an attached error so the engine resumes the actor instead of destroying it) is sound and the new tests/sleep.rs coverage for the straightforward cases is solid. Found one confirmed correctness bug plus a few smaller issues.

🐛 Bug: stop_with_error() followed by destroy() sends two ActorIntentStop messages

rivetkit-rust/packages/rivetkit-core/src/actor/context.rs:565-637 (request_stop)

The old code gated destroy() and stop_with_error() behind a single atomic swap (destroy_requested), making them mutually exclusive. This diff splits them into sleep_requested and destroy_requested, and that split reopens a double-send path:

1. stop_with_error() runs first: destroy_requested is false, so it doesn't early-return. stop_error was previously None, so queued_error.is_some() is false, and it falls through to the shared tail, spawning a task that will call request_stop_from_envoy().
2. destroy() runs next, before that task executes: destroy_requested.swap(true, ...) was false, so it proceeds — clears stop_error back to None (line 611), runs mark_destroy_requested(), and also falls through to the same shared tail, spawning a second task.
3. Neither request_stop_from_envoy() (sleep.rs:171-192), track_shutdown_task, nor EnvoyHandle::stop_actor/sleep_actor (engine/sdks/rust/envoy-client/src/handle.rs) has an idempotency guard. Both spawned tasks read stop_error == None (since destroy() already cleared it) and both call envoy_handle.stop_actor(...), so the engine receives two ActorIntentStop messages for one generation.

The added test destroy_after_stop_with_error_still_sends_stop_intent (tests/sleep.rs) exercises exactly this call sequence but only reads one message via recv_stop_intent — it never calls the sibling assert_no_further_intent helper (unlike repeated_stop_with_error_sends_one_sleep_intent, which does), so the regression isn't caught. Recommend adding a guard so a second request_stop/request_stop_from_envoy invocation for an already-finalized generation is a no-op, and extending that test to assert no second intent is sent.

Other findings

• sqlite/mod.rs:1233 (report_sqlite_worker_fatal) — reports a fatal SQLite worker error straight to the raw EnvoyHandle::sleep_actor(...), bypassing all the new ActorContext crash-reporting bookkeeping (stop_error slot, sleep_requested, cancel_sleep_timer(), flush_on_shutdown()) that stop_with_error() now performs for every other crash path. A SQLite worker crash won't cancel the idle sleep timer (so the ordinary idle path can race its own error-less sleep_actor call) and won't force-flush dirty state before reporting. Worth routing this through ctx.stop_with_error(...) instead, if SqliteRuntimeConfig can get a path back to the owning ActorContext.

• rivetkit-rust/packages/rivetkit-core/CLAUDE.md:14 — now stale: "ctx.stop_with_error(message) shares destroy()'s request flow..." is no longer accurate. stop_with_error() now sets sleep_requested (not destroy_requested) and runs mark_errored_stop_requested() (not mark_destroy_requested()), ultimately sending ActorIntentSleep rather than ActorIntentStop. This doc line should be updated in the same change — leaving it stale is part of what makes the double-send bug above easy to reintroduce, since a reader would assume the two paths are still mutually exclusive via one atomic swap.

• context.rs:588-591 — whether a crash gets a forced flush_on_shutdown() now depends on call ordering rather than on "this generation has a recorded error": if ctx.sleep() already set sleep_requested = true before a crash calls stop_with_error(), the swap guard is false and mark_errored_stop_requested() (and its flush) is skipped for that crash. A fresh crash with no prior sleep() does get the flush. Might be worth a dedicated "error recorded" flag/state instead of overloading sleep_requested for this.

• Minor: request_stop's two branches aren't as symmetric as the surrounding comments suggest — the error arm can return Ok(()) mid-branch (line 599-601) before reaching the "shared" tail, while the destroy arm always falls through. Worth restructuring (e.g. compute a should_report: bool in each arm, then a single if should_report { ...tail... } after the if/else) so the control flow is easier to audit against future edits like this one.

• Minor: the sleep-vs-stop invariant is restated in four places with slightly different wording (handle.rs's sleep_actor/stop_actor docs, context.rs's stop_with_error doc, sleep.rs's inline comment in request_stop_from_envoy). Consider consolidating into one canonical explanation the others reference, so a future refinement doesn't need to be hunted down across four spots (as already happened with the CLAUDE.md line above).

Not flagged

• No em dashes introduced in new comments/docs.
• match error { Some/None } in request_stop_from_envoy matches on Option, which is exempt from the "no _ => on enums" rule.
• Test coverage for the straightforward sleep/destroy/repeated-error paths (tests/sleep.rs, tests/task.rs) looks thorough.