Skip to content

fix(rivetkit): keep UserErrors intact when their metadata is not JSON - #5782

Merged
eersnington merged 3 commits into
mainfrom
stack/fix-rivetkit-leave-error-metadata-out-of-span-error-encoding-skulmuyw
Oct 1, 2026
Merged

eersnington merged 3 commits into
mainfrom
stack/fix-rivetkit-leave-error-metadata-out-of-span-error-encoding-skulmuyw

Conversation

@eersnington

@eersnington eersnington commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

A UserError whose metadata JSON cannot encode, such as a bigint, was replaced by a TypeError when it crossed from TypeScript into core.

throw new UserError("Card declined", {
	code: "card_declined",
	metadata: { amountCents: 1999n },
});
Where it is thrown Before After
action or run client gets internal_error client gets card_declined, without metadata
workflow step, tracing on step fails without retrying step retries
c.client() call, tracing on caller gets the TypeError caller gets the original error
  • encodeBridgeRivetError encodes every structured error that crosses into core. When JSON.stringify throws, it encodes the error again without metadata and returns the cause as droppedMetadataCause.
  • encodeNativeCallbackError logs a warning with the group, code, and cause when metadata was dropped. The span paths ignore it, because spans never read metadata.
  • actor/errors.ts has no runtime imports, so rivetkit/errors bundles to 4.7 KB minified. Importing the logger there would pull in pino and zod and grow it to 304 KB.
  • The errors docs say a UserError is returned to the client and only other errors become internal_error. This keeps that true for metadata JSON cannot encode.

This is part 1 of 3 in a stack:

@eersnington

eersnington commented Sep 23, 2026 •

Copy link
Copy Markdown
Member Author

Stack for rivet-dev/actors

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

change skulmuyw

@railway-app

railway-app Bot commented Sep 23, 2026

Copy link
Copy Markdown

This PR was not deployed automatically as @eersnington does not have access to the Railway project.

In order to get automatic PR deploys, please add @eersnington to your workspace on Railway.

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues found

Reviewed commit 2becb24.

@claude

claude Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Review: fix(rivetkit): leave error metadata out of span error encoding

Summary

Correct, well-scoped fix. Root cause is exactly what the PR description says: encodeErrorForBridge -> encodeBridgeRivetError used to JSON.stringify the full error payload (including metadata), and in actor-handle.ts:315 that call sits inside a catch block ahead of throw error;. When JSON.stringify threw on the bigint in metadata, it did so before throw error ran, so the TypeError silently replaced the real error and the caller/step saw the wrong failure (and workflow retries never got a chance to run).

I traced both call sites (actor-handle.ts:315 for outbound client() calls, napi-runtime.ts:655 for workflow step spans) down into the Rust core (OutboundCallInvocation::finish / WorkflowStepSpan::finish -> record_outcome in rivetkit-rust/packages/rivetkit-core/src/telemetry.rs:1056). record_outcome only ever reads error.group() / error.code() via RivetError::extract. So restricting the bridge-for-telemetry payload to group/code/message is not a functional regression -- metadata, rayId, statusCode, and actor were already unused on this path. Good catch narrowing the encoding to only what is consumed, which also fits this repos convention against payload debug dumps in telemetry.

Also worth noting: the real error thrown to the caller (throw error in actor-handle.ts:316) is unaffected by this change either way -- it is always the original JS error object, not the bridge-encoded string. This fix is purely about not letting telemetry encoding replace that error via an uncaught exception.

Test coverage gap

The new/renamed test (actor-telemetry.test.ts:1177) only exercises the workflow step path (napi-runtime.ts -> startWorkflowSpan().startStep().finish()). The PR description explicitly calls out a second symptom -- "A call through c.client() returned the same TypeError to its caller" -- which goes through the separate actor-handle.ts:315 call.finish(encodeErrorForBridge(error)) path. That path does not appear to have a regression test anywhere (searched for amountCents/1999n/card declined across tests/ and fixtures/).

Since rivet-error.test.ts already has a fast, isolated unit-test suite for the bridge encode/decode helpers (encodeBridgeRivetError/decodeBridgeRivetError), it would be cheap to add a direct unit test there for encodeErrorForBridge itself: assert it does not throw and drops metadata for a RivetError constructed with non-JSON-safe metadata (e.g. a bigint). That would cover both call sites without needing the slower driver-test infrastructure, and directly pins down the "encoding cannot throw" invariant the new doc comment claims.

Minor

  • The updated JSDoc on encodeErrorForBridge and the fixture comment both read well and follow the repos comment conventions (complete sentences, no dashes/parentheticals).
  • No security, performance, or style concerns -- this is a small, targeted fix with hard tabs preserved and no new dependencies.

Verdict

Looks correct and safe to merge. The one actionable suggestion is adding a direct unit test (or a driver test through the client() call-span path) so the second symptom described in the PR body has explicit regression coverage.

🤖 Generated with Claude Code

@eersnington
eersnington force-pushed the stack/fix-rivetkit-leave-error-metadata-out-of-span-error-encoding-skulmuyw branch from 2becb24 to 33c84b3 Compare October 1, 2026 13:59
@eersnington eersnington changed the title fix(rivetkit): leave error metadata out of span error encoding fix(rivetkit): keep UserErrors intact when their metadata is not JSON Oct 1, 2026

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 1 medium · 🔵 1 low

Reviewed commit 33c84b3.

Comment thread rivetkit-typescript/packages/rivetkit/src/actor/errors.ts Outdated
Comment thread rivetkit-typescript/packages/rivetkit/src/actor/errors.ts Outdated
@eersnington
eersnington force-pushed the stack/fix-rivetkit-leave-error-metadata-out-of-span-error-encoding-skulmuyw branch from 33c84b3 to 575812e Compare October 1, 2026 14:36
@eersnington
eersnington merged commit 74a23d3 into main Oct 1, 2026
5 of 11 checks passed
@eersnington
eersnington deleted the stack/fix-rivetkit-leave-error-metadata-out-of-span-error-encoding-skulmuyw branch October 1, 2026 22:49
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.

2 participants