Skip to content

feat(rivetkit): add actor onError hook#5385

Draft
NathanFlurry wants to merge 2 commits into
mainfrom
on-error
Draft

feat(rivetkit): add actor onError hook#5385
NathanFlurry wants to merge 2 commits into
mainfrom
on-error

Conversation

@NathanFlurry

Copy link
Copy Markdown
Member
  • Add the universal actor onError hook across rivetkit-core, NAPI, wasm, and TypeScript runtime plumbing.
  • Report action, hook, queue, internal, fatal, timeout, and state-change errors without allowing onError to suppress the original failure.
  • Document the lifecycle hook and Sentry integration path.
  • Add driver coverage for actor-level and registry-level delivery, ordering, swallowed hook failures, timeout reporting, and actor.aborted suppression.
  • Verified with cargo check -p rivetkit-core, pnpm build:force in packages/rivetkit-napi, pnpm build in packages/rivetkit-wasm, and targeted native/wasm actor-onerror driver tests.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review: universal actor onError hook

Reviewed the Rust core, NAPI/wasm bridges, TS config, and driver tests. Overall the design is sound (typed ActorErrorEvent, HookName/RawErrorRef context markers, suppression of expected lifecycle errors like actor.aborted), but there's one correctness issue worth fixing before merge, plus a few smaller cleanups.

Correctness

report_error's in-flight guard silently drops concurrent error reports (rivetkit-rust/packages/rivetkit-core/src/actor/context.rs:691-724)

if self.0.error_report_in_flight
    .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire)
    .is_err()
{
    return;
}

This is a single AtomicBool shared across the whole actor. If two errors are reported at (nearly) the same instant from different tokio tasks — e.g. two concurrent actions failing, or a persist failure racing an action failure — whichever call loses the compare-exchange returns immediately and its ErrorReport is never delivered to onError, with no retry, queue, or log. This directly contradicts the PR's stated goal ("report ... errors without allowing onError to suppress the original failure") — the original failure still propagates to the caller, but the report to onError/Sentry is silently lost.

I don't see evidence this guards against a real reentrancy hazard: the hook itself is invoked non-blocking (NAPI ThreadsafeFunctionCallMode::NonBlocking) or via Promise.resolve().then(...) in the JS bridge (native.ts), so report_error never recurses synchronously through the hook. If the intent was to coalesce a burst of errors, that should be documented and probably scoped more narrowly (e.g. per error class or with a short debounce window) rather than a bare "only one report in flight for the whole actor, everything else is dropped" gate. As-is this looks like it will cause real, hard-to-notice gaps in Sentry/error-tracker coverage under load, and there's no test exercising concurrent report_error calls to catch it.

Simplification / dead code

Triplicated, effectively-dead string-matching fallback in hook_name_from_error (rivetkit-rust/packages/rivetkit-core/src/actor/task.rs:2249, rivetkit-typescript/packages/rivetkit-napi/src/napi_actor_events.rs:895, rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs:606 — identical ~25-line function in all three files):

match cause.to_string().as_str() {
    "createState" => Some("createState"),
    "onCreate" => Some("onCreate"),
    ...
}

Every call site that can produce these errors already attaches the typed HookName marker via with_timeout(...) (napi) or explicit .context(HookName("...")) (wasm run_preamble), which the first branch (downcast_ref::<HookName>()) already handles. I couldn't find anywhere in the current codebase that produces a bare anyhow error whose Display is literally "onCreate", "run", etc. without already carrying the typed marker — so this string-match branch looks unreachable today. Worse, it's a latent footgun: if a user's own error happens to have a Display that equals one of these literals (e.g. anyhow!("run"), or a config-validation error whose message is literally "onDestroy"), it will be silently misclassified as that hook's error. Recommend either deleting the fallback (relying solely on the typed marker) or, if it's meant as defense-in-depth, extracting it to one shared helper instead of copy-pasting it three times.

Style

Formatting regression in napi_actor_events.rs — the ActorEvent::RunGracefulCleanup and ActorEvent::DisconnectConn match arms (lines ~638-710) are indented one extra tab level relative to their sibling arms in the same match, and use inconsistent indentation internally. Looks like it wasn't run through the formatter (node scripts/format/agent-format.mjs per CLAUDE.md) after editing.

Minor / efficiency

stashRawError runs unconditionally for every native callback error (rivetkit-typescript/packages/rivetkit/src/registry/native.ts:1201-1209), even when neither the actor nor the registry defines onError. It's bounded (evicted past 1024 entries), so not a real leak, but it's unconditional Map churn on every actor error in the common case where nobody consumes onError. Consider gating stashRawError on whether an onError hook is actually configured (this is already known at factory-build time in buildNativeFactory).

Test coverage

Driver coverage for actor/registry ordering, swallowed hook failures, timeout reporting, and actor.aborted suppression looks solid. Given the concurrency concern above, it'd be worth adding a driver test that triggers two failures for the same actor near-simultaneously (e.g. a failing scheduled action plus a concurrent failing direct action) and asserts both show up in onError.

Nice use of the typed ActorErrorEvent/HookName/RawErrorRef markers to thread hook identity and the original JS error object through the Rust boundary back to native.ts — that's a clean way to preserve instanceof checks (e.g. UserError) in the Sentry example doc without re-serializing errors.

@railway-app

railway-app Bot commented Jul 8, 2026

Copy link
Copy Markdown

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

Service Status Web Updated (UTC)
kitchen-sink 😴 Sleeping (View Logs) Web Jul 8, 2026 at 4:01 am
website 😴 Sleeping (View Logs) Web Jul 8, 2026 at 3:59 am
frontend-inspector 😴 Sleeping (View Logs) Web Jul 8, 2026 at 3:59 am
frontend-cloud 😴 Sleeping (View Logs) Web Jul 8, 2026 at 3:57 am
ladle ✅ Success (View Logs) Web Jul 8, 2026 at 3:51 am
mcp-hub ✅ Success (View Logs) Web Jul 8, 2026 at 3:49 am

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