Skip to content

fix: protect active dataset versions from cleanup - #8409

Open
lance-gatefixer[bot] wants to merge 10 commits into
mainfrom
gatekeeper/fix-6607-1
Open

fix: protect active dataset versions from cleanup#8409
lance-gatefixer[bot] wants to merge 10 commits into
mainfrom
gatekeeper/fix-6607-1

Conversation

@lance-gatefixer

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

Copy link
Copy Markdown
Contributor

Summary

  • add renewable, TTL-based version leases for Rust and Python dataset handles
  • use storage-observed timestamps with a conservative whole-second precision interval so accepted TTLs never expire early
  • retire cleanup candidates through draining, sealed, and committed states; admitted leases can renew while draining, and the committed marker is the irreversible deletion boundary
  • publish tags and branches through bounded, uniquely owned durable admission intents and conditional canonical mutations
  • make final cleanup census policy-independent and retain only descendant manifests with actual parent-lineage dependencies
  • keep recovered policy claims subordinate to active leases, tags, reference intents, and descendant branches
  • delete leases, reference intents, and superseded retirement metadata before removing the terminal retry marker
  • bound draining and reference-admission ownership, recover interrupted retirement, and remove branch-incarnation operational state

Root cause

cleanup_old_versions had no liveness signal for older dataset handles. Its recovery path also treated a previously sealed version as unconditionally deletable, allowing stale recovery claims to override leases or durable references. Reference publication used post-write rollback, which could fail or remove another writer result, while the final branch census depended on cleanup policy instead of actual descendant lineage. Finalization could additionally remove the only retry marker before dependent metadata deletion completed.

Validation

  • cargo test -p lance version_lease --lib
  • cargo test -p lance recovery --lib
  • cargo test -p lance cleanup_lineage --lib
  • cargo test -p lance auto_clean_referenced_branches --lib
  • cargo test -p lance recovered_retirement_still_respects_descendant_branch --lib
  • cargo test -p lance reference_intent_blocks_retirement_commit --lib
  • cargo test -p lance cleanup_resumes_sealed_retirement --lib
  • cargo test -p lance can_recover_delete_failure --lib
  • cargo test -p lance refs --lib
  • cargo test -p lance test_tag --lib
  • cargo check -p lance --tests
  • cargo fmt --all -- --check
  • cargo clippy --all --tests --benches -- -D warnings
  • make build from python/
  • uv run pytest python/tests/test_dataset.py::test_cleanup_retains_active_version_lease
  • uv run pytest --doctest-modules python/lance/dataset.py::lance.dataset.LanceDataset.acquire_version_lease
  • uv run make format from python/
  • uv run make lint from python/
  • git diff --check

Fixes #6607

@github-actions github-actions Bot added A-python Python bindings 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 reader-liveness problem is real, but the retirement protocol does not yet establish a safe, bounded admission and expiry contract across concurrent actors.

A viable revision should make acquisition, renewal, and retirement one explicit state machine: already-admitted leases can renew while cleanup drains them; expiry uses storage-observed time or a conservative documented skew bound; and retirement fences remain durable through partial deletion but are finalized or compacted after a terminal outcome.

Comment thread rust/lance/src/dataset/cleanup.rs Outdated
// A lease acquired between the initial list and the fence either sees
// the fence and fails, or appears here and conservatively retains this
// version for the current cleanup pass.
let leased_versions = version_lease_store.active_versions(true).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.

A lease can return successfully and still be made permanently non-renewable here. One valid interleaving is: the initial lease list misses the new acquisition; acquisition passes its second fence check; cleanup creates the marker; this second list sees the lease and retains the version; then the permanent marker makes every later renew fail. The reader is protected only until its original TTL, recreating the mid-scan deletion risk for an API advertised as renewable.

Treat the marker as retirement admission: leases admitted before retirement must be able to renew while cleanup drains and rechecks them, or this pass must safely cancel the fence for a retained version before any deletion.

Reproducer

Added under the version_lease.rs test module:

fn memory_store() -> VersionLeaseStore {
    VersionLeaseStore {
        object_store: Arc::new(ObjectStore::memory()),
        leases_path: Path::from("leases"),
        markers_path: Path::from("markers"),
    }
}

#[tokio::test]
async fn raced_active_lease_remains_renewable() {
    MockClock::set_system_time(Duration::from_secs(100));
    let store = memory_store();
    let mut lease = store.acquire(42, Duration::from_secs(60)).await.unwrap();

    store.fence_versions(&HashSet::from([42])).await.unwrap();
    assert!(store.active_versions(false).await.unwrap().contains(&42));
    assert!(lease.renew(Duration::from_secs(60)).await.is_ok());
}

cargo test -p lance raced_active_lease_remains_renewable --lib failed on this head at the final assertion because renewal returned the fenced-version 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 abe62f1. Draining now blocks new acquisition but permits admitted leases to renew; retained versions cancel the operation fence before deletion, and sealing is the renewal cutoff. Added draining-renewal regression coverage.

Comment thread rust/lance/src/dataset/version_lease.rs Outdated
Err(error) if error.is_not_found() => return Ok(HashSet::new()),
Err(error) => return Err(error),
};
let now_micros = utc_now().timestamp_micros();

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.

Lease expiry is compared across independent host clocks: acquisition writes reader_utc_now + TTL into the filename, while cleanup compares it with its own utc_now. A cleaner ahead by one TTL treats a newly acquired lease as already expired and can delete the version while the reader still considers the lease valid. Use storage-observed time for lease age/renewal, or define and enforce a conservative bounded-skew contract with grace and a minimum TTL.

Reproducer

Using the same in-memory store helper under the version_lease.rs tests:

#[tokio::test]
async fn newly_acquired_lease_survives_cleaner_clock_skew() {
    MockClock::set_system_time(Duration::from_secs(100));
    let store = memory_store();
    let _lease = store.acquire(42, Duration::from_secs(60)).await.unwrap();

    // Model a cleanup host whose clock is one TTL ahead.
    MockClock::set_system_time(Duration::from_secs(160));
    assert!(store.active_versions(false).await.unwrap().contains(&42));
}

cargo test -p lance newly_acquired_lease_survives_cleaner_clock_skew --lib failed on this head because version 42 was reported inactive immediately from the cleaner clock perspective.

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 abe62f1. Lease TTL and cleanup reference times now derive from object-store last_modified metadata, eliminating comparisons between reader and cleaner host clocks. Added clock-skew regression coverage.

Comment thread rust/lance/src/dataset/version_lease.rs Outdated
stream::iter(versions.iter().copied())
.map(Ok)
.try_for_each_concurrent(self.object_store.io_parallelism(), |version| async move {
let path = self.markers_path.clone().join(version.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.

These fence objects have no terminal transition: this code only creates them, and the only other marker operation is exists. A successful cleanup therefore leaves one permanent object per removed version; failed pre-delete passes can permanently reject leases for versions that still exist; and deleted branch generations orphan whole marker namespaces. Keep fences through partial deletion, but add explicit successful/cancelled finalization or bounded compaction, with manifest-identity revalidation so completed retirement metadata need not grow without bound.

Reproducer

I added a cleanup regression that creates two versions, successfully removes version 1, then asserts that _refs/version_lease_gc/main/1 no longer exists. Running cargo test -p lance successful_cleanup_finalizes_version_fence --lib failed on this head at:

assert!(!historical.object_store.exists(&marker).await.unwrap());

The marker was still present after cleanup returned old_versions == 1.

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 abe62f1. Per-operation fences are canceled for retained or pre-delete-aborted versions, kept sealed across partial deletion, and removed with leases only after manifest absence is revalidated; branch deletion also clears incarnation state. Added terminal and branch-cleanup regression coverage.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in abe62f1. Acquisition, renewal, and retirement now use an explicit draining/sealed state machine; liveness is measured from storage timestamps; cancellation, partial-deletion durability, successful finalization, and branch-incarnation cleanup bound retirement metadata.

@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 storage-clock state machine fixes the original host-skew and renewal races, but its object-store lifecycle contract is still incomplete: accepted TTLs must never expire early on supported backends, and retirement state must remain recoverable after cancellation or partial failure.

A viable revision should account conservatively for the coarsest supported metadata timestamp and add bounded ownership and recovery for draining and terminal markers.

return Err(error.into());
}
};
let expires_at = expiration_from_ttl(metadata.last_modified, ttl)?;

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.

Accepted sub-second TTLs can expire early here on cloud stores. object_store obtains cloud last_modified from HTTP Last-Modified, whose whole-second precision truncates the lease creation time. Adding the requested TTL to that value and comparing it with an independently truncated marker can let cleanup delete a version before the promised lifetime ends. Account conservatively for backend timestamp precision—for example, reject or round up short TTLs and include one precision interval in the expiry boundary—rather than treating these timestamps as exact.

Reproducer

Added under this file's existing test module:

#[test]
fn lease_ttl_survives_coarse_storage_timestamps() {
    let storage_second = DateTime::from_timestamp(100, 0).unwrap();
    let acquired_at = storage_second + TimeDelta::try_milliseconds(900).unwrap();
    let cleanup_started_at = storage_second + TimeDelta::try_milliseconds(1_001).unwrap();
    let ttl = Duration::from_millis(900);

    assert!(
        cleanup_started_at < acquired_at + TimeDelta::from_std(ttl).unwrap(),
        "the requested TTL is still active"
    );

    let lease_last_modified = storage_second;
    let marker_last_modified = storage_second + TimeDelta::try_seconds(1).unwrap();
    assert!(
        expiration_from_ttl(lease_last_modified, ttl).unwrap() > marker_last_modified,
        "coarse Last-Modified timestamps must not expire the lease early"
    );
}

cargo test -p lance lease_ttl_survives_coarse_storage_timestamps --lib failed at the final assertion on this head: the requested TTL was still active in real time, but the stored timestamps classified it as expired.

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 c6f89e9. Storage-derived expiration now adds one whole-second precision interval before comparison, so truncated cloud Last-Modified values cannot shorten an accepted TTL. The coarse-timestamp reproducer is included as a regression test.

.fence_old_versions_and_retain_new_leases(inspection, &version_lease_store)
.await?;

let cleanup_result = self.delete_unreferenced_files(inspection).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.

Retirement metadata still has no recovery path when the cleanup future is canceled after publishing a marker; this early return also skips finalize after a partial deletion error. An abandoned draining marker rejects every later acquisition, and once a manifest has been deleted a subsequent cleanup cannot rediscover that version through old_manifests to sweep its remaining sealed markers or leases. Give markers bounded durable ownership and scan/recover existing retirement state: cancel a provably untouched stale drain, resume uncertain/partial retirement, and sweep terminal metadata only after manifest-identity validation.

Reproducer

Added under the version_lease.rs test module:

#[tokio::test]
async fn abandoned_drain_does_not_block_future_acquire() {
    let store = memory_store();
    let guard = store.fence_versions(&HashSet::from([42])).await.unwrap();

    // Model cancellation or process exit before cleanup can cancel/finalize.
    drop(guard);

    store.acquire(42, Duration::from_secs(60)).await.unwrap();
}

cargo test -p lance abandoned_drain_does_not_block_future_acquire --lib failed on this head because acquisition returned version 42 is retiring and cannot accept a new lease; no owner or expiry remains that can clear the drain.

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 c6f89e9. Dropped local drains are abandoned immediately, cross-process drain ownership expires after 15 minutes, and durable markers record manifest identities. Later cleanup removes stale drains, resumes sealed or partial retirement after admitted leases are no longer active, and finalizes metadata only after every recorded manifest is absent. Regression coverage includes dropped drains, bounded ownership, sealed recovery, and policy-independent cleanup resumption.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in c6f89e9. Lease expiry now includes the coarsest supported one-second metadata precision. Drains have a 15-minute ownership bound and immediate in-process abandonment, while cleanup scans durable marker payloads to resume sealed work, waits out admitted leases after uncertain sealing, and sweeps markers and leases only after recorded manifests are absent.

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

Recovery still lacks one durable commit boundary shared by live cleaners, readers, references, and finalization.

A viable revision should make irreversible retirement atomic with lease and reference admission, and preserve a retry anchor until all dependent metadata is removed.

Comment thread rust/lance/src/dataset/cleanup.rs Outdated
.await?;
let retained_versions: HashSet<_> = versions_to_delete
.intersection(&leased_versions)
.filter(|version| !forced_retirement_versions.contains(version))

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 recovered version is permanently excluded from both lease-retention scans. recover_retirements can return version 42 while the original live owner still holds the seal; if that owner subsequently cancels and a lease is admitted, the stale forced_retirement_versions entry suppresses that active lease here, so cleanup can delete a version with a valid lease. A recovery claim needs a durable ownership/commit boundary with seal cancellation and lease admission: either keep the seal authoritative once recovery may act, or make successful admission invalidate the claim before deletion.

Reproducer

I added this regression to version_lease.rs and ran cargo test -p lance recovery --lib against this head:

#[tokio::test]
async fn recovery_claim_does_not_outlive_live_owner_cancellation() {
    let store = memory_store();
    let manifest_paths = manifest_paths(42);
    let manifest_path = manifest_paths[&42][0].clone();
    store.object_store.put(&manifest_path, &[]).await.unwrap();

    let mut owner = store.fence_versions(&manifest_paths).await.unwrap();
    owner.seal_versions(&HashSet::from([42])).await.unwrap();
    let forced_versions = store.clone().recover_retirements().await.unwrap();
    assert_eq!(forced_versions, HashSet::from([42]));

    owner.cancel_all().await.unwrap();
    let _lease = store.acquire(42, Duration::from_secs(60)).await.unwrap();
    owner = store.fence_versions(&manifest_paths).await.unwrap();
    let active = store
        .active_versions_at(&owner.observed_at(), true)
        .await
        .unwrap();
    let retained = active
        .intersection(&HashSet::from([42]))
        .filter(|version| !forced_versions.contains(version))
        .copied()
        .collect::<HashSet<_>>();

    assert_eq!(retained, HashSet::from([42]));
}

The final assertion failed with left: {} and right: {42}: the new lease was active, but the stale recovery claim filtered it out.

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 724e0e0. Recovered policy claims no longer filter either lease-retention scan: an active lease always retains the version and cancels the current retirement fence. Added a regression covering forced retirement with an active lease.

Comment thread rust/lance/src/dataset/cleanup.rs Outdated
let in_working_set = is_latest || !self.policy.should_clean(&manifest) || is_tagged;
let is_leased = leased_versions.contains(&manifest.version);
let is_forced_retirement = forced_retirement_versions.contains(&manifest.version);
let in_working_set = !is_forced_retirement

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.

Forced recovery also overrides durable references created after sealing: is_forced_retirement makes a newly tagged version leave the working set regardless of is_tagged, and the same forced set bypasses descendant-branch retention. The verified tag case deletes a version even though tag creation succeeded. Tag and branch admission must share the retirement commit boundary—either reject admission while an irrevocable seal exists or revalidate/cancel retirement before deletion when a new reference wins.

Reproducer

I added this regression to cleanup.rs and ran cargo test -p lance recovered_retirement_preserves_tag_created_after_seal --lib against this head:

#[tokio::test]
async fn recovered_retirement_preserves_tag_created_after_seal() {
    let fixture = MockDatasetFixture::try_new().unwrap();
    fixture.create_some_data().await.unwrap();
    let historical = fixture.load().await.unwrap();
    fixture.overwrite_some_data().await.unwrap();
    let dataset = fixture.load().await.unwrap();

    let store = VersionLeaseStore::for_dataset(&historical).await.unwrap();
    let manifest_paths = HashMap::from([(
        historical.version().version,
        vec![historical.manifest_location.path.clone()],
    )]);
    let mut guard = store.fence_versions(&manifest_paths).await.unwrap();
    guard.seal_versions(&HashSet::from([1])).await.unwrap();
    drop(guard);

    dataset.tags().create("after-seal", 1).await.unwrap();
    let removed = fixture
        .run_cleanup_with_policy(CleanupPolicy {
            before_version: Some(1),
            error_if_tagged_old_versions: false,
            ..Default::default()
        })
        .await
        .unwrap();

    assert_eq!(removed.old_versions, 0);
    assert!(
        dataset.checkout_version(1).await.is_ok(),
        "a successfully created durable tag must keep its version readable"
    );
}

The cleanup returned old_versions == 1, failing the expected 0; the tagged version was retired.

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 724e0e0. Tags and branches now check retirement admission before and after publication, cleanup repeats its reference scan before committing, and recovered policy claims cannot override existing tags or descendant branches. Added sealed tag/branch admission coverage.

Comment thread rust/lance/src/dataset/version_lease.rs Outdated
paths.push(metadata.location);
}
}
self.delete_paths(paths).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.

Finalization mixes sealed markers and leases into one concurrent fail-fast delete. A marker can be deleted successfully before a lease deletion fails; the next recover_retirements sees no marker, so it has no durable retry anchor and the lease remains orphaned indefinitely. Delete dependent lease state first and remove the terminal marker only as the final commit, with retry-safe ordering.

Reproducer

I added this regression to version_lease.rs and ran cargo test -p lance recovery --lib against this head. Deleting the sealed path first models the successful-marker/failed-lease prefix of delete_paths:

#[tokio::test]
async fn terminal_recovery_survives_marker_first_partial_finalize() {
    let store = memory_store();
    let lease = store.acquire(42, Duration::from_secs(60)).await.unwrap();
    let mut guard = store.fence_versions(&manifest_paths(42)).await.unwrap();
    guard.seal_versions(&HashSet::from([42])).await.unwrap();
    let sealed_path = guard.fences[&42].sealed_path.clone().unwrap();

    store.object_store.delete(&sealed_path).await.unwrap();
    assert!(store.clone().recover_retirements().await.unwrap().is_empty());
    assert!(!store.object_store.exists(&lease.path).await.unwrap());
}

The last assertion failed: after the marker-first partial finalize, recovery returned no work and the lease 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 724e0e0. Finalization now deletes leases first, then superseded markers, and removes terminal committed markers last. The injected lease-deletion regression verifies that recovery retains its anchor and completes after the failure clears.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 724e0e0. Retirement now has a durable committed boundary shared by lease, tag, and branch admission; current leases and references win before commit, and the terminal marker remains until all dependent metadata is removed.

@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 durable commit marker repairs the prior lease and retry-anchor defects, but durable-reference admission is still not atomic: publication rollback can leave a reference behind, and recovery can omit an existing descendant branch.

A viable revision should make reference admission recoverable and ownership-checked (for example, durable per-operation intents plus conditional reference mutation), and the final cleanup census must consider every descendant independently of the current retention policy and fail closed on read errors.

Comment thread rust/lance/src/dataset/refs.rs Outdated
)
.await
{
self.object_store().delete(&tag_file).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.

A failed rollback can leave a durable tag pointing at a version whose retirement is already committed. The first admission check can pass, cleanup can finish its final census and commit while the tag put is paused, and then this postcheck rejects; if this delete fails or the task is cancelled, cleanup proceeds while the tag remains. The same publish/compensate pattern affects tag update and branch create, and unconditional compensation can also remove another same-name writer's successful result. Use a uniquely owned durable admission intent that cleanup can observe before canonical publication, together with conditional/CAS mutation and ownership-checked recovery.

Reproducer

On 724e0e0e7c51c7477dc4ff8d97e866f0a4e06976, I added failed_post_admission_rollback_does_not_leave_tag to the existing cleanup test module. The test pauses the proxy store on the racing tag put after the first check, uses an unwrapped handle to fence/seal/commit version 1, releases the put, injects failure for the rollback delete, and asserts dataset.tags().get("racing").await.is_err().

cargo test -p lance failed_post_admission_rollback_does_not_leave_tag --lib -- --nocapture failed at that assertion: the API returned the injected rollback error but the durable tag remained.

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 fb524c4. Tag and branch publication now creates a uniquely owned, bounded durable intent before conditional create/CAS mutation. Compensating rollback is removed, ambiguous writes retain their intent, and cleanup rechecks intents at the retirement commit boundary.

Comment thread rust/lance/src/dataset/cleanup.rs Outdated
.map(|tag| tag.version)
.collect::<HashSet<_>>();
referenced_versions.extend(
self.find_referenced_branches()

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 final safety scan can omit a durable child branch during recovery. Recovered retirement forces the version independently of the current policy, but find_referenced_branches only returns children whose parent manifest satisfies policy.should_clean; it also treats a manifest read failure as if the child were absent. After an interrupted seal, a retry with a less aggressive policy can therefore commit and delete the forced parent version. Enumerate all descendants whose parent is in versions_to_seal independently of retention policy, and propagate census read failures.

Reproducer

On 724e0e0e7c51c7477dc4ff8d97e866f0a4e06976, I added a test that creates versions 1 and 2, creates child branch child from version 1, seals version 1 and drops the guard, then retries cleanup with CleanupPolicy { before_version: Some(1), ..Default::default() }. It asserts removed.old_versions == 0, that the version-1 manifest remains, and that the child can still be checked out.

cargo test -p lance recovered_retirement_still_respects_descendant_branch --lib -- --nocapture failed on the first assertion with left: 1, right: 0: cleanup removed the branch's parent 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 fb524c4. The sealed census now enumerates every descendant independently of cleanup policy and scans actual descendant manifests for parent-lineage dependencies. Present-manifest read failures propagate, and the interrupted-retirement regression verifies the parent version remains readable.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in fb524c4. Reference publication now uses bounded durable per-operation intents plus conditional canonical mutations without rollback, and cleanup performs policy-independent descendant and intent censuses before retirement commit.

@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 lease problem is real, but reference publication and crash recovery still do not share a complete linearization boundary with retirement. Canonical publication can hand off after the cleanup census, and a recovered pre-commit seal can strand an admitted lease until it expires.

The same boundary must cover every canonical mutation: Branches::replace_metadata still uses a read followed by an unconditional put. A focused interleaving regression read child, deleted it, performed that final put, and failed because the supposedly deleted branch existed again.

A viable revision should keep publication durably protected until cleanup must observe either the intent or canonical reference, fence any writer whose intent has expired, cancel non-committed seals when an admitted lease or reference wins, and route every canonical tag and branch mutation through conditional admission.

Comment thread rust/lance/src/dataset/refs.rs Outdated
.await
{
Ok(_) => {
admission.complete().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 successful canonical write is not atomically handed off from the intent. Cleanup reads canonical tags before it reads intents; publication can therefore finish in between those scans, delete the intent here, and leave cleanup observing neither form of protection before it commits retirement. The same unchecked PUT window lets a stalled writer resume after the fixed intent timeout. Keep a recoverable or fenced publication state until cleanup is guaranteed to observe either the intent or canonical reference; deleting an expired intent must also prevent the old writer from subsequently publishing.

Reproducer

Added under the version_lease.rs test module and ran cargo test -p lance canonical_reference_handoff_blocks_retirement_commit --lib:

#[tokio::test]
async fn canonical_reference_handoff_blocks_retirement_commit() {
    let store = memory_store();
    let manifest_path = Path::from("versions/42.manifest");
    store.object_store.put(&manifest_path, &[]).await.unwrap();
    let (intent_path, created_at) = store.create_reference_intent(42).await.unwrap();
    let admission = ReferenceAdmission {
        store: store.clone(),
        path: intent_path,
        manifest_path: manifest_path.clone(),
        version: 42,
        created_at,
    };
    admission.ensure_owned().await.unwrap();

    let manifest_paths = HashMap::from([(42, vec![manifest_path])]);
    let mut guard = store.fence_versions(&manifest_paths).await.unwrap();
    guard.seal_versions(&HashSet::from([42])).await.unwrap();

    let canonical_reference = Path::from("tags/racing.json");
    assert!(!store.object_store.exists(&canonical_reference).await.unwrap());
    store.object_store.put(&canonical_reference, &[]).await.unwrap();
    admission.complete().await;
    assert!(store.active_reference_versions().await.unwrap().is_empty());

    let retained = guard.commit_versions(&HashSet::from([42])).await.unwrap();
    assert_eq!(retained, HashSet::from([42]));
}

The assertion failed with left: {} and right: {42}: the canonical reference was durable, but retirement still committed.

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 95294fb. Admission intents now retain the exact conditional canonical mutation through handoff; expired intents are replayed or proven conflicted before removal, so a stalled owner cannot publish after cleanup fences it.

Comment thread rust/lance/src/dataset/version_lease.rs Outdated
// A seal is the renewal cutoff. If its owner disappeared before
// confirming the final lease scan, wait out every lease that was
// still entitled to its published TTL before resuming deletion.
if !actively_leased_versions.contains(&version)

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.

Recovery detects the active lease but only omits this version from versions_to_resume; it leaves the pre-commit sealed marker in storage. Every renewal checks for that marker and fails, so a crash after sealing but before the second lease census converts a renewable admitted lease into protection only until its original TTL. Since no committed marker exists yet, recovery should cancel the seal when an active lease or durable reference wins, then let normal cleanup retry later.

Reproducer

Extended the existing recovery test and ran cargo test -p lance sealed_recovery_waits_for_active_lease --lib:

let mut lease = store.acquire(42, Duration::from_secs(60)).await.unwrap();
let mut guard = store.fence_versions(&manifest_paths).await.unwrap();
guard.seal_versions(&HashSet::from([42])).await.unwrap();
drop(guard);

assert!(store.clone().recover_retirements().await.unwrap().is_empty());
assert!(store.object_store.exists(&manifest_path).await.unwrap());
lease.renew(Duration::from_secs(60)).await.unwrap();

Renewal failed with version 42 is retiring and cannot accept a new lease even though recovery had just recognized that lease as active.

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 95294fb. Recovery now removes pre-commit sealed markers when an active lease or durable reference wins, while committed retirement remains irreversible. Lease renewal and active-reference recovery regressions cover both paths.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 95294fb. Branch metadata replacement now uses version-checked conditional admission, so a deletion after its read prevents resurrection. Reference-publishing tag and branch mutations share the replayable admission-intent boundary.

@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 lease protocol now closes the previously reported handoff and renewal races, but canonical-reference lifecycle is still not atomic across cancellation, recovery, deletion, and backends without conditional updates.

A viable revision should make the intent, canonical mutation, and deletion one fenced state machine: recovery must not replay an operation after a later canonical delete, canonical references must cancel recovered pre-commit seals, and a backend without atomic conditional update must reject the operation or provide an equivalent atomic primitive.

Comment thread rust/lance/src/dataset/version_lease.rs Outdated
if self.object_store.exists(&manifest_path).await?
&& !self.has_committed_marker(intent.version).await?
&& self
.apply_reference_mutation(&intent.intent.mutation)

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.

An expired pending Create can resurrect a tag or branch that was deliberately deleted. If the canonical create succeeds but cancellation or a completion-write failure leaves this intent Pending, a later user delete is valid; after the timeout, this replay sees the absent name and creates it again. Absence cannot distinguish “never published” from “published, then deleted.” The canonical state needs a generation/tombstone or another fence that deletion advances, so recovery cannot recreate an absent reference from a stale intent.

Reproducer

I added expired_pending_create_intent_resurrects_deleted_reference to this module. It leaves a successful create pending, deletes the canonical object, ages the intent beyond the ownership timeout, runs recovery, and asserts that the canonical object remains absent.

CARGO_TARGET_DIR=/home/agent/tmp/pr8409-regression-k10g2C-target cargo test -p lance --lib resurrects exited 101; the expected assertion failed because the canonical path existed again:

assertion failed: !store.object_store.exists(&canonical_path).await.unwrap()

@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 ba3e9e9. Expired create intents are no longer replayed: recovery atomically installs a durable canonical tombstone for an absent name, and tag/branch APIs treat that tombstone as deleted while safely supporting later recreation. The stale-create regression verifies deletion is not undone.

}
self.object_store
.inner
.put(&path, Bytes::from(payload.clone()).into())

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 unsupported-CAS fallback still permits the branch-resurrection race. LocalFileSystem returns NotImplemented for PutMode::Update; after this fallback reads the expected payload, a concurrent delete can complete before the unconditional put, which recreates the deleted tag or branch. Backends without an atomic conditional-update primitive should reject this mutation or use a genuinely atomic backend-specific mechanism; a read followed by overwrite cannot preserve delete/update ordering.

Reproducer

I added local_fallback_reference_update_resurrects_concurrent_delete, using a local store and a blocking proxy at the fallback overwrite. The test deletes the canonical object after the fallback GET, releases the PUT, and asserts that the deleted object stays absent.

CARGO_TARGET_DIR=/home/agent/tmp/pr8409-regression-k10g2C-target cargo test -p lance --lib resurrects exited 101; the assertion failed because the fallback PUT recreated the path:

assertion failed: !store.object_store.exists(&canonical_path).await.unwrap()

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 ba3e9e9. Conditional reference update/deletion now rejects non-local backends without atomic CAS. Supported local file APIs serialize canonical mutations with retained per-reference OS advisory locks before their backend-specific overwrite fallback; a regression verifies an unsupported raw conditional update is rejected without changing the reference.

.map(|marker| (marker.version, observed_at))
.collect::<HashMap<_, _>>();
let actively_leased_versions = self.active_versions_at(&sealed_observation, true).await?;
let actively_referenced_versions = self.active_reference_versions().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.

Recovery only recognizes lease and intent sidecars, so it can strand a canonical reference behind a pre-commit seal. The pre-canonical-census handoff removes a completed intent; if cleanup is cancelled or crashes before cancelling the seal, the canonical tag/branch remains but this recovery scan treats the version as unreferenced. A later cleanup retains the manifest through its canonical census without removing that recovered seal, leaving lease acquisition and reference mutations rejected as “retiring.” Recovery must include canonical tags/branch lineage, or the handoff must remain durable until the corresponding seal is cancelled.

Reproducer

I added canonical_only_recovery_leaves_seal_after_completed_handoff. It creates a canonical reference and completed intent, seals the version, runs the pre-census handoff that removes the intent, drops the guard, and expects recovery to cancel rather than resume retirement.

CARGO_TARGET_DIR=/home/agent/tmp/pr8409-regression-k10g2C-target cargo test -p lance --lib dataset::version_lease::tests::canonical_only_recovery_leaves_seal_after_completed_handoff -- --exact exited 101 at:

assertion failed: store.clone().recover_retirements().await.unwrap().is_empty()

The observed recovery returned the version, left its seal present, and subsequent lease acquisition failed as retiring.

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 ba3e9e9. A completed handoff remains durable while a pre-commit seal exists and is tracked by exact intent path. Only the live cleanup that subsequently completes the canonical census consumes that handoff before commit; after a crash, recovery still observes it and cancels the seal. The canonical-only recovery regression verifies leases can be acquired afterward.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in ba3e9e9. Canonical reference lifecycle now uses durable tombstones and conditional or locally serialized mutation, expired intents are inspected without replay, and completed handoffs remain recoverable until pre-commit seals are safely canceled. The three inline regressions cover stale-create deletion, unsupported CAS, and canonical-only seal recovery.

@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 narrow stale-Create, local-fallback, and completed-handoff cases are improved, but the lifecycle still lacks one revocable generation boundary shared by canonical mutation, recovery, and branch deletion. Several independent correctness and compatibility failures remain.

A viable revision should make every canonical and descendant publication CAS a monotonic retirement/incarnation generation that recovery also censuses, while keeping the documented _refs/{tags,branches}/*.json format readable by released clients and bounding retired metadata.

Comment thread rust/lance/src/dataset/version_lease.rs Outdated
}
}
ReferenceMutation::Update { path, payload, .. } => {
self.reference_mutation_conflict_outcome(&Path::parse(path)?, payload)

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.

Expired Update owners are only observed here, not fenced, so they can publish a reference after cleanup commits the target version. A writer can pass ensure_owned, suspend before the canonical CAS, and outlive the 15-minute intent window. This branch then sees the old payload instead of the intended payload and the caller deletes the intent; because the old eTag/version was never changed, the resumed CAS still succeeds and can point at a retired manifest. Conditionally rewrite/fence the expected canonical generation before discarding an expired Update, using the same local lock where needed, or make canonical writes CAS a shared retirement generation.

Reproducer

I added expired_update_owner_cannot_publish_after_retirement_commit beside the existing lease tests and ran:

cargo test -p lance --lib dataset::version_lease::tests::expired_update_owner_cannot_publish_after_retirement_commit -- --exact --nocapture

The test creates an Update intent, lets its owner pass ensure_owned, ages that intent through the existing proxy metadata policy, seals and commits version 42 after the census removes the intent, then resumes the original conditional mutation. The healthy assertion expects Conflict; this head returned Published (left: Published, right: Conflict).

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 a3a30bf. Expired Update recovery now conditionally rewrites the expected canonical payload to advance its store generation before revoking lifecycle ownership, so the suspended owner’s original CAS conflicts; local fallback uses the same advisory lock.

.map(|marker| (marker.version, observed_at))
.collect::<HashMap<_, _>>();
let actively_leased_versions = self.active_versions_at(&sealed_observation, true).await?;
let actively_referenced_versions = self.active_reference_versions().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.

Recovery still treats transient intents as the complete durable-reference census. A completed intent can be consumed by another scan while no seal exists; if a later cleanup seals that canonically referenced version and crashes before its canonical census, this call sees no reference and returns the version for resumed retirement. The next cleanup reads the tag and preserves the manifest, but the old seal is never cancelled, so leases and further reference mutations remain blocked. Recovery must census canonical tags/branches and descendant lineage, or retain an independent handoff until any later seal can be resolved.

Reproducer

I modified the new canonical-recovery regression so a pre-seal reference_versions_before_canonical_census() consumes the completed intent, then sealed version 42, dropped the guard, and ran:

cargo test -p lance --lib dataset::version_lease::tests::recovery_retains_canonical_reference_after_completed_intent_was_consumed -- --exact --nocapture

The canonical tag payload remained present and the intent was absent. The healthy assertion expects recover_retirements() to return an empty set and cancel the seal; this head returned {42}.

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 a3a30bf. Recovery now censuses live lifecycle targets plus canonical tags, branches, and descendant lineage, so a canonical reference cancels a recovered pre-commit seal even after its completed intent was consumed.

Comment thread rust/lance/src/dataset/refs.rs Outdated
.inner
.put_opts(
path,
Bytes::from_static(REFERENCE_TOMBSTONE_PAYLOAD).into(),

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.

Persisting the deletion marker at the canonical .json path breaks the documented reference-file contract for released clients. The stable v10 reader enumerates every .json in these directories and deserializes each as TagContents or BranchContents; this payload lacks version and parentVersion, so one new-client deletion makes the older client fail the entire list. Older create/update calls also see the name as occupied, while an older delete physically removes the marker and reopens the stale-Create race. These tombstones are never reclaimed, so current list cost also grows with every historically deleted name. Keep lifecycle fencing in a versioned, non-enumerated representation that preserves the released canonical format and has bounded reclamation.

Reproducer
#[test]
fn tombstone_payload_remains_readable_by_pre_tombstone_clients() {
    let tag = serde_json::from_slice::<TagContents>(REFERENCE_TOMBSTONE_PAYLOAD);
    let branch = serde_json::from_slice::<BranchContents>(REFERENCE_TOMBSTONE_PAYLOAD);
    assert!(tag.is_ok() && branch.is_ok(), "tag={tag:?}, branch={branch:?}");
}
cargo test -p lance --lib dataset::refs::tests::tombstone_payload_remains_readable_by_pre_tombstone_clients -- --exact --nocapture

Observed errors were missing field version for tags and missing field parentVersion for branches. The released v10 fetch paths use the same required schemas with try_collect, so either error fails the whole listing.

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 a3a30bf. Canonical tombstones are replaced by non-enumerated lifecycle sidecars. Canonical tag and branch JSON retains the released schema with an ignored optional generation field, and deletion reclaims the sidecar after removing the canonical object.

Comment thread rust/lance/src/dataset/refs.rs Outdated
if let Some(snapshot) = reference_snapshot(self.object_store(), &branch_file).await?
&& !snapshot.is_tombstone()
{
tombstone_reference(self.object_store(), &branch_file, &snapshot).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.

Parent deletion is not fenced against descendant admission. Its dependency snapshot occurs before this tombstone; a child can resolve the parent and pass ensure_owned in that gap. Deletion then removes the parent incarnation state, including the child intent, but the admitted child can resume its unchanged PutMode::Create and succeed, leaving child metadata whose parent data was removed. Publish a parent-incarnation termination generation before the dependency census and require child publication to CAS that same generation immediately at publication.

Reproducer

I added deleting_parent_branch_state_fences_admitted_child_publication, admitted a child Create in the parent incarnation, let it pass ensure_owned, called remove_branch_state, and then resumed the canonical Create:

cargo test -p lance --lib dataset::version_lease::tests::deleting_parent_branch_state_fences_admitted_child_publication -- --exact --nocapture

The deletion removed the intent. The healthy assertion expects the resumed mutation to return Conflict; this head returned Published.

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 a3a30bf. Branch deletion now publishes an incarnation-termination marker before its dependency census, and child publication rechecks both that fence and its lifecycle generation before the canonical mutation; incarnation intents and state are revoked before teardown completes.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in a3a30bf. Canonical tag and branch publications now use monotonic lifecycle generations stored in non-enumerated sidecars and compatible JSON payloads; cleanup and recovery census those generations and canonical lineage, deletion reclaims retired sidecars, and branch-incarnation termination fences descendant publication.

@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 generation sidecars remove the tombstone compatibility hazard and repair canonical-only recovery, but they still do not form a crash-safe, mixed-version linearization boundary with canonical references. Independent publication and deletion paths can hide or orphan references or let cleanup delete a referenced version.

A viable revision should make canonical content and lifecycle state reconcile after every partial or legacy transition: use a byte-changing fence for stale CAS, recover or roll back Deleting and lost-ownership writes, and keep released-writer rewrites visible and protected.

},
Some((metadata, current_payload)),
) if current_payload.as_ref() == expected_payload => {
self.rewrite_reference_payload(

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.

Rewriting identical bytes does not fence the expired update on unversioned S3. object_store 0.13.2 maps AWS UpdateVersion to ETag-only If-Match and ignores version; for a normal single-part canonical JSON object, this overwrite can retain the content ETag, leaving the stale owner's original condition valid and allowing publication after retirement committed. Please write a new fence generation into the canonical payload and lifecycle state, or use another token guaranteed to change on every accepted fence.

Reproducer

No live S3 endpoint was available, so I inspected object_store-0.13.2/src/aws/mod.rs:202-217 to confirm the ETag-only condition and ran the bounded content-ETag case exercised by these identical inputs:

original='{"version":1}'
stale_if_match=$(printf '%s' "$original" | md5sum | cut -d' ' -f1)
fence_rewrite_etag=$(printf '%s' "$original" | md5sum | cut -d' ' -f1)
printf 'stale_if_match=%s\nfence_rewrite_etag=%s\n' "$stale_if_match" "$fence_rewrite_etag"
test "$stale_if_match" != "$fence_rewrite_etag"

Expected the final assertion to pass because fencing must invalidate the stale If-Match; it exited 1. Both values were 77efb8e3fa276d4674932392a66555e4, so S3 can accept the stale condition.

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 9097168. Expired Update recovery now conditionally rewrites the canonical JSON with an added whitespace byte, guaranteeing different content and therefore a different content ETag while preserving the decoded reference. The regression asserts both byte inequality and semantic equality.

let result = self
.object_store
.inner
.put_opts(

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 branch-termination and lifecycle checks are not atomic with this canonical create. Branch removal can cancel the pending lifecycle after line 1183, then this delayed create succeeds; lifecycle completion reports conflict but never removes the canonical object. New clients hide the generated object and cannot reuse the name, while released clients can see an orphan child of the deleted branch. The write needs a post-write ownership check with conditional rollback, or a generation boundary that atomically commits or rejects both objects.

Reproducer

I added a temporary regression using the existing admission helpers. It created a child admission, executed the same termination and Pending checks above, called remove_branch_state, executed this PutMode::Create, observed complete_reference_lifecycle == false, and asserted that the canonical child did not survive:

CARGO_TARGET_DIR=/home/agent/cache/cargo-target cargo test -p lance checked_child_create_cannot_publish_after_parent_state_removal --lib -- --nocapture

Expected the final nonexistence assertion to pass. The command exited 101 with a canonical child must not survive after its checked lifecycle is cancelled.

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 9097168. Lifecycle completion and the second branch-incarnation check now occur inside canonical mutation application. A create that loses ownership is conditionally fenced and removed, while an Update is conditionally restored; the delayed-child regression verifies no orphan canonical survives.

};
Ok(match snapshot.state {
ReferenceLifecycleState::Live { live, .. } => {
generation.as_deref() == Some(live.generation.as_str())

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.

Strict generation equality makes a released-client rewrite disappear. Released readers accept the unknown field, but their TagContents/BranchContents serialization drops it on update; this then hides the valid canonical reference while lifecycle retention continues protecting the old target, so cleanup can delete the version the released client just referenced. Rolling upgrades need generation mismatch to reconcile from authoritative canonical content (or another protocol that does not require old writers to preserve a hidden field).

Reproducer

A temporary exact-head regression created generated tag and branch payloads, deserialized and reserialized them through structures containing only the released fields, overwrote both canonicals with the released-client payloads, then required current visibility:

CARGO_HOME=/home/agent/tmp/lifecycle-reverify.KNr4b4/cargo-home \
CARGO_TARGET_DIR=/home/agent/tmp/lifecycle-reverify.KNr4b4/target \
cargo test -p lance --lib \
  dataset::version_lease::tests::released_client_rewrite_keeps_generated_references_visible \
  -- --exact --nocapture

Expected both references to remain visible. The command exited 101 with released-client rewrites must remain visible: tag=false, branch=false.

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 9097168. Generation-less canonical JSON is now treated as an authoritative released-writer rewrite, so it remains visible and enters the canonical cleanup census even when a generated sidecar exists. The regression covers both released tag and branch serialization.

Comment thread rust/lance/src/dataset/version_lease.rs Outdated
});
}

object_store.delete(canonical_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.

Persisting Deleting before this fallible delete creates an unrecoverable state. If the canonical delete fails or the process stops here, current readers immediately hide the still-present reference; retries start from an invisible snapshot and return not found, and no code transitions an abandoned Deleting state back to live or finishes it. Make deletion retryable by recovering from canonical/state contents, and roll back Deleting when a definitive canonical-delete failure leaves the object present.

Reproducer

I added a temporary test that published a generated tag, injected a canonical delete failure through FailingProxyStore, cleared the failure, and asserted that the still-present payload remained visible:

CARGO_TARGET_DIR=/home/agent/cache/cargo-target cargo test -p lance deletion_failure_does_not_strand_reference_lifecycle --lib -- --nocapture

Expected visibility to remain true. The command exited 101 with a failed delete must not hide the still-present reference.

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 9097168. Deleting now records the prior live or legacy state, keeps a still-present canonical visible, resumes an interrupted deletion, and conditionally restores the prior state after a definitive delete failure. The injected-failure regression verifies visibility and a successful retry.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in 9097168. Canonical and lifecycle state now reconcile across partial publication, deletion, and mixed-version rewrites: stale CAS fencing changes bytes, lost-ownership writes roll back, Deleting is visible/retryable or restored after failure, and released-writer JSON remains authoritative and protected by canonical census.

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

Labels

A-python Python bindings bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cleanup_old_versions can delete files held by long-running readers

0 participants