Skip to content

fix: decouple branch names from storage paths - #8403

Closed
lance-gatefixer[bot] wants to merge 19 commits into
mainfrom
gatekeeper/fix-7185-1
Closed

fix: decouple branch names from storage paths#8403
lance-gatefixer[bot] wants to merge 19 commits into
mainfrom
gatekeeper/fix-7185-1

Conversation

@lance-gatefixer

@lance-gatefixer lance-gatefixer Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Root cause

Branch lineage already had stable UUIDs, but branch datasets and deletion still derived physical storage paths from mutable logical names. Earlier revisions also mixed logical names and UUIDs in one path namespace, lost physical generation identity across managed namespace requests, allowed durable tags to outlive the branch metadata they require, and split reference state between an authoritative version tree and a flat discovery mirror.

Fix

  • Persist an explicit detached storage layout and generation in new branch metadata.
  • Store detached generations under _branch_generations/<generation>, disjoint from logical tree/<branch-name> aliases.
  • Resolve logical aliases only when the suffix is a valid branch name and branch metadata verifies it; otherwise paths such as /tree/main remain ordinary main dataset roots.
  • Carry the authoritative physical branch base through directory and REST namespace requests, including bootstrap commits before metadata publication.
  • Keep legacy name-backed metadata readable and preserve its deletion conflict while descendants reference it.
  • Block branch deletion while a durable tag refers to that logical branch, preserving the metadata and generation required to resolve the tag.
  • Serialize reference mutations with reclaimable fenced lease epochs and resumable operation intent.
  • Revalidate renewed lease ownership before protected work receives its first poll.
  • Publish tags and branches through one epoch-qualified authoritative catalog, so point reads and enumeration observe the same atomic snapshot.
  • Preserve legacy flat references as a durable migration baseline and reconcile any later released-writer change instead of silently ignoring it.
  • Keep post-commit cleanup best-effort so an authoritative mutation never reports cleanup failure.
  • Retain a reader-safe predecessor catalog, retry compaction races, and compact older snapshots to bound history.
  • Reclaim detached generations by lineage reachability and document the revised layout.

Validation

  • cargo fmt --all -- --check
  • cargo check -p lance --tests
  • cargo clippy --all --tests --benches -- -D warnings
  • full dataset::refs::tests module (74 passed)
  • focused committed-cleanup, legacy-writer, expired-lease, and compaction-race regressions
  • catalog publication, predecessor reconciliation, and 100-update compaction regressions
  • test_branch
  • test_tag

Fixes #7185

@github-actions

github-actions Bot commented Aug 7, 2026

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 A-format On-disk format: protos and format spec docs bug Something isn't working labels Aug 7, 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 UUID-backed generation is the right root-cause direction, but this revision does not yet keep logical aliases, physical generations, and namespace commits under one unambiguous identity. A viable revision should persist an explicit layout/generation mapping, resolve public aliases without guessing from path text, route namespace reads and writes through that physical generation (including bootstrap), and retain enough reachability information to reclaim legacy parents safely.

Please mark this PR with the breaking-change label.

impl ExternalManifestStore for LanceNamespaceExternalManifestStore {
fn register_branch_path(&self, base_path: &Path, branch: Option<&str>) {
if let Ok(mut branch_paths) = self.branch_paths.write() {
branch_paths.insert(base_path.to_string(), branch.map(ToString::to_string));

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 map loses the physical generation at the namespace boundary: subsequent namespace requests carry only the logical branch name. DirectoryNamespace::resolve_branch_for_commit still derives tree/<logical-name>, while the loaded dataset is rooted at tree/<uuid>; managed appends therefore finalize and read back different chains. The namespace contract needs to carry the authoritative generation/physical root, including the bootstrap commit before branch metadata is published.

Reproducer

cargo test -p lance-namespace-impls test_managed_branch_open_and_commit -- --nocapture first fails its old tree/exp path assertion. After changing only that assertion to accept the UUID path, the same test reaches branch_ds.append(...) and returns CommitStatusUnknownError { version: 3, ... }.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 43f13ad. Namespace manifest requests now carry the authoritative physical branch base in request context, and directory/REST routing validates and uses it for reads, writes, and bootstrap commits before metadata publication.

// moved under an opaque UUID. Physical UUID URLs retain their path while recovering the
// logical branch name from metadata when one exists.
if let Some((root_path, path_branch)) =
BranchLocation::split_branch_path(base_path.as_ref())

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.

Every URI containing /tree/<suffix> is treated as a branch alias here, even when tree is an ordinary component of the dataset root. For a managed namespace this registers the main table path as branch <suffix>, so main-version operations are sent to a nonexistent branch. Alias resolution needs an explicit branch selector or verified branch metadata; path text alone is ambiguous.

Reproducer

I added a focused DirectoryNamespace test whose namespace root was temp/tree/catalog, then called the existing create_managed_table helper. Dataset creation failed with CommitStatusUnknownError { version: 1, source: "dataset creation reported a conflict but the manifest could not be read back" } instead of creating main.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 43f13ad. Logical tree aliases are rewritten only when branch metadata verifies the alias; a regression now covers a managed main-table root whose ordinary path contains /tree/.

}

pub(crate) async fn resolve_path_location(&self, path_branch: &str) -> Result<BranchLocation> {
match self.get(path_branch).await {

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.

Logical names and physical UUIDs occupy the same tree/<value> namespace, and this lookup gives a matching logical name precedence. If branch first has storage ID U and a user then creates a logical branch named U (currently valid), reopening first.uri() silently opens the second branch. Use a disjoint physical namespace/encoding, or otherwise make logical and physical addressing unambiguous.

Reproducer

A focused test created first, read U = first.identifier.storage_id(), created another branch named U, and reopened the physical URI returned for first. The observed manifest.branch was Some(U); the expected value was Some("first").

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 43f13ad. Detached generations now use _branch_generations/, disjoint from logical tree/ aliases; the collision regression verifies a UUID-shaped logical branch cannot redirect a physical URI.

if referenced_versions.is_empty() {
self.cleanup_branch_directories(branch).await?;
}
return Ok(());

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.

A referenced legacy branch reaches this return after its metadata is deleted but its name-backed directory is retained. The metadata-missing fallback then keeps the deleted logical branch readable, and deleting the last child later treats lineage UUIDs as physical directories, so it never reclaims tree/<legacy-name>. Either keep the legacy delete conflict, or persist an explicit layout/tombstone mapping until reachability drops to zero.

Reproducer

I emulated the supported legacy layout by creating parent, moving its physical directory to tree/parent, creating child from it, then deleting parent followed by child. After parent deletion, checkout_branch("parent") still succeeded; after child deletion, tree/parent still existed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 43f13ad. Metadata without an explicit storage mapping remains legacy name-backed, and deleting a referenced legacy branch now preserves the RefConflict until descendants are removed; the lifecycle regression also verifies final directory reclamation.

@github-actions github-actions Bot added the A-namespace Namespace impls label Aug 7, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 43f13ad. New branch metadata now persists an explicit detached layout/generation; aliases are metadata-verified; managed namespace requests retain the physical base through bootstrap; detached cleanup follows lineage reachability; and referenced legacy layouts retain their deletion conflict. The requested breaking-change label is also applied.

@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 detached-generation mapping now fixes the earlier namespace, UUID-collision, and legacy-parent failures, but two durable-reference paths still bypass the intended identity boundary. Alias probing must treat non-branch-shaped /tree/ suffixes as ordinary dataset roots, and every long-lived ref must either carry the physical generation or prevent deleting the branch metadata it needs.

match self.get(path_branch).await {
Ok(_) => self.resolve_location(Some(path_branch)).await.map(Some),
Err(Error::RefNotFound { .. }) => Ok(None),
Err(error) => Err(error),

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 propagates branch-name validation errors from a path probe, so an ordinary dataset rooted at /tree/main (or any suffix that is legal in a storage path but not a branch name) is rejected before its own manifest is loaded. Because invalid branch text cannot identify branch metadata, this probe should treat it as None instead of failing the dataset open.

Reproducer

I added a focused test that wrote a dataset at a neutral local path, moved the complete dataset root to <tmp>/tree/main, and called Dataset::open on the new path. cargo test -p lance reproduce_tree_main_root_is_rejected -- --nocapture passed while asserting the observed Error::InvalidRef; the expected behavior is to open that root as main.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in b58c962. Invalid branch-shaped path probes now return no alias, so ordinary dataset roots such as /tree/main load as main; the focused regression covers this path.


// A deleted branch may still provide files to descendants. UUID-backed directories are
// reclaimed only after their identifiers disappear from every remaining lineage.
let remaining_branches = self.list().await?;

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.

Generation reachability is collected only from remaining branch metadata, but TagContents still records just the logical branch name and version. Deleting a detached branch therefore removes the metadata/generation needed by a tag, and reusing that branch name can redirect the tag to a different generation. Either persist the generation identity in tags and include it in reachability, or keep a deletion conflict while any tag refers to the branch.

Reproducer

I added a focused test that created branch dev, created tag snapshot for dev@1, deleted dev, and checked out snapshot. cargo test -p lance reproduce_tag_breaks_when_branch_is_deleted -- --nocapture passed while asserting the observed Error::DatasetNotFound; a durable tag should still resolve its tagged version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in b58c962. Branch deletion now returns RefConflict while any durable tag refers to that branch, preserving the metadata and generation needed to resolve the tag; the branch regression covers the conflict and retained tag.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in b58c962. Alias probes now leave invalid branch suffixes as ordinary dataset roots, and branch deletion is blocked while a durable tag depends on its metadata and detached generation. Focused regressions cover both cases.

@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.

Branch deletion still does not preserve durable tag references when ref operations overlap. The new guard checks only a snapshot, so this revision can publish a tag that immediately points to removed storage.

A viable revision must serialize tag creation and update with branch deletion, or persist immutable generation identity in tags and defer generation reclamation while any tag references it.

let mut referencing_tags = self
.refs
.tags()
.fetch_tags()

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 check is only a snapshot, so a tag created or retargeted concurrently can be published after deletion has passed the check. Both operations can succeed, leaving the tag pointing at branch metadata and generation storage that deletion just removed.

Ref mutation needs a common serialization or fencing boundary. Alternatively, tags can persist immutable generation identity and generation reclamation can honor those identities, which removes the dependency on mutable branch metadata.

Reproducer

I ran this regression against b58c962c3c5a58caf1b9aef14510522da0dec0eb with a fresh one-row dataset:

let mut dataset = Dataset::write(reader, &uri, None).await.unwrap();
dataset.create_branch("race", 1, None).await.unwrap();

let barrier = Arc::new(tokio::sync::Barrier::new(3));
let tag_dataset = Dataset::open(&uri).await.unwrap();
let mut delete_dataset = Dataset::open(&uri).await.unwrap();

let create_task = tokio::spawn({
    let barrier = barrier.clone();
    async move {
        barrier.wait().await;
        tag_dataset.tags().create("race-tag", ("race", 1)).await
    }
});
let delete_task = tokio::spawn({
    let barrier = barrier.clone();
    async move {
        barrier.wait().await;
        delete_dataset.delete_branch("race").await
    }
});
barrier.wait().await;

assert!(create_task.await.unwrap().is_ok());
assert!(delete_task.await.unwrap().is_err());
dataset.checkout_version("race-tag").await.unwrap();

Expected branch deletion to return RefConflict and tag checkout to remain readable. On the first iteration, both mutations returned Ok(()); checkout then failed with NotFound for tree/race/_versions/1.manifest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 90d6ec5. Tag mutations now publish durable intents and share an atomic object-store mutation lock with branch deletion; deletion yields to in-flight tag mutations and rechecks after locking. The concurrent regression verifies tag creation succeeds, deletion returns RefConflict, and tag checkout remains readable.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 90d6ec5. Reference mutations now use a shared durable serialization boundary, with tag intents giving concurrent tag creation or retargeting priority over branch deletion. The supplied concurrent regression passes and preserves readable tag checkout.

@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 new serialization prevents the observed tag/delete interleaving, but it is not recoverable and does not cover the full branch-creation transaction. A crashed ref writer can permanently wedge later mutations, and force deletion can remove an in-flight generation before its metadata becomes visible.

The coordination protocol needs fenced, reclaimable leases or resumable intents, and branch creation must participate in the same deletion boundary or atomically revalidate and repair before publishing metadata.

Comment thread rust/lance/src/dataset/refs.rs Outdated
#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
struct RefMutationLockState {
owner: String,

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.

The lock state has no expiry or recovery protocol, and tag intents are empty objects with the same unbounded lifetime. If the process dies after either create succeeds—or a successful PUT is reported ambiguously—cleanup cannot run. _refs/mutation.json then makes every later ref mutation time out forever, while one _refs/tag_mutations/*.json makes every branch deletion time out forever. Use a durable expiry and fencing epoch with conditional takeover/release, or a resumable operation record whose recovery is safe after crashes.

Reproducer

On this head I added and ran a #[tokio::test(start_paused = true)] that inserted a stale _refs/mutation.json, then invoked two tag creations while advancing time past the 30-second timeout. Both returned RefConflict, and the lock still existed. The same test inserted a stale _refs/tag_mutations/*.json and ran two branch deletions while advancing past the timeout. Both returned RefConflict, and the intent still existed; retry never recovered either artifact.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 02b5d7a. Reference coordination now uses renewable append-only lease epochs with atomic-create fencing, expiry, heartbeats, and release markers; stale legacy locks and tag intents no longer block retries. The crash-recovery regression covers both record types and repeated mutations.

Comment thread rust/lance/src/dataset.rs Outdated
self.branches()
.create(branch, version_number, source_branch.as_deref())
.await?;
self.branches().create(branch, branch_contents).await?;

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.

Branch metadata is published only here, after builder.execute has physically created the generation. During that interval, concurrent force_delete_branch sees no BranchContents, treats the generation as a zombie, and can delete it before this call publishes metadata pointing to the missing storage. Publish a creation intent before generation work and include it in deletion reachability, or atomically revalidate the generation's existence and repair it before publishing metadata.

Reproducer

On this head I ran a phase-equivalent test: create a branch and save its BranchContents, remove the metadata to model a pause after the physical clone, force-delete the now-zombie generation, then republish the saved metadata through Branches::create. The publish succeeded, but checkout of the branch failed because its generation had been removed.

@lance-gatefixer lance-gatefixer Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 02b5d7a. Branch preparation, physical cloning, and metadata publication now execute under one renewable fenced reference lease, so force cleanup cannot reclaim an in-flight generation. The regression verifies no generation is written before the creation transaction owns the deletion boundary.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 02b5d7a. Reference mutations now use reclaimable fenced lease epochs with expiry and heartbeats, and branch creation holds that boundary from preparation through physical cloning and metadata publication. Focused regressions cover stale coordination recovery and the creation/deletion phase gap.

@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 stale-file recovery and ordinary branch create/delete boundary are now covered, but the lease epoch is not enforced at the writes it protects. A delayed heartbeat or reference publication can complete after takeover, letting the expired owner continue alongside its successor.

A viable revision must either bind the current epoch to every visible reference publication or persist resumable operation state that a successor reconciles before deleting or publishing dependent storage.

..Default::default()
},
)
.await?;

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.

The fence is checked only before this awaited heartbeat PUT. If that PUT stalls past expiry, a successor can create epoch N+1, then epoch N's PUT can complete; this method records the renewed state and returns Ok, so drive_with_lease resumes the expired mutation concurrently with its successor. The tag and BranchContents writes are likewise not conditioned on the current epoch, so a post-heartbeat check alone would not prevent a delayed or ambiguously successful protected write from landing after successor cleanup. Bind the epoch to visible publication, or retain operation state that the successor must reconcile before proceeding.

Reproducer

On this head I added test_delayed_heartbeat_cannot_revive_fenced_epoch with a proxy store. It created epoch 1, blocked its heartbeat PUT after the latest_lease_epoch check, made epoch 1 visible as expired to the other client, created epoch 2, and then released the PUT. Running cargo test -p lance test_delayed_heartbeat_cannot_revive_fenced_epoch -- --nocapture failed with an expired owner renewed successfully after epoch 2 acquired the fence: renewal returned Ok instead of rejecting the lost fence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 0a9239f. Heartbeat renewal now rechecks both epoch ownership and expiry after the awaited write. Tag and branch publications first persist resumable intent, then append an immutable epoch-qualified record; successors reconcile prior intent before proceeding, and readers ignore delayed lower epochs. Regressions cover delayed heartbeat takeover, successor reconciliation, and stale publication after update or deletion.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 0a9239f. Visible tag and branch states are now immutable epoch-qualified records. Each owner persists operation intent before publication, and a successor reconciles unreconciled earlier intent before dependent cleanup or publication. Heartbeat renewal also rechecks the fence after its awaited write. Focused regressions cover delayed takeover, predecessor recovery, and stale writes after newer updates or tombstones.

@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 fenced epoch records protect current point reads, but this revision still splits one reference state across an authoritative version tree and an unfenced compatibility/discovery mirror. That leaves three independent failures under crash, mixed-version, and sustained-churn conditions.

A viable revision should give point reads and enumeration one atomically discoverable authoritative namespace, define an earlier-reader transition that cannot expose tombstones or stale lower epochs, and bound immutable history with safe compaction or a current-state index.

Comment thread rust/lance/src/dataset/refs.rs Outdated
// mirror, so a delayed lower-epoch mirror cannot supersede a newer reference for current
// readers.
let version_path = ref_version_path(&path, self.epoch)?;
create_immutable_body(object_store, &version_path, self.body.as_bytes()).await?;

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.

Publishing the authoritative object before the only name-discovery entry makes a successful point read invisible to list() after an error or crash between these awaits. Both tag and branch enumeration discover names only from the flat *.json files, while read_stored_ref prefers _versions; successor reconciliation cannot repair read-only clients. The name index must be authoritative and committed with the state, or enumeration must discover names from the authoritative namespace.

Reproducer

I added a focused test that wrote only the first, authoritative step, verified read_stored_ref returned live contents, then checked the directory used by Tags::list:

create_immutable_body(
    &object_store,
    &ref_version_path(&tag_path(&root, "pending"), 1).unwrap(),
    serialize_ref_file(Some(&contents), 1).unwrap().as_bytes(),
)
.await
.unwrap();

assert!(
    read_stored_ref::<TagContents>(&tag_path(&root, "pending"), &object_store)
        .await
        .unwrap()
        .and_then(|stored| stored.contents)
        .is_some()
);
assert!(
    object_store
        .read_dir(base_tags_path(&root))
        .await
        .unwrap()
        .iter()
        .any(|name| name == "pending.json")
);

cargo test -p lance test_partial_publication_is_discoverable_by_list -- --nocapture failed at the final assertion: get sees the authoritative version, but list cannot discover its name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5f0bd50. Point reads and enumeration now derive from the same epoch-qualified authoritative catalog snapshot, so catalog publication exposes names and contents together. The partial-publication regression covers this discoverability boundary.

Comment thread rust/lance/src/dataset/refs.rs Outdated
&path,
Bytes::copy_from_slice(self.body.as_bytes()).into(),
PutOptions {
mode: PutMode::Overwrite,

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 unconditional overwrite is not a compatibility mirror. Deletion writes {_mutationEpoch, _deleted} to the flat file instead of removing it, so earlier readers cannot deserialize the required tag/branch fields; a delayed lower epoch can then overwrite a newer deletion with stale live JSON. Current readers ignore that stale mirror, but the documented earlier-reader contract does not. Preserve the old flat semantics with epoch-conditioned update/delete, or define an explicit migration boundary instead of exposing this file to earlier readers.

Reproducer

I applied a live epoch followed by a deletion epoch and asserted the flat tag path had the behavior expected by the pre-change reader:

DurableRefPublication {
    epoch: 1,
    path: path.to_string(),
    body: serialize_ref_file(Some(&contents), 1).unwrap(),
}
.apply(&object_store)
.await
.unwrap();
DurableRefPublication {
    epoch: 2,
    path: path.to_string(),
    body: serialize_ref_file::<TagContents>(None, 2).unwrap(),
}
.apply(&object_store)
.await
.unwrap();

assert!(!object_store.exists(&path).await.unwrap());

cargo test -p lance test_flat_delete_mirror_is_legacy_compatible -- --nocapture failed with legacy readers expect deletion to remove the flat tag file; the path instead contained the new tombstone schema.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5f0bd50. The first catalog publication imports live legacy flat references and removes the flat files before establishing the catalog as the explicit migration boundary, so no tombstone or stale lower-epoch mirror remains exposed.

Comment thread rust/lance/src/dataset/refs.rs Outdated
async fn latest_ref_version_path(object_store: &ObjectStore, path: &Path) -> Result<Option<Path>> {
let versions_path = ref_versions_path(path)?;
Ok(object_store
.read_dir(versions_path.clone())

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.

Every point read lists the complete immutable history here, but no code reclaims _refs/{tags,branches}/_versions, and deleted names retain flat tombstones permanently. Each mutation therefore adds durable storage and per-read listing work without a bound; each deleted name also adds permanent enumeration work. Use a bounded current-state pointer/index with fenced replacement and safe compaction, or define and implement an explicit retention/GC contract.

Reproducer

A focused churn probe applied 100 updates to one tag, then counted the authoritative objects:

for epoch in 1..=100 {
    DurableRefPublication {
        epoch,
        path: path.to_string(),
        body: serialize_ref_file(
            Some(&TagContents {
                branch: None,
                version: epoch,
                created_at: None,
                updated_at: None,
                manifest_size: 1,
                metadata: HashMap::new(),
            }),
            epoch,
        )
        .unwrap(),
    }
    .apply(&object_store)
    .await
    .unwrap();
}
assert_eq!(
    object_store
        .read_dir(ref_versions_path(&path).unwrap())
        .await
        .unwrap()
        .len(),
    100
);

cargo test -p lance test_reference_history_growth_probe -- --nocapture passed while confirming all 100 versions remain; source inspection shows this function enumerates all 100 again for the next point read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5f0bd50. Each catalog publication compacts superseded snapshots after the new snapshot is visible. The 100-update regression verifies that one authoritative catalog snapshot remains instead of accumulating unbounded per-reference history.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 5f0bd50. Tags and branches now use one atomically discoverable authoritative catalog for point reads and enumeration; first publication establishes an explicit migration boundary by importing and removing legacy flat refs; and post-publication compaction bounds retained catalog history. Focused regressions cover all three cases.

@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 catalog fixes the prior split between point reads and enumeration, but this protocol still violates four independent durability contracts: lease fencing, snapshot availability, mixed-version migration, and commit-result atomicity.

A viable revision should revalidate the lease before protected work begins, retain a reader-safe catalog generation, require an explicit quiescent/version-fenced migration, and keep cleanup outside the mutation's committed success result.

Comment thread rust/lance/src/dataset/refs.rs Outdated
// A complete catalog snapshot is the atomically discoverable state for point reads and
// enumeration. Delayed lower epochs are harmless because readers select the greatest
// epoch, and the next successful publication compacts them.
create_immutable_body(object_store, &path, self.body.as_bytes()).await?;

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.

After this PUT succeeds, the catalog is authoritative, but the next fallible cleanup can still make apply return an error. A caller can therefore retry a mutation that is already committed and visible. Move fallible preparation before this linearization point, or make post-commit cleanup best-effort/resumable so a committed mutation never reports failure.

Reproducer

I used a fault-injection store that fails legacy deletes, applied a catalog containing tag release, asserted that apply returned Err, and then asserted that release was not authoritative. cargo test -p lance test_catalog_cleanup_failure_does_not_fail_committed_mutation -- --nocapture failed at the visibility assertion with a failed publication became authoritative before cleanup returned its error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 070c46a. Catalog creation remains the authoritative linearization point; legacy deletion was removed and all later compaction is best-effort with warning-only failure. The fault-injection regression verifies a committed catalog returns success and remains readable when cleanup fails.

Comment thread rust/lance/src/dataset/refs.rs Outdated
// enumeration. Delayed lower epochs are harmless because readers select the greatest
// epoch, and the next successful publication compacts them.
create_immutable_body(object_store, &path, self.body.as_bytes()).await?;
remove_legacy_ref_files(object_store, &root).await?;

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.

Deleting the flat refs here does not fence released writers. After the first catalog exists, an older writer can still successfully PUT flat JSON, while current readers ignore that acknowledged update forever. This is durable-data loss during rolling upgrade or rollback, independent of the breaking-change label. Require an explicit quiescent migration plus a feature/version fence across every old writer entry point (or migrate to a new dataset URI) before removing legacy state.

Reproducer

I published catalog tag release at version 1, let a simulated legacy writer successfully PUT version 99 to the flat tag path, and then read through the current implementation. cargo test -p lance test_legacy_writer_is_not_ignored_after_catalog_migration -- --nocapture failed with a successful legacy write must not be silently ignored: the reader returned 1 instead of 99.

@lance-gatefixer lance-gatefixer Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 070c46a. Migration no longer deletes released-format refs: each catalog persists the observed flat state as its legacy baseline, and current reads reconcile any later legacy create, update, or delete before returning. The reproduced version-99 legacy write is now returned; the format contract explicitly requires older clients to be quiesced before catalog mutations.

// detached cleanup task can otherwise remove an epoch directory while the next
// mutation is walking it, which makes the local object-store listing fail with
// AccessDenied.
cleanup_old_lease_epochs(&cleanup_object_store, &cleanup_path, next_epoch).await;

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.

The acquired lease can expire during this synchronous cleanup, but drive_with_lease starts polling the protected mutation before its first renewal. An expired force-delete can therefore overlap a successor's branch creation and remove its in-flight generation. Renew throughout or immediately after cleanup, and revalidate before the first mutation poll.

Reproducer

I delayed cleanup until the lease expired and used a protected future that records its first poll. cargo test -p lance test_expired_lease_does_not_start_mutation -- --nocapture failed with an expired lease must be rejected before polling its mutation: the expired owner started the mutation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 070c46a. The lease driver now renews and revalidates the acquired lease before pinning or polling protected work. The regression uses an expired owner and verifies the mutation future is never polled.

continue;
}
let path = catalog_path.clone().join(file_name);
if let Err(error) = object_store.delete(&path).await

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.

Immediately deleting every lower catalog can invalidate a reader between its listing and GET. After three such races, read_latest_ref_catalog returns None; read_ref_catalog can repeat the race and then fall back to empty legacy state even though a live catalog always existed. Retain a reader-safe predecessor or keep restarting until a listed catalog is fetched instead of translating a raced NotFound into absence.

Reproducer

I delayed each catalog GET while successive publications advanced through epochs 2–7 and compacted the object the reader had just listed. cargo test -p lance test_catalog_reader_survives_compaction_churn -- --nocapture failed after observing None where the current catalog contained version 7.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 070c46a. Compaction now retains the immediate predecessor, and readers restart discovery whenever a listed catalog disappears instead of translating the race into absence. The regression injects four consecutive raced GETs and still reads epoch 7.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 070c46a. Protected work now begins only after lease renewal/revalidation; catalog readers retain a predecessor and restart raced discovery; legacy flat state is preserved as a reconciliation baseline; and post-commit cleanup is warning-only. Focused regressions cover each durability contract.

@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 submitted fixes close the previously reproduced narrow cases, but the reference transition still lacks an end-to-end linearization contract across migration and cleanup.

A viable revision needs an enforceable one-way migration boundary (or a revisioned protocol shared by old and new writers), and every action after authoritative catalog publication must preserve the committed API outcome.

}
for (name, value) in legacy {
if baseline.get(name) != Some(value) {
current.insert(name.clone(), value.clone());

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 value-diff gives legacy state precedence whenever it differs from the saved baseline, regardless of actual commit order. A released-format PUT that completes before a later catalog publication therefore overwrites that newer catalog value on the next read. _legacyBaseline detects a change but cannot order the two protocols, so it is not the documented quiescence fence. Enforce a one-way migration/version boundary before creating catalogs, or give both protocols a shared monotonic revision that can resolve order.

Reproducer

I seeded flat tag release at version 1, let the current writer snapshot that baseline and prepare catalog version 2, completed a released-format PUT of version 99, and only then applied the catalog at epoch 1. cargo test -p lance test_later_catalog_commit_wins_over_earlier_legacy_write -- --nocapture failed with left: 99, right: 2: the earlier legacy write won over the later catalog commit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in db5858f. Catalog publication now snapshots live legacy references immediately before atomic creation, so completed earlier flat writes become the baseline and the later catalog wins; flat writes completed afterward remain reconcilable. The supplied ordering regression now passes.

Comment thread rust/lance/src/dataset/refs.rs Outdated
match lease.release().await {
Ok(()) => mutation_result,
Err(release_error) => match mutation_result {
Ok(_) => Err(release_error),

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.

A successful mutation may already have published its authoritative catalog, but a release-marker failure is converted here into caller-visible Err. Retrying an acknowledged create/delete can then observe already exists/not found, while the original operation is already visible. Treat release and other post-commit cleanup as best-effort/resumable, or return an explicit committed/unknown outcome; apply that boundary consistently to tag-intent and branch-storage cleanup too.

Reproducer

I published catalog epoch 1, configured FailingProxyStore to fail the subsequent released.json PUT, and passed the successful mutation result through finish_mutation. cargo test -p lance test_release_failure_does_not_fail_committed_mutation -- --nocapture failed with the committed catalog at epoch 1 was reported as a failed mutation and the injected I/O error, while epoch 1 remained readable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in db5858f. Lease and tag-intent release, post-publication fence checks, and committed branch-storage cleanup are now best-effort after catalog publication, preserving the committed API result. The supplied release-failure regression now passes.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in db5858f. Catalog publication now establishes the legacy ordering boundary at atomic create, and every fallible action after authoritative publication preserves the committed mutation result. Focused regressions cover both reported failures.

@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 detached-generation and catalog changes now close the previously reproduced ordering and post-commit cleanup failures, but the mutation protocol can still report failure after recovery has committed the operation, and point reads now scale linearly in serialized object-store requests with the full migrated ref set.

A viable revision should make durable intent replay and the original caller share one committed/aborted outcome, while keeping single-ref lookups bounded by reconciling only the addressed legacy entry.

Comment thread rust/lance/src/dataset/refs.rs Outdated
};
let intent_path = self.path.clone().join(LEASE_PUBLICATION_FILE);
create_serialized_file(&self.object_store, &intent_path, &publication).await?;
self.ensure_current().await?;

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.

Once publication.json exists, a successor treats it as commit-ready and calls apply during reconciliation. If this lease expires between writing the intent and this check, the successor commits this exact catalog, but the original writer resumes here and returns RefConflict. Retrying a create/delete can then observe already exists/not found even though the first operation is durable.

On fence loss, read back the exact catalog/intent and return success when it was committed, or distinguish prepared from commit-ready intents so successor replay cannot commit an operation the original caller reports as failed.

Reproducer

I extended the existing test_successor_reconciles_prior_reference_publication to retain the epoch-1 fence, let epoch 2 reconcile and commit epoch 1's intent, and then require the original path not to fail:

let first_fence = DurableLeaseFence {
    object_store: object_store.clone(),
    path: first_path,
    fence_path: fence_path.clone(),
    epoch: 1,
};
// Existing epoch-2 reconciliation commits the epoch-1 publication.
first_fence
    .ensure_current()
    .await
    .expect("a successor-committed publication must not report failure");

cargo test -p lance test_successor_reconciles_prior_reference_publication failed with RefConflict { message: "reference mutation lease epoch 1 lost its fence" }; the existing final assertion still showed the reconciled tag at version 7.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 3721f58. Commit-ready publications now adopt an exact catalog already applied by a successor; if fencing is lost before replay finishes, the original path completes the same durable intent instead of returning RefConflict. The extended successor-reconciliation regression passes.

base_tags_path(base_path).join(format!("{}.json", branch))
async fn read_ref_catalog(object_store: &ObjectStore, root: &Path) -> Result<RefCatalog> {
let latest = read_latest_ref_catalog(object_store, root).await?;
let legacy = read_legacy_ref_state(object_store, root).await?;

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.

Every call here scans both legacy directories and awaits one object GET per file in read_legacy_ref_entries. This includes Tags::get and Branches::get through read_stored_ref, so one point lookup now has O(all migrated refs) serialized remote round trips in addition to downloading the complete catalog. On cloud storage, even a modest legacy ref set can turn a point lookup into seconds of latency and proportional request cost.

Keep point reads bounded by reconciling only the addressed catalog entry against its _legacyBaseline value and one live flat-file path. Reserve the complete scan for enumeration/publication, using bounded parallel reads there if necessary.

Reproducer

I instrumented RefTestStore::get_opts with an AtomicUsize, seeded 100 legacy tag files through an unwrapped writer, published catalog epoch 1, then called read_stored_ref once through the instrumented reader. cargo test -p lance disposable_test_point_read_request_count recorded and passed assert_eq!(get_count, 101): 100 flat-file GETs plus one catalog GET for one tag lookup. A bounded keyed lookup should remain O(1) as the ref set grows.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 3721f58. Point reads now fetch only the addressed flat ref and reconcile it against the addressed _legacyBaseline value; full legacy scans remain confined to enumeration and publication. The 100-ref regression verifies one catalog GET plus one flat-file GET.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 3721f58. Durable intent replay and original callers now converge on the same committed publication, and point reads reconcile only the addressed legacy entry. The focused regressions, all 77 refs tests, formatting, and workspace clippy pass.

@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 keyed point-read change resolves the request-amplification finding, and successor adoption handles intents that were visible before takeover. The recovery boundary still lets an expired writer publish a lower catalog after its successor has already marked that epoch reconciled, so the caller can succeed without its change becoming authoritative.

A viable revision must close each expired epoch before advancing reconciliation: either make late intent creation impossible, or retry it under a fresh epoch. A fenced lower epoch should return success only when the authoritative state is proven to contain its change.

Comment thread rust/lance/src/dataset/refs.rs Outdated
);
return Ok(());
}
// publication.json is the durable commit decision. A successor will replay it,

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 assumption fails when a successor finishes reconciliation before this publication.json appears. The successor can record through_epoch = 1 and commit catalog epoch 2; a late epoch-1 writer then enters this branch, creates catalog epoch 1, and returns Ok. Readers keep selecting epoch 2, and later successors skip epoch 1 because it was already marked reconciled, so the acknowledged mutation is invisible.

Close the expired epoch so no intent can appear after its reconciliation marker, or reacquire a fresh epoch and rebuild the mutation against current state. Do not return success for a lower publication merely because its immutable catalog object was created.

Reproducer

I added a test that creates an expired epoch-1 fence, creates epoch 2 with LeaseReconciliationState { through_epoch: 1 }, commits catalog epoch 2, and only then writes the epoch-1 publication intent. It checks that the old call either fails or its tag is visible:

let result = first_fence
    .apply_committed_publication(&late_publication)
    .await;
let is_visible = read_stored_ref::<TagContents>(
    &tag_path(&root, "late"),
    &object_store,
)
.await
.unwrap()
.is_some();
assert!(result.is_err() || is_visible);

cargo test -p lance disposable_test_late_lower_publication_is_not_success failed with a fenced lower-epoch publication returned success while the higher catalog hid it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 86c6202. Each lease epoch now atomically records exactly one immutable decision: publish or closed. Successors close absent intents before advancing reconciliation, and cleanup preserves that decision, so late lower writers receive RefConflict while earlier publish decisions are replayed. The regression covers closure, cleanup, a higher catalog, and a late lower publication.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 86c6202. An atomic close-vs-publish decision now closes every expired epoch before reconciliation advances and remains durable after lease cleanup, preventing invisible late lower publications while preserving successor replay. Both race regressions, all 78 refs tests, formatting, and workspace clippy pass.

@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 immutable decision closes the reproduced late-publication race, but this revision still leaves the reference mutation protocol unbounded and permits an already-committed operation to be reported as failed.

A viable revision must keep decision arbitration durable while making normal lease discovery and cleanup independent of the full mutation history, and determine success from the exact committed catalog before returning a recoverable coordination-read error.

Comment thread rust/lance/src/dataset/refs.rs Outdated
let path = base_path.clone().join(entry);
// Keep the immutable decision forever: removing it would reopen the epoch and let a
// suspended writer publish after a successor had already advanced reconciliation.
for file_name in [LEASE_FILE, LEASE_RELEASED_FILE, LEASE_RECONCILED_FILE] {

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.

Retaining each decision in the lease prefix makes every acquisition revisit the entire mutation history. This function lists every epoch and then performs three deletes plus a recursive heartbeat listing for every prior epoch; because decision.json keeps each directory alive, the same work repeats on every later mutation. It runs after creating the new 30-second lease but before drive_with_lease performs its first renewal, so once these sequential object-store operations exceed 30 seconds, renewal rejects the expired lease and no future reference mutation reaches its protected operation. latest_lease_epoch and reconciliation also enumerate this unbounded prefix.

Keep the immutable per-epoch arbitration, but separate it from a bounded active-lease discovery/cleanup path and ensure each superseded epoch's auxiliary state is cleaned at most once. A scale/latency regression should prove acquisition can renew without traversing all historical decisions.

Reproducer run against this head

Added this temporary test beside the existing refs tests:

#[tokio::test]
async fn cleanup_should_be_bounded() {
    use lance_core::utils::testing::{
        CountingObjectStore, ProxyObjectStore, ProxyObjectStorePolicy,
    };

    let listings = Arc::new(AtomicUsize::new(0));
    let deletes = Arc::new(AtomicUsize::new(0));
    let mut policy = ProxyObjectStorePolicy::new();
    let observed_deletes = deletes.clone();
    policy.set_before_policy(
        "count_deletes",
        Arc::new(move |method, _| {
            if method == "delete" {
                observed_deletes.fetch_add(1, AtomicOrdering::SeqCst);
            }
            Ok(())
        }),
    );
    let target: Arc<dyn object_store::ObjectStore> =
        Arc::new(object_store::memory::InMemory::new());
    let proxy: Arc<dyn object_store::ObjectStore> = Arc::new(ProxyObjectStore::new(
        target,
        Arc::new(std::sync::Mutex::new(policy)),
    ));
    let mut object_store = ObjectStore::memory();
    object_store.inner =
        Arc::new(CountingObjectStore::new(proxy, listings.clone()));
    let fence_path = Path::from("dataset/_refs/mutation_leases");

    for epoch in 1..=10 {
        record_epoch_decision(
            &object_store,
            &lease_epoch_path(&fence_path, epoch),
            &LeaseEpochDecision::Closed,
        )
        .await
        .unwrap();
    }
    cleanup_old_lease_epochs(&object_store, &fence_path, 11).await;

    listings.store(0, AtomicOrdering::SeqCst);
    deletes.store(0, AtomicOrdering::SeqCst);
    cleanup_old_lease_epochs(&object_store, &fence_path, 11).await;
    assert!(
        listings.load(AtomicOrdering::SeqCst) <= 2,
        "already-cleaned history should take bounded listings, observed {} listings and {} deletes",
        listings.load(AtomicOrdering::SeqCst),
        deletes.load(AtomicOrdering::SeqCst)
    );
}

cargo test -p lance dataset::refs::tests::cleanup_should_be_bounded -- --nocapture failed with: already-cleaned history should take bounded listings, observed 11 listings and 30 deletes. A separate instrumentation run observed the same 11 listings and 30 deletes on both consecutive cleanup passes, with all 10 decision directories retained.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 7e0e63f. Immutable decisions now live under sibling _refs/mutation_decisions/, while superseded numeric lease directories are removed once. The scale regression verifies repeated cleanup performs no deletes and reconciliation and renewal use bounded listings despite retained decision history.

Err(error) if matches!(&error, Error::RefConflict { .. }) => {
let expected = LeaseEpochDecision::Publish(publication.clone());
let decision_path = self.path.clone().join(LEASE_DECISION_FILE);
if read_serialized_file::<LeaseEpochDecision>(&self.object_store, &decision_path)

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 reads decision.json before checking whether the exact catalog is already committed. If a successor has applied this publication and the decision GET then has a transient failure, the original mutation returns an I/O error even though its durable state is authoritative. Retrying a create/delete can then report “already exists”/“not found”, so the caller cannot recover a truthful outcome.

Check publication.is_committed first on the lost-fence path (an exact catalog match is sufficient proof of success), or otherwise reconcile decision-read errors with that committed state before returning them.

Reproducer run against this head

Added this temporary test beside the existing refs tests:

#[tokio::test]
async fn committed_publication_survives_decision_read_failure() {
    let target: Arc<dyn object_store::ObjectStore> =
        Arc::new(object_store::memory::InMemory::new());
    let failing = Arc::new(FailingProxyStore::new());
    let mut object_store = ObjectStore::memory();
    object_store.inner = failing.wrap("", target);
    let object_store = Arc::new(object_store);
    let root = Path::from("dataset");
    let fence_path = base_ref_mutation_leases_path(&root);
    let first_path = lease_epoch_path(&fence_path, 1);
    assert!(
        create_lease_file(
            &object_store,
            &first_path,
            &DurableLeaseState {
                owner: "expired".to_string(),
                epoch: 1,
                expires_at_millis: 0,
            },
        )
        .await
        .unwrap()
    );
    let first_fence = DurableLeaseFence {
        object_store: object_store.clone(),
        path: first_path,
        fence_path: fence_path.clone(),
        epoch: 1,
    };
    let publication = DurableRefPublication {
        epoch: 1,
        path: ref_catalog_version_path(&root, 1).to_string(),
        body: serde_json::to_string_pretty(&RefCatalog {
            mutation_epoch: 1,
            ..Default::default()
        })
        .unwrap(),
    };
    first_fence.record_publication(&publication).await.unwrap();
    publication.apply(&object_store).await.unwrap();

    let second_path = lease_epoch_path(&fence_path, 2);
    assert!(
        create_lease_file(
            &object_store,
            &second_path,
            &DurableLeaseState::acquired("successor".to_string(), 2).unwrap(),
        )
        .await
        .unwrap()
    );
    failing.fail_when(
        "get_opts",
        LEASE_DECISION_FILE,
        "injected decision read failure",
    );

    first_fence
        .apply_committed_publication(&publication)
        .await
        .expect("the exact catalog is already committed");
}

cargo test -p lance dataset::refs::tests::committed_publication_survives_decision_read_failure -- --nocapture failed at the final expectation with the injected decision-read I/O error, after publication.apply had succeeded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 7e0e63f. The lost-fence path now checks the exact committed catalog before reading the epoch decision, so a transient decision GET cannot turn an already-authoritative mutation into an error. The injected-read-failure regression passes.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 7e0e63f. Durable decision arbitration is now separate from bounded active-lease discovery and cleanup, and committed publication proof precedes coordination-state reads. Both regressions, all 80 refs tests, formatting, and workspace Clippy pass.

@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.

Active lease discovery and committed-publication readback are now fixed, but the permanent decision archive retains a complete reference catalog for every successful mutation. This makes durable storage grow with every historical catalog snapshot and defeats the two-catalog compaction boundary.

A viable revision should keep only compact immutable arbitration data permanently and retain the full replay body only until durable reconciliation has applied it. For example, keep Closed or Publish { digest }, then let late owners authenticate a temporary body against that digest.

Comment thread rust/lance/src/dataset/refs.rs Outdated
async fn record_publication(&self, publication: &DurableRefPublication) -> Result<()> {
let expected = LeaseEpochDecision::Publish(publication.clone());
let decision_path = lease_epoch_decision_path(&self.fence_path, self.epoch)?;
let actual = record_epoch_decision(&self.object_store, &decision_path, &expected).await?;

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.

expected includes DurableRefPublication.body, so this create-only decision writes the entire serialized RefCatalog into _refs/mutation_decisions/{epoch}.json. Nothing compacts this directory. Each successful mutation therefore retains another full catalog forever; if refs grow along with mutations, storage is Θ(mutations × catalog size) and becomes quadratic.

Keep a small immutable decision (Closed or Publish { digest }) for arbitration, store the full replay body separately only until a durable applied/reconciled marker exists, then delete it. A late owner can authenticate its local body against the digest.

Reproducer

I added a temporary test in dataset::refs::tests that created 100-entry catalogs for 10 successful publication decisions, invoked compact_ref_catalog after each publication, then summed object sizes under _refs/catalog/ and _refs/mutation_decisions/.

cargo test -p lance dataset::refs::tests -- --nocapture

Only two catalog files remained, but the assertion failed with:

catalog compaction retained 268372 bytes in publish decisions;
one current decision is 26839 bytes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in f005739. Permanent decisions now contain only Closed or a BLAKE3 publication digest. The full replay body lives in the epoch lease until a successor authenticates it, applies it, persists reconciliation, and reclaims that lease. The 10-epoch/100-reference regression verifies ten decisions are smaller than one replay body, all reconciled bodies are removed, and only two catalogs remain.

@github-actions github-actions Bot added A-python Python bindings A-java Java bindings + JNI A-deps Dependency updates labels Aug 8, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in f005739. Durable arbitration now retains only compact closed/digest decisions, while authenticated full replay bodies are temporary and reclaimed after durable reconciliation. The storage-bound regression, all 81 refs tests, formatting, workspace Clippy, and Python/JNI manifest checks pass.

@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 compact digest decision and temporary authenticated replay body fix the storage-growth blocker while preserving publish/close arbitration and recovery ordering. Reconciliation applies and checkpoints publications before lease cleanup, and the storage regression confirms that full catalog bodies are reclaimed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-deps Dependency updates A-format On-disk format: protos and format spec docs A-java Java bindings + JNI A-namespace Namespace impls A-python Python bindings breaking-change bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can not delete branches referenced by other branches

1 participant