feat(sqlite): add cold read benchmarks and simplify optimizations

[NathanFlurry]

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]

• chore(driver-test-runner): add native/wasm runtime matrix to skill #4884 : 4 dependent PRs (#4892 , #4894 , #4896 and 1 other)
• chroe: rename rivetkit-sqlite to depot-client #4883
• fix(rivetkit-wasm): fix mem leaks #4880
• chore(rivetkit): wasm support #4860
• feat(sqlite): add cold read benchmarks and simplify optimizations #4857 👈 (View in Graphite)
• feat(sqlite): benchmark cold reads #4848 : 1 other dependent PR (#4864 )
• docs(actors): document manual lifecycle controls #4847
• test(sqlite): expand db stress coverage #4846 : 1 other dependent PR (#4849 )
• test(raw-websocket): add serverless smoke coverage #4845
• feat(serverless): configure drain grace period #4844
• fix(publish): enable frontend build for engine target and use turbo build instead of turbo build:engine #4843
• feat(rivetkit): expose conn handle in onWebSocket context #4842
• fix(rivetkit): normalize waitUntil/keepAwake promises to null to avoid serde undefined error #4841
• fix(rivetkit): remove requirement for token #4839 : 1 other dependent PR (#4840 )
• fix(pegboard-envoy): allow sqlite requests across lifecycle states #4838
• fix(pegboard): route scoped actor key reads by runner dc #4837
• fix(sqlite-storage): let sqlite open take over stale owners #4834
• fix(engine-tests): parallelize multi-DC startup + enable 6 multi-DC tests #4833
• fix(api): honor cursor in actor_ids list path #4832
• fix(api): silently filter invalid IDs from actor_ids query instead of 400 #4831
• fix(api-peer): make actors_delete idempotent for already-destroyed actors #4830
• fix(pegboard): use Iterator streaming mode in list_names so limit param is honored #4829
• chore(engine-tests): convert envoy tests to Sleep crash policy + triage ignored #4828
• feat(engine-tests): wire envoy actor names through prepopulate_actor_names #4827
• chore(depot): fault injection tests #4868
• feat(sqlite): pitr & forking #4859 : 1 other dependent PR (#4863 )
• chore(sqlite): stateless storage refactor #4856
• main

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

[github-actions]

Preview packages published to npm

Install with:

npm install rivetkit@pr-4857

All packages published as 0.0.0-pr.4857.3a10f72 with tag pr-4857.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-3a10f72
docker pull rivetdev/engine:full-3a10f72

Individual packages

npm install rivetkit@pr-4857
npm install @rivetkit/react@pr-4857
npm install @rivetkit/rivetkit-napi@pr-4857
npm install @rivetkit/workflow-engine@pr-4857

[claude]

Code Review

Overview

This draft PR adds cold-read benchmarks for the SQLite real-world workload harness and lays the groundwork for a read/write connection pool (per .agent/specs/sqlite-read-connection-pool.md). Key additions:

• New NativeConnectionManager — read/write mode state machine with idle TTL pruning, lease types, metrics, and a Notify-based admission gate.
• Statement classification — classify_statement() and install_reader_authorizer() in query.rs using SQLite's authorizer API.
• Unified execute() API — added from NAPI through rivetkit-core up to the TypeScript public surface; removes the AsyncMutex serialization from mod.ts, native-database.ts, and drizzle.ts.
• NativeCloseGate — replaces the single-caller mutex approach with an active-count gate that lets concurrent reads proceed and waits for them before closing.
• Benchmark scripts — new sqlite-realworld-bench.ts runner and result files under .agent/benchmarks/.

The connection manager is new infrastructure not yet wired into SqliteDb (step 7 of the spec). The execute() API routes through the existing single-connection NativeDatabaseHandle for now.

---

Bugs

NativeCloseGate.close is not wait-idempotent (native-database.ts)

A second concurrent db.close() call sees #closed === true and returns immediately — before the first caller has finished awaiting the native close. The old AsyncMutex approach serialized these; the new gate does not. If close() is called at most once this is harmless, but the interface should be documented or the gate should track and re-join the in-flight close promise (e.g. store and return closePromise on the second call instead of returning early).

The declared-but-never-assigned closePromise variable in wrapJsNativeDatabase looks like the intended fix was started but not completed.

---

closePromise declared but never used (native-database.ts, wrapJsNativeDatabase)

let closePromise: Promise<void> | undefined; is dead code. Either wire it up to make close idempotent (see above), or remove it.

---

Design / Architecture

Connection manager not yet integrated

NativeConnectionManager is fully implemented with correct mode transitions, idle pruning, and metrics, but SqliteDb.execute() / execute_write() still route through the existing NativeDatabaseHandle single-connection path (spec step 7 is pending). Worth calling out explicitly so reviewers know read parallelism is not yet active.

PRAGMA query_only = ON not applied to reader connections

The spec's Read-Only Enforcement section calls for PRAGMA query_only = ON on reader connections as a safety belt alongside the authorizer. The current code opens with SQLITE_OPEN_READONLY and installs install_reader_authorizer, but the pragma isn't applied. A pragma set on open would catch write attempts even if the authorizer is ever bypassed.

__rivetWriteMode duck-typing escape hatch (mod.ts)

RawAccessWithWriteMode brands the client with __rivetWriteMode, accessed by dbWriteMode via an as cast. This bypasses the public SqliteDatabase interface. The writeMode method already exists on SqliteDatabase — consider surfacing it on RawAccess directly, or narrowing the type instead of casting through an undocumented internal property.

---

Code Quality

Redundant guard in reader_authorizer_action (query.rs)

if kind.is_data_write()
    || kind.is_schema_write()
    || kind.is_temp_schema_write()
    || (kind.is_data_write() && database_name.as_deref() == Some("temp"))
    //                          ^^^ always subsumed by the first arm

The last condition is always covered by the first kind.is_data_write() check. The match after the guard correctly re-enumerates INSERT/UPDATE/DELETE as explicit arms per CLAUDE.md's exhaustive-match rule, making those arms dead code by design. A short comment noting this is intentional for exhaustiveness would help future readers.

acquire_read has no starvation bound

If a writer continuously increments pending_writers, readers loop forever. Fine for v1 given single-actor concurrency, but worth a comment on the invariant.

Lease Drop impls don't clean up counters

The NativeReadConnectionLease and NativeWriteConnectionLease Drop impls correctly emit tracing::warn! (good fail-by-default hygiene). However, they don't decrement active_readers/active_writer, so a dropped-without-release lease leaves the manager stuck in a mode it cannot exit. Consider a best-effort try_lock + decrement in Drop.

---

Tests

• statement_classification.rs covers SELECT, read/write PRAGMA, INSERT RETURNING, etc. Consider adding a case for "SELECT 1; SELECT 2" to verify has_trailing_sql = true.
• native-database.test.ts covers concurrent reads and write-mode depth, but not the idempotent-close race described above. A test for concurrent close() calls would catch that gap.
• sqlite_read_pool_metrics_render in tests/metrics.rs — good smoke test covering all new metric names.

---

Minor

• scripts/ralph/ artifacts (prd.json, progress.txt, archive entries): look like agent workflow tracking files committed by mistake; they add noise to the diff.
• rivetkit-typescript/packages/rivetkit/src/db/drizzle.ts: the method === "run" branch routes through execute and discards the result with { rows: [] }. Correct for the Drizzle callback contract, but a one-line comment would confirm the discard is intentional.

[claude]

PR Review: feat(sqlite): add cold read benchmarks and simplify optimizations

Note: This PR targets 04-28-feat_sqlite_benchmark_cold_reads (not main). The Rust/TS source files all show 0 net diff here. The actual new content is the benchmark harness, specs, benchmark result artifacts, and CLAUDE.md/doc changes.

---

Blocking Issues

1. Five load-bearing VFS invariants removed from CLAUDE.md without being re-homed

The following bullet points were removed from CLAUDE.md but do not appear in the updated docs-internal/engine/sqlite-vfs.md:

• SQLite VFS direct tests should record workflow compaction wakes through CompactionSignaler, not call legacy compact_default_batch.
• SQLite VFS correctness tests should use DirectStorage; do not reintroduce mock or envoy transport variants for those tests.
• SQLite VFS xSync durability depends on depot's sqlite_commit reply waiting for the FDB transaction commit.
• SQLite VFS process-global registrations must be owned by a Drop guard so panics unwind through sqlite3_vfs_unregister.
• NativeDatabase::Drop must bound dirty-page flushes with a short timeout and return after logging if the commit future never resolves.

The xSync/depot durability rule and the Drop flush-bound rule are correctness constraints encoding easy-to-miss invariants. Please re-home them in docs-internal/engine/sqlite-vfs.md and docs-internal/engine/napi-bridge.md respectively.

The invariant comment rule was also removed. This removal is premature. The connection manager pending_writers increment-before-gate-check and write-mode admit ordering are exactly the kind of subtle ordering this rule was designed to protect.

The reference to docs-internal/engine/depot.md was also removed without explanation; the depot interaction is still active in the VFS.

2. Read/write lease Drop does not decrement counters (base branch, connection_manager.rs)

When a NativeReadConnectionLease or NativeWriteConnectionLease is dropped, Drop emits a tracing::warn! but does not decrement active_readers or active_writers. After an unreturned lease the manager remains stuck: active_readers is permanently incremented, blocking write mode admission and preventing the manager from closing.

3. closePromise is declared but never assigned in the TS NAPI bridge (base branch)

In wrapJsNativeDatabase, the intent is to serialize concurrent close() calls via closePromise. However closePromise is initialized to undefined and the ??= assignment is missing. A second concurrent db.close() sees #closed === true and returns immediately without waiting.

---

Important Issues

4. Spec step 12 (default off) vs. merge plan branch 7 (default on) conflict

sqlite-read-connection-pool.md step 12 says Default off until stress tests pass. The sqlite-optimization-merge-plan.md Branch 7 says the read pool feature flag defaults on. Resolve this before implementation starts.

5. hasMultipleStatements SQL string inspection in mod.ts (base branch)

A semicolon inside a SQL comment or string literal will false-positive. Flag this as dead code to be removed in the appropriate transplant branch.

6. bigint to Number() precision loss in toNativeBinding (base branch)

Values above 2^53 - 1 are silently truncated. Use BigInt NAPI serialization or throw explicitly on out-of-range values.

---

Benchmark Harness Feedback (sqlite-realworld-bench.ts)

Positives:

• Exhaustive match on SizeClass in targetBytesFor with no _ => fallthrough.
• parseProfile/parseMatrix throw early on invalid values.
• retryTransient classifies errors by pattern.
• findOpenPort correctly closes before returning the port (avoids TOCTOU).

Minor issues:

• metricValue returns 0 for unknown metric names silently.
• Benchmark result JSON under .agent/benchmarks/ adds ~24K lines of diff noise. Add *.json to .gitattributes as linguist-generated=true.

---

Summary

Priority	Issue
Blocking	5 VFS invariants removed from CLAUDE.md not re-homed in docs
Blocking	Lease Drop does not decrement counters, manager gets stuck
Blocking	closePromise never assigned, concurrent close not serialized
Important	default off (spec) vs default on (merge plan) misalignment
Important	hasMultipleStatements string inspection (remove per plan)
Important	bigint to Number() precision loss for i64 > 2^53
Nice to have	metricValue silent zero on unknown metric names
Nice to have	Benchmark JSON diff noise, add linguist-generated attribute