Skip to content

fix(rivetkit): keep actor event loop alive through shutdown state serialization - #5546

Open
abcxff wants to merge 1 commit into
stack/feat-container-runner-report-actors-as-crashed-on-unexpected-platform-sigterm-lzuymmykfrom
stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt
Open

fix(rivetkit): keep actor event loop alive through shutdown state serialization#5546
abcxff wants to merge 1 commit into
stack/feat-container-runner-report-actors-as-crashed-on-unexpected-platform-sigterm-lzuymmykfrom
stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt

Conversation

@abcxff

@abcxff abcxff commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review

Root cause and fix check out. RunGracefulCleanup (start.rs:433) previously returned Ok(true) to end the run_actor event loop right after on_sleep/on_destroy. But rivetkit-core's shutdown path (task.rs::run_shutdown -> save_final_state) always sends a follow-up ActorEvent::SerializeState after that cleanup hook, and only closes the event channel afterward (close_actor_event_channel, called after save_final_state). With the old early return, events/rx was already dropped by the time that SerializeState was enqueued, so send_actor_event hit the closed channel and failed with not_ready (task.rs:1071-1084), silently losing any state the sleep/destroy hook just wrote. Keeping the loop alive until core closes the channel matches the NAPI adapter's existing behavior (napi_actor_events.rs drains its own JoinSet independently of cleanup dispatch, and start_grace already cancels the run task's abort signal before RunGracefulCleanup is even sent, so extending the loop's lifetime doesn't race the user run() handler), so this brings the Rust crate to parity as the comment claims.

The new regression test (run_actor_serializes_state_after_cleanup) is a good direct repro: it fails pre-fix (the request_serialize send would panic against a closed channel) and passes post-fix, verifying on_sleep-written state actually survives serialization. The systematic drop(tx) additions to the other request_sleep/request_destroy tests are correct and necessary now that the loop no longer self-terminates on RunGracefulCleanup; all 17 call sites got the matching drop, otherwise actor.await would hang forever waiting on the channel to close.

Minor: now-dead should_stop plumbing. handle_actor_event still returns Result<bool>, and the run_actor loop still does:

let should_stop = handle_actor_event(actor.clone(), ctx.clone(), event).await?;
if should_stop {
    break;
}

That was the only Ok(true) arm in the whole match (no other variant returns true), so should_stop is now unconditionally false and the loop's only real exit is events.recv_raw().await returning None. Not a bug, but worth simplifying to -> Result<()> and dropping the should_stop/break scaffolding so a future reader doesn't think the loop can still be ended from inside handle_actor_event.

Other notes:

  • No security or performance concerns: this doesn't introduce polling, just extends the existing async recv().await loop lifetime to match core's actual event sequence. Since save_final_state bounds its SerializeState wait with a timeout (SERIALIZE_STATE_SHUTDOWN_SANITY_CAP or the configured sleep grace period) and always closes the channel afterward regardless of outcome, the loop cannot hang indefinitely even if a SerializeState reply is dropped or delayed.
  • Scope is tightly contained to rivetkit-rust/packages/rivetkit/src/start.rs; no parity gap opened against NAPI (already behaved this way) or engine-runner/pegboard-envoy (unrelated).

Re-verified independently against the current diff on 2026-08-10; findings unchanged from the prior review.

@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from a4f9445 to a08e479 Compare August 4, 2026 16:15
@abcxff
abcxff force-pushed the stack/feat-container-runner-report-actors-as-crashed-on-unexpected-platform-sigterm-lzuymmyk branch 2 times, most recently from 2e35cdc to 3f1016b Compare August 5, 2026 17:09
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from a08e479 to 40d03bf Compare August 5, 2026 17:09
@abcxff
abcxff force-pushed the stack/feat-container-runner-report-actors-as-crashed-on-unexpected-platform-sigterm-lzuymmyk branch from 3f1016b to 41fad8a Compare August 10, 2026 17:16
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from 40d03bf to 13cd549 Compare August 10, 2026 17:16
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from 13cd549 to fbd0e22 Compare August 10, 2026 19:03
@abcxff
abcxff force-pushed the stack/feat-container-runner-report-actors-as-crashed-on-unexpected-platform-sigterm-lzuymmyk branch 2 times, most recently from ddda4b1 to a3dec62 Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from fbd0e22 to 7df3e38 Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from 7df3e38 to 5c46256 Compare August 10, 2026 22:09
@abcxff
abcxff force-pushed the stack/feat-container-runner-report-actors-as-crashed-on-unexpected-platform-sigterm-lzuymmyk branch from a3dec62 to 1b7821e Compare August 10, 2026 22:09
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