fix(storage): don't reuse freed pages that still have a WAL image (#168)

[CaffeinatedCoder]

Fixes #168.

Problem

ReadPage prefers a page's committed image in the WAL over the page file, and recovery replays that image over the page file. FreePage put a page straight back on the free list. When it was reused and initialized with WritePageImmediate (a new B-Tree root, for example), reads kept returning the old page until the next checkpoint. Dropping and recreating an index made forward range scans follow the old tree's leaf chain and return entries several times (263, 264, 248, 242 entries for 200 documents in the #168 repro).

Fix

StorageEngine.FreePage now checks whether the page has a WAL image: it is in the WAL index, was checkpointed since the last WAL truncation (the record is still in the WAL file and would be replayed), or is written by an open transaction.

• No WAL image: freed immediately, as before. Time-series pruning and most frees keep their current behaviour.
• WAL image: queued, and returned to its file's free list right after the next WAL truncation (checkpoint, backup or recovery). By then no image exists for reads or replay to bring back.
• Multi-process mode: always queued, since another process may have written the page.

Supporting changes:

• GetCollectionPageIds skips queued pages. In single-file mode RebuildFreeSpaceIndex decides which pages are free by scanning for PageType.Data, and a queued page still carries its old contents on disk. SchemaEvolutionTests.InsertAfterOrphanCollectionIsDropped_DoesNotRouteToFreedPages fails without this filter.
• DropCollectionFile discards queued frees for that collection's file. The slot keeps its number, so releasing them later would free pages of a recreated file.
• Dispose checkpoints only when frees are queued, so a clean close doesn't leak them. It runs on the thread pool so a captured SynchronizationContext cannot deadlock. A crash still leaks queued pages, the same trade-off RecreateIndex already documents.
• CheckpointAsync returned immediately when the WAL index was empty, so it never truncated. When frees are queued it now runs the truncation step. That step moved unchanged into TryTruncateWalAsync.
• FreeCollectionPagesCore no longer removes freed pages from the WAL index. This was the partial in-memory fix for the same problem; it had no effect on replay.

Tests

FreePageReuseTests adds five tests:

• The A freed page that gets reused reads back its old contents from the WAL: dropping and recreating an index duplicates entries #168 repro: drop and recreate an index four times, then check range scans.
• A freed page is not handed out before the WAL is truncated, and reads back what is written to it afterwards.
• A checkpoint releases a queued page even when the WAL index is empty.
• A page still queued at Dispose is reused after reopening.
• Dropping a collection file discards its queued frees.

The first two fail on main. The other three cover the queue's edge cases.

Full suite on macOS: 2384 passed, 3 failed, 4 skipped. The 3 failures are the known MultiProcessWalSharedMemoryTests .wal-shm lock timeouts, which also fail without this change. BLite.NetStandard21.Tests could not run locally because the .NET 6 runtime isn't installed; BLite.Core builds for netstandard2.1 without warnings.

Not in scope

• FreePage is still not transactional. B-Tree merges and overflow-chain frees release the page even if the transaction rolls back.
• Pages written both through the WAL and with WritePageImmediate outside the free-and-reuse case can still be shadowed by a stale image.

This unblocks #167: rebuilding into a fresh index frees the old tree and allocates the new one in the same session.

🤖 Generated with Claude Code

[mrdevrobot]

Thanks for this, the deferred-free approach is the right one and the tests show it fixes #168 (3 of the 5 new tests fail on main). Full suite is green locally on macOS apart from the 3 known .wal-shm failures. Two things need to change before merging, plus two hardening items.

1. A page freed by a still-open transaction is released before that transaction's image reaches the WAL (StorageEngine.Memory.cs:92-139).
FreePage correctly queues a page that an open transaction has in _walCache, but OnWalTruncated releases everything queued at the next truncation, whether or not that transaction has committed yet. Sequence:

1. txn.WritePage(P); FreePage(P) → queued.
2. CheckpointAsync() → _walIndex empty, HasPendingFreePages → TryTruncateWalAsync → OnWalTruncated → PageFile.FreePage(P).
3. txn.CommitAsync() → the old image of P is now in the WAL and _walIndex.
4. AllocatePage() returns P, WritePageImmediate(P, fresh), ReadPage(P) returns the old image. Same bug as #168. The next checkpoint also writes the old image over the Free header while P sits on the free list, breaking the free-list chain.

This is reachable from an ordinary B-Tree delete: BTreeIndex.Delete writes the leaf through the transaction and then MergeWithSibling calls _storage.FreePage(rightNodeId) (BTreeIndex.cs:1335-1341, :1661). Any checkpoint (size-triggered, explicit, backup) between the merge and the commit hits it. I reproduced it with a small test on this branch.

Fix: in OnWalTruncated (runs under _commitLock, so no promotion can race) keep pages that are still in any _walCache queued, and release them after the truncation that follows their commit. Please add a test for this sequence.

2. Multi-process: truncating with an empty local _walIndex can discard other processes' un-replayed WAL records (StorageEngine.Recovery.cs:41-50, :178-224).
Before, CheckpointAsync returned at once when _walIndex was empty, so a process that never replayed anything never truncated. Now every FreePage queues in multi-process mode, so any process with a queued free and an empty _walIndex, including on Dispose, takes the SHM writer lock and truncates the shared WAL. TryTruncateWalAsync never checks that this process has replayed up to _shm.ReadWalEndOffset(). If process B committed and A disposes before its next BeginTransaction, B's records are gone from the WAL, and if B crashes before its own checkpoint, committed data is lost.
Fix: skip the truncation unless _shm.ReadWalEndOffset() <= _lastKnownWalEndOffset (or require checkpointed offset >= WAL end).

3. (hardening) HasWalImage can observe neither cache nor index during promotion. GroupCommit.cs:196-198, Transactions.cs:223-227, :372-377 do _walCache.TryRemove(txn) and then _walIndex[page] = .... Promote into _walIndex first, then remove from _walCache.

4. (hardening) OnWalTruncated aborts on the first failing PageFile.FreePage and loses the rest of the batch (Memory.cs:137-138); the exception escapes after the WAL is already truncated. Note that after #178 on main, PageFile.FreePage throws InvalidDataException for an id past EOF, so this matters more after a rebase. Catch per page and re-queue.

Merge status: no conflicts with main (5.2.3). #178 is complementary, it does not address #168.

Nits: Recovery.cs:46 uses ConfigureAwait(false) while :127 does not; the early-return path skips the Checkpoint metric event (fine, worth a comment); SnapshotPendingFreePages copies the set on every GetCollectionPageIds.

Posted from the wrong account by mistake; the maintainer review from @mrdevrobot supersedes this.

[CaffeinatedCoder]

Thanks for the careful review and the repro. All four points are addressed in 7ae1bbd. I also merged main (5.2.3) into the branch, so the multi-process tests run with the .wal-shm fix.

1. Open transaction released early. OnWalTruncated (under _commitLock) now keeps a queued page that is still in any _walCache. The page is released at the truncation after its commit; after a rollback it is released at the next one. New tests:

• FreedPage_WrittenByAnOpenTransaction_IsNotReleasedBeforeThatCommitIsCheckpointed: your sequence at the storage level.
• BTreeMergesInAnOpenTransaction_SurviveACheckpointBeforeTheCommit: deletes 350 of 400 keys so leaves merge, checkpoints before the commit, then builds a second tree on the freed pages and checks both.

Both fail on the previous commit.

2. Multi-process truncation. TryTruncateWalAsync returns without truncating when _shm.ReadWalEndOffset() > _lastKnownWalEndOffset, checked after taking the SHM writer lock. New test MultiProcess_Truncation_KeepsRecordsAnotherProcessHasNotReplayed: B commits, A has a queued free and an empty _walIndex, and A's checkpoint must leave the WAL length unchanged. It fails on the previous commit.

3. Promotion order. Group commit and both direct commit paths now write _walIndex first and remove the transaction from _walCache afterwards.

4. Per-page release. Each PageFile.FreePage is wrapped individually; a page whose release throws (e.g. InvalidDataException past EOF after #178) stays queued for the next truncation.

Nits:

• The early-return TryTruncateWalAsync await no longer uses ConfigureAwait(false), matching :127.
• That path now has a comment explaining why no Checkpoint metric is published (nothing is written to the page files).
• SnapshotPendingFreePages only copies when the queue is non-empty. It returns null otherwise, which is the common case, so GetCollectionPageIds pays for the copy only between a free and the next truncation.

Full suite on macOS: 2400 passed, 0 failed, 4 skipped.

🤖 Generated with Claude Code

[mrdevrobot]

Thanks, points 1, 3 and 4 are fixed and I checked that each new test fails with its fix reverted. Full suite passes here too (2400 / 0 / 4 skipped). One gap left in point 2.

_lastKnownWalEndOffset can be ahead of what this process has actually replayed. Group commit sets it to the new WAL end under the SHM lock (GroupCommit.cs ~L222-228), even when another process appended records after this process's last replay. Sequence:

1. B begins a transaction; its bookmark is X.
2. A commits; the SHM WAL end is now Y.
3. B commits; group commit sets B's bookmark to Z, past A's records, which B never replayed.
4. B's _walIndex drains; at the next checkpoint ReadWalEndOffset() == _lastKnownWalEndOffset, so the new check passes and B truncates the WAL holding A's committed record.

I reproduced it with a scratch test: after b.CheckpointAsync() the WAL went from 16,435 bytes to 0, and B read A's page as zeros. A's commit then lives only in A's memory until A checkpoints.

The bookmark overwrite predates this PR, but it defeats the new check. Fix options: in ProcessBatchAsync, after taking the SHM lock, replay [lastKnown, shmEnd) before appending; or only advance the bookmark when the SHM end at lock time equals lastKnown. Please add a test for begin B, commit A, commit B, checkpoint B.

Not blocking, for awareness: PrepareTransactionAsync writes WAL records without _commitLock, so a truncation can drop records of a prepared 2PC transaction. Worth an issue. Nit: the empty-index path at Recovery.cs ~L47 dropped ConfigureAwait(false). Library code should keep it; I'd add it to the other call site rather than remove it here.

[CaffeinatedCoder]

Thanks. Fixed in b721fee.

Bookmark gap (point 2): I went with replaying before appending. Both replay paths now share one helper, TryCatchUpWithSharedWal, which runs under _commitLock.

• ProcessBatchAsync, after taking the SHM writer lock, replays [_lastKnownWalEndOffset, SHM end) into _walIndex before writing. It advances the bookmark past its own batch only if that replay succeeded. Otherwise the bookmark stays put, and the next catch-up replays this batch too.
• The scenario also hid a second bug. In multi-process mode, group commit appended at the engine's own FileStream position, which is stale once another process has appended, so in your sequence B overwrote A's records. That is why the WAL was 16,435 bytes, exactly one transaction. Group commit now moves to the end of the file first (WriteAheadLog.SeekToEnd, under the SHM writer lock).
• BeginTransaction replays through the same helper. It used to skip any page present in _walOffsets, so if B committed P and A later committed P, B kept its older P and still advanced the bookmark. A checkpoint on B then wrote the stale P and truncated A's record: the same gap through a different path. Under _commitLock no local commit can interleave, and the replayed range runs to the end of the WAL, so the newest image always wins.
  • Cost: in multi-process mode, BeginTransaction takes _commitLock (waiting at most ReadTimeoutMs) when there are records to replay. On a timeout it starts without replaying, which only means stale reads; the bookmark doesn't move.
  • This also removes the old window between claiming the range by CAS and replaying it, where a truncation could run.
• Other setters of _lastKnownWalEndOffset: the truncation path resets it under both locks. The direct paths (CommitTransactionAsync(ulong), MarkTransactionCommittedAsync) never set it, but they also skip the SHM lock and don't publish the WAL end. They're in the issue below together with PrepareTransactionAsync, rather than widening this PR.

Tests, in FreePageReuseTests:

• MultiProcess_CommitAfterAnotherProcessCommitted_CheckpointKeepsThatCommit: your sequence (begin B, commit A, commit B, checkpoint B), then reads A's and B's pages from B and from a third engine opened afterwards. It fails without the replay before appending.
• MultiProcess_PageCommittedByBothProcesses_CheckpointKeepsTheNewerImage: B commits P, A commits P, B begins and checkpoints, and both B and a fresh engine must read A's image. It fails without the move to the end of the WAL, and again with the old skip filter put back.

ConfigureAwait: restored on the empty-index call site and added on the other TryTruncateWalAsync call site.

2PC: filed as #188.

Full suite: 2402 passed, 0 failed, 4 skipped.

[mrdevrobot]

Thanks, and good catch on the stale append position. I verified b721fee:

• The begin B / commit A / commit B / checkpoint B scenario now keeps A's commit (WAL 16,435 → 32,870 → 0, both pages read back correctly). The same probe fails on 7ae1bbd and on main.
• Each new test fails with its fix reverted: the catch-up before appending, SeekToEnd, and the old _walOffsets skip in the replay helper.
• Lock order is _commitLock then the SHM writer lock everywhere, BeginTransaction never takes the SHM lock, and nothing in Core calls it while holding _commitLock. Single-process mode is unchanged, with no new lock on the hot path.
• Full suite 2402 / 0 / 4 skipped; the MultiProcess tests passed 5 runs in a row with no hangs; a 2-engine stress probe with interleaved checkpoints kept every commit (7ae1bbd lost one). netstandard2.1 builds clean, no conflicts with main.

Approving. One line in the docs would help: in multi-process mode BeginTransaction can now wait up to ReadTimeoutMs when another process has committed and a local group commit holds _commitLock.

While testing I found four multi-process defects. None comes from this PR; all exist on main too. Could you file them, alongside #188?

1. Data loss, please file it as such. After another process truncates the WAL, entries this engine replayed earlier stay in _walIndex. Sequence: A commits P = 0x11; B begins and replays it; A commits P = 0x22, checkpoints and truncates; B begins again, sees shmEnd < lastKnown and resets the bookmark but keeps the stale entry; B checkpoints and writes 0x11 over 0x22 on disk. A fresh engine then reads 0x11. TryCatchUpWithSharedWal (Transactions.cs:79-80) should drop the replayed entries when it detects a truncation, or better, compare a WAL generation counter kept in the SHM.
2. A replay that stops early still advances the bookmark. ReadCommittedPagesSince (WriteAheadLog.cs:786-831) breaks without error on a torn record, and the helper still returns true and sets the bookmark to shmEnd. With a torn record left by a crashed writer, a later commit's record is skipped and then truncated by the next checkpoint. Reproduced with a hand-truncated record. Fix: have the parser report whether it reached toOffset and treat a short read as failure.
3. Plausible: the catch-up in BeginTransaction reads the WAL without the SHM writer lock, so a concurrent truncate-and-append can make it parse new bytes at old offsets and skip records. The generation counter from 1 would cover this too.
4. Plausible: opening a database with a non-empty WAL runs RecoverAsync, which truncates the shared WAL without the SHM lock and without resetting the SHM end. Since AdvanceWalEndOffset is monotonic, other processes then stop seeing new records.

[CaffeinatedCoder]

Thanks for the thorough verification.

Filed the four multi-process defects:

• Data loss in multi-process mode: pages replayed before another process truncated the WAL are written back over newer data #197: data loss. Replayed pages survive another process's truncation and are written back over newer data.
• Multi-process WAL catch-up: a replay that stops at a torn record still advances the bookmark past later commits #198: a replay that stops at a torn record still advances the bookmark.
• Multi-process WAL catch-up in BeginTransaction reads the WAL without the SHM writer lock #199: the catch-up in BeginTransaction reads the WAL without the SHM writer lock (plausible).
• Multi-process: RecoverAsync on open truncates the shared WAL without the SHM lock and leaves the SHM end offset stale #200: RecoverAsync on open truncates the shared WAL without the SHM lock and leaves the SHM end stale (plausible).

#197 and #199 both point at the SHM generation counter you suggested.

On the docs line about BeginTransaction waiting up to ReadTimeoutMs when a local group commit holds _commitLock: pushing it here would dismiss your approval again, as happened to #170–#175. I'd rather add it with the fix for #197/#198, which touches the same catch-up code. If you prefer it in this PR before merging, say so and I'll push it here.