Skip to content

feat(rivetkit): report actor run() errors to the engine as errored stops#5406

Open
abcxff wants to merge 1 commit into
mainfrom
stack/feat-rivetkit-report-actor-run-errors-to-the-engine-as-errored-stops-nzkvyrmr
Open

feat(rivetkit): report actor run() errors to the engine as errored stops#5406
abcxff wants to merge 1 commit into
mainfrom
stack/feat-rivetkit-report-actor-run-errors-to-the-engine-as-errored-stops-nzkvyrmr

Conversation

@abcxff

@abcxff abcxff commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@abcxff

abcxff commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/rivet

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

change nzkvyrmr

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review

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.

No blocking issues found.

@railway-app

railway-app Bot commented Jul 14, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5406 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Jul 14, 2026 at 3:09 pm
kitchen-sink 😴 Sleeping (View Logs) Web Jul 14, 2026 at 3:09 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jul 14, 2026 at 3:04 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jul 14, 2026 at 3:02 pm
ladle ✅ Success (View Logs) Web Jul 14, 2026 at 2:59 pm
mcp-hub ✅ Success (View Logs) Web Jul 14, 2026 at 2:57 pm

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