fix(sqlite): serialize putBulk across shared-connection SqliteTabularStorage instances#655
Open
sroussey wants to merge 1 commit into
Open
fix(sqlite): serialize putBulk across shared-connection SqliteTabularStorage instances#655sroussey wants to merge 1 commit into
sroussey wants to merge 1 commit into
Conversation
…Storage instances
_putBulkInternal guarded on better-sqlite3's driver-level db.inTransaction,
which is scoped to the CONNECTION, not to the storage instance. When two
SqliteTabularStorage instances shared one Sqlite.Database (repos on the
same file), a second putBulk starting during another's inter-chunk await
gap read inTransaction === true, skipped its own BEGIN/COMMIT, executed
its rows inside the OTHER instance's transaction, emitted put events,
and returned success. If the first instance later rolled back on a unique-
constraint violation, the second's rows silently disappeared even though
its caller had received the returned entities and subscribers had seen
put events for phantom rows.
Add a module-scope WeakMap<Sqlite.Database, Promise<void>> and a protected
connectionMutex() helper (GC-safe, keyed by the DB handle) and:
- _putBulkInternal now keys off this.inTransaction (per-instance), and
when not already in a transaction wraps the whole BEGIN/runBulkPut/
COMMIT|ROLLBACK bracket in connectionMutex(...).
- withTransaction wraps its BEGIN/fn/COMMIT|ROLLBACK bracket in the
same connectionMutex, inside the existing per-instance mutex —
instance-first / connection-second so two instances on one handle
cannot deadlock.
- createTxView reports inTransaction === true through the proxy so the
tx-routed putBulk path takes the nested branch and skips both BEGIN
and the connection lock (the outer withTransaction already holds
both).
- SqliteAiVectorStorage's overridden _putBulkInternal now also keys
off this.inTransaction and takes the connectionMutex on the owned-
transaction path, mirroring the parent.
Regression test in packages/test/src/test/storage-tabular:
- Constructs two SqliteTabularStorage instances on DIFFERENT tables
sharing one Sqlite.Database.
- Races Promise.allSettled([repoA.putBulk(large), repoB.putBulk(small)])
with runBulkPut spied to yield microtasks and then throw on A.
- Asserts A rejects, B fulfills, and repoB's rows survive.
- Second test asserts B's put event stream never includes rows that
were rolled back with A.
Co-Authored-By: Claude <noreply@anthropic.com>
Coverage Report
File CoverageNo changed files found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HIGH-priority correctness fix from a scheduled review of the last 24h of
main(surfaced by review of theputBulkengine refactor inchore: release 0.3.27).The bug
SqliteTabularStorage._putBulkInternalguarded on better-sqlite3's driver-leveldb.inTransaction, which is scoped to the connection, not to the storage instance. When twoSqliteTabularStorageinstances share oneSqlite.Database(the normal case for repos on the same file), a secondputBulkstarting during another's inter-chunkawaitgap readinTransaction === true, skipped its ownBEGIN/COMMIT, executed rows inside the OTHER instance's transaction, emittedputevents, and returned success. If the first instance later rolled back on a unique-constraint violation, the second's rows silently disappeared even though its caller had received the returned entities and subscribers had already seenputevents for phantom rows.The fix
WeakMap<Sqlite.Database, Promise<void>>plus a protectedconnectionMutex()helper — GC-safe, keyed by the DB handle._putBulkInternalkeys offthis.inTransaction(per-instance) and wraps itsBEGIN/runBulkPut/COMMIT|ROLLBACKbracket inconnectionMutex(...)when not already in a transaction.withTransactionwraps its own bracket the same way, inside the existing per-instance mutex — instance-first / connection-second so two instances on one handle cannot deadlock.createTxViewreportsinTransaction === truethrough the proxy so thetx-routedputBulkpath takes the nested branch and skips bothBEGINand the connection lock (the outerwithTransactionalready holds both).SqliteAiVectorStorage's overridden_putBulkInternalmirrors the parent — keys offthis.inTransactionand takesconnectionMutexon the owned-transaction path.Verification
Regression tests added under
packages/test/src/test/storage-tabular/SqliteTabularStorage.integration.test.ts:SqliteTabularStorageinstances on DIFFERENT tables sharing oneSqlite.Database.Promise.allSettled([repoA.putBulk(large), repoB.putBulk(small)])withrunBulkPutspied to yield microtasks and then throw on A.putevent stream never includes rows that were rolled back with A.Run:
Rollback
Revert the single file (
providers/sqlite/src/storage/SqliteTabularStorage.ts, plus the mirror inSqliteAiVectorStorage.ts, plus the test). No schema/migration/API changes.🤖 Generated with Claude Code
Generated by Claude Code