|
| 1 | +# Driver Test Fix Audit |
| 2 | + |
| 3 | +Audited: 2026-04-18 |
| 4 | +Updated: 2026-04-18 |
| 5 | +Scope: All uncommitted changes on feat/sqlite-vfs-v2 used to pass the driver test suite |
| 6 | +Method: Compared against original TS implementation (ref 58b217920) across 5 subsystems |
| 7 | + |
| 8 | +## Verdict: No test-overfitting found. 3 parity gaps fixed, 1 architectural debt item remains intentionally unchanged. |
| 9 | + |
| 10 | +--- |
| 11 | + |
| 12 | +## Issues Found |
| 13 | + |
| 14 | +### BARE-only encoding on actor-connect WebSocket (fixed) |
| 15 | + |
| 16 | +The Rust `handle_actor_connect_websocket` in `registry.rs` rejects any encoding that isn't `"bare"` (line 1242). The original TS implementation accepted `json`, `cbor`, and `bare` via `Sec-WebSocket-Protocol`, defaulting to `json`. Tests only exercise BARE, so this passed. Production JS clients that default to JSON encoding will fail to connect. |
| 17 | + |
| 18 | +**Severity**: High (production-breaking for non-BARE clients) |
| 19 | +**Type**: Incomplete port, not overfit |
| 20 | + |
| 21 | +### Error metadata dropped on WebSocket error responses (fixed) |
| 22 | + |
| 23 | +`action_dispatch_error_response` in `registry.rs` hardcodes `metadata: None` (line 3247). `ActionDispatchError` in `actor/action.rs` lacks a `metadata` field entirely, so it's structurally impossible to propagate. The TS implementation forwarded CBOR-encoded metadata bytes from `deconstructError`. Structured error metadata from user actors is silently lost on WebSocket error frames. |
| 24 | + |
| 25 | +**Severity**: Medium (error context lost, but group/code preserved) |
| 26 | +**Type**: Incomplete port |
| 27 | + |
| 28 | +### Workflow inspector stubs (fixed) |
| 29 | + |
| 30 | +`NativeWorkflowRuntimeAdapter` has two stubs: |
| 31 | +- `isRunHandlerActive()` always returns `false` — disables the safety guard preventing concurrent replay + live execution |
| 32 | +- `restartRunHandler()` is a no-op — inspector replay computes but never takes effect |
| 33 | + |
| 34 | +Normal workflow execution (step/sleep/loop/message) works. Inspector-driven workflow replay is broken on the native path. |
| 35 | + |
| 36 | +**Severity**: Low (inspector-only, not user-facing) |
| 37 | +**Type**: Known incomplete feature |
| 38 | + |
| 39 | +### Action timeout/size enforcement in wrong layer (left as-is) |
| 40 | + |
| 41 | +TS `native.ts` adds `withTimeout()` and byte-length checks for actions. Rivetkit-core also has these in `actor/action.rs` and `registry.rs`. However, the native HTTP action path bypasses rivetkit-core's event dispatch (`handle_fetch` instead of `actor/event.rs`), so TS enforcement is the pragmatic correct location. Not duplicated at runtime for the same request, but the code exists in both layers. |
| 42 | + |
| 43 | +**Severity**: Low (correct behavior, architectural debt) |
| 44 | +**Type**: Wrong layer, but justified by current routing |
| 45 | + |
| 46 | +--- |
| 47 | + |
| 48 | +## Confirmed Correct Fixes |
| 49 | + |
| 50 | +- **Stateless actor state gating** — Config-driven, matches original TS behavior |
| 51 | +- **KV adapter key namespacing** — Uses standard `KEYS.KV` prefix, matches `ActorKv` contract |
| 52 | +- **Error sanitization** — Uses `INTERNAL_ERROR_DESCRIPTION` constant and `toRivetError()`, maps by group/code pairs |
| 53 | +- **Raw HTTP void return handling** — Throws instead of silently converting to 204, matches TS contract |
| 54 | +- **Lifecycle hooks conn params** — Fixed in client-side `actor-handle.ts`, correct layer |
| 55 | +- **Connection state bridging** — `createConnState`/`connState` properly wired, fires even without `onConnect` |
| 56 | +- **Sleep/lifecycle/destroy timing** — `begin_keep_awake`/`end_keep_awake` tracked through `ActionInvoker.dispatch()`, no timing hacks |
| 57 | +- **BARE codec** — Correct LEB128 varint, canonical validation, `finish()` rejects trailing bytes |
| 58 | +- **Actor key deserialization** — Faithful port of TS `deserializeActorKey` with same escape sequences |
| 59 | +- **Queue canPublish** — Real `NativeConnHandle` via `ctx.connectConn()` with proper cleanup |
| 60 | + |
| 61 | +## Reviewed and Dismissed |
| 62 | + |
| 63 | +- **`tokio::spawn` for WS action dispatch** — Not an issue. Spawned tasks call `invoker.dispatch()` which calls `begin_keep_awake()`/`end_keep_awake()`, so sleep is properly blocked. The CLAUDE.md `JoinSet` convention is about `envoy-client` HTTP fetch, not rivetkit-core action dispatch. |
| 64 | +- **`find()` vs `strip_prefix()` in error parsing** — Intentional. Node.js can prepend context to NAPI error messages, so `find()` correctly locates the bridge prefix mid-string. Not a bug, it's a fix for errors being missed. |
| 65 | +- **Hardcoded empty-vec in `connect_conn`** — Correct value for internally-created connections (action/queue HTTP contexts) which have no response body to send. |
| 66 | + |
| 67 | +## Minor Notes |
| 68 | + |
| 69 | +- `rearm_sleep_after_http_request` helper duplicated in `event.rs` and `registry.rs` — intentional per CLAUDE.md (two dispatch paths), but could be extracted |
| 70 | +- `_is_restoring_hibernatable` parameter accepted but unused in `handle_actor_connect_websocket` |
| 71 | +- Unused `Serialize`/`Deserialize` derives on protocol structs (hand-rolled BARE used instead) |
| 72 | +- No tests for `Request` propagation through connection lifecycle callbacks |
| 73 | +- No tests for message size limit enforcement at runtime |
0 commit comments