fix: manifest file walkers miss data-overlay files

[LuciferYang]

A fragment references its base files through files and its overlay files through overlays[].data_file. Six paths read files directly and so miss every overlay data file. Three enumerate, and three rewrite or retain.

Enumeration:

• process_manifest builds the cleanup keep set, so an overlay old enough to be a deletion candidate is irreversibly deleted from the live dataset.
• collect_paths feeds deep clone's copy loop. It copies exactly the paths returned, then commits an Operation::Clone whose manifest carries the source fragments verbatim, overlays included, so the clone references files that were never copied and reading it fails with a not-found error.
• manifest_file_rows powers tracked_files, which under-reports its documented "every file referenced in any manifest" contract. That becomes a deletion risk of its own once its output drives an external cleanup.

Rewriting and retention:

• Shallow clone stamps base_id on every local file so the clone resolves it against the parent. Skipping overlays left theirs at None, so the clone looked for the overlay under its own root, where it was never written.
• Deep clone clears base_id on the same fields. An overlay kept a base_id naming a base the new manifest no longer lists.
• Branch lineage retention promotes a path from verified_files into referenced_files when its base_id resolves to the parent's own URI. An overlay a branch inherited never got promoted, so the parent's cleanup deleted a file the branch still reads.

The last two are inseparable. Before this change an inherited overlay carried no base_id at all, so retention never examined it; fixing the clone alone would give overlays a base_id while retention still skipped them, which is what turns the omission into a deletion.

All six misbehave in any build that can open an overlay-bearing dataset: debug builds unconditionally, release builds only with LANCE_ENABLE_UNSTABLE_DATA_OVERLAY_FILES set, since a release reader otherwise refuses the manifest at open and these paths never see one. Within that gate none of them needs a further code change to be reachable.

The fix

Fragment::data_files() yields the base files chained with each overlay's data file, and data_files_mut() is its counterpart for the two paths that rewrite base_id. Both destructure Fragment exhaustively, so a new field fails to compile there until someone decides whether it references files — the prompt that was missing when overlays was added in #7535. The mutable one also needs the destructure so the disjoint field borrows are visible to the borrow checker.

manifest_file_rows derives its exact_size precount from the same accessor. Counting files.len() separately would underflow the moment an overlay appeared, since ExactSize::next decrements an unsigned counter.

cleanup_data_fragments deliberately stays on files. It deletes the files a caller hands it, and callers decide which files belong to the failed write: schema_evolution clones a live committed fragment and narrows files to the newly written ones while leaving overlays untouched, so including overlays there would delete live data. The second commit records that reasoning at the loop.

Each converted caller keeps its own base_id handling: collect_paths resolves it to a base root, manifest_file_rows to an external base URI, and process_manifest ignores it. Those differ by design, so unifying them is out of scope.

Tests

Six cases, one per path, each verified to fail with the corresponding accessor reverted to files-only:

• keep_set_covers_referenced_overlay_files — the keep set contains the overlay path.
• deep_clone_copies_overlay_files — the clone returns the overlaid values rather than failing to find the file.
• test_manifest_file_rows_per_file_base_id (extended) — the overlay row appears with its own base_id resolved.
• shallow_clone_stamps_base_id_on_overlay_files — the overlay resolves against the parent.
• deep_clone_of_shallow_clone_clears_overlay_base_id — deep-cloning a shallow clone leaves the overlay with no base_id.
• lineage_retention_covers_inherited_overlay_files — the parent's keep set promotes an overlay its branch inherited. It asserts the branch actually inherited a base_id first, so it cannot pass vacuously.

The keep-set and lineage cases assert on process_manifests and retain_branch_lineage_files output rather than driving cleanup end to end. An end-to-end version cannot reach the deletion decision: build_listing_stream passes earliest_retained_manifest_time to read_dir_all, which lists only files whose mtime predates it, and the test clock does not move real file mtimes, so a file written during the test is never a candidate.

One pre-existing test helper needed a fix to support this: commit_overlay wrote through a store-root-relative path that only resolved on an in-memory store, and the clone cases need real stores.

cargo test -p lance --lib across dataset::cleanup, dataset::files, dataset::fragment, dataset::write, io::commit, plus cargo test -p lance-table --lib format, all pass. cargo clippy -p lance -p lance-table --all-targets -- -D warnings, RUSTDOCFLAGS="-D warnings" cargo doc -p lance -p lance-table --no-deps, and cargo fmt --all --check are clean.

Relation to #8097

This came out of the #8097 discussion, where the suggestion was to factor out the reusable part before rebasing that PR onto it. The overlay drift is an independent defect, so it is split out here rather than mixed into the API discussion. It is not by itself the consolidation asked for there — Fragment::data_files() is a fragment-level accessor, and whether that is the "reusable part" or whether referenced_files should be rebuilt on tracked_files' pipeline is still open. I will follow up on #8097 with that question.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The shared iterator is useful for complete reference enumeration, but these paths have different ownership contracts: reporting and retention need every referenced data file, failure cleanup must delete only files owned by that attempt, and clone/branch transformations must rebase and retain every referenced file.

A safe revision should keep attempt ownership explicit and add mutable clone rebasing plus descendant-lineage coverage, with regressions for inherited-overlay failure cleanup and overlay-bearing shallow/deep clones.

[lance-gatekeeper]

cleanup_data_fragments is not always handed an entirely attempt-owned fragment. cleanup_new_column_data_files clones updated fragment metadata, replaces only .files with current-attempt files, and leaves inherited .overlays attached. This loop therefore deletes an already-committed overlay when a later add-columns fragment fails, corrupting the live dataset.

Keep this deletion boundary attempt-owned—for example, clear inherited overlays in the synthetic cleanup fragment or pass an explicit set of files created by the attempt—while allowing genuine overlay writers to opt their new overlay files into cleanup explicitly.

Reproducer

At this head I added a two-fragment regression that commits data/committed-overlay.lance on fragment 0, runs an add_columns UDF that succeeds for fragment 0 and returns injected second-fragment failure for fragment 1, then asserts the committed overlay path still exists.

CARGO_TARGET_DIR=/home/agent/tmp/pr8267-target-impl cargo test -p lance --lib repro_add_columns_failure_deletes_committed_overlay -- --nocapture
...
committed overlay must survive cleanup of uncommitted add-columns files
test result: FAILED. 0 passed; 1 failed

The test exited 101 because the overlay file had been deleted.

[lance-gatekeeper]

This abstraction covers only immutable enumeration, so the matching file-ownership transformations still iterate .files. Manifest::shallow_clone assigns the source base ID only to ordinary files, leaving a normal overlay at base_id=None so it resolves under the clone's empty data directory. Deep-clone normalization likewise clears base_paths while clearing IDs only on ordinary files.

Add a mutable all-data-files traversal (or explicit overlay handling) to both clone normalizations. The descendant-branch retention walker should use the immutable traversal too, so parent cleanup protects overlay files inherited through the source base.

Reproducer

I added a lance-table unit test that creates a manifest with one overlay at base_id=None, shallow-clones it with source base ID 7, and asserts the cloned overlay was rebased:

assert_eq!(
    cloned.fragments[0].overlays[0].data_file.base_id,
    Some(7),
);

CARGO_TARGET_DIR=/home/agent/tmp/pr8267-target-repro cargo test -p lance-table --lib shallow_clone_rebases_overlay_data_files -- --nocapture
assertion failed
  left: None
 right: Some(7)
test result: FAILED. 0 passed; 1 failed

The command exited 101 on this head.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The failed-write cleanup boundary is now safe, but the new all-data-files traversal is still applied only to immutable enumeration. Clone normalization and descendant-lineage retention continue to omit overlays, so clones can reference the wrong base and parent cleanup can delete data still needed by a branch.

Please pair this iterator with a mutable all-data-files traversal (or explicit overlay handling) in both clone normalization paths, and use the immutable traversal in descendant-lineage retention.

[lance-gatekeeper]

This now copies overlay bytes, but the corresponding clone metadata transforms still walk only .files. Manifest::shallow_clone leaves a local overlay at base_id=None, so it resolves under the empty clone; deep clone clears base_paths while leaving an external overlay ID in the manifest. Apply the base-ID transformation to every overlay data file as well, ideally through a mutable all-data-files traversal so copy and normalization cannot drift again.

Reproducer

On this head I added disposable regressions that commit a real overlay, assert shallow-clone rebasing, then repair that independent omission to isolate deep-clone normalization:

assert_eq!(overlay_base_id, Some(source_base_id));
assert!(deep.manifest().base_paths.is_empty());
assert!(deep.manifest().fragments.iter()
    .flat_map(|fragment| &fragment.overlays)
    .all(|overlay| overlay.data_file.base_id.is_none()));

CARGO_TARGET_DIR=/home/agent/tmp/gate-8267-verify-target-yj6JPO cargo test -p lance verification_shallow_clone_rebases_overlay_base_id -- --nocapture
... left: None
... right: Some(0)
test result: FAILED

CARGO_TARGET_DIR=/home/agent/tmp/gate-8267-verify-target-yj6JPO cargo test -p lance verification_deep_clone_normalizes_external_overlay_base_id -- --nocapture
... a deep clone has no external bases, so every copied overlay must be normalized to local
test result: FAILED

Both commands exited 101.

[lance-gatekeeper]

This fixes the main-manifest keep set, but process_branch_referenced_manifests still iterates fragment.files. A descendant overlay whose base_id points to the parent therefore remains in verified_files instead of being promoted to referenced_files, allowing parent cleanup to delete data the branch still owns. Use data_files() in the lineage walker too.

Reproducer

On this head I added a disposable regression that creates a parent overlay and child branch, repairs the independently tested shallow-clone omission, seeds data/lineage-overlay.lance as verified, and calls the exact lineage ownership routine:

assert!(inspection.referenced_files.data_paths.contains(&overlay_path));
assert!(!inspection.verified_files.data_paths.contains(&overlay_path));

CARGO_TARGET_DIR=/home/agent/tmp/gate-8267-verify-target-yj6JPO cargo test -p lance verification_descendant_branch_retains_parent_overlay_reference -- --nocapture
... a descendant's external overlay must promote the parent file into the retained set
test result: FAILED

The command exited 101. This bounded test exercises the exact retention transition; it does not run the destructive delete step.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[lance-gatekeeper]

✅ Gate recommendation: approve.

The revision now preserves the distinct file-ownership contracts: failed-write cleanup remains attempt-owned, while immutable and mutable all-data-file traversals cover cleanup retention, shallow/deep clone normalization, and descendant branch lineage. Focused current-head regressions pass for the previously failing paths, including deep-clone readback.

[wjones127]

This seems good, although I am unsure about the method name. I worry it could be confusing. Any thoughts on that?

[wjones127]

nitpick: "data files" is the name I use for just the file; I consider overlays to be separate. I'm not sure what name I would use for the two combined, but I don't think it would be overlay files.

Maybe this could be referenced_lance_files(), since they are both that format?

Suggested change

	pub fn data_files(&self) -> impl Iterator<Item = &DataFile> + '_ {
	pub fn referenced_lance_files(&self) -> impl Iterator<Item = &DataFile> + '_ {

[LuciferYang]

Done in 72b87ed

[LuciferYang]

Thank you @wjones127