Skip to content

fix(rivetkit-core): always signal actor stop handle on teardown failure - #5593

Open
abcxff wants to merge 1 commit into
stack/fix-envoy-client-ack-terminating-stop-commands-so-pegboard-envoy-stops-replaying-them-zrklppqyfrom
stack/fix-rivetkit-core-always-signal-actor-stop-handle-on-teardown-failure-xyxnuuus
Open

fix(rivetkit-core): always signal actor stop handle on teardown failure#5593
abcxff wants to merge 1 commit into
stack/fix-envoy-client-ack-terminating-stop-commands-so-pegboard-envoy-stops-replaying-them-zrklppqyfrom
stack/fix-rivetkit-core-always-signal-actor-stop-handle-on-teardown-failure-xyxnuuus

Conversation

@abcxff

@abcxff abcxff commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@abcxff

abcxff commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/actors

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

change xyxnuuus

@railway-app

railway-app Bot commented Aug 24, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5593 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink 😴 Sleeping (View Logs) Web Aug 25, 2026 at 10:01 am
frontend-cloud 😴 Sleeping (View Logs) Web Aug 25, 2026 at 9:55 am
frontend-inspector 😴 Sleeping (View Logs) Web Aug 25, 2026 at 9:20 am
website ❌ Build Failed (View Logs) Web Aug 24, 2026 at 2:50 pm
ladle ✅ Success (View Logs) Web Aug 24, 2026 at 2:46 pm
mcp-hub ✅ Success (View Logs) Web Aug 24, 2026 at 2:44 pm

@abcxff
abcxff force-pushed the stack/fix-rivetkit-core-always-signal-actor-stop-handle-on-teardown-failure-xyxnuuus branch from 7b51b04 to 9800e86 Compare August 24, 2026 14:50
@claude

claude Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Review: fix(rivetkit-core): always signal actor stop handle on teardown failure

Re-checked against the current diff (single commit 9800e86, unchanged since the last review) - the analysis below still applies.

Small, well-scoped fix. Moving the join outcome out of an early ? return and into final_result correctly ensures stop_handle is always completed or failed (the stated goal), which is a real improvement over the previous behavior where a join failure would propagate via ? and skip signaling the stop handle entirely.

Bug: shutdown_result.and(join_result) can still discard the more informative error

rivetkit-rust/packages/rivetkit-core/src/registry/mod.rs:1157

let final_result = shutdown_result.and(join_result);

Result::and keeps the outer Err whenever it is already Err, regardless of what the other side contains. So when both shutdown_result and join_result are Err, the join error (and any real panic message it carries) is silently dropped in favor of shutdown_result.

This matters because of how shutdown_result is produced a few lines up:

Ok(()) => reply_rx
    .await
    .context("receive actor task stop reply")
    .and_then(|result| result),

If the actor task panics before it reaches the reply-send path, reply_tx is dropped mid-panic and reply_rx.await fails with a bare RecvError, a generic error with no hint that a panic occurred. Meanwhile join.await on the same task yields a JoinError via join_result, wrapped with .context("join actor task"), which does carry the real panic payload and message.

Because shutdown_result is Err first, .and(join_result) throws away the diagnostic join_result and keeps the uninformative RecvError. Both the error returned to the caller and the one handed to stop_handle.fail(...) end up losing the real panic diagnostic, exactly the class of problem this change is otherwise fixing.

Suggested fix: when both results are Err, merge them (for example attach the join_result message as additional context on shutdown_result) instead of using .and(), which is order-sensitive and silently prefers one side:

let final_result = match (shutdown_result, join_result) {
    (Err(e1), Err(e2)) => Err(e1.context(format!("{e2:#}"))),
    (Err(e), Ok(())) | (Ok(()), Err(e)) => Err(e),
    (Ok(()), Ok(())) => Ok(()),
};

Test coverage

No test was added. rivetkit-rust/packages/rivetkit-core/tests/registry.rs has no coverage of stop-handle completion/failure signaling, panic-during-shutdown, or join-failure paths. Since the fix is specifically about making sure stop_handle is always signaled (including on panic/join failure), a regression test exercising an actor task that panics during shutdown and asserting stop_handle still gets fail()-ed (not silently dropped) would directly validate the fix, per the repo Rust test-layout convention (tests under tests/, not inline).

Other notes

  • The rest of the diff (restructuring ? into explicit result-folding, matching on &final_result by reference so final_result can still be moved afterward for .with_context(...)) is clean and idiomatic.
  • No security or performance concerns; this is a narrow, local lifecycle-signaling fix that does not cross trust boundaries.

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