fix(io): prune empty local directories after their files are deleted

[wombatu-kun]

Closes #7937

Problem

After cleanup_old_versions removes an obsolete index, its files under _indices/<uuid>/ are deleted one at a time, but on a local filesystem the now-empty _indices/<uuid>/ directory is left behind. Cleanup deletes files only and never removes directories. On object stores (S3/GCS/Azure) directories are virtual prefixes that vanish with their last object, so this only affects local filesystems.

Fix

ObjectStore::delete and ObjectStore::remove_stream walk up from the deleted file and remove the parent directories that became empty, stopping at the first one that is not.

The gate is wider than is_local(): it also covers file-object-store, which stores real directories that have no remote equivalent.

Why not LocalFileSystem::with_automatic_cleanup

arrow has this built in and the first revision of this PR used it, but it is broken on Windows. LocalFileSystem::new() roots the store at file:///, and delete_location bounds its upward walk by resolving that root through Url::to_file_path(), which on Windows requires a drive letter and so returns Err. Every delete then removed the file and reported InvalidUrl; since RenameCommitHandler is copy + delete, 1055 of 2531 lance tests failed on windows-build.

Tests

test_delete_prunes_empty_dirs_local_store / ..._file_object_store in lance-io delete a file nested three levels deep and assert the emptied directories are gone while a non-empty sibling survives.

Two existing cleanup tests are extended to assert the directory itself is gone, not just its files, via a filesystem check the object-store exists() cannot observe: cleanup_old_replaced_segment_keeps_still_referenced_segments (flat layout; still-referenced dirs preserved) and cleanup_old_uncommitted_index_artifacts (nested layout).

All four fail without the fix. The full lance-io, lance-table and lance unit suites pass.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 08a94649-b732-4436-bbf6-875b6b19100b

📥 Commits

Reviewing files that changed from the base of the PR and between f3924aa and beda2f2.

📒 Files selected for processing (3)

• rust/lance-io/src/local.rs
• rust/lance-io/src/object_store.rs
• rust/lance/src/dataset/cleanup.rs

---

📝 Walkthrough

Walkthrough

Local filesystem-backed object stores now remove empty parent directories after direct and streamed deletions, including file-object-store. Tests cover both schemes and verify obsolete on-disk index directories are removed during dataset cleanup.

Changes

Local index cleanup

Layer / File(s)	Summary
Local deletion and cleanup behavior rust/lance-io/src/local.rs, rust/lance-io/src/object_store.rs	Adds best-effort ancestor-directory pruning and applies it to direct and streamed deletions for filesystem-backed schemes, including file-object-store.
Index directory cleanup validation rust/lance-io/src/object_store.rs, rust/lance/src/dataset/cleanup.rs	Parameterizes pruning tests across local schemes and asserts removal of replaced and uncommitted index directories.

Estimated code review effort: 2 (Simple) | ~15 minutes

Suggested reviewers: wjones127, jackye1995

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: pruning empty directories after file deletion.
Description check	✅ Passed	The description is directly related to the directory-pruning cleanup fix and its tests.
Linked Issues check	✅ Passed	The PR removes empty _indices// directories after cleanup_old_versions deletes their files, matching #7937.
Out of Scope Changes check	✅ Passed	The added file-object-store support and test updates are aligned with the stated fix and are not out of scope.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 99.00990% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
rust/lance-io/src/local.rs	93.75%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[coderabbitai]

Actionable comments posted: 1

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)

rust/lance-io/src/object_store.rs-946-954 (1)

946-954: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add regression coverage for streamed deletion.

This branch bypasses ObjectStore::delete and performs its own inner deletion plus pruning. The new tests exercise only store.delete at Line [1486], so a regression here could pass unnoticed. Add equivalent remove_stream tests for both file and file-object-store.

As per coding guidelines: “Every bugfix and feature must have corresponding tests.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-io/src/object_store.rs` around lines 946 - 954, Add regression
coverage for the streamed deletion branch in the relevant test module: add
equivalent remove_stream tests for both the file and file-object-store
configurations, exercising deletion and empty-parent-directory pruning through
the stream path rather than only ObjectStore::delete. Reuse the existing test
setup and assertions around the store.delete test near the referenced location.

Source: Coding guidelines

🧹 Nitpick comments (1)

rust/lance-io/src/local.rs (1)

60-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Do not silently discard unexpected cleanup errors.

is_err() conflates expected DirectoryNotEmpty/NotFound cases with permission and I/O failures. Handle expected stop conditions separately, but log unexpected failures with the directory path and error context before stopping.

As per coding guidelines: “Log warnings on best-effort or cleanup failures instead of silently swallowing or propagating errors.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-io/src/local.rs` around lines 60 - 63, Update the parent-directory
cleanup loop to match on the error returned by std::fs::remove_dir rather than
using is_err(). Stop quietly for expected DirectoryNotEmpty and NotFound
conditions, but log a warning containing the directory path and error context
for all other failures before breaking.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rust/lance-io/src/local.rs`:
- Around line 57-65: Update prune_empty_parent_dirs and its callers, including
ObjectStore::delete and remove_stream, to accept and propagate an explicit
store-root boundary alongside the object path. Stop pruning when the next
directory is the store root, never remove that root, and retain the existing
behavior of removing empty directories below it; add or adjust regression
coverage for an empty store root.

---

Other comments:
In `@rust/lance-io/src/object_store.rs`:
- Around line 946-954: Add regression coverage for the streamed deletion branch
in the relevant test module: add equivalent remove_stream tests for both the
file and file-object-store configurations, exercising deletion and
empty-parent-directory pruning through the stream path rather than only
ObjectStore::delete. Reuse the existing test setup and assertions around the
store.delete test near the referenced location.

---

Nitpick comments:
In `@rust/lance-io/src/local.rs`:
- Around line 60-63: Update the parent-directory cleanup loop to match on the
error returned by std::fs::remove_dir rather than using is_err(). Stop quietly
for expected DirectoryNotEmpty and NotFound conditions, but log a warning
containing the directory path and error context for all other failures before
breaking.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 93205f61-5992-4516-a099-de2b56028105

📥 Commits

Reviewing files that changed from the base of the PR and between 941785d and f3924aa.

📒 Files selected for processing (2)

• rust/lance-io/src/local.rs
• rust/lance-io/src/object_store.rs

[wombatu-kun]

Addressed the other two in beda2f2:

• remove_stream: added test_remove_stream_prunes_empty_dirs_{local_store,file_object_store}, verified they fail when the pruning call is removed.
• prune_empty_parent_dirs: now matches on ErrorKind, stops quietly on DirectoryNotEmpty/NotFound and logs a warn! with the path on anything else.