Skip to content

fix(namespace): fence stale table deregistration - #8605

Closed
jackye1995 wants to merge 31 commits into
lance-format:mainfrom
jackye1995:jack/expected-deregister-location
Closed

fix(namespace): fence stale table deregistration#8605
jackye1995 wants to merge 31 commits into
lance-format:mainfrom
jackye1995:jack/expected-deregister-location

Conversation

@jackye1995

@jackye1995 jackye1995 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Make manifest-backed deregistration conditional on the physical location observed by the caller and re-check that generation inside every manifest rewrite attempt. Expected-generation deregistration writes an out-of-prefix sibling tombstone so the old location cannot be re-registered until physical cleanup finishes.

With async_drop_enabled=true, generated table paths use UUID-sized incarnation IDs and overwrite publishes a fresh physical generation with a conditional manifest swap. The default overwrite path retains its existing version-history semantics.

Also paginate DynamoDB external-manifest deletion so asynchronous physical cleanup removes every manifest entry for the dropped table.

@github-actions github-actions Bot added A-namespace Namespace impls bug Something isn't working labels Aug 17, 2026
@jackye1995
jackye1995 marked this pull request as ready for review August 17, 2026 20:52
@jackye1995
jackye1995 marked this pull request as draft August 17, 2026 21:47
@jackye1995
jackye1995 marked this pull request as ready for review August 17, 2026 21:47
@github-actions

Copy link
Copy Markdown
Contributor

Important

This PR touches the Lance format specification.

Substantive changes to the format specification — the .proto definitions
and the spec docs under docs/src/format/ — require a PMC vote before merge.
Minor edits such as typo fixes, wording, or formatting are excluded; use your
judgment.

If this is a meaningful format change:

  • Start a vote following the Lance community voting process.
    Format specification modifications need 3 binding +1 votes (excluding the
    proposer), held on GitHub Discussions, with a minimum voting period of 1 week.
  • Once the vote passes, link the completed vote in this PR. It should not be
    merged until the vote is linked.

@github-actions github-actions Bot added the A-format On-disk format: protos and format spec docs label Aug 17, 2026

@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: request changes.

The async-drop safety contract must be catalog-wide: once durable drop fencing is enabled, every instance that can physically drop a table must use the tombstone-and-epoch transition.

Persist and enforce that mode in the manifest, or fail closed on the legacy route, so a process-local default cannot bypass fencing during rollout.

// Delete from manifest first
self.delete_from_manifest(&object_id).boxed().await?;
self.ensure_manifest_writable().await?;
let tombstone_id = if self.async_drop_enabled {

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.

This branch makes the data-loss fence process-local. After an async-enabled instance has activated the catalog, a peer opened with the default async_drop_enabled=false still takes the else path: it removes T@L without a tombstone or drop-epoch advance. Another peer can then register U@L before this method reaches remove_dir_all(L), so the cleanup deletes U's data while the catalog still points to it.

Persist the async-drop mode in the manifest and require every physical drop to honor it (or reject legacy drops after activation), so mixed rollout cannot bypass the tombstone/epoch transition.

Reproducer run against this head

I added and ran a detached unit regression that:

  1. opens two ManifestNamespaces on one root, one enabled and one disabled;
  2. creates and completes an async tombstone, leaving drop epoch 1;
  3. registers old at L;
  4. lets the disabled instance run delete_from_manifest_if_location("old", Some(L), true);
  5. registers replacement at L through the enabled instance; and
  6. runs the disabled instance's remove_dir_all(L).

cargo test -p lance-namespace-impls mixed_async_drop_modes_allow_legacy_cleanup_to_delete_reregistered_location --lib --quiet passed with assertions that replacement remains in the manifest and its data file has been deleted.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Aug 18, 2026
@jackye1995 jackye1995 closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-format On-disk format: protos and format spec docs A-namespace Namespace impls bug Something isn't working K-changes Latest Gatekeeper recommendation requests changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant