Move delete-only tombstoning into per-format update-only id trackers (#10212)

* Move delete-only tombstoning into per-format update-only id trackers

DeleteOnlySegment::tombstone_points wrote the deleted mask file directly,
hard-coding that both immutable id-tracker formats store it the same way.
Give each immutable format (in-RAM and disk-resident) its own update-only
tracker that owns the decision of where its tombstones go, and dispatch
through DeleteOnlyIdTrackerEnum, which lives next to ReadOnlyIdTrackerEnum.
Both formats share one deleted-mask file today, so the trackers delegate to
a shared writer in deleted_storage.rs; a format that diverges later changes
only its own update_only module.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Enforce empty-batch guard in the shared tombstone writer

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

lib/segment/src/id_tracker/disk_id_tracker/mod.rs

@@ -16,6 +16,7 @@ pub mod mappings;
 pub mod on_disk_format;
 pub mod read_only;
 mod reader;
+pub mod update_only;
 
 #[cfg(test)]
 mod tests;

lib/segment/src/id_tracker/disk_id_tracker/update_only.rs

@@ -0,0 +1,54 @@
+//! The write half of the disk-resident id tracker for an update-only segment.
+
+use std::path::{Path, PathBuf};
+
+use common::bitvec::BitVec;
+use common::types::PointOffsetType;
+use common::universal_io::{UniversalRead, UniversalWriteFileOps};
+
+use crate::common::operation_error::OperationResult;
+use crate::id_tracker::immutable_id_tracker::tombstone_points_in_stored_mask;
+use crate::types::PointIdType;
+
+/// The mapping is frozen, so the one thing to write is the stored deleted
+/// mask — which the disk-resident format keeps in the same file as the in-RAM
+/// immutable format, hence the shared implementation. Needs only reads plus
+/// [`atomic_save`] from the backend, so object stores qualify.
+///
+/// [`atomic_save`]: UniversalWriteFileOps::atomic_save
+pub struct UpdateOnlyDiskIdTracker<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> {
+    fs: S::Fs,
+    segment_path: PathBuf,
+    /// Consumed by the first [`tombstone_points`](Self::tombstone_points) in
+    /// place of reading the mask file.
+    deleted: Option<BitVec>,
+}
+
+impl<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> UpdateOnlyDiskIdTracker<S> {
+    /// `deleted` is the mask as the read phase held it in memory, when it
+    /// did; nothing is read here.
+    pub fn new(fs: S::Fs, segment_path: &Path, deleted: Option<BitVec>) -> Self {
+        Self {
+            fs,
+            segment_path: segment_path.to_path_buf(),
+            deleted,
+        }
+    }
+
+    /// Retire the given points by marking the slots they occupy in the stored
+    /// deleted mask — the only thing written, the data on those slots stays.
+    /// The mask is replaced whole, see [`StoredBitSlice::atomic_update`].
+    ///
+    /// [`StoredBitSlice::atomic_update`]: common::stored_bitslice::StoredBitSlice::atomic_update
+    pub fn tombstone_points(
+        &mut self,
+        points: &[(PointIdType, PointOffsetType)],
+    ) -> OperationResult<()> {
+        tombstone_points_in_stored_mask::<S>(
+            &self.fs,
+            &self.segment_path,
+            &mut self.deleted,
+            points,
+        )
+    }
+}

lib/segment/src/id_tracker/id_tracker_base/delete_only_tracker_enum.rs

@@ -0,0 +1,28 @@
+use common::types::PointOffsetType;
+use common::universal_io::{UniversalRead, UniversalWriteFileOps};
+
+use crate::common::operation_error::OperationResult;
+use crate::id_tracker::disk_id_tracker::update_only::UpdateOnlyDiskIdTracker;
+use crate::id_tracker::immutable_id_tracker::update_only::UpdateOnlyImmutableIdTracker;
+use crate::types::PointIdType;
+
+/// The update-only tracker of whichever immutable id-tracker format a segment
+/// holds. Each variant decides where its tombstones go.
+pub enum DeleteOnlyIdTrackerEnum<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> {
+    Immutable(UpdateOnlyImmutableIdTracker<S>),
+    DiskResident(UpdateOnlyDiskIdTracker<S>),
+}
+
+impl<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> DeleteOnlyIdTrackerEnum<S> {
+    /// Retire the given points by marking the slots they occupy in the stored
+    /// deleted mask — the only thing written, the data on those slots stays.
+    pub fn tombstone_points(
+        &mut self,
+        points: &[(PointIdType, PointOffsetType)],
+    ) -> OperationResult<()> {
+        match self {
+            Self::Immutable(id_tracker) => id_tracker.tombstone_points(points),
+            Self::DiskResident(id_tracker) => id_tracker.tombstone_points(points),
+        }
+    }
+}

lib/segment/src/id_tracker/id_tracker_base/mod.rs

@@ -2,6 +2,7 @@ mod point_mappings_ref;
 mod tracker_enum;
 mod trait_def;
 
+pub mod delete_only_tracker_enum;
 pub mod read_only_tracker_enum;
 
 pub use point_mappings_ref::{PointMappingsGuard, PointMappingsRefEnum};

lib/segment/src/id_tracker/immutable_id_tracker/deleted_storage.rs

@@ -1,7 +1,60 @@
 use std::path::{Path, PathBuf};
 
+use common::bitvec::BitVec;
+use common::mmap::AdviceSetting;
+use common::stored_bitslice::StoredBitSlice;
+use common::types::PointOffsetType;
+use common::universal_io::{OpenOptions, Populate, UniversalRead, UniversalWriteFileOps};
+
+use crate::common::operation_error::{OperationError, OperationResult};
+use crate::types::PointIdType;
+
 pub const DELETED_FILE_NAME: &str = "id_tracker.deleted";
 
 pub(crate) fn deleted_path(base: &Path) -> PathBuf {
     base.join(DELETED_FILE_NAME)
 }
+
+/// Retire `points` by setting the slots they occupy in the stored deleted
+/// mask of the segment at `segment_path`. The mask is replaced whole, see
+/// [`StoredBitSlice::atomic_update`]; `seed` is consumed in place of reading
+/// the mask file when the caller held it in memory — and kept for a later
+/// call when `points` is empty and nothing is written.
+pub(crate) fn tombstone_points_in_stored_mask<S: UniversalRead<Fs: UniversalWriteFileOps>>(
+    fs: &S::Fs,
+    segment_path: &Path,
+    seed: &mut Option<BitVec>,
+    points: &[(PointIdType, PointOffsetType)],
+) -> OperationResult<()> {
+    if points.is_empty() {
+        return Ok(());
+    }
+    StoredBitSlice::<S>::atomic_update(
+        fs,
+        deleted_path(segment_path),
+        OpenOptions {
+            writeable: false,
+            need_sequential: false,
+            populate: Populate::No,
+            advice: AdviceSetting::Global,
+        },
+        Default::default(),
+        seed.take(),
+        |mask| {
+            for &(point_id, internal_id) in points {
+                let slot = internal_id as usize;
+                // A slot beyond the mask names a point this segment cannot hold.
+                if slot >= mask.len() {
+                    return Err(OperationError::service_error(format!(
+                        "cannot tombstone point {point_id} of segment {}: slot {internal_id} \
+                         is beyond its deleted mask ({} slots)",
+                        segment_path.display(),
+                        mask.len(),
+                    )));
+                }
+                mask.set(slot, true);
+            }
+            Ok(())
+        },
+    )?
+}

lib/segment/src/id_tracker/immutable_id_tracker/mod.rs

@@ -6,6 +6,7 @@ mod versions_storage;
 pub(super) mod tests;
 
 pub mod read_only;
+pub mod update_only;
 
 use std::fmt::Debug;
 use std::io::{BufReader, BufWriter, Write};
@@ -23,7 +24,7 @@ use common::universal_io::{
 use fs_err::File;
 
 pub use self::deleted_storage::DELETED_FILE_NAME;
-pub(crate) use self::deleted_storage::deleted_path;
+pub(crate) use self::deleted_storage::{deleted_path, tombstone_points_in_stored_mask};
 pub use self::mappings_storage::{MAPPINGS_FILE_NAME, mappings_path};
 use self::mappings_storage::{load_mapping, store_mapping};
 pub use self::versions_storage::VERSION_MAPPING_FILE_NAME;

lib/segment/src/id_tracker/immutable_id_tracker/update_only.rs

@@ -0,0 +1,54 @@
+//! The write half of the in-RAM immutable id tracker for an update-only
+//! segment.
+
+use std::path::{Path, PathBuf};
+
+use common::bitvec::BitVec;
+use common::types::PointOffsetType;
+use common::universal_io::{UniversalRead, UniversalWriteFileOps};
+
+use super::deleted_storage::tombstone_points_in_stored_mask;
+use crate::common::operation_error::OperationResult;
+use crate::types::PointIdType;
+
+/// The mapping is frozen, so the one thing to write is the stored deleted
+/// mask ([`DELETED_FILE_NAME`](super::DELETED_FILE_NAME)). Needs only reads
+/// plus [`atomic_save`] from the backend, so object stores qualify.
+///
+/// [`atomic_save`]: UniversalWriteFileOps::atomic_save
+pub struct UpdateOnlyImmutableIdTracker<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> {
+    fs: S::Fs,
+    segment_path: PathBuf,
+    /// Consumed by the first [`tombstone_points`](Self::tombstone_points) in
+    /// place of reading the mask file.
+    deleted: Option<BitVec>,
+}
+
+impl<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> UpdateOnlyImmutableIdTracker<S> {
+    /// `deleted` is the mask as the read phase held it in memory, when it
+    /// did; nothing is read here.
+    pub fn new(fs: S::Fs, segment_path: &Path, deleted: Option<BitVec>) -> Self {
+        Self {
+            fs,
+            segment_path: segment_path.to_path_buf(),
+            deleted,
+        }
+    }
+
+    /// Retire the given points by marking the slots they occupy in the stored
+    /// deleted mask — the only thing written, the data on those slots stays.
+    /// The mask is replaced whole, see [`StoredBitSlice::atomic_update`].
+    ///
+    /// [`StoredBitSlice::atomic_update`]: common::stored_bitslice::StoredBitSlice::atomic_update
+    pub fn tombstone_points(
+        &mut self,
+        points: &[(PointIdType, PointOffsetType)],
+    ) -> OperationResult<()> {
+        tombstone_points_in_stored_mask::<S>(
+            &self.fs,
+            &self.segment_path,
+            &mut self.deleted,
+            points,
+        )
+    }
+}

lib/segment/src/segment/update_only/delete_only/mod.rs

@@ -1,87 +1,56 @@
 //! The write phase for an immutable segment: retiring points, and nothing
 //! else.
 
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
-use common::mmap::AdviceSetting;
-use common::stored_bitslice::StoredBitSlice;
 use common::types::PointOffsetType;
-use common::universal_io::{OpenOptions, Populate, UniversalRead, UniversalWriteFileOps};
+use common::universal_io::{UniversalRead, UniversalWriteFileOps};
 
 use super::DeleteOnlyIdTrackerState;
-use crate::common::operation_error::{OperationError, OperationResult};
-use crate::id_tracker::immutable_id_tracker::deleted_path;
+use crate::common::operation_error::OperationResult;
+use crate::id_tracker::delete_only_tracker_enum::DeleteOnlyIdTrackerEnum;
+use crate::id_tracker::disk_id_tracker::update_only::UpdateOnlyDiskIdTracker;
+use crate::id_tracker::immutable_id_tracker::update_only::UpdateOnlyImmutableIdTracker;
 use crate::types::PointIdType;
 
 /// A segment open for deletes: nothing in it can grow, so the only thing a
-/// batch can do here is retire points that are already there. Needs only
-/// reads plus [`atomic_save`] from the backend, so object stores qualify.
-///
-/// [`atomic_save`]: UniversalWriteFileOps::atomic_save
+/// batch can do here is retire points that are already there. Where their
+/// tombstones go is the id tracker's decision.
 pub struct DeleteOnlySegment<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> {
-    fs: S::Fs,
-    segment_path: PathBuf,
-    /// Consumed by the first [`tombstone_points`](Self::tombstone_points) in
-    /// place of reading the mask file.
-    id_tracker_state: Option<DeleteOnlyIdTrackerState>,
+    id_tracker: DeleteOnlyIdTrackerEnum<S>,
 }
 
 impl<S: UniversalRead<Fs: UniversalWriteFileOps> + 'static> DeleteOnlySegment<S> {
-    /// Open the segment directory at `segment_path` for deletes; nothing is
+    /// Open the segment directory at `segment_path` for deletes, resuming the
+    /// tracker kind its [`DeleteOnlyIdTrackerState`] variant names; nothing is
     /// read.
     pub fn open(
         fs: S::Fs,
         segment_path: &Path,
-        id_tracker_state: Option<DeleteOnlyIdTrackerState>,
+        id_tracker_state: DeleteOnlyIdTrackerState,
     ) -> Self {
-        Self {
-            fs,
-            segment_path: segment_path.to_path_buf(),
-            id_tracker_state,
-        }
+        let id_tracker = match id_tracker_state {
+            DeleteOnlyIdTrackerState::Immutable(deleted) => DeleteOnlyIdTrackerEnum::Immutable(
+                UpdateOnlyImmutableIdTracker::new(fs, segment_path, deleted),
+            ),
+            DeleteOnlyIdTrackerState::DiskResident(deleted) => {
+                DeleteOnlyIdTrackerEnum::DiskResident(UpdateOnlyDiskIdTracker::new(
+                    fs,
+                    segment_path,
+                    deleted,
+                ))
+            }
+        };
+        Self { id_tracker }
     }
 
-    /// Retire the given points by marking the slots they occupy in the
-    /// deleted-points bitmask (`id_tracker.deleted`) — the only thing
-    /// written, the data on those slots stays. The mask is replaced whole,
-    /// see [`StoredBitSlice::atomic_update`].
+    /// Retire the given points by marking the slots they occupy in the id
+    /// tracker's stored deleted mask — the only thing written, the data on
+    /// those slots stays.
     pub fn tombstone_points(
         &mut self,
         points: &[(PointIdType, PointOffsetType)],
     ) -> OperationResult<()> {
-        if points.is_empty() {
-            return Ok(());
-        }
-
-        let seed = self.id_tracker_state.take().map(|state| state.deleted);
-
-        StoredBitSlice::<S>::atomic_update(
-            &self.fs,
-            deleted_path(&self.segment_path),
-            OpenOptions {
-                writeable: false,
-                need_sequential: false,
-                populate: Populate::No,
-                advice: AdviceSetting::Global,
-            },
-            Default::default(),
-            seed,
-            |mask| {
-                for &(point_id, internal_id) in points {
-                    let slot = internal_id as usize;
-                    // A slot beyond the mask names a point this segment cannot hold.
-                    if slot >= mask.len() {
-                        return Err(OperationError::service_error(format!(
-                            "cannot tombstone point {point_id} of segment {}: slot {internal_id} \
-                             is beyond its deleted mask ({} slots)",
-                            self.segment_path.display(),
-                            mask.len(),
-                        )));
-                    }
-                    mask.set(slot, true);
-                }
-                Ok(())
-            },
-        )?
+        self.id_tracker.tombstone_points(points)
     }
 }

lib/segment/src/segment/update_only/lookup/mod.rs

@@ -56,15 +56,13 @@ impl<S: UniversalRead + 'static> LookupSegment<S> {
                 })
             }
             ReadOnlyIdTrackerEnum::Immutable(id_tracker) => {
-                WriterIdTrackerState::DeleteOnly(Some(DeleteOnlyIdTrackerState {
-                    deleted: id_tracker.deleted_point_bitslice().to_bitvec(),
-                }))
+                WriterIdTrackerState::DeleteOnly(DeleteOnlyIdTrackerState::Immutable(Some(
+                    id_tracker.deleted_point_bitslice().to_bitvec(),
+                )))
             }
             ReadOnlyIdTrackerEnum::DiskResident(id_tracker) => {
-                WriterIdTrackerState::DeleteOnly(id_tracker.deleted_full_if_materialized().map(
-                    |deleted| DeleteOnlyIdTrackerState {
-                        deleted: deleted.clone(),
-                    },
+                WriterIdTrackerState::DeleteOnly(DeleteOnlyIdTrackerState::DiskResident(
+                    id_tracker.deleted_full_if_materialized().cloned(),
                 ))
             }
         }

lib/segment/src/segment/update_only/mod.rs

@@ -39,9 +39,7 @@ use crate::types::PointIdType;
 /// decides the writer's kind.
 pub enum WriterIdTrackerState {
     Appendable(AppendableIdTrackerState),
-    /// `None` when the read phase did not hold the deleted mask in memory;
-    /// the writer then reads it itself.
-    DeleteOnly(Option<DeleteOnlyIdTrackerState>),
+    DeleteOnly(DeleteOnlyIdTrackerState),
 }
 
 /// The tail of an appendable segment's mappings log, as the read phase saw it.
@@ -59,8 +57,11 @@ pub struct AppendableIdTrackerState {
     pub mappings_end: u64,
 }
 
-/// An immutable segment's deleted-points mask as the read phase held it in
-/// memory, sparing the writer the read of the mask file.
-pub struct DeleteOnlyIdTrackerState {
-    pub deleted: BitVec,
+/// Which immutable id tracker the read phase found — deciding which
+/// update-only tracker the writer resumes with — carrying the deleted-points
+/// mask when the read phase held it in memory, sparing the writer the read of
+/// the mask file. `None` means the writer's tracker reads it itself.
+pub enum DeleteOnlyIdTrackerState {
+    Immutable(Option<BitVec>),
+    DiskResident(Option<BitVec>),
 }