engine: gate self-host SQLite writes so a raw write can't deadlock a transaction#257
Conversation
…transaction (#250) On the file-backed better-sqlite3 adapter, `withTransaction` holds `BEGIN IMMEDIATE`'s write lock across `await`s on a dedicated connection while the main connection runs raw, non-transactional writes. better-sqlite3 is synchronous, so a raw write that finds the file locked busy-waits and freezes the Node event loop for up to `busy_timeout` — during which the lock-holding transaction can never progress to COMMIT. The raw write then times out with SQLITE_BUSY and its losing statement rolls back with the error swallowed by the caller. Under concurrent node registration this silently drops a provider's capabilities from the node aggregate (which node loses flips run to run). Funnel every write on the file-backed path — raw statement writes on the main connection and `withTransaction` bodies alike — through a single async write gate, so no raw write ever executes while a transaction holds the lock. Writes never contend, `busy_timeout` stops being load-bearing, and the transaction always commits. Reads stay ungated (WAL readers never block the single writer). `:memory:` and D1/DO paths are unchanged. Adds a two-node conformance test that registers both nodes' broker providers concurrently over the real attach paths with concurrent event-queue poll writes: it fails on current main with `database is locked` and passes with the gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe SQLite adapter now serializes transactional and raw writes, using explicit transaction handling for memory databases and gated connections for file-backed databases. Tests cover gate behavior and concurrent node registration preserving broker capabilities. ChangesSQLite contention handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NodeRegistration
participant EventQueuePolling
participant SQLiteWriteGate
participant SQLite
NodeRegistration->>SQLiteWriteGate: register node and provider
EventQueuePolling->>SQLiteWriteGate: enqueue and sweep probe events
SQLiteWriteGate->>SQLite: serialize transactional and raw writes
SQLite-->>NodeRegistration: persist complete capabilities
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a deadlock issue in self-hosted environments using file-backed SQLite databases by routing all write operations through a single async write gate, preventing event-loop freezing under concurrent write contention. A new conformance test suite has been added to verify this behavior. Feedback is provided to optimize the gateWriteBuilder proxy by caching the thenability check of the builder outside of the proxy handler to avoid redundant evaluations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e782575c28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/engine/src/adapters/node/database.ts (1)
107-140: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider reusing the transaction connection
configureSqlite(txSqlite)does not change WAL mode here. If this path is hot, a long-lived dedicated transaction connection would avoid the per-call open/close overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/adapters/node/database.ts` around lines 107 - 140, In withFileWriteGate, avoid opening and closing a new Database connection for every withTransaction call; create and configure a reusable dedicated transaction connection when the gate is initialized, use it for each serialized transaction, and close it through an appropriate lifecycle cleanup mechanism while preserving rollback and write-gating behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/adapters/node/database.ts`:
- Around line 142-166: Extend gateRawWrites to wrap the BaseSQLiteDatabase
methods all, get, values, and transaction so their execution is routed through
runGated(), including transaction callbacks and returned lazy builders where
applicable. Preserve target binding for non-gated methods and ensure raw SQL
with RETURNING cannot bypass the write gate.
---
Nitpick comments:
In `@packages/engine/src/adapters/node/database.ts`:
- Around line 107-140: In withFileWriteGate, avoid opening and closing a new
Database connection for every withTransaction call; create and configure a
reusable dedicated transaction connection when the gate is initialized, use it
for each serialized transaction, and close it through an appropriate lifecycle
cleanup mechanism while preserving rollback and write-gating behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa6324bb-e6fa-4996-a8f3-4b016cb430ff
📒 Files selected for processing (3)
packages/engine/CHANGELOG.mdpackages/engine/src/__tests__/conformance/twoNodeWriteContention.test.tspackages/engine/src/adapters/node/database.ts
…ites Address review of the self-host write gate: the invariant "no un-gated write runs while a transaction holds the lock" now holds structurally, not by current-usage accident. - Gate every raw-SQL executor on the handle (`all`/`get`/`values`/`transaction`, not just `run`), so a raw statement — including `RETURNING` writes — cannot bypass the gate. - Gate `values` on thenable statements and wrap `prepare()`d statements so a prepared write routes through the gate too. - Reject a write issued through the engine handle inside a `withTransaction` body (a statement bound to the main connection cannot run inside the isolated transaction connection) with a clear error instead of deadlocking, using an AsyncLocalStorage re-entrancy marker. The builder form of `runAtomicWrites`, which rebuilds on the transaction handle, is unaffected; `:memory:` keeps its shared-connection behavior. - Evaluate builder thenability once per wrap instead of per property access. Adds adapter tests covering the re-entrancy rejection, the builder form, the `:memory:` prebuilt-array path, and prepared/raw writes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The AsyncLocalStorage re-entrancy marker stored a bare `true`, which propagates to every descendant async context — so a detached async resource created inside a transaction body would inherit it permanently and have its writes rejected even after the transaction commits. Store a per-run token and clear it when the task settles, so only writes issued while the enclosing gate task is still active are rejected; a resource that outlives its transaction gates normally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/src/adapters/node/database.ts (1)
146-170: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPer-transaction connection open/close on the write path.
Each
withTransactionopens and closes a freshbetter-sqlite3connection (Lines 151, 165). SincerunGatedalready serializes all writes, at most one transaction connection is live at a time, so a single reusable dedicated writer connection would avoid the per-call open/configure/close cost on the hot write path. Correctness and cleanup here are fine; this is purely an efficiency consideration for high write throughput.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/adapters/node/database.ts` around lines 146 - 170, Reuse a single dedicated writer connection instead of opening and closing a better-sqlite3 Database for every withTransaction call. Create and configure the connection during database initialization, use it in withTransaction while retaining BEGIN IMMEDIATE, commit/rollback, and runGated serialization, and close it from the database cleanup lifecycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/engine/src/adapters/node/database.ts`:
- Around line 146-170: Reuse a single dedicated writer connection instead of
opening and closing a better-sqlite3 Database for every withTransaction call.
Create and configure the connection during database initialization, use it in
withTransaction while retaining BEGIN IMMEDIATE, commit/rollback, and runGated
serialization, and close it from the database cleanup lifecycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a0cd53a-15c7-4751-a6a4-d5900870ea55
📒 Files selected for processing (3)
packages/engine/CHANGELOG.mdpackages/engine/src/adapters/node/__tests__/writeGate.test.tspackages/engine/src/adapters/node/database.ts
✅ Files skipped from review due to trivial changes (1)
- packages/engine/CHANGELOG.md
1 participant
Fixes #250.
What this is
On the self-host, file-backed better-sqlite3 adapter, every write — raw statement writes on the main connection and
withTransactionbodies alike — is serialized through a single async write gate. No raw write executes while a transaction holds the write lock, so writes never contend for the file lock andbusy_timeoutis not load-bearing. Reads stay ungated; WAL readers never block the single writer. The:memory:adapter and the Cloudflare D1/DO paths are unchanged.Why
withTransactionruns an interactive transaction on a dedicated connection: it takesBEGIN IMMEDIATE's write lock, thenawaits the async transaction body. better-sqlite3 is synchronous, so a raw, non-transactional write on the main connection that finds the file locked busy-waits and freezes the Node event loop for up tobusy_timeout. While frozen, the lock-holding transaction can never run toCOMMIT, so the raw write times out withSQLITE_BUSYand its losing statement rolls back — with the error swallowed by the caller.Under two nodes registering their providers at the same instant (with the node event-queue poll issuing raw writes on the main connection), this deadlock drops one node's broker capabilities from the node aggregate, alternating which node loses. It is self-host only: production per-node Durable Object storage has no cross-connection writer contention. The per-node serialization from #251 is correct for the production D1-latency aggregate race and is retained.
Test
twoNodeWriteContentionregisters both nodes' broker providers concurrently over the realattachNodeSocket/handleMessagepaths on file-backed storage, with concurrent event-queue poll raw-write traffic, looped. It fails on currentmain(SqliteError: database is lockedfrom the poll's rawdb.update, each run frozen ~5.4s onbusy_timeout) and passes with the gate (~210ms). The existing single-nodeproviderAttachRacesuite is unaffected. Full engine (424) and types (161) suites pass.🤖 Generated with Claude Code