Skip to content

feat(rivetkit): add synchronous sqlite operations - #5659

Open
NathanFlurry wants to merge 1 commit into
mainfrom
sync-sqlite
Open

feat(rivetkit): add synchronous sqlite operations#5659
NathanFlurry wants to merge 1 commit into
mainfrom
sync-sqlite

Conversation

@NathanFlurry

@NathanFlurry NathanFlurry commented Sep 4, 2026

Copy link
Copy Markdown
Member
  • Add opt-in synchronous SQLite execution and callback-based transactions for Node.js raw and Drizzle database clients.
  • Begin, commit, and roll back synchronous transactions before returning while rejecting async callbacks and unsupported runtimes.
  • Warn that synchronous operations can block every actor sharing a Node.js runtime and document the upcoming worker-thread isolation mode.
  • Cover queries, multi-statement execution, transaction lifecycle, runtime parity, and integration type contracts.

@railway-app

railway-app Bot commented Sep 4, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5659 environment in rivet-frontend

Service Status Web Updated
frontend-cloud 😴 Sleeping (View Logs) Web Sep 6, 2026 at 2:59 pm UTC
frontend-inspector 😴 Sleeping (View Logs) Web Sep 6, 2026 at 12:39 pm UTC
kitchen-sink 😴 Sleeping (View Logs) Web Sep 6, 2026 at 12:16 pm UTC
website ❌ Build Failed (View Logs) Web Sep 4, 2026 at 8:20 am UTC
ladle ✅ Success (View Logs) Web Sep 4, 2026 at 4:50 am UTC
mcp-hub ✅ Success (View Logs) Web Sep 4, 2026 at 4:48 am UTC

@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review: synchronous SQLite operations

Solid feature addition with good breadth (napi bindings, both db() and drizzle() wrappers, wasm rejection path, docs, and driver-test coverage). Two things worth a closer look before merge:

1. Unverified assumption behind wait_for_runtime (medium-high risk)

rivetkit-napi/src/database.rs:370-386:

fn wait_for_runtime<T, F>(future: F) -> napi::Result<T>
where F: Future<Output = anyhow::Result<T>> {
    let runtime = tokio::runtime::Handle::try_current().map_err(|error| { ... })?;
    // NAPI-RS enters its multithreaded runtime before invoking synchronous exports.
    tokio::task::block_in_place(|| runtime.block_on(future)).map_err(...)
}

Every executeSync/transactionSync/commitSync/rollbackSync NAPI export depends on Handle::try_current() succeeding on the thread that N-API uses to invoke synchronous exports (i.e. the JS calling thread, not the pool used for async fn exports). The comment states this as a fact, but I could not find anywhere else in rivetkit-napi that builds a runtime and calls .enter()/leaks the guard on that thread (the only Builder::new_multi_thread()...enter() in the crate is inside the new unit test itself, database.rs:485-497). If that assumption is wrong, every executeSync call would fail immediately at the "cannot run synchronous SQLite operation" branch.

The new Rust test (synchronous_wait_uses_the_active_multithreaded_runtime) only proves the helper works when a runtime is manually built and entered on the same thread inside the test process, it does not exercise the real N-API call path, so it cannot actually validate the comments claim. The real validation is the driver test at tests/driver/actor-db.test.ts:710-739 ("supports synchronous queries only in the Node.js native runtime"), which does go through the real native binding. Worth double-checking that this specific test actually ran (and passed) against the native/NAPI driver in CI for this PR, not just the wasm-skip branch, since that is the only thing in this diff that would empirically confirm the runtime-context assumption holds.

2. transactionSync with a callback that does partial work before its first await (low-medium)

common/database/shared.ts:39-70 (runSqliteTransactionSync) detects a returned promise after the callback has already run synchronously up to its first await, then immediately rolls back and throws. But the original async callback is still suspended and will resume later (e.g. once its awaited promise settles), potentially calling tx.executeSync(...) again on the closure-captured tx.

I confirmed the underlying transaction coordinator (rivetkit-cores SqliteTransaction/tx.rs) tracks commit/rollback centrally and returns an explicit "already committed/rolled back" error for any post-finish execute, so this does not silently corrupt data, but it does surface as an unhandled promise rejection disconnected from the original transactionSync() call/catch site, which is confusing and could crash the process if nothing has a global unhandledRejection handler. The only async-callback test case covers the trivial async () => undefined (no work before the await); consider adding a test for "sync work, then await, then another tx.executeSync" so the failure mode is at least pinned down, and maybe mention this specific footgun in the sqlite.mdx docs alongside the "must not return a promise" note.

Minor

  • The new #[cfg(test)] mod tests block in rivetkit-napi/src/database.rs:485 is inline in src/ rather than under tests/, which CLAUDE.md asks for. wait_for_runtime is private so this likely qualifies as a justified exception, but per the documented convention the preferred form is a source-owned #[path = "..."] shim into a file under tests/ rather than a fully inline body. Low priority since this file already had inline test blocks before this PR.
  • native-database.tss isSynchronousTransaction() runtime type-guard (used in wrapTransaction) looks like it is now unreachable/always-true, since JsNativeDatabaseLike.beginTransaction was retyped to return Promise<JsNativeSynchronousTransactionLike> (no longer the plain JsNativeTransactionLike), so every implementer is required to provide commitSync/rollbackSync at the type level already. Not a bug, just a possible simplification.

What looks good

  • Consistent mirroring of the sync surface across mod.ts, drizzle.ts, native-database.ts, napi-runtime.ts/wasm-runtime.ts, with wasm-runtime.ts correctly rejecting every sync entry point with a clear "only available in the Node.js native runtime" error.
  • synchronousTransactionActive guarding against nested transactionSync() calls and against using the outer (non-transaction-scoped) client for queries while a sync transaction is in flight, with matching test coverage (mod.test.ts, drizzle.test.ts).
  • Docs update clearly warns that synchronous calls block the whole Node.js runtime, not just the current actor, and calls out the future worker-thread isolation plan.
  • SQL parameter binding still goes through the existing toNativeBindings/toSqliteBindings path, so no new injection surface.

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