Cleanup leaves a table version's manifest readable after deleting its files #1094

Activity

1. github-actions

   added

   needs-triageAwaiting maintainer triageAwaiting maintainer triage

   on Oct 19, 2026
2. ragnorc commented on Oct 20, 2026

   

   ragnorc

   on Oct 20, 2026 – with Cursor

   ContributorAuthor

   More actions

   Summary

   cleanup deletes the files of a swept table version, and then deletes the manifest. A stop between those steps leaves the manifest in _versions/. Lance opens that manifest. Objects it names are already gone. One fact, "cleanup reclaimed this version", then has two engine answers. A missing manifest is HistoricalVersionReclaimed. A present manifest with a missing data file is a raw storage NotFound.

   The property we want: a manifest in _versions/ names only objects that are present. I reproduced the hole on main at d4aa7fe9f2c55719096441a8c3aa02219ebecef4. No open pull request implements a fix. This issue stays open. No other issue supersedes it.

   Reproduction

   Reproduced, on all three claims. Environment: Linux 6.12.94+, rustc 1.97.1, Lance 13.0.0, a local file:// temp directory.

   Command (a local test, not committed):

   cargo test -p omnigraph-engine --features failpoints --test repro_issue_1094 --locked -- --nocapture half_swept_historical_read_should_be_typed_reclaimed
   

   Fixture: init a Person graph, overwrite-load Alice and Bob, ensure_indices, overwrite-load Zoe, then cleanup with keep_versions: Some(1). The seam cleanup.sweep_pre_manifest_delete stops the sweep after delete_paths and before the manifest loop. That seam is the production line in sweep_table.

   One run:

   • The plan's sweep paths were one data file, three _indices/ directories, and four _transactions/*.txn files. The load pin was in the sweep.
   • The interrupted row had manifests_removed=0, bytes_removed=3269, and error injected failpoint triggered: cleanup.sweep_pre_manifest_delete.
   • _versions/ still held d16171805821540867662.manifest. The data file was gone.
   • DatasetBuilder open of that version succeeded.
   • A scan failed with Lance NotFound for .../data/110001111001100000100100a8567243ba8f2f1a70e41965ea.lance. The message continues with rustc source frames from the error chain.
   • The retained head still returned Zoe (graph_manifest_version=4).
   • entity_at of Alice at the load commit (graph_manifest_version=2) returned Storage(NotFound) with that same Lance message.
   • branch_create_from at that commit returned BranchStartRefused { NotRetained { table_key: "node:Person", published_dataset_version: 2 } } with CompletionEvidence::BeforeEffect. branch_list stayed ["main"].
   • A second cleanup, with the seam clear, removed 4 manifests. The load manifest was gone. The same entity_at returned HistoricalVersionReclaimed(16171805821540867662): "historical published dataset version 16171805821540867662 was reclaimed".

   A temporary assertion required the half-swept read to be HistoricalVersionReclaimed. That assertion failed. The controls above passed. The test is not in the tree.

   Difference from the report: none on the three claims. Two extra observations:

   1. The branch-start refusal names the registration's published_dataset_version (2). The reclaimed-read error names the detached id that open requested. Both mean this pin.
   2. commit_changes_page and the change feed rewrite only HistoricalVersionReclaimed into ChangeFeedGap. A half-swept NotFound stays a storage error there. I read the match. I did not run the feed.

   Root cause

   sweep_table deletes sweep_paths, then the published manifests, then dead_paths, then the dead manifests, then orphans: collector.rs.

   The detached-only tables RFC, step 4, chose this order so a stopped pass leaves the manifest as the list for the next pass: RFC.

   open_dataset maps VersionNotFound and DatasetNotFound to HistoricalVersionReclaimed. A bare NotFound stays storage. The comment there says a live manifest that names a missing object is corruption, and must stay loud: instrumentation.rs.

   Branch start probes every object of every pin the source head does not hold. The head's pins are roots. The collector leaves a root's files in place. The probe runs under the exclusive schema permit: omnigraph.rs, writes.md. The cost gate is one data read per added fragment (branch_start_at_a_commit_probes_once_per_pinned_object in integration/branch_control_cost.rs). I did not re-run that gate.

   The server maps both HistoricalVersionReclaimed and Storage to HTTP 500, with different messages. Branch start maps NotRetained to 410. The change feed maps the typed reclaimed error to 410 change_feed_gap.

   Design

   Put the property on the collector. A manifest that readers can open must name only present objects. Hide every swept manifest from _versions/ before any swept file is deleted.

   Use a batch form of option 2:

   1. Copy each swept manifest to a collector prefix outside _versions/, for example _omnigraph/sweep/<version>.manifest. Lance does not list that prefix. Stock cleanup lists _versions/, _transactions/, data/, _indices/, and _deletions/. orphan_key does not claim this path, so an old collector keeps the copy.
   2. Delete the visible manifests.
   3. Delete sweep_paths.
   4. Delete the copies.

   Use the same order for dead stagings. They use files-then-manifest today.

   Resume from the listing:

   • A copy exists and the visible manifest exists. Files are still present. Delete the visible manifest, then the files, then the copy.
   • A copy exists and the visible manifest is gone. Delete the remaining files, then the copy.
   • No copy exists and the visible manifest exists. Start at step 1.
   • Delete a swept file only after every visible manifest that names it is gone. Two swept manifests can share a file. Hiding one manifest and then deleting "its" files can remove a file that another visible manifest still names.

   A crash in step 1 or 2 leaves every still-visible manifest readable. A crash in step 3 leaves no swept manifest visible, so readers get HistoricalVersionReclaimed. The next run finishes the files. A crash in step 4 leaves a small copy. The next run deletes it.

   This is an on-storage protocol. It needs an amendment to the detached-only tables RFC. Each window needs a seam in failpoints.rs, a row in detached_commit_matrix.rs, and a case in the DST collector oracle.

   Alternatives

   Option 1. Delete the manifest first, and let the orphan rule delete the files. Readers get the typed error at once. A crash leaves files that no manifest names. The orphan rule deletes them after 7 days (UNVERIFIED_THRESHOLD_DAYS, Lance's unverified window). That rule exists so the collector does not delete a writer's files before the manifest that will name them. Option 1 makes the rule the normal end of a sweep. A version swept with --keep 1 can be minutes old. Its files then wait for the age gate. Invariant 5's progress clause says a table version that no retained __manifest names is gone after a run that retains nothing else. Option 1 meets that clause only after the age gate. #1095's nightly window already kills runs at 600 seconds, so the space would sit until the files age out.

   Option 3. Keep this order. Probe on the read path, or map NotFound to the typed error. This adds no storage protocol. The hole stays. A map from every historical NotFound to HistoricalVersionReclaimed also hides corruption of a retained version. The collector's root set is the authority for that split. A reader that rebuilds the root set does a second collection. The branch-start probe is already that cost, under the schema permit, and only for pins the head does not hold. The same probe on every historical read is one request per object on the read path.

   Tradeoff

   Option 2 adds one put and one delete per swept manifest, and a larger crash matrix. #1095 shows that cleanup time tracks manifest opens during planning, not deletions. The extra requests are small beside that cost. We spend them so the reader contract and the next-run progress both hold. A wrong historical answer is a correctness failure. Saving two requests is the wrong place to give up either property. Sweep efficiency is the quality we give up, on purpose.

   Liability added: a collector-owned prefix that later cleanup must finish, and that must stay outside Lance's version listing. An old binary does not finish copies. It also keeps them, because the path is an unknown layout. Files that no manifest names still fall under the 7-day orphan rule, including on an old collector. Readers are safe once the visible manifest is gone. Liability removed: the half-swept read, the second answer on the change feed, and the per-object branch-start probe.

   Test plan

   1. Add seams for "copy written", "visible manifest deleted", "files deleted", and "copy deleted". Run them on file, RustFS/S3, and Azurite, as the collector retry owners do now. In each window: a retained root still reads; cleanup_plan_missing_paths is empty; a historical read of a hidden version is HistoricalVersionReclaimed; a historical read of a still-visible swept version succeeds; the retry removes the files and the copy.
   2. Keep historical_read_of_reclaimed_version_is_typed. Add the hidden-manifest window beside it. The error class is the same. The window is a mechanism, so the failpoint case belongs in failpoints.rs.
   3. Update branch_start_refuses_a_commit_an_interrupted_cleanup_emptied. After the hide seam, the manifest is absent from _versions/. The refusal stays NotRetained, from HistoricalVersionReclaimed on open.
   4. Update branch_start_at_a_commit_probes_once_per_pinned_object. Extra object reads go flat. One open per probed pin remains.
   5. Cover the new windows in the DST collector oracle. If Cleanup in crates/omnigraph-dst/cost_table.txt changes, the pull request explains each changed line.
   6. Update the branch-start paragraph in docs/dev/writes.md, amend the RFC, and add a changelog note. The user-visible change is the error class after a stopped cleanup. A historical HTTP read stays 500 unless we later map HistoricalVersionReclaimed to its own status. That map is a separate decision. Branch start already returns 410.

   Lance

   Lance does not fix this. OmniGraph pins unmodified Lance 13.0.0. No open OmniGraph pull request upgrades Lance.

   Lance 13 cleanup_old_versions deletes unreferenced files and old manifests in one concurrent stream. The order is not manifest-first. The same stream is on Lance main (the v14.0.0-beta.21 line). See delete_unreferenced_files at v13.0.0 and the combined stream on main.

   lance#8944 (merged) makes old data files eligible while a detached manifest remains. That is this hole in stock cleanup. OmniGraph does not call cleanup_old_versions on a graph table.

   lance#8097 (open) adds Dataset::referenced_files() for an external orphan driver. The set covers every present manifest. It does not change delete order. A driver that deletes files of a manifest that is still present creates the same hole.

   Recent Lance cleanup commits (#9774, #9747, #9709) change concurrency, temporary files, and missing-object handling. They do not give this property.

   Lance's 7-day unverified threshold is the age gate option 1 would use. Lance's branch guide says cleanup keeps files a branch still references. It does not promise that a present manifest's objects exist during cleanup. The collector is the right place for the property. Stock cleanup does not trace detached manifests.

   Related work

   • #1016 (merged) added the branch-start probe because of this order.
   • #1095 (open): planning opens every version before any delete, so a killed run reclaims nothing. Option 2 does not fix the planning cost. A kill during the sweep can hide manifests before the process dies, so readers converge while files wait for the next run. The protocol stays inside sweep_table, after capture validation. It stays valid if performance: cleanup opens every table version before deleting any, so a run cut short reclaims nothing #1095 later sweeps a table when its trace finishes.
   • #1082 and #646: on a served cluster, cleanup runs only while the server is stopped. Readers see this hole after restart, until the next cleanup. Served cleanup would show the hole during every sweep. Option 2 does not need that route. It matters more once the route exists.
   • #1093 (open RFC): storage outcomes belong to the operation that sent them. The copy is a re-issuable put of known bytes. delete_object already treats NotFound as success. Delete the visible manifest only after the copy put has succeeded. Delete files only after a read shows the visible manifest is absent. This issue can land before that RFC. The crash matrix should treat an ambiguous copy put as "the copy may exist".
   • #1017: the accepted graph-access RFC. It keeps the single-writer boundary. Cleanup stays one writer process. It does not fold history extents.
   • #1068: a cold commit list reads every __history extent. One direction is to fold small branch blocks. Those objects are not table data files. Folding them leaves this hole in place.
   • #644 reclaims old __manifest versions. That is a different dataset.
   • #958: cleanup can drop a version a live read still holds. That is a different failure.

   No open pull request changes this sweep order. No roadmap item replaces this issue. The owning decision is the detached-only tables RFC. I am not closing this issue.

   Limitations

   • The run used local file:// and the failpoint. It did not kill a process, and it did not use S3 or Azure. sweep_table is the same function on those stores.
   • I did not re-run the failpoints.rs binary or the branch-start cost gate. This run called the public branch_create_from path and got the same refusal.
   • I did not run a change-feed poll. The ChangeFeedGap note is the match in changes/feed.rs and commit_changes_page.
   • The temporary test is not in the tree.