Skip to content

fix(storage): detect write-write conflicts at commit (#304) - #310

Open
CaffeinatedCoder wants to merge 6 commits into
EntglDb:mainfrom
CaffeinatedCoder:fix/304-write-conflicts
Open

CaffeinatedCoder wants to merge 6 commits into
EntglDb:mainfrom
CaffeinatedCoder:fix/304-write-conflicts

Conversation

@CaffeinatedCoder

@CaffeinatedCoder CaffeinatedCoder commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

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 CommitAsync throws the new BLiteTransactionConflictException and 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:

  • A write made without a transaction (auto-commit) was lost the same way to an explicit transaction that was open on the same page and committed after it.
  • Concurrent auto-commit inserts through different BLiteSessions lost most of their documents: each session builds its own DynamicCollection for 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 on 0450b97, 221 of 300 in total, while every InsertAsync returned an id. BLiteSessionTests.ConcurrentAsyncSessions_AllInsertSuccessfully only 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.

File Change
Storage/StorageEngine.Conflicts.cs (new) Page versions, per-transaction base versions, FindConflict, PublishPageVersions, pruning
Storage/StorageEngine.Pages.cs ReadPage, ReadPageHeader, ReadPageAsync record the version before they copy a committed page; WritePage records it for a page written without a read
Storage/StorageEngine.GroupCommit.cs Validation per commit before the WAL write; a rejected commit writes no record, is not published, frees nothing, and gets the conflict exception while the rest of the batch completes; versions are raised after the images are published
Storage/StorageEngine.Transactions.cs Multi-process catch-up raises the versions of replayed pages; a WAL truncation by another process raises every version; the direct CommitTransactionAsync(ulong) and MarkTransactionCommittedAsync check too; CompleteTransaction forgets the base versions
Transactions/Transaction.cs CommitAsync rolls the transaction back on a conflict (caches, free-space index handlers, state Aborted) and rethrows
Storage/StorageEngine.cs, DynamicCollection.cs, Collections/DocumentCollection.cs GetCollectionWriteLock(name) replaces CreateCollectionWriteLock(): single-file mode keeps the one engine-wide lock (now also for DynamicCollection, which shares the free-space index and allocator with the typed collections, as StorageEngine.cs:105-111 already requires); collection-per-file mode keeps one lock per collection name, shared by every instance. DynamicCollection.Dispose no longer disposes the shared lock
BLiteTransactionConflictException.cs (new) Public, derives from InvalidOperationException so existing catch blocks keep working; carries TransactionId and the first conflicting PageId
README.md "Snapshot Isolation" replaced by what holds: read committed, first committer wins, the conflict exception; same for the two session paragraphs

Orderings that keep the check from missing a conflict (a spurious conflict is possible in rare races and is retryable; a lost write is not):

  • A read captures the page version before it copies the page; a commit publishes the image before it raises the version. A reader never pairs a new version with old data.
  • Pruning the version map and a truncation by another process raise a floor every version is compared against, so versions only grow.

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):

Test Without the fix With the fix
Typed context, two open transactions, same collection fails: first insert lost passes (second commit rejected)
BLiteEngine + DynamicCollection, two open transactions fails passes
Two BLiteSessions with open transactions fails passes
Open transaction vs an auto-commit insert on the same page fails: the auto-commit insert is lost passes
Rejected transaction leaves no trace; retry and a plain insert succeed; count survives reopen fails: no exception passes
Transactions on different collections both commit passes passes
50 sequential transactions never conflict passes passes
4 concurrent auto-commit writers × 25 inserts never conflict passes passes
10 concurrent sessions × 5 rounds of auto-commit inserts lose nothing fails: documents lost passes
300 auto-commit inserts racing 300 explicit transactions, typed and dynamic (2 tests) — passes: no insert throws, nothing lost
Page versions are pruned after 5,096 pages, and a pruned page still conflicts — passes
Two engines with AllowMultiProcessAccess: open transaction vs the other engine's commit — passes
Collection-per-file mode: one lock per collection name, sessions lose nothing, open transactions conflict — passes
PrepareAsync votes no for a transaction whose commit would be rejected — passes
A conflict through StorageEngine.CommitTransactionAsync(Transaction) leaves nothing behind — passes

Proven 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:

Code Passed Failed Failures
0450b97 (baseline) 3,083 9 all TimeoutException ("Timed out acquiring PageFile write lock (AllocatePage)" and similar)
this PR, run 1 3,087 13 11 TimeoutException, the 1 % timeout-rate threshold of ConcurrencyPressureTests, and BLiteSessionTests.ConcurrentAsyncSessions_AllInsertSuccessfully with a conflict — which led to the shared-lock change
this PR, run 2 (final) 3,089 12 11 TimeoutException and the 1 % timeout-rate threshold; no conflict anywhere

After 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, DynamicCollection writes 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.Core also builds for netstandard2.1; BLite.NetStandard21.Tests was not run.

Not covered here

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.

🤖 Generated with Claude Code

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 mrdevrobot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. 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).
  4. 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.

CaffeinatedCoder and others added 5 commits October 19, 2026 06:25
…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>
@CaffeinatedCoder

Copy link
Copy Markdown
Collaborator Author

Thanks for the review and the measurements. All four points are addressed on the branch; I merged main (b260100) first, no force-push. One commit per point:

Point Commit Tests
1. Auto-commit writes threw the conflict bc6bed6 Typed_auto_commit_inserts_racing_explicit_transactions_retry_instead_of_throwing, Dynamic_…
2. Pruning never ran 973358a Page_versions_are_pruned_once_no_open_transaction_needs_them
4. Minor b41031e Prepare_votes_no_…, A_conflict_in_the_engine_commit_overload_leaves_nothing_behind, the two MultiProcessWalTruncationTests below
3. Missing tests 3718934 Multi_process_an_open_transaction_conflicts_with_a_commit_of_the_other_process, Collection_per_file_mode_shares_one_write_lock_per_collection

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 TruncateAsync; the BLiteSession methods go through them) now run in a loop inside the collection lock. When the commit of the transaction they opened is rejected (WriteFailure.ShouldRetry: our own transaction, the conflict exception, fewer than 10 attempts), CommitAsync has already rolled it back and nothing was written, so they run again in a new transaction. A conflict from anything else, like the OnInsert retention that runs after the commit, is not retried. The bulk methods read their input once (ToList()), so a retry runs over the same items. Most of the diff is indentation: in src it is +502/−303, and +218/−19 with whitespace hidden.

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 AutoCommitAttempts = 1.

The XML doc and the README now say which operations retry and which do not: index creation, VacuumAsync, retention and time-series pruning, and DynamicCollection.TruncateAsync commit their own transaction without retrying and can throw it.

2. Pruning. ProcessBatchAsync drops the base versions of each commit it published before TryPrunePageVersions (StorageEngine.GroupCommit.cs). The test commits 5,096 pages in transactions of 500 and asserts that the map holds at most 4,096 entries and no transaction entry is left, then checks that a conflict on a pruned page is still detected. Without the change the map holds 5,096 entries and the test fails.

4. Minor.

  • PrepareAsync: PrepareTransactionAsync (both overloads) returns false and writes nothing when the commit would be rejected. This is checked without the commit lock, so MarkTransactionCommittedAsync still checks again under it.
  • The exception's constructor is internal.
  • CommitTransactionAsync(Transaction) rolls the transaction back on a conflict, as Transaction.CommitAsync does. The test asserts that no base versions or WAL-cache entries are left and the state is Aborted.
  • !caughtUp: when other processes' commits could not be replayed, every commit in the batch that writes pages is rejected with the conflict exception (SharedWalNotReplayed, with its own message); empty commits still go through. Two existing tests committed from B while B's catch-up failed. I changed them so that B commits its own page before A's records exist, and the commit after the corruption is now asserted to be rejected without writing anything. Their checks on the WAL and the stall reports are unchanged. One consequence: while a record in the shared WAL stays unreadable, a process that cannot read it cannot commit writes. Before, it committed without being able to check them. It recovers once another process checkpoints and truncates the WAL.

3. Tests.

  • Multi-process: engines A and B on the same files. A's transaction reads a page, B commits it, A writes it and commits: rejected and Aborted. B still sees its own value, and A's retry goes through.
  • Collection-per-file mode (PageFileConfig.Server): GetCollectionWriteLock returns one instance per collection name (case-insensitive) and a different one for another name. 10 sessions × 5 rounds of auto-commit inserts lose nothing, and two open transactions on one collection never both commit over each other. It fails when the lock is created per call.
  • Pruning and auto-commit vs explicit transaction: see 1 and 2.

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 (dotnet test tests/BLite.Tests, macOS, net10.0) on 3718934 (merge with main b260100), two runs: 3187 passed, 0 failed, 4 skipped each. In an earlier run, 6 tests failed with TimeoutException (AllocatePage / collection lock, the #303 class). One of them, BTreeSplitTests.Dynamic_NonUniqueIndex_EveryDuplicatedKey_FoundForwardAndBackward, also fails alone, single-threaded, 1 in 15 on plain main b260100 and 2 in 15 on this PR before these commits; 0 in 15 with them. TransactionConflictTests ran 10 times in a row: 16/16 each time.

PR #246 conflict note: its resolution now also moves the WouldConflict vote into PrepareCoreAsync. I'll update the patch.

This branch had an error being deployed

1 failed deployment
Production — 3718934d Deployed Oct 19, 2026 by CaffeinatedCoder via build_and_publish #800
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working package/core Affects BLite.Core severity/critical Data loss, corruption, or silently wrong results in common use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two concurrent transactions both commit, and one of them is silently lost: no write-write conflict detection

2 participants