Skip to content

chore: fix driver test suite#4591

Merged
MasterPtato merged 1 commit intomainfrom
04-08-chore_fix_driver_test_suite
Apr 13, 2026
Merged

chore: fix driver test suite#4591
MasterPtato merged 1 commit intomainfrom
04-08-chore_fix_driver_test_suite

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

@NathanFlurry NathanFlurry mentioned this pull request Apr 8, 2026
11 tasks
@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

PR Review: chore: fix driver test suite

This PR refactors the driver test suite and includes runtime fixes in the envoy client, native SQLite database bindings, and the dynamic isolate runtime.


Bugs / Correctness Issues

  1. std::sync::Mutex for JsNativeDatabase needs a justifying comment

File: rivetkit-typescript/packages/rivetkit-native/src/database.rs

CLAUDE.md prohibits Mutex for shared containers. The lock is never held across an .await (only acquired inside spawn_blocking), so std::sync::Mutex is acceptable here. Without a comment explaining this, a future reader will flag it as a policy violation.

  1. as_ptr() silently returns null when database is closed

File: rivetkit-typescript/packages/rivetkit-native/src/database.rs

as_ptr() uses .unwrap_or(ptr::null_mut()), silently returning null if the database is closed or mutex is poisoned. Any caller passing this pointer to raw SQLite C functions without a null check will trigger undefined behavior. Either remove as_ptr() from the public surface or document that callers must null-check the return value.

  1. EMPTY_KEY constant reused as join separator

File: engine/packages/guard/src/routing/pegboard_gateway/resolve_actor_query.rs

EMPTY_KEY is named for the empty-key sentinel but is reused as the join separator via escaped_parts.join(EMPTY_KEY). The naming is misleading. Consider renaming to KEY_SEPARATOR for the join, keeping EMPTY_KEY only for the early-return path.

  1. bigint truncation in toNativeBinding

File: rivetkit-typescript/packages/rivetkit-native/wrapper.js

Number(bigint) loses precision for values outside Number.MAX_SAFE_INTEGER (2^53 - 1). SQLite INTEGER supports 64-bit signed integers and the Rust side accepts i64, so this silently truncates large values. Either throw for out-of-safe-range values or preserve full 64-bit precision across the boundary.

  1. normalizeBindings throws on object bindings with positional ? params

File: rivetkit-typescript/packages/rivetkit-native/wrapper.js

When a caller passes a plain object as the binding but the SQL uses positional ? placeholders (valid SQLite), normalizeBindings throws "native sqlite object bindings require named placeholders". The check should fall through to positional binding when the SQL has no named params.


Code Quality Issues

  1. execute_statement silently discards rows from RETURNING clauses

File: rivetkit-typescript/packages/rivetkit-native/src/database.rs

execute_statement steps through rows but discards all SQLITE_ROW results, silently discarding results from INSERT ... RETURNING. The JS execute wrapper guards this in practice, but a comment explaining the limitation would prevent future misuse if execute_statement is called directly.

  1. Post-dispose pool re-creation risk in loadSqliteVfsPool

File: rivetkit-typescript/packages/rivetkit/dynamic-isolate-runtime/src/index.cts

The module-level sqliteVfsPoolPromise singleton is reset to undefined in dynamicDisposeEnvelope. If loadSqliteVfsPool is called after disposal, a new pool will be created post-dispose. Consider guarding with a disposed flag that permanently blocks re-initialization.


Convention Violations

  1. console.error usage in wrapper.js

Several error paths use console.error for runtime errors. These are pre-existing but the PR converts these callbacks to async functions. CLAUDE.md requires structured logging rather than console.error for observable runtime errors.


Minor / Nits

  • sandboxImageReady singleton reset-on-failure (sandbox.ts): Correctly resets to undefined in the catch block. Good pattern.
  • onOpen retroactive fire (actor-conn.ts): The queueMicrotask pattern for late subscribers is correct. The openHandlers guard is a nice safety touch.
  • schedule-sleep.ts: Removing the setTimeout wrapper changes observable timing. Confirm the test suite validates this does not cause a premature sleep response race.
  • .agent/notes/driver-engine-static-test-order.md: Correct placement per CLAUDE.md.

Summary

Priority File Issue
High database.rs as_ptr() can return null - document or remove from public API
High wrapper.js bigint truncation in toNativeBinding loses precision silently
Medium resolve_actor_query.rs Rename EMPTY_KEY used as join separator
Medium database.rs Add comment justifying std::sync::Mutex in spawn_blocking context
Medium wrapper.js normalizeBindings throws on object bindings with positional params
Low index.cts Guard loadSqliteVfsPool against post-dispose re-creation

@NathanFlurry NathanFlurry force-pushed the 04-08-chore_fix_driver_test_suite branch from 1919c35 to 7283250 Compare April 8, 2026 11:01
@MasterPtato MasterPtato changed the base branch from 04-07-chore_remove_udb_as_a_dependency_of_envoy-client to graphite-base/4591 April 8, 2026 20:11
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 7283250 to 5c3dbdf Compare April 8, 2026 20:11
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 5c3dbdf to 5037448 Compare April 8, 2026 20:14
@MasterPtato MasterPtato changed the base branch from graphite-base/4591 to 04-08-chore_tunnel_auth April 8, 2026 20:14
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 5037448 to ffdf968 Compare April 8, 2026 20:30
@NathanFlurry NathanFlurry changed the base branch from 04-08-chore_tunnel_auth to graphite-base/4591 April 8, 2026 20:39
@NathanFlurry NathanFlurry force-pushed the 04-08-chore_fix_driver_test_suite branch from ffdf968 to 9b6c03d Compare April 8, 2026 20:39
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4591 to 04-07-chore_remove_udb_as_a_dependency_of_envoy-client April 8, 2026 20:39
@MasterPtato MasterPtato changed the base branch from 04-07-chore_remove_udb_as_a_dependency_of_envoy-client to graphite-base/4591 April 8, 2026 20:46
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 9b6c03d to 7319f2d Compare April 8, 2026 20:46
@MasterPtato MasterPtato changed the base branch from graphite-base/4591 to 04-08-chore_tunnel_auth April 8, 2026 20:46
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 7319f2d to 8a1d3a0 Compare April 8, 2026 20:49
@MasterPtato MasterPtato changed the base branch from 04-08-chore_tunnel_auth to graphite-base/4591 April 8, 2026 21:32
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 8a1d3a0 to a84fd3d Compare April 8, 2026 21:38
@graphite-app graphite-app bot changed the base branch from graphite-base/4591 to main April 8, 2026 21:38
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from a84fd3d to 124d6d9 Compare April 8, 2026 21:38
@NathanFlurry NathanFlurry force-pushed the 04-08-chore_fix_driver_test_suite branch from 124d6d9 to c84f9db Compare April 8, 2026 22:11
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from c84f9db to ab96a82 Compare April 8, 2026 23:26
@MasterPtato MasterPtato mentioned this pull request Apr 9, 2026
11 tasks
@NathanFlurry NathanFlurry mentioned this pull request Apr 9, 2026
11 tasks
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from ab96a82 to 91a3a2b Compare April 12, 2026 19:42
@NathanFlurry NathanFlurry force-pushed the 04-08-chore_fix_driver_test_suite branch from 91a3a2b to ab96a82 Compare April 12, 2026 22:30
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from ab96a82 to 56f7599 Compare April 12, 2026 23:38
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 56f7599 to ea36259 Compare April 13, 2026 00:20
Copy link
Copy Markdown
Contributor

MasterPtato commented Apr 13, 2026

Merge activity

  • Apr 13, 12:21 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 13, 12:22 AM UTC: @MasterPtato merged this pull request with Graphite.

@MasterPtato MasterPtato merged commit a22b07c into main Apr 13, 2026
5 of 14 checks passed
@MasterPtato MasterPtato deleted the 04-08-chore_fix_driver_test_suite branch April 13, 2026 00:22
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