You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reviewed the diff (envoy-client, rivetkit-core, rivetkit). Overall this is a well-scoped, well-tested change with good test coverage on both the rivetkit-core lifecycle path and the rivetkit (Rust) run_actor event loop.
What it does
Adds ActorContext::stop_with_error / Ctx::stop_with_error, sharing destroy()'s request-flow (single-writer swap guard) but attaching a length-capped error message that the envoy forwards as StopCode::Error so the engine records a crash instead of a clean destroy.
ActorTask::handle_run_handle_outcome now reports a failed/panicked run() handler as an errored stop while keeping the generation alive (Started) so the engine's answering Stop still drives the destroy grace hooks, per the existing invariant documented in rivetkit-core/CLAUDE.md.
rivetkit's own run_actor loop (the legacy Rust event-loop API) mirrors this via catch_unwind + report_run_error, since per rivetkit/CLAUDE.md lifecycle logic is intentionally kept in run_actor for this wrapper.
Removes the now-redundant EnvoyHandle::destroy_actor in favor of the pre-existing stop_actor(actor_id, generation, error).
Correctness
Traced the potential double-report path: run_actor's inner spawn_run_task calls report_run_error promptly when actor.run() fails/panics (since the outer run_actor loop won't otherwise observe that failure until it later processes a RunGracefulCleanup event). That inner failure later also propagates out of run_actor itself via stop_run_task, so ActorTask::handle_run_handle_outcome (in rivetkit-core) sees the same failure a second time. This is safe: stop_with_error/destroy() share a single-writer swap, so the second attempt fails with ActorLifecycleError::Stopping and is handled as an expected no-op (self.ctx.destroy_requested() branch) rather than causing a duplicate envoy message. Good defensive design, worth calling out since it wasn't obvious without tracing both call sites.
The JoinError::is_cancelled() check correctly distinguishes deliberate task aborts (during grace/shutdown, which go through a separate join_aborted_run_handle path, not handle_run_handle_outcome) from genuine panics.
truncate_stop_error_message is char-boundary-safe.
Confirmed stop_actor's error: Option<String> parameter and the engine-side StopCode::Error / crash-state handling in pegboard/src/workflows/actor2/runtime.rs already existed pre-PR, so this is wiring into real, non-vestigial plumbing rather than a dead field.
No stray references to the removed destroy_actor remain anywhere in the tree.
Minor notes (non-blocking)
stop_with_error_truncates_long_message in tests/sleep.rs asserts error.len() < 4096, which is a fairly loose bound given the actual cap is 2048 + len("... (truncated)") (~2064). Not wrong, just looser than necessary if you want the test to pin the exact cap.
The crash-classification logic now exists in two places (ActorTask::handle_run_handle_outcome in rivetkit-core and report_run_error/spawn_run_task in rivetkit's start.rs). This matches rivetkit/CLAUDE.md's explicit carve-out ("Keep lifecycle and dispatch logic in run_actor"), so it's not a layering violation, but future changes to the crash-detection heuristic (e.g. what counts as "shutdown-induced") will need to be kept in sync manually between the two sites.
Test coverage
Good coverage: dedicated stop_intent test module in tests/sleep.rs verifying the envoy message carries (or omits) the error, lifecycle-error tests for calling stop_with_error too early/late, a task.rs integration test asserting the generation stays alive through cleanup after a failed run, and start.rs unit tests covering error, panic, and shutdown-induced-error (not reported) cases for the run_actor path.
Caveat
I wasn't able to execute cargo check/cargo test in this review environment (sandbox blocked cargo invocations), so this review is based on static/manual tracing of the diff and cross-referencing existing definitions rather than a build/test run. Worth confirming CI is green before merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.