Skip to content

fix(storage): drain only the flushed WAL image, atomically, after a checkpoint (#311) - #313

Merged
mrdevrobot merged 1 commit into
EntglDb:mainfrom
CaffeinatedCoder:fix/311-checkpoint-drain-race
Oct 18, 2026
Merged

mrdevrobot merged 1 commit into
EntglDb:mainfrom
CaffeinatedCoder:fix/311-checkpoint-drain-race

Conversation

@CaffeinatedCoder

Copy link
Copy Markdown
Collaborator

Fixes #311.

Problem

After a checkpoint flushes the WAL index to the page files, it drains each flushed entry like this (StorageEngine.Recovery.cs:148-150):

if (_walIndex.TryGetValue(kvp.Key, out var current) && ReferenceEquals(current, kvp.Value))
    _walIndex.TryRemove(kvp.Key, out _);

Commits do not wait for this loop. Suppose a commit publishes a newer image of the page between the check and the remove. Then the newer image is removed and the older flushed one stays on disk. The consequences:

  • A committed index entry can disappear.
  • An insert can read the stale data page back and reuse the slot of a committed document, so that document's id returns the next document.

The second case is the HighContention_ManySmallTransactions_ConsistentFinalState failure in CI run 38749678260. That run failed at line 244, the "d" assertion: same writer, same transaction, another document index, and the count was still right. #311 has the full mechanism and the reproductions.

Change

  • StorageEngine.Recovery.cs: the drain removes an entry only if it still holds the flushed image, in one atomic step. It uses ((ICollection<KeyValuePair<uint, byte[]>>)_walIndex).Remove(kvp), which compares the byte[] by reference under the bucket lock and works on both TFMs.
  • ConcurrentConsistencyTests.cs: the three field assertions of the high-contention test keep their conditions. On failure they now name the id, the expected fields, and every field of the document that came back.

Evidence

The fix closes the gap between two adjacent dictionary calls, so no deterministic test can recreate the old interleaving without a hook in product code. I measured it instead, in a scratch copy:

Setup as on main with this change
Thread.Sleep(1) injected at the drain (between check and remove on main; before the atomic remove here), 8 collections × 300 inserts, CheckpointAsync() loop 7 / 8 runs lost data 0 / 8 (and 0 / 6)
Storage level, no code change: 8 pages committed in a loop, read back after each commit, checkpoint loop, 32 CPU-burning threads, 3 s 2 / 16 runs failed (committed image 9061, read back image 9060) not measured
Same, idle machine 0 / 20 not measured

I did not add a stress test as a regression gate. Without oversubscription it practically never fails on the old code. With oversubscription inside the test process it would slow down the other test classes and cause the lock timeouts of #302 / #303.

Tests

  • dotnet test tests/BLite.Tests -c Release: 3092 passed, 4 skipped, 0 failed.
  • BLite.Core builds for net10.0 and netstandard2.1. BLite.NetStandard21.Tests builds, but I could not run it here because it needs the .NET 6 runtime.

Not in this PR:

🤖 Generated with Claude Code

…heckpoint (EntglDb#311)

The checkpoint drained each flushed entry with TryGetValue + ReferenceEquals
followed by a separate TryRemove(key). Commits do not wait for the drain
(single-process mode takes no lock; multi-process mode has released
_commitLock by then), so an image a commit published between the check and
the remove was removed instead of the flushed one. The page file kept the
older image and nothing held the newer one: a committed index entry
vanished, or an insert read the stale data page back and reused the slot of
a committed document, so that document's id returned the next one. This is
the HighContention_ManySmallTransactions_ConsistentFinalState failure in CI
run 38749678260 (line 244, the "d" assertion).

The drain now removes the entry only if it still holds the flushed image,
in one step: ICollection<KeyValuePair>.Remove compares the byte[] value by
reference under the bucket lock.

The test's three field assertions keep their conditions and now name the
document that came back, so a future failure says what was wrong.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mrdevrobot

Copy link
Copy Markdown
Contributor

Verified on 0450b97: a storage-level stress repro (commit loop plus checkpoint loop under CPU load) read back an older image than the one just committed on main, and did not on this branch. The atomic conditional remove closes the window. Full suite passes. The PR's own test does not reproduce the race. The same window on _walOffsets (multi-process) remains.

This branch was successfully deployed

1 active deployment
Production — edd4647e Deployed Oct 17, 2026 by CaffeinatedCoder via build_and_publish #765
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.

A checkpoint drops a page image committed while it drains the WAL index: the commit is lost and an id can return another document

2 participants