fix(index): keep a commit alive when an index cannot be opened

rust/lance-table/src/transaction/index_maintenance.rs

@@ -257,6 +257,16 @@ impl Transaction {
             .collect::<RoaringBitmap>();
 
         for (_, same_name_indices) in indices_by_name {
+            // Unknown coverage is not empty coverage: a segment whose bitmap is
+            // missing has never been measured, and dropping it deletes an index
+            // that migration could not open yet.
+            let (unknown_coverage, same_name_indices): (Vec<_>, Vec<_>) = same_name_indices
+                .into_iter()
+                .partition(|index| index.fragment_bitmap.is_none());
+            for index in unknown_coverage {
+                uuids_to_keep.insert(index.uuid);
+            }
+
             if same_name_indices.len() > 1 {
                 let (empty_indices, non_empty_indices): (Vec<_>, Vec<_>) =
                     same_name_indices.iter().partition(|index| {
@@ -626,12 +636,31 @@ mod tests {
         Transaction::retain_relevant_indices(&mut scalar_indices, &schema, &fragments);
         Transaction::retain_relevant_indices(&mut vector_indices, &schema, &fragments);
 
-        // Both kept: a None bitmap counts as empty coverage, and empty
-        // definitions are retained regardless of index type.
+        // Both kept: a None bitmap is unknown coverage, not empty coverage, and
+        // an unmeasured segment is retained regardless of index type.
         assert_eq!(scalar_indices.len(), 1);
         assert_eq!(vector_indices.len(), 1);
     }
 
+    #[test]
+    fn test_retain_unknown_coverage_alongside_nonempty_sibling() {
+        let schema = create_test_schema(&[1]);
+        let fragments = vec![Fragment::new(1), Fragment::new(2)];
+
+        let mut indices = vec![
+            create_test_index("idx", 1, 1, None, false), // Coverage never measured
+            create_test_index("idx", 1, 2, Some(RoaringBitmap::from_iter([2])), false),
+        ];
+
+        Transaction::retain_relevant_indices(&mut indices, &schema, &fragments);
+
+        // The unmeasured segment must survive its non-empty sibling: its bitmap
+        // is missing because migration could not open the index, and deleting
+        // the segment would take the only record of it with it.
+        assert_eq!(indices.len(), 2);
+        assert!(indices.iter().any(|idx| idx.fragment_bitmap.is_none()));
+    }
+
     #[test]
     fn test_retain_multiple_empty_scalar_indices_keeps_oldest() {
         let schema = create_test_schema(&[1]);

rust/lance/src/dataset/tests/dataset_migrations.rs

@@ -281,6 +281,43 @@ async fn test_v0_8_14_invalid_index_fragment_bitmap(
     assert_eq!(row_count, 1900);
 }
 
+/// The repair above is triggered by the writer version of the manifest being
+/// committed *from*, and a successful commit stamps the current one. So a
+/// commit that cannot open the index has exactly one chance at the corrupt
+/// bitmap, and carrying it through unverified would hand a later build a
+/// bitmap that looks migrated.
+#[tokio::test]
+async fn test_v0_8_14_invalid_index_fragment_bitmap_repair_is_not_lost() {
+    let test_dir = copy_test_data_to_tmp("v0.8.14/corrupt_index").unwrap();
+    let test_uri = test_dir.path_str();
+
+    let indices_dir = test_dir.std_path().join("_indices");
+    let stashed_dir = test_dir.std_path().join("_indices_stashed");
+    std::fs::rename(&indices_dir, &stashed_dir).unwrap();
+
+    let mut dataset = Dataset::open(&test_uri).await.unwrap();
+    dataset.delete("false").await.unwrap();
+
+    for idx in dataset.load_indices().await.unwrap().iter() {
+        assert_eq!(
+            idx.fragment_bitmap, None,
+            "a bitmap the migration could not verify must be recorded as unknown"
+        );
+    }
+
+    std::fs::rename(&stashed_dir, &indices_dir).unwrap();
+
+    let mut dataset = Dataset::open(&test_uri).await.unwrap();
+    dataset.delete("false").await.unwrap();
+
+    for idx in dataset.load_indices().await.unwrap().iter() {
+        assert!(
+            idx.fragment_bitmap.as_ref().unwrap().contains(0),
+            "the first build that can open the index must repair the coverage"
+        );
+    }
+}
+
 #[tokio::test]
 async fn test_fix_v0_10_5_corrupt_schema() {
     // Schemas could be corrupted by successive calls to `add_columns` and
@@ -354,6 +391,50 @@ async fn test_fix_v0_21_0_corrupt_fragment_bitmap() {
     assert_eq!(get_bitmap(&indices[1]), vec![1]);
 }
 
+/// Unlike the pre-0.8.15 trigger, an overlap is re-derived from the index
+/// metadata on every commit, so it asks to be recalculated again on its own. A
+/// commit that cannot open the index has nothing to preserve and must leave the
+/// coverage alone: `None` is a state modern indices are not built to recover
+/// from, since `calculate_included_frags` exists only for old manifests.
+#[tokio::test]
+async fn test_v0_21_0_corrupt_fragment_bitmap_kept_when_index_cannot_be_opened() {
+    let test_dir = copy_test_data_to_tmp("v0.21.0/bad_index_fragment_bitmap").unwrap();
+    let test_uri = test_dir.path_str();
+
+    std::fs::rename(
+        test_dir.std_path().join("_indices"),
+        test_dir.std_path().join("_indices_stashed"),
+    )
+    .unwrap();
+
+    fn coverage(indices: &[IndexMetadata]) -> Vec<(String, Option<Vec<u32>>)> {
+        let mut coverage = indices
+            .iter()
+            .map(|idx| {
+                (
+                    idx.uuid.to_string(),
+                    idx.fragment_bitmap
+                        .as_ref()
+                        .map(|bitmap| bitmap.iter().collect()),
+                )
+            })
+            .collect::<Vec<_>>();
+        coverage.sort();
+        coverage
+    }
+
+    let mut dataset = Dataset::open(&test_uri).await.unwrap();
+    let before = coverage(&dataset.load_indices().await.unwrap());
+
+    dataset.delete("false").await.unwrap();
+
+    assert_eq!(
+        coverage(&dataset.load_indices().await.unwrap()),
+        before,
+        "coverage the overlap check will ask about again must be left as it stands"
+    );
+}
+
 #[tokio::test]
 async fn test_v8_decimal_zonemap_missing_extrema() {
     async fn query_ids(

rust/lance/src/io/commit.rs

@@ -955,9 +955,12 @@ async fn migrate_indices(dataset: &Dataset, indices: &mut [IndexMetadata]) -> Re
         if unsupported_index_version(index).is_some() {
             continue;
         }
+        // Also true when the bitmap is missing entirely, so the failure path below
+        // pairs it with `is_some` to mean "written before the 0.8.15 fix".
+        let bitmap_missing_or_legacy =
+            must_recalculate_fragment_bitmap(index, dataset.manifest.writer_version.as_ref());
         if needs_recalculating.contains(&index.name)
-            || must_recalculate_fragment_bitmap(index, dataset.manifest.writer_version.as_ref())
-                && !is_system_index(index)
+            || bitmap_missing_or_legacy && !is_system_index(index)
         {
             // A covered index still has exactly one keyed field; the trailing
             // `covering_fields` are carried, not keyed, so counting them
@@ -970,14 +973,49 @@ async fn migrate_indices(dataset: &Dataset, indices: &mut [IndexMetadata]) -> Re
             );
             let idx_field = dataset.schema().field_by_id(index.fields[0]).ok_or_else(|| Error::internal(format!("Index with uuid {} referred to field with id {} which did not exist in dataset", index.uuid, index.fields[0])))?;
             // We need to calculate the fragments covered by the index
-            let idx = dataset
-                .open_generic_index(&idx_field.name, &index.uuid, &NoOpMetricsCollector)
-                .await?;
-            let recalculated = idx.calculate_included_frags().await?;
-            if index.fragment_bitmap.as_ref() != Some(&recalculated) {
-                recovered_coverage.push(index.name.clone());
+            let recalculated = async {
+                let idx = dataset
+                    .open_generic_index(&idx_field.name, &index.uuid, &NoOpMetricsCollector)
+                    .await?;
+                idx.calculate_included_frags().await
+            }
+            .await;
+            match recalculated {
+                Ok(fragment_bitmap) => {
+                    if index.fragment_bitmap.as_ref() != Some(&fragment_bitmap) {
+                        recovered_coverage.push(index.name.clone());
+                    }
+                    index.fragment_bitmap = Some(fragment_bitmap);
+                }
+                Err(e) => {
+                    // Recalculating means opening the index, and failing here fails
+                    // every commit the dataset takes, since migration runs on all of
+                    // them. A missing bitmap and overlapping segment bitmaps are both
+                    // re-derived from the index metadata, so they ask again on their
+                    // own; the pre-0.8.15 trigger reads the previous manifest's writer
+                    // version, which this commit replaces with the current one, and a
+                    // bitmap left in place would look migrated from here on.
+                    let repair_ends_with_this_commit =
+                        index.fragment_bitmap.is_some() && bitmap_missing_or_legacy;
+                    log::warn!(
+                        "Could not recalculate the fragment bitmap for index {} (uuid: {}): {}. {}",
+                        index.name,
+                        index.uuid,
+                        e,
+                        if repair_ends_with_this_commit {
+                            "Dropping its coverage to unknown so a build that can open the index recalculates it."
+                        } else {
+                            "Leaving the repair to a build that can open the index."
+                        }
+                    );
+                    if repair_ends_with_this_commit {
+                        index.fragment_bitmap = None;
+                        // Derivation ran before this and may have credited a
+                        // catch-up position off the bitmap being dropped here.
+                        recovered_coverage.push(index.name.clone());
+                    }
+                }
             }
-            index.fragment_bitmap = Some(recalculated);
         }
         // We can't reliably recalculate the index type for label_list and bitmap indices and so we can't migrate this field.
         // However, we still log for visibility and to help potentially diagnose issues in the future if we grow to rely on the field.
@@ -1942,6 +1980,117 @@ mod tests {
         assert!(dataset.checkout_version(4).await.is_err());
     }
 
+    /// Every commit runs `migrate_indices`, and recalculating a missing
+    /// `fragment_bitmap` there means opening the index. An index this build
+    /// cannot open must not take the write path down with it: the dataset would
+    /// be unwritable, not merely unreadable, and every later commit would fail
+    /// the same way.
+    #[tokio::test]
+    async fn test_commit_survives_an_index_it_cannot_open() {
+        use lance_index::scalar::{BuiltinIndexType, ScalarIndexParams};
+        use lance_table::io::manifest::read_manifest_indexes;
+
+        let test_dir = TempStrDir::default();
+        let test_uri = test_dir.as_str();
+
+        let reader = gen_batch()
+            .col("id", array::step::<Int32Type>())
+            .col("payload", array::step::<Int32Type>())
+            .into_reader_rows(RowCount::from(10), BatchCount::from(1));
+        let mut dataset = Dataset::write(reader, test_uri, None).await.unwrap();
+
+        // The readable companion is what makes the difference visible: with a
+        // single index, "carried through the one it cannot open" and "stopped
+        // recalculating altogether" answer every assertion below the same way.
+        let btree_params = ScalarIndexParams::for_builtin(BuiltinIndexType::BTree);
+        for column in ["id", "payload"] {
+            dataset
+                .create_index_builder(&[column], IndexType::BTree, &btree_params)
+                .name(format!("{column}_idx"))
+                .await
+                .unwrap();
+        }
+
+        let broken = dataset.load_index_by_name("id_idx").await.unwrap().unwrap();
+        dataset
+            .object_store
+            .remove_dir_all(dataset.indices_dir().join(broken.uuid.to_string()))
+            .await
+            .unwrap();
+
+        // Reopened so the fixture is judged on what is on disk rather than on
+        // what this process still holds from building the index.
+        let mut dataset = Dataset::open(test_uri).await.unwrap();
+        assert!(
+            dataset
+                .open_generic_index("id", &broken.uuid, &NoOpMetricsCollector)
+                .await
+                .is_err(),
+            "the fixture is supposed to leave an index this build cannot open"
+        );
+
+        // Migration recalculates a bitmap that is missing, and no current writer
+        // emits one - untrained indices get an empty bitmap, not none at all - so
+        // the state an old manifest arrives in is set here by hand.
+        let indices = read_manifest_indexes(
+            &dataset.object_store,
+            &dataset.manifest_location,
+            &dataset.manifest,
+        )
+        .await
+        .unwrap();
+        let without_bitmaps = indices
+            .iter()
+            .map(|index| IndexMetadata {
+                fragment_bitmap: None,
+                ..index.clone()
+            })
+            .collect::<Vec<_>>();
+        let transaction = Transaction::new(
+            dataset.manifest.version,
+            Operation::CreateIndex {
+                new_indices: without_bitmaps,
+                removed_indices: indices,
+            },
+            None,
+        );
+        dataset
+            .apply_commit(transaction, &Default::default(), &Default::default())
+            .await
+            .unwrap();
+
+        // 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();
+
+        let migrated = read_manifest_indexes(
+            &dataset.object_store,
+            &dataset.manifest_location,
+            &dataset.manifest,
+        )
+        .await
+        .unwrap();
+        let coverage = |name: &str| {
+            migrated
+                .iter()
+                .find(|index| index.name == name)
+                .unwrap_or_else(|| panic!("no index named {name} in the manifest"))
+                .fragment_bitmap
+                .as_ref()
+                .map(|bitmap| bitmap.iter().collect::<Vec<_>>())
+        };
+        assert_eq!(
+            coverage("id_idx"),
+            None,
+            "an index that cannot be opened must report unknown coverage"
+        );
+        assert_eq!(
+            coverage("payload_idx"),
+            Some(vec![0]),
+            "an index that opens must still have its coverage recalculated"
+        );
+    }
+
     #[tokio::test]
     async fn test_load_and_sort_new_transactions() {
         // Create a dataset