fix(index): fill model-declared indexes from existing documents (#171)

[CaffeinatedCoder]

Fixes #171.

Problem

When a DocumentDbContext opens, it applies each model-declared index with DocumentCollection.ApplyIndexBuilder. For an index that didn't exist yet, that created and persisted it empty. Only documents written afterwards were indexed, so queries routed to the index silently missed every older document. This is the usual way of adding an index: a new app version that adds HasIndex, HasVectorIndex or HasSpatialIndex to an existing collection.

Fix

ApplyIndexBuilder now:

1. Resolves the index name without compiling the key selector, and returns at once if the index exists. Existing indexes stay a dictionary lookup on every open, including the session path.
2. Builds the missing index unregistered (CollectionIndexManager.CreateUnregistered), so neither queries nor the collection metadata see it yet.
3. Fills it from the primary index with RebuildIndexAsync(skipUnreadableDocuments: false), on the thread pool like the decimal key-format migration, so a captured SynchronizationContext cannot deadlock the wait.
4. Only then registers and persists it (CollectionIndexManager.Register).

Building before registering makes the fill crash-safe. If the process dies mid-fill, the metadata has no entry for the index, and the next open builds it again. If the index were persisted first and filled afterwards, a crash would leave an empty index that later opens trust.

If a document cannot be loaded or indexed, the copy's pages are freed and the open fails with an InvalidOperationException naming the index and collection. Nothing is persisted, so the next open retries, and a context without that index can still open the data. The test uses a vector index whose dimension doesn't match the stored embeddings.

The B-Tree, vector and spatial cases share the same definition builder, CollectionIndexManager.BuildDefinition. It produces the same definitions as the previous EnsureIndex / CreateVectorIndexUntyped / CreateSpatialIndexUntyped paths.

Tests

IndexBackfillTests adds 5 tests, all failing on main. They use new contexts in tests/BLite.Shared/IndexEvolutionContexts.cs: v1 without indexes, v2 with them, and one with a mismatched vector dimension.

• An added B-Tree index covers existing documents through LINQ and QueryIndexAsync.
• An added vector index covers existing documents.
• An added spatial index covers existing documents.
• The index is filled once and maintained afterwards: an insert in v2 and a reopen give 11 rows, not 21.
• An index that cannot be filled fails the open and is not persisted. A v1 context still reads the data, and the next v2 open fails again instead of trusting an empty index.

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

Behaviour change

Opening a context whose model declares a new index that cannot be built from the existing documents now throws. Before, it silently created an empty or partial index.

Note on unique: true

#171 also suggested that a unique index added this way would not check existing duplicates. While testing that, I found that secondary indexes don't enforce unique: true at all, even for documents inserted afterwards: secondary B-Tree keys include the document ID, and nothing else checks for duplicates. That is a separate issue, so I left it out of this PR.

Merges cleanly with #169, #170, #172, #173 and #174.

🤖 Generated with Claude Code

[mrdevrobot]

Thanks. The fix follows the decimal-migration precedent and is correct: index name resolution matches the old EnsureIndex/CreateVectorIndexUntyped/CreateSpatialIndexUntyped paths, the thread-pool wait can't deadlock (no _collectionLock or manager lock held, CreateUnregistered takes none), metadata has no entry until Register, read-only opens are no worse than before, and the existing-index early return is actually fewer writes than before. Full suite green on macOS apart from the known .wal-shm failures; merges cleanly with main (5.2.3) and with #170. A few things before merging.

1. Failing the open needs better guidance (DocumentCollection.cs:1086-1094). The most likely real-world hit is HasIndex(..., unique: true) added over data that already has duplicates (BTreeIndex.cs:107), and the app then cannot start at all. The message says "it is built again on the next open", but it will fail on every open until the model or the data changes, and the only repair is removing the HasIndex temporarily. Please reword the message, add a CHANGELOG entry for the behaviour change (open can now throw and can take long on big collections), and consider an opt-in to skip unindexable documents or a public rebuild entry point. Please also add a test for the unique-over-duplicates case.
2. Overlap with #170. #170 adds CreateUnregisteredCopy(name)/ReplaceIndex(name, fresh), this one CreateUnregistered(definition)/Register(index). They merge without conflicts but leave four near-duplicate helpers. Whichever lands second should express CreateUnregisteredCopy as CreateUnregistered(GetIndex(name).Definition) and share Register's validation. BuildDefinition also makes CreateVectorIndexUntyped/CreateSpatialIndexUntyped/EnsureIndexUntyped dead code; please remove them.
3. FreeAllPages is a no-op for R-Tree (CollectionSecondaryIndex.cs:639-648), so the failure path leaks spatial pages; the doc comment in CreateUnregistered promises a release that doesn't happen for spatial. Adjust the comment (pre-existing limitation).

Noted, not for this PR: an existing index with a changed definition (unique flag, vector dimension) is silently kept, before and after; and in multi-process mode two processes opening the same new model both backfill and the second SaveMetadata wins. Both pre-existing, worth separate issues.

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

[CaffeinatedCoder]

Thanks. Addressed in 1c04a62; the branch also merges main (5.2.3).

1. Guidance when the open fails. The message now includes the underlying error, says that nothing was saved, and says that every open fails the same way until the documents can be indexed or the index is removed from OnModelCreating. The existing failure test checks the wording and the inner exception.

On the cases you raised:

• Unique index over duplicates: this does not fail today, because typed unique indexes don't enforce uniqueness at all (Unique secondary indexes on typed collections accept duplicate values #176). Secondary keys are [value][id], so the backfill builds the index without complaint, and so do normal inserts. Once Unique secondary indexes on typed collections accept duplicate values #176 is fixed, a duplicate makes the backfill throw through this same path. I'll add the unique-over-duplicates test on top of that fix rather than a test that pins the current wrong behaviour; tell me if you'd prefer a skipped placeholder now.
• CHANGELOG: it is generated by versionize from commit subjects, so I put the behaviour change in the commit message. A hand-written entry would sit oddly next to the generated sections. Should this go into release notes, an "Upgrading" note in the README, or a footer that versionize picks up?
• Opt-in skip / public rebuild: I'd like to keep those separate. A ModelBuilder option such as OnIndexBuildFailure(Skip | Throw) plus a public RebuildIndexAsync(name) would give apps a way out without editing the model. I can open an issue with that proposal if you agree with the direction.

2. Overlap with #170. Whichever lands second, I'll express CreateUnregisteredCopy through CreateUnregistered(GetIndex(name).Definition) and share Register's validation. I removed EnsureIndexUntyped and the public CreateVectorIndexUntyped / CreateSpatialIndexUntyped; nothing in the repo uses them. Note that the last two were public on CollectionIndexManager.

3. Spatial FreeAllPages. The CreateUnregistered doc comment now says a spatial index cannot release its pages yet.

The two pre-existing problems you noted (a changed definition silently kept, and two processes both backfilling) look worth separate issues; I can file them.

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

🤖 Generated with Claude Code

[mrdevrobot]

Thanks, all three points are addressed. I confirmed the #176 claim: a unique HasIndex over duplicates opens without error on this branch, so the test belongs on top of that fix. The whole solution builds and the full suite passes (2397 / 0 / 4 skipped). Approving, with two small asks.

1. Keep the two public methods, marked obsolete. CollectionIndexManager<TId,T> is public sealed with a public constructor over public types, so CreateVectorIndexUntyped and CreateSpatialIndexUntyped are reachable, even if unlikely. Nothing in the repo uses them, but please keep them with [Obsolete("Use model HasVectorIndex/HasSpatialIndex or CreateIndex(definition); will be removed in 6.0")]. Removing the internal EnsureIndexUntyped is fine.
2. Message punctuation. The inner message has no trailing full stop, so it reads ...Expected 4, got 3 Nothing was saved, .... Use {ex.Message.TrimEnd('.')}. or move the inner message to the end.

On your questions:

• CHANGELOG: no BREAKING CHANGE footer here. Throwing only happens when the documents can't be indexed, which used to produce a silently wrong index, so this is a fix. versionize only puts the subject in the changelog, so I'll squash with a subject that states the behaviour: fix(index): build model-declared indexes over existing documents when the context opens (#171). The details (open can block while filling, throws if documents can't be indexed) go in the squash body and the release notes.
• Opt-in skip / public rebuild: agreed on the direction (OnIndexBuildFailure(Skip | Throw) plus a public RebuildIndexAsync(name)). Please open the issue.
• The two pre-existing problems: yes, please file both.

[CaffeinatedCoder]

Thanks. Addressed in 1cb3a8e.

1. Public methods: CreateVectorIndexUntyped and CreateSpatialIndexUntyped are restored unchanged from main, with [Obsolete("Use model HasVectorIndex/HasSpatialIndex or CreateIndex(definition); will be removed in 6.0")]. EnsureIndexUntyped stays removed. As before, they create an empty index and don't fill it from existing documents.
2. Punctuation: the inner message now goes in as {ex.Message.TrimEnd('.')}., and IndexThatCannotBeFilled_FailsTheOpen_AndIsNotPersisted asserts "<inner message>. Nothing was saved".

Squash subject and release notes: agreed.

Issues filed:

• Model indexes: OnIndexBuildFailure(Skip | Throw) and a public RebuildIndexAsync(name) #190: OnIndexBuildFailure(Skip | Throw) and a public RebuildIndexAsync(name)
• Changing a model-declared index definition is silently ignored for an existing index #191: a changed model index definition is silently kept
• Multi-process mode: two processes opening a new model both backfill the same index #192: two processes backfill the same index concurrently in multi-process mode

[mrdevrobot]

Thanks, both asks are done: the two methods are back byte-for-byte with [Obsolete] and no CS0618 in the repo, and the message reads correctly. With #170 now on main, this PR is the second one to land, and two things need to change before merging.

1. The failure path leaks every page except the root. ApplyIndexBuilder calls RebuildIndexAsync(index, skipUnreadableDocuments: false), which rolls back internally, and then index.FreeAllPages(_storage) (DocumentCollection.cs ~1095), which walks the committed view, CollectAllPages(0). After the rollback only the root is reachable. This is the bug #170 fixed in RebuildIndexFromScratchAsync. Probe on the merged state, 3,000 documents with one wrong-dimension embedding at the end: the transaction view had 322 pages, the committed view 1, so 321 pages leak per failed open, and the file grew 192 → 320 → 384 → 448 → 512 pages over four opens. Since the message tells users every open fails the same way, this repeats on every start until the data is fixed. The existing test fails on the first document, before any split, so it can't see it. Please reuse #170's pattern: own transaction, FillIndexAsync, CollectAllPages(txnId) before the rollback, then FreeUnregisteredCopy. Ideally one shared "fill an unregistered index or free it" helper for both callers. Please add a test that fails late, after splits.

2. The helper unification you offered. On the merged state, CollectionIndexManager has:

• CreateUnregistered(definition) and CreateUnregisteredCopy(name) doing the same construction; only the second checks _disposed. CreateUnregisteredCopy can become CreateUnregistered(GetIndex(name).Definition).
• Register and ReplaceIndex repeating the same steps, but only ReplaceIndex restores the old state when SaveMetadata throws. With Register a failed save leaves the index registered in memory but not on disk, and ApplyIndexBuilder can't release it.

Merges cleanly with main; full suite on the merged state 2415 / 0 / 4 skipped.

[CaffeinatedCoder]

Merged main (7e9773a, no conflicts) and addressed both points in 4a15f87.

1. Leak on the failure path. ApplyIndexBuilder and the vacuum rebuild now share two helpers in DocumentCollection:

• FillUnregisteredIndexAsync: own transaction, FillIndexAsync, and on failure CollectAllPages(txnId) before the rollback, then free and dispose (fix(vacuum): rebuild typed-collection indexes into fresh trees (#167) #170's pattern, moved out of RebuildIndexFromScratchAsync).
• PublishFilledIndex: hands the filled index to Register / ReplaceIndex and frees it if it did not get registered.

New test IndexThatFailsLate_FreesEveryPageItAdded_SoRepeatedOpensDoNotGrowTheFile: 3,000 vectors, the last primary-key entry gets a wrong-dimension embedding, 4 failing opens, and the file size must stay constant. Without the fix it grows 9437184 → 15728640 → 22020096 → 28311552 bytes.

2. Helper unification.

• CreateUnregisteredCopy is gone; callers use CreateUnregistered(existing.Definition), which now has the _disposed check.
• Register and ReplaceIndex go through one PublishLocked, which restores the previous registration (or removes the new one) when SaveMetadata throws. ApplyIndexBuilder then frees the index via PublishFilledIndex.
• The merge also left a gap: Register never called RefreshHasUniqueBTreeIndex() (added in fix(index): enforce unique secondary indexes on typed collections (#176) #182), so a unique HasIndex added to an existing collection was not enforced until the next open. PublishLocked refreshes it.

Tests:

• AddedIndex_WhoseMetadataCannotBeSaved_IsNotRegistered_AndItsPagesAreFreed holds _metadataLock, as in the fix(vacuum): rebuild typed-collection indexes into fresh trees (#167) #170 FailedSwap test.
• AddedUniqueIndex_IsEnforcedRightAway.

Each of the three new tests fails when only its own part of the fix is reverted. Full suite on 4a15f87: 2465 / 0 / 4 skipped.

Not changed here:

• MigrateLegacyDecimalIndexes still uses RecreateIndex + RebuildIndexAsync. Reading the code, it looks like the same leak when a migration fails late; I haven't reproduced it. I can move it onto the same helpers here or in a follow-up, whichever you prefer.
• While writing the unique test I hit an unrelated bug on main: with a B-Tree index on a string property, Where(p => p.Name == "p7") also returns p70..p79 (11 instead of 1), and so does QueryIndexAsync("idx_Name", "p7", "p7"). I'll file an issue for it.

[CaffeinatedCoder]

I also moved the decimal key-format migration onto the same helpers, in 758717b.

MigrateLegacyDecimalIndexes used RecreateIndex, which swaps in an empty tree, and then filled that tree in place. So a rebuild that failed late leaked the split pages on every open, and the legacy tree was gone after the first failure. It now fills an unregistered copy with the current key format via FillUnregisteredIndexAsync and swaps it in with ReplaceIndex through PublishFilledIndex, so the new root and the new format go in one metadata save. On failure the copy is freed and the old index stays registered in its old format, so the next open retries. RecreateIndex had no callers left and is removed.

Test DecimalIndexTests.LegacyIndexMigration_ThatFailsLate_KeepsTheOldIndex_AndDoesNotGrowTheFile: 3,000 products, a legacy-format price index, the last document unreadable. Four failing opens leave the file size unchanged and the stored index keeps its root and legacy format. After the document is repaired, the migration succeeds and queries are correct. Without the fix the file grows 7340032 → 9437184 → 11534336 → 13631488 bytes.

Full suite: 2466 / 0 / 4 skipped.

Note for #177: this supersedes the RecreateIndex change in its retry fix. When I rebase #177 after this lands, the migration will set fresh.TargetKeyFormat on the copy. The foreign-key retry test will then check that the original index is kept, instead of a LegacyKeyFormat marker on an emptied tree.

[CaffeinatedCoder]

I merged main (f8f437b) into the branch in 48a8145.

• One conflict, in VacuumAsync: fix(spatial): enumerate R-tree pages so spatial indexes are freed and rebuilt (#189) #205 removed the spatial skip. So every index, spatial included, now goes through RebuildIndexFromScratchAsync(index), and a failed spatial rebuild frees its copy through FillUnregisteredIndexAsync. fix(spatial): enumerate R-tree pages so spatial indexes are freed and rebuilt (#189) #205's failed-rebuild test passes on it.
• Stale remark removed: the merge drops the CreateUnregistered remark that spatial indexes can't enumerate their pages.
• Coverage: model indexes, the decimal migration and the vacuum rebuild all fill through FillUnregisteredIndexAsync, and publish through PublishFilledIndex/PublishLocked.

Tests: IndexBackfill 8/8, DecimalIndex 11/11, Vacuum 27/27; full suite 2532 / 0 / 4 skipped.