[blarghmatey]

Refs #644 and #1095. Before this change, cleanup read and decoded every __manifest version of every live branch on every run, whatever the policy: plan_collection called CollectorBranch::versions, which is Lance's Dataset::versions, and that lists _versions/ and then reads each manifest one after another. Nothing deletes those versions yet, so the plan grew by one read per graph commit forever. On our Production store, measured 2026-10-28, that is 15,832 __manifest versions out of 42,354 _versions/ objects, at about 0.19 s per sequential GET on S3, so about 3,000 s of every plan goes to this read before the first table is traced (the table side of the same plan is #1095).

Every policy now takes each live branch's version numbers from Dataset::version_refs (CollectorBranch::version_numbers), which lists _versions/ and reads no manifest, as Lance's retain_n_versions does since lance#9019; this is the "Reads" step of the fix recommended on #644, without the delete. The newest-N, HEAD and branch-base rule decides from the numbers alone (retained_by_position). Only under --older-than, and only for the versions that rule leaves undecided, is a manifest read for its commit time: through checkout_version, as many at once as the store's io_parallelism allows (64 on S3, 8 locally; CollectorBranch::read_versions). The times cannot be binary-searched, because Lance stamps each manifest with its writer's clock. So --keep N alone reads the manifests of the versions it retains and of the merge base, and --keep N --older-than D, the form every documented example uses, reads the times of the versions below the keep floor concurrently instead of every manifest one after another. CollectorCost::manifest_time_reads counts those reads. The head and each retained version below it are still opened (manifest_snapshots).

maintenance::cleanup_plans_from_the_manifest_listing_issue_644 builds main plus a live feature branch tree and plans --keep 2 and --keep 2 --older-than 0s at two history depths, under a __manifest store probe that also sees Lance's direct local reads. Per branch it checks that the listing names every attached manifest on disk, that --keep reads manifests only of the versions it retains and of the merge base (main's commit at the fork, which retained_merge_bases keeps), that the cutoff reads every version below the floor and manifest_time_reads equals their count, growing by one per new commit, and that both plans retain, prune, root and sweep the same versions. With the old read path restored it fails: 12 manifests read for 5 retained. collector_cost_is_bounded_by_retained_versions_and_garbage_examined asserts manifest_time_reads is 0 under --keep, and retention_tests covers the rule. In the DST storage-cost golden, Cleanup l.get goes 163 → 147, and _audit 693 → 679 and _verify 1520 → 1488, which run the same collector plan with keep_versions: 1 as their oracle; l.list does not move, since version_refs lists the directory versions listed.

For review: the decision to challenge is reading commit times through checkout_version. Each read is a HEAD plus a GET, where Dataset::versions did one GET with the size from the listing, but the reads run concurrently. A version the cutoff retains is read again by snapshot_at, which the session cache does not serve, so it costs four requests. A corrupt manifest the run prunes unread no longer refuses a --keep plan, since its pins are not roots either way; every manifest the run reads must still decode, and --older-than still refuses (documented in docs/dev/lance.md). DST cannot exercise an --older-than cutoff: Lance stamps __manifest commits with SystemTime::now(), while the DST cutoff comes from the logical clock, so every version is newer than the cutoff and retained. I have not measured the patched build against S3, so the ~3,000 s figure is the Production manifest count times the per-object latency we measured on 0.13.0, not a timing. Nothing here deletes a __manifest version or changes the snapshot_at term, and the store format, CLI and HTTP surface are unchanged. #644 still carries needs-triage rather than accepted; I opened this because the analysis there says the version_refs switch does not wait on the delete work.

Run locally on the head: the maintenance suite, the collector failpoint and unit tests, dst_bench_cost_count_golden, clippy with -D warnings, fmt, typos and check-docs. Not run locally: the workspace gate, GQT, and the S3 and Azure suites.

https://claude.ai/code/session_01FGZebF1Mp287NfGK4u4qbc

[ragnorc]

I pushed one commit on top of yours (7eb2b5dc) instead of leaving review notes, because the main point was a design change.

What it changes

• The split by policy is gone. Every policy now gets version numbers from version_refs. The newest-N / HEAD / branch-base rule decides from numbers alone. A manifest is read for its time only under --older-than, and only for versions that rule leaves undecided. Those reads go through checkout_version, as many at once as the store's io_parallelism allows (64 on S3, 8 locally). The times can't be binary-searched, because Lance stamps each manifest with its writer's clock. The reason for the change: every documented command (the maintenance guide, the CLI guide, the skill) is --keep N --older-than 7d, which still read every manifest one after another.
• ManifestVersion and its optional timestamp are gone. retained_by_position replaces them and has a unit test.
• CollectorCost::manifest_time_reads counts the time reads. The existing collector-cost test now asserts it is 0 under --keep.
• Your test now runs at two history depths. Under --keep, reads stay within the retained versions plus the merge base, so they don't grow with history. Under the cutoff, time reads equal the undecided count and grow by one per commit.

What testing turned up

• --keep also reads the merge base. With deeper history, --keep reads main's commit at the fork point. That is the merge base, which retained_merge_bases keeps by a separate rule. Your fixture was too shallow to show it; the test now allows exactly that read.
• Each time read costs two requests. It is a HEAD plus a GET, where the old path did one GET, but the reads now run concurrently. A version kept by the cutoff is read four times, because snapshot_at doesn't hit the session cache for it. I didn't change that here.
• Corrupt pruned manifests are now skipped. Before, a corrupt manifest below the keep floor made --keep refuse the plan; now it is pruned unread, and --older-than still refuses. That's safe, since a pruned version's table versions aren't protected either way. It's documented in docs/dev/lance.md.
• The simulation tests (DST) can't exercise --older-than. Lance stamps __manifest commits with the real SystemTime::now(), while the DST cutoff comes from the logical clock (epoch 2026-01-01). Every version is therefore newer than the cutoff and kept. Fixing that is a separate change.

Checked locally

• Your test passes on 072a14aa and fails with the old read path: on main it reads 8 manifests for 2 kept, 12 for 5 across both branches, as you said.
• On the new head: the maintenance suite (57), the collector failpoint tests (10) and unit tests, dst_bench_cost_count_golden (rows unchanged), clippy with -D warnings, fmt, typos and check-docs.
• Not run: the workspace gate, GQT, and the S3/Azure suites.

I've updated the description to match: the design paragraph, the tests, the review question and what ran locally.

[ragnorc]

Why

Cleanup can delete table data. The plan must keep every version the policy retains. --keep used to open every __manifest version to learn the version numbers. On a long branch that read dominates plan time.

What

Every policy now takes version numbers from the _versions/ listing (Dataset::version_refs, Lance 13.0.0). retained_by_position keeps the highest version, the newest N (at least one), and the oldest version of a named branch. A commit time is read only when --older-than is set, and only for versions that rule leaves undecided. Those reads use Dataset::checkout_version, up to the store I/O parallelism. That is 8 on a local store and 64 on object storage.

Retention

The position rule is the same comparison as the old loop. I ran one history on the merge base 2ca84a79 and on this head.

Main kept versions 8 and 9. Main pruned versions 1 through 7. The feature branch kept versions 6, 7 and 8 and pruned none. --keep 2, --keep 2 --older-than 0s and --keep 2 --older-than 2s produced those sets on both commits. Each policy also produced the same root counts and the same sweep lengths. Those counts are 1, 1, 1 and 5 roots, and 1, 1, 2 and 6 swept versions. Fresh table ids differ per process.

HEAD is the maximum listed version. Newest N uses the same floor index as before. A keep of 0 still keeps HEAD. A named branch keeps its oldest listed version. retained_merge_bases still pins each merge base after that rule. Main version 6 is in would_prune and is still read. Tags, unpublished commits and unattached snapshots still record pins on their own path. The plan rechecks the branch set and each branch head. A change returns a manifest conflict before any sweep.

A version published after the open is listed and then dropped when it is above the opened head. The recheck then sees the new head. CLEANUP_COLLECTOR_MID_BRANCH_READ still fires after the listing and before rows, time reads and the recheck. collector_refuses_mixed_branch_captures_before_deleting passed on this head.

A listing that omits a version at or below the opened head leaves that version out of both sets. Dataset::versions used the same list_manifest_locations call. The recheck does not see a missing middle key. This change does not widen that race.

Corrupt pruned manifests

This is a real contract change. Dataset::versions reads every manifest, so a corrupt body below the floor used to fail the plan. --keep on this head does not read that body. I corrupted main version 1, which is not the merge base. The merge base refused the next --keep 2 plan at lance-table manifest.rs:61. This head accepted the plan. The retained set and the sweep matched the plan from before the corruption. --older-than 0s on this head still failed. The error is a Lance I/O error on the bytes not a manifest. A corrupt retained manifest still fails in snapshot_at. A corrupt merge base or tag still fails, because those paths still open the manifest. docs/dev/lance.md says this. The changelog and the user page do not.

Cost

On the merge base, --keep read every pruned main version once. On this head those versions had no manifest read, except the merge base. Under --older-than, each undecided version was two get_opts calls. Lance's tracker records an object_store HEAD as get_opts. default_resolve_version in Lance 13.0.0 does that HEAD, then the GET. Version 6 was read for its time and again as the merge base. That was four get_opts calls. The session cache did not remove the second read.

version_refs keeps the version number and drops the size the listing already returned. The later read has to HEAD. checkout_version also builds a Dataset and can insert the manifest into the shared session cache. That includes manifests the cutoff will prune. read_version_transaction does not fill that cache. It also opens an inline transaction, which reads the manifest again. Lance has no public call that returns only the timestamp and uses the listed size. checkout_version is a fair choice here. A later scan that only needs a timestamp should not copy it.

The golden dst_bench_cost_count_golden passed on this head in 6.45s. Cleanup l.get is 147, _audit l.get is 679, and _verify l.get is 1488. l.list is unchanged. I did not re-run the golden on the merge base. DST still cannot exercise --older-than. Lance stamps the manifest with SystemTime::now(). The DST cutoff uses the logical clock, epoch 2026-01-01. Every manifest is newer than that cutoff. The golden covers keep_versions: 1 only.

I did not time this on S3. The request counts above are from a local file. The large wall-time figure in the discussion is not a measurement of this build.

Tests

GQT cannot see a cleanup plan, a manifest read, or a corrupt manifest body. The Rust owners are the right tier.

Passed on 58e05f2e318f81be0292741da1a97db6b6692382:

• maintenance::cleanup_plans_from_the_manifest_listing_issue_644 (0.37s)
• maintenance::collector_cost_is_bounded_by_retained_versions_and_garbage_examined (0.12s)
• db::omnigraph::collector::retention_tests::version_numbers_decide_head_newest_and_base
• collector_refuses_mixed_branch_captures_before_deleting with --features failpoints (0.29s)
• dst_bench_cost_count_golden from crates/omnigraph-dst (6.45s)

The same history on merge base 2ca84a79243610259b149ebd2b31de661f499f0f produced the retained and pruned sets above. A corrupt pruned manifest failed that plan. The temporary oracle is not in the branch.

Liability

retained_by_position is one function with a unit test. That replaces a second copy of the keep rule. The new machinery is the time read, the counter manifest_time_reads, and the choice to build a Dataset per undecided version. Five more scans of that shape would fill the control session cache and repeat this HEAD. The owner that would hold the rule is one listing of version, size and path, then one GET for a timestamp, with no Dataset. Lance does not offer that call. version_refs drops the size. A private read_manifest would couple Omnigraph to Lance internals. The public checkout_version call is the smaller coupling for this change.

The quality this change spends is peak memory and the request count on the --older-than path. It spends them to cut plan latency when many versions are undecided. The retention set for a readable manifest stays the same. The fail-closed read of a corrupt pruned manifest is dropped for --keep only.

This change does not delete __manifest versions. Issue 644's physical reclaim stays open. I do not see a conflict with the open maintenance PRs. A future Lance that makes version_refs read bodies would remove the win and keep the same numbers. A future Lance that includes detached versions in version_refs would change the set. The method documents that exclusion today.

Recommendation

Ready. The optional notes below are doc and test gaps. They do not block the retention result.

Optional

1. The user page, docs/dev/lance.md, and the changelog say --keep reads only the commits it retains. It also reads each merge base. That version can sit in would_prune. The changelog also omits the corrupt-pruned contract change.
2. checkout_version does a HEAD and a GET, builds a Dataset, and can cache every undecided manifest. Please classify the requested listing number. The body version can disagree, and checkout does not check it.
3. read.is_subset accepts a --keep plan that reads no retained manifest. Please assert equality with the retained set plus main's merge base. Please add the corrupt pruned manifest case. --keep should keep the same sweep. --older-than should fail.

Inline comments

docs/dev/lance.md:265 — Please include the merge base in this sentence. --keep reads each merge base, and that version can sit in would_prune. Main version 6 did that in the fixture. The same claim is in docs/user/operations/maintenance.md and in the changelog note. The unread-prune sentences below are accurate. The changelog and the user page should also say that a corrupt pruned manifest no longer fails a --keep plan.

crates/omnigraph-catalog/src/lib.rs:1216 — checkout_version is a HEAD plus a GET (default_resolve_version in Lance 13.0.0), then a full Dataset. When the HEAD returns a size, the manifest goes into the shared session cache. That includes versions the cutoff will prune. On a local plan each undecided version was two get_opts calls. Lance records HEAD as get_opts. Version 6, read for time and again as the merge base, was four. The cache did not serve the second read. version_refs drops the size the listing already had, so the public API has to HEAD. read_version_transaction skips the cache and opens an inline transaction again. I would keep this call for this change. A later scan that only needs a timestamp should not copy it. Please classify the requested version number. dataset.version() is the body version. Checkout does not check that it matches the path. A body that decodes with a different number can drop the listed version out of both sets. Use the body only for the timestamp.

crates/omnigraph/tests/integration/maintenance.rs:4472 — is_subset accepts a plan that reads no retained manifest. Please assert that the --keep read set equals the retained versions plus main's merge base. Please also keep a case where a manifest below the floor is corrupt and is not the merge base. --keep should return the same sweep. --older-than should fail. I ran that case on this head and on 2ca84a79. It is not in the PR.