fix(index): keep a commit alive when an index cannot be opened - #8441
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Making index repair best-effort is the right boundary, but the deferred state is not yet durable: one path can stamp unverified legacy coverage as current and suppress future repair, while another can discard an unknown same-name segment on a later commit.
A viable revision should persist deferred coverage as unknown and preserve unknown segments through manifest filtering, so commits succeed without turning recoverable index metadata into stale or lost state.
| // The bitmap is optional metadata and recalculating it means | ||
| // opening the index. Failing here fails every commit the | ||
| // dataset takes, since migration runs on all of them, so keep | ||
| // the coverage as it stands and leave the repair to a build |
There was a problem hiding this comment.
A failed legacy repair is not actually left for a later compatible build. must_recalculate_fragment_bitmap uses the old manifest writer version, but a successful commit writes WriterVersion::default(). For the checked-in v0.8.14 fixture, this branch carries the present-but-corrupt bitmap into a current-writer manifest; after index storage recovers, the next commit skips repair and fragment 0 remains absent, which can omit rows from prefiltered index results. Persist None (the existing safe unknown-coverage state) or another durable retry signal when this repair fails.
Reproducer
I added a regression that copies v0.8.14/corrupt_index, renames _indices away, commits delete(false), restores _indices, reopens, commits again, and asserts the compatible writer restored fragment 0.
CARGO_TARGET_DIR=/home/agent/tmp/target-pr8441-implementation-2177bdd cargo test -p lance test_unopenable_old_index_still_retries_bitmap_migration -- --nocapture
Observed: exit 101 with the compatible writer must repair the old corrupt fragment bitmap.
There was a problem hiding this comment.
Fixed on 0b7802db3. The failure path now distinguishes the one-shot pre-0.8.15 repair from retryable missing/overlap triggers and persists fragment_bitmap: None when that legacy repair cannot open the index. I verified test_v0_8_14_invalid_index_fragment_bitmap_repair_is_not_lost passes, including the later compatible repair restoring fragment 0. Resolving this finding.
|
|
||
| // And an unrelated commit after it, since the missing bitmap is now what | ||
| // the manifest holds and migration retries on every commit. | ||
| dataset.delete("false").await.unwrap(); |
There was a problem hiding this comment.
This later-commit check misses same-name multi-segment indices. After the first commit such a group can contain the unavailable segment with fragment_bitmap: None and a readable segment with known non-empty coverage. delete(false) calls retain_relevant_indices before migration; that function currently classifies None as empty and retains only the non-empty same-name segment, permanently deleting the unavailable UUID instead of carrying it through. Preserve unknown segments until they can be repaired.
Reproducer
I added a unit regression with same-name segments [fragment_bitmap: None, fragment_bitmap: Some({2})], called retain_relevant_indices, and asserted that both remain.
CARGO_TARGET_DIR=/home/agent/tmp/target-pr8441-implementation-2177bdd cargo test -p lance test_retain_unknown_and_nonempty_segments_keeps_unknown_segment -- --nocapture
Observed: exit 101 with unknown coverage is not empty coverage; the retained length was 1 instead of 2.
There was a problem hiding this comment.
Fixed on 0b7802db3. retain_relevant_indices now partitions unknown coverage before same-name pruning and preserves every segment whose fragment_bitmap is None, instead of treating it as known-empty coverage. I verified test_retain_unknown_coverage_alongside_nonempty_sibling passes with both segments retained. Resolving this finding.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The two durability gaps from the previous review are fixed: failed legacy repairs now persist unknown coverage for a compatible retry, and same-name retention preserves unknown segments. The write path remains best-effort only for optional index reconstruction while schema invariants stay fatal, with historical and multi-segment regressions covering recovery and preservation.
wjones127
left a comment
There was a problem hiding this comment.
This looks reasonable. Happy to merge once merge conflicts are addressed.
0b7802d to
bc07c4d
Compare
@wjones127 thank you for the review. Conflicts are resolved, CI is green. |
bc07c4d to
e5f6af7
Compare
migrate_indicesruns on every commit and recalculates a missingfragment_bitmapby opening the index, propagating the open with?. An index this build cannot open - files removed or never finished, a newer writer, an out-of-reach shallow-clone base - therefore fails that commit and every one after it, leaving the dataset unwritable rather than merely unreadable. The open and the coverage calculation are now caught together and logged atwarn, like the two neighbouring best-effort steps. The field lookup above them stays fatal: a manifest naming a field the schema lacks is a broken invariant, not an environment condition.Only the pre-0.8.15 trigger drops the coverage to unknown. The other two re-derive from the index metadata and ask again for free, and
calculate_included_fragsis unimplemented for the modern index types, so they keep what they have. The pre-0.8.15 trigger reads the previous manifest's writer version, which a successful commit replaces, so a bitmap left in place would look migrated from then on - how the corrupt bitmap inv0.8.14/corrupt_index, missing fragment 0, would become permanent.retain_relevant_indicesmust then stop counting a missing bitmap as empty coverage: it runs before migration in the same commit and was deleting the segment before the retry could reach it.An absent bitmap reads as unmeasured, not empty:
index_fragment_coverageandPreFilter::new_with_filter_futureboth fall back to full coverage, so this only ever widens a scan or a rewrite group. The drop ridesmigrate_indices's coverage report (#8481), withdrawing any MemWAL catch-up credited off that bitmap. #8427's guard just above is a different case, an index whose version has no reader here.