fix: list unreferenced data files newer than the retained floor - #8945
Open
jennifermell wants to merge 3 commits into
Open
fix: list unreferenced data files newer than the retained floor#8945jennifermell wants to merge 3 commits into
jennifermell wants to merge 3 commits into
Conversation
The data/ listing was clamped to earliest_retained_manifest_time, which is sound only for files owned by a manifest being removed. A file owned by no manifest is removed on its age, so its mtime is unrelated to that floor and it was never listed. Take the later of the retained floor and the unverified threshold, and drop the cutoff entirely under delete_unverified. Fixes lance-format#8942
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The earlier safety-documentation finding is addressed. The data listing now advances with the seven-day verification threshold, removes that cutoff only when delete_unverified is enabled, and has focused regression coverage for both policy branches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cleanup_old_versionsclamped itsdata/listing toearliest_retained_manifest_time. That floor is only sound for a file owned by a manifest being removed; a file owned by no manifest — abandonedwrite_fragments()output — has an mtime unrelated to it.delete_unverified=truehad no effect on it.data/listing now runs through the moving unverified threshold —earliest_retained.max(verification_threshold)— and drops the cutoff entirely underdelete_unverified, where a file is a candidate at any age.CleanupInspection::listing_unmodified_sinceis unchanged, so fix: delete data files when cleanup retains an older tag #8708's sparse-retain case and the other listings behave exactly as before.now - older_than, per A tagged version permanently preventscleanup_old_versionsfrom reclaiming newer data files #8516). A tagged version permanently preventscleanup_old_versionsfrom reclaiming newer data files #8516 and cleanup_old_versions on a tagged dataset removes manifests but strands their data files (permanent, silent leak) #8705 reported the tagged facet of the same clamp and fix: delete data files when cleanup retains an older tag #8708 fixed it; this is the facet left over, reachable with no tag at all.older_than, anddelete_unverifiedremoves the age protection entirely and requires that no other process is operating on the dataset. The Python docstring already said this.cleanup_deletes_aged_orphan_newer_than_retained_manifests, covering both paths: an aged orphan under the default policy, and a fresh orphan underdelete_unverified. Both are written after every manifest, under suffix-only retention with no tag anywhere._transactions/and_deletions/are clamped the same way and can strand a file for the same reason. Left for a follow-up so each gets its own regression test rather than widening destructive discovery across four subtrees at once.Fixes #8942
Test plan
cargo fmt --all --check— cleancargo check -p lance --tests— cleancargo clippy -p lance --tests -- -D warnings— cleancargo test -p lance --lib dataset::cleanup— 46 passed, 0 failed, including both new cases and fix: delete data files when cleanup retains an older tag #8708'scleanup_deletes_data_files_newer_than_tagged_versiondata/cutoff tounmodified_sincefailscase_1_aged; removing only thedelete_unverifiedarm failscase_2_delete_unverified. Each case is gated on the branch it covers, and no other test changes outcome.