fix: protect active dataset versions from cleanup - #8409
fix: protect active dataset versions from cleanup#8409lance-gatefixer[bot] wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
❌ 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.
| // 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?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Err(error) if error.is_not_found() => return Ok(HashSet::new()), | ||
| Err(error) => return Err(error), | ||
| }; | ||
| let now_micros = utc_now().timestamp_micros(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
❌ 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)?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
❌ 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.
| .await?; | ||
| let retained_versions: HashSet<_> = versions_to_delete | ||
| .intersection(&leased_versions) | ||
| .filter(|version| !forced_retirement_versions.contains(version)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| paths.push(metadata.location); | ||
| } | ||
| } | ||
| self.delete_paths(paths).await |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
❌ 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.
| ) | ||
| .await | ||
| { | ||
| self.object_store().delete(&tag_file).await?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| .map(|tag| tag.version) | ||
| .collect::<HashSet<_>>(); | ||
| referenced_versions.extend( | ||
| self.find_referenced_branches() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
❌ 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.
| .await | ||
| { | ||
| Ok(_) => { | ||
| admission.complete().await; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
❌ 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.
| if self.object_store.exists(&manifest_path).await? | ||
| && !self.has_committed_marker(intent.version).await? | ||
| && self | ||
| .apply_reference_mutation(&intent.intent.mutation) |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
❌ 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.
| } | ||
| } | ||
| ReferenceMutation::Update { path, payload, .. } => { | ||
| self.reference_mutation_conflict_outcome(&Path::parse(path)?, payload) |
There was a problem hiding this comment.
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 --nocaptureThe 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).
There was a problem hiding this comment.
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?; |
There was a problem hiding this comment.
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 --nocaptureThe 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}.
There was a problem hiding this comment.
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.
| .inner | ||
| .put_opts( | ||
| path, | ||
| Bytes::from_static(REFERENCE_TOMBSTONE_PAYLOAD).into(), |
There was a problem hiding this comment.
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 --nocaptureObserved 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.
There was a problem hiding this comment.
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.
| if let Some(snapshot) = reference_snapshot(self.object_store(), &branch_file).await? | ||
| && !snapshot.is_tombstone() | ||
| { | ||
| tombstone_reference(self.object_store(), &branch_file, &snapshot).await?; |
There was a problem hiding this comment.
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 --nocaptureThe deletion removed the intent. The healthy assertion expects the resumed mutation to return Conflict; this head returned Published.
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
❌ 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 -- --nocaptureExpected the final nonexistence assertion to pass. The command exited 101 with a canonical child must not survive after its checked lifecycle is cancelled.
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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 --nocaptureExpected both references to remain visible. The command exited 101 with released-client rewrites must remain visible: tag=false, branch=false.
There was a problem hiding this comment.
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.
| }); | ||
| } | ||
|
|
||
| object_store.delete(canonical_path).await?; |
There was a problem hiding this comment.
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 -- --nocaptureExpected visibility to remain true. The command exited 101 with a failed delete must not hide the still-present reference.
There was a problem hiding this comment.
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.
|
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. |
Summary
Root cause
cleanup_old_versionshad 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 --libcargo test -p lance recovery --libcargo test -p lance cleanup_lineage --libcargo test -p lance auto_clean_referenced_branches --libcargo test -p lance recovered_retirement_still_respects_descendant_branch --libcargo test -p lance reference_intent_blocks_retirement_commit --libcargo test -p lance cleanup_resumes_sealed_retirement --libcargo test -p lance can_recover_delete_failure --libcargo test -p lance refs --libcargo test -p lance test_tag --libcargo check -p lance --testscargo fmt --all -- --checkcargo clippy --all --tests --benches -- -D warningsmake buildfrompython/uv run pytest python/tests/test_dataset.py::test_cleanup_retains_active_version_leaseuv run pytest --doctest-modules python/lance/dataset.py::lance.dataset.LanceDataset.acquire_version_leaseuv run make formatfrompython/uv run make lintfrompython/git diff --checkFixes #6607