fix(rivetkit): keep actor event loop alive through shutdown state serialization - #5546
Conversation
ReviewRoot 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 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 existing behavior (napi_actor_events.rs drains its own JoinSet independently of cleanup dispatch, and start_grace already cancels the run task abort signal before RunGracefulCleanup is even sent, so extending the loop lifetime does not 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, and the run_actor loop still does: 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 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 does not think the loop can still be ended from inside handle_actor_event. Other notes:
Re-verified independently against the current diff on 2026-08-11; findings unchanged from the prior review. |
a4f9445 to
a08e479
Compare
2e35cdc to
3f1016b
Compare
a08e479 to
40d03bf
Compare
3f1016b to
41fad8a
Compare
40d03bf to
13cd549
Compare
13cd549 to
fbd0e22
Compare
ddda4b1 to
a3dec62
Compare
fbd0e22 to
7df3e38
Compare
7df3e38 to
5c46256
Compare
a3dec62 to
1b7821e
Compare
1b7821e to
f623340
Compare
5c46256 to
b8b0fd3
Compare
No description provided.