Skip to content

fix(rivetkit): preserve bridge error messages#4959

Merged
NathanFlurry merged 1 commit intomainfrom
bridge-errors/preserve-message
May 5, 2026
Merged

fix(rivetkit): preserve bridge error messages#4959
NathanFlurry merged 1 commit intomainfrom
bridge-errors/preserve-message

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

PR 4959 Review: fix(rivetkit): preserve bridge error messages

Overview

This PR fixes a bug where error messages were lost when bridging RivetErrors across the NAPI/WASM boundary. The root cause: BridgeRivetErrorContext Display was showing internal debug metadata (public, status_code) instead of the actual error message. Since anyhow Error to_string() renders only the outermost context Display, the human-readable message was invisible at the top level.

The fix adds a message field to BridgeRivetErrorContext and routes the actual error text through it.

What is correct

  • Root cause and fix are right. anyhow to_string() renders only the outermost context. Storing the message in the context struct and forwarding it through Display is the correct pattern.
  • Both runtimes updated consistently. actor_factory.rs (NAPI) and rivetkit-wasm/src/lib.rs (WASM) receive identical changes, maintaining parity.
  • actor_context.rs capture order is correct. error.to_string() is evaluated before error is moved into .context(...), so the message is captured from the original error chain.
  • Tests added for both paths. assert_eq!(first.to_string(), ...) in the NAPI test and WASM equivalent verify round-trip message preservation through parse_bridge_rivet_error.

Minor observations

  1. Option vs String for BridgeRivetErrorContext::message

Both call sites always pass Some(message). payload.message is a non-optional String, and error.to_string() in actor_context.rs always yields a String. The None fallback (bridged RivetError) is dead code today. Making the field String would be more precise and eliminate the dead branch, though Option is a reasonable future-safe default.

  1. Message duplication is inherent, not a bug

The message is stored in both RivetError::message (inner anyhow error) and BridgeRivetErrorContext::message (outer context). This is required because anyhow to_string() surfaces only the outermost context. The clone at the parse_bridge_rivet_error call site is the minimal overhead needed.

  1. No test for the actor_context.rs verify path

The change in actor_context.rs (capturing error.to_string() before the verify error is consumed) is not covered by a new test. An integration test that triggers a 401 from verify_token and checks the surfaced error message would close this gap. Not a blocker.

  1. Pre-existing duplication between NAPI and WASM

BridgeRivetErrorContext is defined independently in actor_factory.rs and rivetkit-wasm/src/lib.rs. This PR correctly keeps both in sync, but a shared crate would eliminate future drift risk.

Summary

The fix is correct, targeted, and well-tested for its primary paths. No blocking issues. All observations above are minor or pre-existing.

Copy link
Copy Markdown
Member Author

NathanFlurry commented May 4, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@NathanFlurry NathanFlurry changed the base branch from abort-signal/finalize-once to graphite-base/4959 May 5, 2026 11:38
@NathanFlurry NathanFlurry force-pushed the bridge-errors/preserve-message branch from d612792 to 99e74a3 Compare May 5, 2026 11:53
@NathanFlurry NathanFlurry force-pushed the graphite-base/4959 branch from c2198e9 to 7764046 Compare May 5, 2026 11:53
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4959 to abort-signal/finalize-once May 5, 2026 11:53
Base automatically changed from abort-signal/finalize-once to main May 5, 2026 14:58
@NathanFlurry NathanFlurry merged commit 99e74a3 into main May 5, 2026
6 of 22 checks passed
@NathanFlurry NathanFlurry deleted the bridge-errors/preserve-message branch May 5, 2026 14:58
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