Skip to content

Commit 97f85c9

Browse files
committed
feat: US-011 - Drop message from wasm BRIDGE_RIVET_ERROR_SCHEMAS cache key
1 parent a5dcca2 commit 97f85c9

4 files changed

Lines changed: 102 additions & 7 deletions

File tree

rivetkit-typescript/CLAUDE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ Cloudflare Workers forbid `setTimeout`, `fetch`, `connect`, and other async I/O
144144
- Validate wasm-only serve config constraints before converting to core `ServeConfig` or starting registry side effects.
145145
- Run `pnpm --filter @rivetkit/rivetkit-wasm run check:package` after wasm package export or files changes to verify the published tarball includes the root entrypoint and wasm artifacts.
146146
- Build `@rivetkit/rivetkit-wasm` with its package-local pinned `wasm-pack` dependency. Do not use `npx -y wasm-pack`.
147+
- Intern wasm bridged `RivetErrorSchema` values by `(group, code)` only; live per-error messages must stay on `RivetError.message` instead of expanding the schema cache key.
147148
- Track wasm websocket callback regions in a map keyed by monotonic region IDs and remove entries on end so repeated callbacks do not retain empty slots.
148149

149150
## Workflow Context Actor Access Guards

rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ use wasm_bindgen_futures::{JsFuture, spawn_local};
3030

3131
const BRIDGE_RIVET_ERROR_PREFIX: &str = "__RIVET_ERROR_JSON__:";
3232

33-
type BridgeRivetErrorSchemaKey = (String, String, String);
33+
type BridgeRivetErrorSchemaKey = (String, String);
3434

3535
static BRIDGE_RIVET_ERROR_SCHEMAS: LazyLock<
3636
SccHashMap<BridgeRivetErrorSchemaKey, &'static RivetErrorSchema>,
@@ -2604,11 +2604,7 @@ fn leak_str(value: String) -> &'static str {
26042604
}
26052605

26062606
fn bridge_rivet_error_schema(payload: &BridgeRivetErrorPayload) -> &'static RivetErrorSchema {
2607-
let key = (
2608-
payload.group.clone(),
2609-
payload.code.clone(),
2610-
payload.message.clone(),
2611-
);
2607+
let key = (payload.group.clone(), payload.code.clone());
26122608
match BRIDGE_RIVET_ERROR_SCHEMAS.entry_sync(key) {
26132609
scc::hash_map::Entry::Occupied(entry) => *entry.get(),
26142610
scc::hash_map::Entry::Vacant(entry) => {
@@ -2790,6 +2786,15 @@ mod tests {
27902786
.schema
27912787
}
27922788

2789+
fn transport_message(error: &anyhow::Error) -> String {
2790+
error
2791+
.chain()
2792+
.find_map(|cause| cause.downcast_ref::<RivetTransportError>())
2793+
.expect("bridge error should carry RivetTransportError")
2794+
.message()
2795+
.to_owned()
2796+
}
2797+
27932798
#[test]
27942799
fn parse_bridge_rivet_error_reuses_interned_schema() {
27952800
let initial_count = BRIDGE_RIVET_ERROR_SCHEMAS.len();
@@ -2818,6 +2823,18 @@ mod tests {
28182823
parse_bridge_rivet_error(&different_message).expect("bridge error should decode");
28192824
let schema = transport_schema(&error) as *const RivetErrorSchema;
28202825

2826+
assert_eq!(schema, first_schema);
2827+
assert_eq!(transport_message(&error), "different default message");
2828+
assert_eq!(BRIDGE_RIVET_ERROR_SCHEMAS.len(), initial_count + 1);
2829+
2830+
let different_code = bridge_reason(
2831+
"wasm_schema_cache_test",
2832+
"different_code",
2833+
"different code message",
2834+
);
2835+
let error = parse_bridge_rivet_error(&different_code).expect("bridge error should decode");
2836+
let schema = transport_schema(&error) as *const RivetErrorSchema;
2837+
28212838
assert_ne!(schema, first_schema);
28222839
assert_eq!(BRIDGE_RIVET_ERROR_SCHEMAS.len(), initial_count + 2);
28232840
}

scripts/ralph/prd.json

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,73 @@
162162
"priority": 10,
163163
"passes": true,
164164
"notes": "Investigated and documented the Env-bearing cleanup path plus constraints in docs-internal/engine/napi-bridge.md. Implementation deferred until a NAPI integration test can verify TSF drain and reference counts."
165+
},
166+
{
167+
"id": "US-011",
168+
"title": "Drop message from wasm BRIDGE_RIVET_ERROR_SCHEMAS cache key",
169+
"description": "As an operator running a long-lived wasm worker, I want bridged JS error decoding to intern at most one schema per `(group, code)` so callers whose error messages vary per call (timestamps, request IDs, dynamic context) cannot grow the schema map and `Box::leak` arena unboundedly. The current US-002 fix keys on `(group, code, message)`, which still leaks one schema per unique message and re-introduces the same class of leak the PR is trying to close. The NAPI-side `STRUCTURED_TIMEOUT_SCHEMAS` already keys only on `(&'static str, &'static str)` (`rivetkit-typescript/packages/rivetkit-napi/src/napi_actor_events.rs:96-98`); wasm must match.",
170+
"acceptanceCriteria": [
171+
"`BridgeRivetErrorSchemaKey` in `rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs:2596` becomes `(String, String)` (or equivalent two-tuple) keyed only on `(group, code)`",
172+
"`bridge_rivet_error_schema` (`lib.rs:2599-2618`) inserts the first-seen `default_message` for a given `(group, code)` and reuses that schema for subsequent decodes regardless of payload message",
173+
"The actual per-error `message` field continues to flow through `RivetTransportError.message: Some(payload.message)` so callers still see the live message",
174+
"Update `parse_bridge_rivet_error_reuses_interned_schema` to assert that varying only the message for the same `(group, code)` reuses the schema (and therefore does NOT increment `BRIDGE_RIVET_ERROR_SCHEMAS.len()`)",
175+
"Add a second test (or extend the existing one) that varying `(group, code)` does still allocate a new schema",
176+
"`pnpm --filter @rivetkit/rivetkit-wasm run check:package` passes; `cargo test -p rivetkit-wasm` passes",
177+
"Typecheck passes",
178+
"Tests pass"
179+
],
180+
"priority": 11,
181+
"passes": true,
182+
"notes": "Wasm bridged RivetError schemas now intern by `(group, code)` only while preserving each payload's live message on the transport error. Native `cargo test -p rivetkit-wasm` still hits the pre-existing host-target `wasm_bindgen_futures` compile issue; wasm-target cargo test compilation and package checks pass."
183+
},
184+
{
185+
"id": "US-012",
186+
"title": "Bound register_task shutdown drain against shutdown deadline",
187+
"description": "As an operator, I want `ctx.registerTask(promise)` not to block actor shutdown indefinitely when the user-supplied JS promise never resolves. `WasmActorContext::register_task` (`rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs:1345-1356`) and the analogous NAPI path route through `ActorContext::register_task` -> `track_shutdown_task`, so a never-resolving promise hangs shutdown drain. This contradicts the new CLAUDE.md rule added in this PR (\"Spawned futures that capture JS callbacks or other heavy resources must have a guaranteed completion path (e.g. a `CancellationToken` whose clones are guaranteed to drop)\").",
188+
"acceptanceCriteria": [
189+
"`ActorContext::register_task` in `rivetkit-rust/packages/rivetkit-core/src/actor/context.rs:552-587` races the user future against `ctx.shutdown_deadline_token().cancelled()` (or equivalent) so registered tasks unblock shutdown when the grace deadline elapses",
190+
"Cancellation cause is logged at `tracing::warn!` with `actor_id` and a stable `reason` field so operators can find hanging registered tasks",
191+
"Behavior is identical for the `wasm-runtime` and native `cfg(not(feature = \"wasm-runtime\"))` variants of `register_task`",
192+
"Wasm `WasmActorContext::register_task` does not need a separate cancel path because core handles it",
193+
"Add a Rust integration test in `rivetkit-rust/packages/rivetkit-core/tests/` that registers a never-completing future, triggers shutdown, and asserts shutdown completes within a bounded deadline rather than hanging",
194+
"Typecheck passes",
195+
"Tests pass"
196+
],
197+
"priority": 12,
198+
"passes": false,
199+
"notes": ""
200+
},
201+
{
202+
"id": "US-013",
203+
"title": "Avoid panic on websocket_callback_regions ID overflow",
204+
"description": "As an operator running a long-lived wasm actor with frequent websocket callbacks, I do not want my actor to crash after ~4B handshakes because `next_websocket_callback_region_id` overflows `u32`. The current code (`rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs:1351-1355`) uses `checked_add(1).expect(\"websocket callback region id overflow\")`, which crashes the actor process instead of degrading.",
205+
"acceptanceCriteria": [
206+
"Either bump `next_websocket_callback_region_id` to `Cell<u64>` (and update the `begin_websocket_callback` return type plus any wasm-bindgen surface that exposes it) so overflow becomes operationally unreachable",
207+
"Or implement wraparound that skips IDs already present in `websocket_callback_regions` so reuse is safe while regions are in flight",
208+
"Either approach must keep IDs strictly monotonic for IDs currently in the map so no two live regions ever share an ID",
209+
"Add a wasm crate test that exercises the wraparound (or large-counter) path to confirm no panic and no ID collision with live regions",
210+
"If u32 is preserved, the test seeds `next_websocket_callback_region_id` near `u32::MAX` and verifies wraparound",
211+
"Typecheck passes",
212+
"Tests pass"
213+
],
214+
"priority": 13,
215+
"passes": false,
216+
"notes": ""
217+
},
218+
{
219+
"id": "US-014",
220+
"title": "Document global cache delta in parse_bridge_rivet_error test",
221+
"description": "As a future contributor adding tests under `rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs`, I want a comment on `parse_bridge_rivet_error_reuses_interned_schema` (`lib.rs:2715-2737`) explaining that its `BRIDGE_RIVET_ERROR_SCHEMAS.len()` delta assertions only hold because the test owns a unique `(group, code)` namespace (`wasm_schema_cache_test`/`same_payload`). Without the comment, a future test that shares the global cache namespace will silently flip this assertion under parallel `cargo test`.",
222+
"acceptanceCriteria": [
223+
"Add a one or two line comment immediately above `parse_bridge_rivet_error_reuses_interned_schema` (or above the `initial_count` capture) noting that the test relies on a dedicated `(group, code)` namespace so concurrent tests do not perturb the global `BRIDGE_RIVET_ERROR_SCHEMAS.len()` delta",
224+
"Comment names the namespace strings (`wasm_schema_cache_test`, `same_payload`) so future contributors know what to avoid",
225+
"If US-011 lands first and changes the cache key shape, update the comment to reflect the new key",
226+
"Typecheck passes",
227+
"Tests pass"
228+
],
229+
"priority": 14,
230+
"passes": false,
231+
"notes": ""
165232
}
166233
]
167234
}

scripts/ralph/progress.txt

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
- Wasm `JsValue` callbacks are not Send/Sync; `WasmFunction` uses an unsafe Send/Sync impl because the wasm runtime is single-threaded.
1111
- Wasm host smoke tests use fake `WasmBindings` in `rivetkit-typescript/packages/rivetkit/tests/wasm-host-smoke.test.ts` to exercise the TypeScript `WasmCoreRuntime` adapter without requiring a built wasm artifact.
1212
- Wasm bindings that return `Result<_, JsValue>` should map core `anyhow::Error` values with `anyhow_to_js_error`.
13-
- Wasm bridged RivetError schemas are process-interned with `scc::HashMap` by `(group, code, default_message)` to avoid per-error schema leaks while preserving distinct defaults.
13+
- Wasm bridged RivetError schemas are process-interned with `scc::HashMap` by `(group, code)` only; live per-error messages stay on `RivetError.message`.
1414
- Validate wasm-only `ServeConfig` constraints at wasm entrypoints before converting to core `ServeConfig` or starting registry side effects.
1515
- Runtime-owned promises that must drain during shutdown use core `ActorContext::register_task(...)`, not public `wait_until(...)`, so NAPI and wasm can keep `registerTask` intent distinct.
1616
- Public HTTP status promotion for bridged errors lives in `rivetkit-core::error::public_error_status_code`; TS native/wasm adapters should not duplicate promotion tables.
@@ -123,3 +123,13 @@ Started: Sat May 2 02:13:32 AM PDT 2026
123123
- `napi::Ref::unref(env)` cannot be called from receive-loop worker reset paths or arbitrary `Drop` paths because both can lack a valid `Env`.
124124
- A future implementation should wrap stale refs in a TSF payload that forgets the reference on failed delivery, then unrefs only inside the TSF callback while an `Env` is in scope.
125125
---
126+
## 2026-05-02 03:21:47 PDT - US-011
127+
- Changed wasm bridged `RivetErrorSchema` interning to key by `(group, code)` only, reusing the first schema default message even when later payload messages vary.
128+
- Extended the wasm crate cache test so same `(group, code)` with a different live message reuses the schema pointer and does not grow the cache, while a different code still allocates one new schema.
129+
- Files changed: `rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs`, `rivetkit-typescript/CLAUDE.md`, `scripts/ralph/prd.json`, `scripts/ralph/progress.txt`.
130+
- Checks: `cargo test -p rivetkit-wasm --target wasm32-unknown-unknown --no-run parse_bridge_rivet_error_reuses_interned_schema`, `pnpm --filter @rivetkit/rivetkit-wasm run check:package`, `pnpm --filter @rivetkit/rivetkit-wasm run check:wasm`, `pnpm --filter @rivetkit/rivetkit-wasm run check-types`.
131+
- Plain `cargo test -p rivetkit-wasm` still fails on the pre-existing host-target `rivetkit-core` `wasm_bindgen_futures` dependency issue recorded in earlier progress.
132+
- **Learnings for future iterations:**
133+
- Use unique `(group, code)` namespaces when asserting `BRIDGE_RIVET_ERROR_SCHEMAS.len()` deltas because the intern map is process-global.
134+
- Schema defaults are only the first-seen fallback for a `(group, code)`; callers should assert live bridged messages through `RivetTransportError.message()`.
135+
---

0 commit comments

Comments
 (0)