Repository navigation
fix(storage): detect write-write conflicts at commit (#304) - #310
CaffeinatedCoder wants to merge 6 commits into
Conversation
Two transactions open at the same time that changed the same page both committed, and the second one's page copies replaced the first one's changes. Every page now carries the sequence number of the commit that last published it; a transaction records the version of each page it first read (or first wrote), and group commit rejects, before writing any WAL record, a commit whose pages moved on since or that an earlier commit of the same batch also writes. The first committer wins; the other gets BLiteTransactionConflictException and is rolled back. Every object that writes to a collection now shares one write lock (StorageEngine.GetCollectionWriteLock): before, each DynamicCollection had its own, so concurrent auto-commit inserts through different sessions ran on the same pages and most of them were lost. README no longer claims snapshot isolation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mrdevrobot
left a comment
There was a problem hiding this comment.
Verified on the merge with main 3648a02: build clean, full suite 3180/3180, and the 6 fix tests fail on main. Probes on main lose 100–360 documents per run (concurrent explicit transactions, sessions, two multi-process engines); with this PR none are lost. The approach is accepted. Changes needed before merge:
- BLiteTransactionConflictException.cs:20 and the README say auto-commit writes only make an open transaction fail. Not true: an explicit CommitAsync doesn't take the collection lock, so an auto-commit insert that read the same B-tree leaf gets rejected. 300 auto-commit inserts racing 300 explicit begin/insert/commit: 118 InsertAsync calls threw BLiteTransactionConflictException. Either retry the auto-commit write inside the lock (nothing was written, the lock is held) or document that every write method can throw it.
- TryPrunePageVersions never prunes: it runs under _commitLock before the batch's waiters reach CompleteTransaction, so the batch's own _baseVersions entries keep the map non-empty. After 60k sequential inserts _pageVersions holds 5,142 entries (threshold 4,096) and never shrinks. Remove the batch's entries before pruning, and add a test.
- Add tests for the multi-process path (two engines with AllowMultiProcessAccess: A's open transaction vs B's commit on the same page), for pruning, for auto-commit vs explicit transaction, and for collection-per-file mode (GetCollectionWriteLock).
- Minor: PrepareAsync still returns true for a transaction whose commit will be rejected; the exception constructor can be internal; the public CommitTransactionAsync(Transaction) path leaves _baseVersions and _walCache entries behind on a conflict; with a failed shared-WAL catch-up (!caughtUp) the batch is validated against stale versions, so reject page-writing commits in that case.
…ntglDb#304) A caller-owned transaction commits without taking the collection lock, so its commit can land between the reads and the commit of a write made without a transaction. That write was rejected with BLiteTransactionConflictException, although its caller never opened a transaction: 118 of 300 auto-commit inserts threw while racing 300 explicit transactions. The 17 write methods that open their own transaction (typed and dynamic insert, update, upsert, delete and their bulk variants, typed TruncateAsync) now run in a loop inside the collection lock. When the commit of their own transaction is rejected, CommitAsync has rolled it back and nothing was written, so they run again in a new transaction, up to 10 attempts (WriteFailure.ShouldRetry). The bulk methods read their input once. The exception's documentation and the README say which operations retry and which can still throw it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lDb#304) TryPrunePageVersions runs under the commit lock before the committing transactions' waiters reach CompleteTransaction, so the batch's own base versions kept the map in use and the prune never ran: after 60k sequential inserts the map held 5,142 entries against a threshold of 4,096. Group commit now drops the base versions of each commit it published before pruning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#304) - PrepareTransactionAsync (both overloads) votes no and writes nothing for a transaction whose commit would be rejected. MarkTransaction- CommittedAsync still checks again under the commit lock. - The exception's constructor is internal. - CommitTransactionAsync(Transaction) rolls the transaction back on a conflict, as Transaction.CommitAsync does, so that its uncommitted pages and base versions are not left behind. - Multi-process: when the catch-up with the shared WAL fails, the page versions do not include the other processes' commits, so group commit rejects every commit that writes pages instead of validating it against stale versions. Two MultiProcessWalTruncationTests committed from the engine whose catch-up failed; they now commit its page before the other engine's records exist and assert that the commit after the corruption is rejected and writes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mode (EntglDb#304) - Two engines with AllowMultiProcessAccess on the same files: A's open transaction reads a page, B commits it, A writes it; A's commit is rejected, B's value stays, and A's retry goes through. - PageFileConfig.Server: GetCollectionWriteLock returns one instance per collection name, sessions' auto-commit inserts lose nothing, and two open transactions on one collection never both commit over each other. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review and the measurements. All four points are addressed on the branch; I merged
1. Auto-commit writes retry instead of throwing. I took the retry. The 17 write methods that open their own transaction (typed and dynamic: insert, update, upsert, delete, their bulk variants, typed Your scenario (300 auto-commit inserts and 300 explicit begin/insert/commit, all started at once, default lock timeout), 5 runs: 0 of 300 inserts threw (was 118), 44–242 explicit commits were rejected per run, and the count always matched. The tests use 3 + 3 writers × 100 and fail with The XML doc and the README now say which operations retry and which do not: index creation, 2. Pruning. 4. Minor.
3. Tests.
Each new test fails when its fix is reverted. I checked all seven, including the existing multi-process catch-up publish and the per-name lock. Full suite ( PR #246 conflict note: its resolution now also moves the |
Fixes #304.
Two transactions that are open at the same time and change the same page could both commit, and the first one's changes were silently replaced by the second one's page copies. Now the first committer wins: the second
CommitAsyncthrows the newBLiteTransactionConflictExceptionand the transaction is rolled back, so the caller can retry it. No file-format change.Two more cases of the same defect came up while testing:
BLiteSessions lost most of their documents: each session builds its ownDynamicCollectionfor the collection, each instance had a write lock of its own, so their writes ran concurrently on the same pages. Ten sessions inserting one document each at the same time lost documents in all 30 runs on0450b97, 221 of 300 in total, while everyInsertAsyncreturned an id.BLiteSessionTests.ConcurrentAsyncSessions_AllInsertSuccessfullyonly checks that the ids are distinct, so it passed. With the conflict check alone these inserts would now throw; this PR also gives every object that writes to a collection the same lock, so they serialize as typed collections already do.How
Every page gets a version: the sequence number of the last commit that published it. A transaction remembers the version of each page it first saw, at its first read from the committed state or, for a page it writes without reading (a new page), at its first write. At commit, inside the group-commit batch and before any WAL record is written, a commit whose pages have a newer version, or that an earlier commit of the same batch also writes, is rejected. The rest of the batch commits as before.
Storage/StorageEngine.Conflicts.cs(new)FindConflict,PublishPageVersions, pruningStorage/StorageEngine.Pages.csReadPage,ReadPageHeader,ReadPageAsyncrecord the version before they copy a committed page;WritePagerecords it for a page written without a readStorage/StorageEngine.GroupCommit.csStorage/StorageEngine.Transactions.csCommitTransactionAsync(ulong)andMarkTransactionCommittedAsynccheck too;CompleteTransactionforgets the base versionsTransactions/Transaction.csCommitAsyncrolls the transaction back on a conflict (caches, free-space index handlers, stateAborted) and rethrowsStorage/StorageEngine.cs,DynamicCollection.cs,Collections/DocumentCollection.csGetCollectionWriteLock(name)replacesCreateCollectionWriteLock(): single-file mode keeps the one engine-wide lock (now also forDynamicCollection, which shares the free-space index and allocator with the typed collections, asStorageEngine.cs:105-111already requires); collection-per-file mode keeps one lock per collection name, shared by every instance.DynamicCollection.Disposeno longer disposes the shared lockBLiteTransactionConflictException.cs(new)InvalidOperationExceptionso existingcatchblocks keep working; carriesTransactionIdand the first conflictingPageIdREADME.mdOrderings that keep the check from missing a conflict (a spurious conflict is possible in rare races and is retryable; a lost write is not):
The version map is pruned when it holds more than 4,096 pages and no active transaction has recorded a version.
Why not hold the collection lock until commit
The alternative is to keep the collection write lock from a transaction's first write to its commit or rollback. It is smaller, but it serializes every explicit transaction per collection (per database in single-file mode, where collections share one lock), and turns a long transaction into lock timeouts for everybody else (500 ms by default, see #303). Page versions keep concurrent transactions fully concurrent and fail only the ones that actually collide. Auto-commit writes, which hold the lock until they commit, now share one lock per collection and so never collide with each other.
Tests
tests/BLite.Tests/TransactionConflictTests.cs, 16 tests (the last 7 added in review):BLiteEngine+DynamicCollection, two open transactionsBLiteSessions with open transactionsAllowMultiProcessAccess: open transaction vs the other engine's commitPrepareAsyncvotes no for a transaction whose commit would be rejectedStorageEngine.CommitTransactionAsync(Transaction)leaves nothing behindProven by reverting: with the engine changes removed (tests and exception type kept) the 5 conflict tests fail; with only the shared lock removed, the session test fails with
BLiteTransactionConflictException(the check catches what used to be lost); with neither, it loses documents.Full suite (
dotnet test tests/BLite.Tests, macOS, net10.0), three runs:0450b97(baseline)TimeoutException("Timed out acquiring PageFile write lock (AllocatePage)" and similar)TimeoutException, the 1 % timeout-rate threshold ofConcurrencyPressureTests, andBLiteSessionTests.ConcurrentAsyncSessions_AllInsertSuccessfullywith a conflict — which led to the shared-lock changeTimeoutExceptionand the 1 % timeout-rate threshold; no conflict anywhereAfter the review fixes (3718934, merged with main b260100), two full runs: 3,187 passed, 0 failed, 4 skipped.
Every test that failed in either PR run passes when run on its own (17 tests, 20 cases), and 8 of them also fail on the baseline: this is the 500 ms lock-timeout flakiness of #303. The slightly higher count with this PR is consistent with the shared lock: in single-file mode,
DynamicCollectionwrites now wait for the same lock as typed writes, which they always needed because they share the free-space index and the allocator, but which adds contention in the concurrency tests.BLite.Corealso builds for netstandard2.1;BLite.NetStandard21.Testswas not run.Not covered here
WritePageImmediate(allocation, catalog, dictionary) are not versioned; they are not part of a transaction today (A crash leaks pages freed or allocated outside the WAL (queued frees, index roots on first open) #261, Epic: v6.0.0 — migration guide, changelog, docs, samples and release engineering #296).Conflicts with other open PRs
These are independent of this PR and can merge in either order. Whichever lands second needs the resolution below; I have it as a patch for either order and will push it to that PR.
src/BLite.Core/Storage/StorageEngine.Transactions.cs(CompleteTransaction). Resolution: keep both removals (_baseVersionshere,_preparedTransactionsthere). fix(storage): keep prepared transactions' WAL records until they commit or roll back (#188) #246 moves the prepare's WAL writes under the commit lock (PrepareCoreAsync), soThrowIfConflictinggoes there too, before the Begin record, replacing theWouldConflictvote this PR adds toPrepareTransactionAsync. A conflicting prepare then votes no and writes nothing, andMarkTransactionCommittedAsynckeeps the check for a commit that lands between the prepare and the commit record. Two tests inPreparedTransactionWalTestscover this.src/BLite.Core/Storage/StorageEngine.GroupCommit.csandREADME.md. Resolution: fix(wal): sync the WAL to disk before a commit is acknowledged #280's sync-before-free loop skips commits rejected here, as the free loop does. A batch whose commits were all rejected skips itsFlushAsync(an fsync under fix(wal): sync the WAL to disk before a commit is acknowledged #280), so a rejected commit is neither written nor synced. The README keeps fix(wal): sync the WAL to disk before a commit is acknowledged #280's Durable line and this PR's Isolated line. Test:WalCommitSyncTests.A_Commit_Rejected_As_A_Conflict_Writes_And_Syncs_Nothing.README.md(roadmap list). Resolution: this PR's BLiteSession line and docs(readme): show only APIs that exist in the source #298's migration line (ToMultiFileAsync/ToSingleFileAsync).🤖 Generated with Claude Code