fix(vacuum): rebuild typed-collection indexes into fresh trees (#167)

[CaffeinatedCoder]

Fixes #167.

Stacked on #169 (fix for #168). Until that merges, this PR also shows its commit. Only the last commit, fix(vacuum): rebuild typed-collection indexes into fresh trees, belongs here. Without #169, the second rebuild in a session reuses the freed pages of the first and duplicates entries again.

Problem

VacuumAsync(RebuildIndexes = true) on a typed collection re-inserted every document into the existing index without clearing it. Each rebuild duplicated every B-Tree entry and kept the stale spatial and vector entries from #164 next to the current ones. DynamicCollection.VacuumAsync already builds into fresh indexes.

Fix

• CollectionIndexManager.RecreateIndex is split into two steps. CreateUnregisteredCopy creates an empty index with the same definition that queries don't see yet. ReplaceIndex swaps it in, persists its root page, then frees the old tree. RecreateIndex, used by the decimal key-format migration, now calls both and behaves as before.
• VacuumAsync rebuilds each index with RebuildIndexFromScratchAsync: fill the copy, commit, and only then swap it in. Queries keep using the old index until the swap, and the collection write lock blocks writers for the whole rebuild.
• Unreadable documents fail the rebuild. If a document can't be loaded or indexed, vacuum throws, the copy's pages are freed, and the existing index stays untouched. Before, skipUnreadableDocuments silently dropped those documents from the rebuilt index. Index creation still skips unreadable documents as before.

ICollectionIndex handles obtained before the vacuum point at the replaced instance afterwards. DropIndexAsync and the decimal migration already behave this way.

Tests

New in VacuumTests, all failing on main:

• A B-Tree rebuild doesn't duplicate entries (the VacuumAsync(RebuildIndexes = true) duplicates every secondary-index entry on typed collections #167 repro).
• Five rebuilds back to back don't duplicate entries; once the WAL is checkpointed, further rebuilds don't grow the file.
• A rebuilt index is still correct after reopening the database.
• A rebuilt spatial index no longer finds a moved document at its old position.
• A rebuilt vector index uses the updated embedding.

The last two make a rebuild a workaround for #164 until that is fixed.

Full suite on macOS: 2389 passed, 3 failed, 4 skipped. The 3 failures are the known MultiProcessWalSharedMemoryTests .wal-shm lock timeouts.

Not in scope

• Spatial (R-Tree) indexes still have no CollectAllPages, so their old pages leak on each rebuild, as on DropIndex today.

🤖 Generated with Claude Code

[mrdevrobot]

Reviewed only 56e1651. The fix for #167 is correct and the 5 new tests cover it (23/23 in VacuumTests, full suite green apart from the known .wal-shm failures). Merges cleanly with main (5.2.3). One thing to fix, one to decide.

1. The failure path leaks every page of the half-built tree except the root (DocumentCollection.cs:1603-1617).
RebuildIndexFromScratchAsync calls RebuildIndexAsync, which rolls the transaction back on exception (:1641), and only then calls fresh.FreeAllPages(_storage). BTreeIndex.CollectAllPages (BTreeIndex.cs:1970-1996) walks the committed view, which after the rollback is just the empty root written by WritePageImmediate in the constructor. Split pages allocated via _storage.AllocateIndexPage() (BTreeIndex.cs:921, :984, non-transactional) are never freed. Same for VectorSearchIndex.CollectAllPages. So "the copy's pages are freed" holds only for the root. Collect the page ids before the rollback with the rebuild transaction id (e.g. a transactionId parameter on CollectAllPages/FreeAllPages, called in the catch before RollbackAsync), and add a test for the failure path; none of the new tests exercise it.

2. Spatial rebuild now leaks a full R-Tree per vacuum. You mention it under "not in scope", but before this PR the tree was reused, so for spatial users this is a new unbounded leak with no reclaim path. Either skip spatial indexes in the rebuild until RTreeIndex.CollectAllPages exists, or document it on VacuumOptions.RebuildIndexes and in the CHANGELOG. I'd prefer the former.

Worth noting for the description, not blocking:

• Readers never take _collectionLock; a reader mid-Range on the old index keeps walking pages that ReplaceIndex just freed (pages without a WAL image are freed immediately). Same exposure as DropIndex/RecreateIndex today, but vacuum newly exposes it. User-held ICollectionIndex handles become silently dead after a vacuum. Please document that.
• Crash between the rebuild commit and SaveMetadata leaks the new tree (the other window is documented in the comment).
• DynamicCollection.VacuumAsync does not free old trees either and skips unreadable documents silently; a follow-up to align both would be good.

Nits: if fresh.FreeAllPages throws in the catch it masks the original exception; ReplaceIndex/CreateUnregisteredCopy don't check _disposed like CreateIndex; the two ForceCheckpoint() → CheckpointAsync() test edits aren't mentioned in the commit message; the message says the stale spatial entries from #164 are fixed but the old R-Tree leaks.

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

[CaffeinatedCoder]

Thanks. Both points are addressed in 50ef143. The branch also picks up the updated #169 (review fixes and main 5.2.3).

1. Failure-path leak. RebuildIndexFromScratchAsync now runs its own transaction. On failure it collects the copy's pages through that transaction before RollbackAsync, then frees them; the free is wrapped so it never masks the original exception. For this, BTreeIndex and VectorSearchIndex gain an internal CollectAllPages(ulong transactionId) overload; the public parameterless one is unchanged. New test Vacuum_RebuildIndexes_DocumentCollection_FailedRebuild_KeepsIndexAndFreesCopy:

• It corrupts the last document of a 3,000-entry collection so the rebuild fails late.
• It checks that the old index still answers queries.
• It counts the pages that come back to the free list. On the previous commit that count is 1, matching your analysis; now it is the whole copy.

2. Spatial. The rebuild now skips spatial indexes until RTreeIndex.CollectAllPages exists. The old spatial test now checks that the R-Tree root is unchanged after a vacuum and still answers queries. I dropped the claim about stale spatial entries; with #172 those are removed at update/delete time anyway.

Documentation. VacuumOptions.RebuildIndexes now states three things:

• earlier ICollectionIndex handles refer to the replaced tree and must be fetched again;
• a query running during the swap may read pages being freed (same exposure as DropIndex / RecreateIndex);
• spatial indexes are not rebuilt.

The crash window between the rebuild commit and SaveMetadata (it leaks the new tree) is noted here. DynamicCollection.VacuumAsync freeing old trees and reporting unreadable documents is a follow-up.

Nits:

• The exception-masking nit is covered under point 1.
• CreateUnregisteredCopy and ReplaceIndex now check _disposed.
• The commit message mentions the ForceCheckpoint() → CheckpointAsync() test change.

On the helper overlap with #175: agreed. Whichever of the two lands second, I'll express CreateUnregisteredCopy through CreateUnregistered(GetIndex(name).Definition) and share Register's checks with ReplaceIndex.

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

🤖 Generated with Claude Code

[mrdevrobot]

Both points are fixed. I checked the failure path against #169's deferred-free queue: after the rollback the copy's pages have no WAL image, so they go straight back to the free list, and nothing shared with the old tree is walked. The leak test fails with the fix reverted, and it isn't flaky: over 8 runs it got 66 to 77 reused pages against a threshold of 20. 28/28 in Vacuum, full suite 2406 / 0 / 4 skipped.

Approving. It still has to land after #169.

Two small things in RebuildIndexFromScratchAsync, fine here or in a follow-up:

• await transaction.RollbackAsync() in the catch isn't wrapped. If writing the abort record throws, that exception replaces the original one and the free and fresh.Dispose() are skipped.
• _indexManager.ReplaceIndex(name, fresh) runs after the commit and outside the try. If it throws, the committed copy's pages leak.

Please also open an issue for RTreeIndex.CollectAllPages so spatial rebuild can come back.

[CaffeinatedCoder]

Thanks. Both done in 6c4df5d.

• Rollback in the catch: RollbackAsync is now wrapped, so a failing abort record no longer replaces the rebuild's exception, and freeing and disposing the copy always run. Test: Vacuum_RebuildIndexes_DocumentCollection_FailedRollback_KeepsOriginalErrorAndFreesCopy (a WAL whose abort-record write throws).
• ReplaceIndex after the commit: if it throws and the copy is not the registered index, the copy's committed pages are freed and the copy is disposed. ReplaceIndex also puts the old index back when SaveMetadata fails. Before, the copy stayed registered in memory while the metadata on disk still pointed at the old tree. If only freeing the old tree fails (after the new root is persisted), the copy is in use and isn't touched. Test: Vacuum_RebuildIndexes_DocumentCollection_FailedSwap_KeepsIndexAndFreesCopy. It holds the metadata lock so the save times out, then checks the root in memory and on disk, queries, freed pages and a reopen.

Both tests fail with the fix reverted. VacuumTests passed 26/26 on four runs.

I also merged #169's latest commit (b721fee) into this branch (77052e8). Full suite on the merge: 2410 passed, 0 failed, 4 skipped.

RTreeIndex.CollectAllPages: filed as #189.