Skip to content

fix(namespace): handle legacy auto-cleanup policy on directory manifest - #9600

Merged
Xuanwo merged 2 commits into
mainfrom
xuanwo/dir-manifest-legacy-auto-cleanup
Oct 1, 2026
Merged

Xuanwo merged 2 commits into
mainfrom
xuanwo/dir-manifest-legacy-auto-cleanup

Conversation

@Xuanwo

@Xuanwo Xuanwo commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Directory __manifest datasets created before Lance 7.0 carry a default auto-cleanup policy. If an old writer removes version N+1 while a reader holds N, the successor-only reload check leaves the reader on an obsolete catalog, potentially referencing deleted data. Debug builds also panic when opening these manifests.

This change removes the assertion and resolves the latest version while the held manifest carries lance.auto_cleanup.*. Policy-free manifests retain the successor-probe fast path. Successful manifest rewrites remove the inherited policy, so subsequent writers stop automatic old-version cleanup.

The branch incorporates main, including #9285. Both the read-side probe and reload_under_write_lock() use the compatibility-aware check; get_mut() still acquires the write lock before reloading.

Validation:

  • cargo test --locked -p lance-namespace-impls --lib: 319 passed, including the fix(namespace): avoid write starvation under heavy reads #9285 fairness regression.
  • A checked-in fixture generated by released Lance 6.0.1 exercises get(), get_refreshed() and get_mut() after cleanup removes the immediate successor, then verifies refresh continues after the policy is removed. All three pass; temporarily restoring successor-only logic makes all three fail with held version 5 instead of latest version 7. The correct code was restored and the full suite rerun.
  • cargo fmt --all -- --check, git diff --check, and Ruff 0.11.2 lint/format checks for the fixture generator pass.
  • Required local cargo clippy --all --tests --benches -- -D warnings is blocked by existing single_range_in_vec_init and implicit_clone errors in lance-io. A separate namespace-only clippy run is blocked by 12 existing implicit_clone errors in unchanged code. No lint suppression or unrelated refactoring is included; fresh PR CI provides the full platform checks.

Fresh CI for 608146047: 36 checks passed and the Draft Lance Gatekeeper was skipped. This includes Rust platform tests and clippy, Python platform, compatibility and AWS tests, and Java 11/17/21/25 tests.

Manual cleanup of a policy-free __manifest remains outside this module's contiguous-version contract. This PR remains Draft.

Directory manifests created by Lance before 7.0 carry the default
lance.auto_cleanup.* config, and writers of that era remove old versions
under it. A reader that only probes the immediate successor manifest then
stops reloading once that successor is cleaned up.

Resolve the latest version while the held manifest carries the policy,
drop the policy on the next manifest rewrite, and replace the debug
assertion that rejected such manifests.
@Xuanwo Xuanwo added the bug Something isn't working label Sep 29, 2026
@github-actions github-actions Bot added the A-namespace Namespace impls label Sep 29, 2026
@Xuanwo
Xuanwo marked this pull request as ready for review October 1, 2026 05:57

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Gate recommendation: approve.

The legacy freshness failure is reproduced by the released fixture and fixed across all three access paths. The policy transition preserves the existing fast path for current catalogs. No merge-blocking issue found.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Oct 1, 2026
@Xuanwo
Xuanwo merged commit a20ad6a into main Oct 1, 2026
41 of 43 checks passed
@Xuanwo
Xuanwo deleted the xuanwo/dir-manifest-legacy-auto-cleanup branch October 1, 2026 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-namespace Namespace impls bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants