performance: cleanup opens every table version before deleting any, so a run cut short reclaims nothing #1095

Activity

1. github-actions

   added

   needs-triageAwaiting maintainer triageAwaiting maintainer triage

   on Oct 19, 2026
2. blarghmatey

   added 2 commits that reference this issue on Oct 20, 2026

   feat(omnigraph): run the window's cleanup on a list of weekdays (#519)

   ...

   f98b538

   fix(omnigraph): clean up every night at 2d so the window can finish (#…

   ...

   9d04c95
3. ragnorc commented on Oct 20, 2026

   

   ragnorc

   on Oct 20, 2026 – with Cursor

   Contributor

   More actions

   Summary

   cleanup decides every delete before it deletes anything. On current main that decision opens every published table version about twice, including versions the policy keeps. A run that stops during the decision deletes nothing. A graph whose plan does not fit the maintenance window is never cleaned, and the backlog grows.

   The safety rule is right: one capture, then deletes. The cost is wrong. The cost follows versions that exist, not versions this run will remove.

   Reproduction

   Checked out 7fd9cc9037c634e2f3284f1ede55a85a6f7b28e8 (7fd9cc90, fix(storage): keep a bound handle's re-reads on its own session). Issue #1095 was open, with no comments and no pull request that implements a fix. The same was true immediately before this comment.

   Environment: Linux, rustc 1.97.1, debug (unoptimized) test profile, one local file:// graph, one Person table. This is not S3, not a cluster, and not 0.13.0. The harness built the graph with Omnigraph::init and N inserts, then called Omnigraph::cleanup_plan and Omnigraph::cleanup. The harness was local only. It is not in the tree.

   cargo test -p omnigraph-engine --features failpoints --test issue_1095_repro -- --nocapture --test-threads=1
   

   older-than 30d keeps every new graph commit. keep 1 keeps the head.

   Writes	Policy	Present	Published	Roots	Would remove	table_opens	manifest_snapshots	Cold plan
   8	30 days	9	9	8	1	19	9	31 ms
   8	keep 1, same graph, warm session	9	9	1	8	19	1	10 ms
   32	30 days	33	33	32	1	67	33	127 ms
   32	keep 1, same graph, warm session	33	33	1	32	67	1	24 ms
   32	keep 1, fresh graph	33	33	1	32	67	1	28 ms

   cleanup with the 30-day policy removed 1 version at both sizes (33 ms and 125 ms). That matches would_remove. One published detached version is not a root. I did not name that version's write.

   A second test armed cleanup.collector_post_snapshot so plan_collection returns an error before the table trace:

   INTERRUPT error=injected failpoint triggered: cleanup.collector_post_snapshot manifests_before=19 manifests_after=19 would_remove=8
   AFTER_PLAN removed=8 manifests_before=19 manifests_after=11
   

   The failing check on current main: a 30-day plan may open only the versions it will remove, plus a small fixed budget. The run opened 67 versions to remove 1. The budget was 10.

   Claim by claim

   1. Time tracks versions present, not versions removed. Reproduced for table_opens and for the 30-day series. Present versions went from 9 to 33 (3.7 times). table_opens went from 19 to 67 (3.5 times). Plan time went from 31 ms to 127 ms (4.1 times). Removals stayed 1. The same 33 versions produce 67 opens under both policies, while removals are 1 or 32. Local cold time does not follow table_opens alone. The 30-day plan also opens every retained __manifest version (manifest_snapshots 33 versus 1) and took 127 ms. The fresh keep 1 plan took 28 ms with the same 67 table opens. The 0.14 s per table version on S3 was not reproduced. Local opens are about 0.4 ms to 3 ms.

   2. cleanup_detached_only finishes plan_collection before the first sweep_table. Reproduced. optimize.rs line 801 plans. line 837 sweeps. The v0.13.0 shape is the same (optimize.rs around the lines the report cites).

   3. Locations are traced one at a time. The sweep runs up to OMNIGRAPH_MAINTENANCE_CONCURRENCY. Reproduced. The trace is a sequential loop at collector.rs line 1201. The sweep uses buffer_unordered at optimize.rs line 863. The default concurrency is 8 (optimize.rs line 39).

   4. discover_published opens every published version. Each root is opened again. plan_sweep opens each sweep candidate again. Reproduced. Chain walk: collector.rs line 1507. Root paths: line 1339. Sweep candidates: line 1852. For 33 published versions the count is 2 * 33 + 1 linear root = 67. A kept version is opened in the chain and again as a root. A removed version is opened in the chain and again in plan_sweep. sweep_table then opens one root per location (line 300). That extra open is per table, not per version. The v0.13.0 discover_published has this same chain open.

   5. CollectorCost.table_opens counts this. --json omits it. A run without --confirm does not plan. Reproduced. The counter is CollectorCost.table_opens. JSON rows are built in output.rs line 196 and have no cost field. Without --confirm, maintenance/mod.rs line 146 writes the policy and returns before open. Omnigraph::cleanup_plan already plans with no writer gate (omnigraph.rs line 3586). The CLI does not call it. The CLI text has moved since v0.13.0 (main.rs to these files). The behavior matches the report and maintenance.md.

   6. A run cut short during planning deletes nothing. Reproduced for a failure after the graph capture and before the table trace. I did not kill a process inside the per-version loop. That loop has no seam. sweep_table runs only after plan_collection returns success, so a kill inside the loop has the same result: no sweep has started.

   What differed

   S3 latency, the 18-graph cluster, the server stop window, and the 600 s kill were not run. Debug file:// time is not 0.14 s per version. Since v0.13.0 the same double open remains. Extra work before the table loop now includes unpublished commits and unattached branch tags. manifest_snapshots is a second history term the report did not separate. On this one-table graph it was the larger share of local 30-day plan time.

   The existing ceiling collector_cost_is_bounded_by_retained_versions_and_garbage_examined allows published + roots + sweep. That is about two opens per present version. The test passes while this bug is present.

   Root cause

   A retained root is already known from the __manifest versions the policy keeps (collector.rs line 1041). The _versions/ listing already names every present manifest (line 1459). listings stayed 4 at both sizes.

   discover_published still opens each chain member to read a predecessor that is already on the manifest. StagedTransactionIdentity::recorded_by reads {read_version}-{uuid}.txn from the manifest alone (staging.rs line 40, table_store.rs line 4036). The handle is then dropped. The root loop and plan_sweep open the same version again for fragment, deletion, transaction, and index paths.

   open_at is a full Dataset open (promotion.rs line 38). Lance resolves a detached version by path with no latest-version lookup. The open still builds a dataset. The shared session can make the second open cheap. The counter still records both. One uncached GET per present version is enough to explain about 0.14 s on S3 (3522 * 0.14 s ≈ 493 s, next to the reported 488 s).

   The capture check runs after every table inventory (collector.rs line 1234). Borrowed-file protection also waits for every table (protect_borrowed_files). Both are why a partial trace does not delete. They are also why a timeout deletes nothing.

   Recommended design

   Keep one capture, and check it immediately before deletes. Change what that capture has to read.

   1. The delete set is the listing minus the known roots, after staging judgment. Roots come from retained __manifest pins. Open a table manifest only when that version can change the delete set: a non-root candidate, or a root whose file list is required because this run will delete files. When every listed version is a root and no orphan is old enough, do not open table versions. That is the nightly case where the policy keeps almost everything.

   2. Parse each manifest once. Keep paths and the predecessor from that parse. Do not Dataset::open the same version again. Prefer the manifest bytes the listing already named (read_manifest in Lance), not a dataset session. The second open is pure waste even when the session cache hides its S3 cost.

   3. A short run should delete unpinned manifests after the capture check, and leave their files for a later mark. Deleting a manifest that no retained pin names does not require every root's file list. Deleting its files does, because a kept version may share them. File bytes then wait for a run that has read the mark set, or for the 7-day orphan rule. Version count drops, so the next plan is cheaper and can finish the mark. Do this only for versions the capture has classed as unpinned. A live or undecidable staging stays. Re-check branch head and tag inventory immediately before those deletes.

   4. Do not sweep a table as soon as its trace finishes. A publication during the remaining tables can pin a version this table already dropped. protect_borrowed_files is also global. Per-table sweep during the trace fights both. A bound fits the capture model: --max-tables or an explicit table list, the same shape Feature request: table scoping for optimize (--table / --exclude-table) #432 asks for optimize. One capture covers the subset. Delete a file only when borrowed-origin closure says it is local to finished tables. A killed run still deletes nothing. A finished subset shrinks the backlog.

   5. Trace independent locations concurrently, up to the sweep limit. Capture branch views and tag inventories first. judge_staging walks a view's lineage and mutates that cursor, so that walk stays before the parallel section, or each view has its own lock. table_tags is already keyed by dataset path.

   6. Put CollectorCost on cleanup --json, and add a plan-only CLI path through Omnigraph::cleanup_plan. The engine method exists and takes no writer gate. Say in the output that the plan deleted nothing and that a later publication changes the next plan. Without --confirm, today's command still returns before open. A plan flag is a different command from that preview.

   Caching the second open, or parallel traces alone, cuts a constant factor. The plan still grows with retained versions. That does not fix a 600 s window on thousands of kept versions.

   Tradeoff. One run no longer promises to reclaim every unpinned file in the whole graph. It promises a checked subset, and manifest removal for versions that are not roots. File bytes can outlive those manifests until a mark completes. That is the orphan rule's job. It is safer than today's file-then-manifest order, which leaves a readable manifest whose files are already gone (#1094). Robustness of the capture and the orphan rule stays ahead of "one run cleans every table inside the window."

   Liability removed. Full-history double opens. A timeout that never reclaims. Operators with no cost in the report.

   Liability added if the subset is wrong. A file delete before borrowed-origin closure can remove a file a kept snapshot still uses. Fail closed: no file delete until that closure is known. A plan-only command must not be read as a delete.

   The detached-only RFC acceptance text bounds discovery by present detached manifests. That text matches today's ceiling. A change that stops opening kept versions when they cannot affect the delete set should update collector_cost_is_bounded_by_retained_versions_and_garbage_examined and that threshold text in the same change. Safety (every kept pin still opens, every path it names still exists) stays.

   Lance

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

   Stock cleanup_old_versions lists attached manifests through list_manifest_locations. parse_version does not accept d*.manifest. Detached manifests have a separate list, list_detached_manifests. process_manifests uses only the attached list. Lance main (checked 2026-10-20) still does that. lance#8944 (merged, in this pin) makes old data files eligible once they pass max(retained manifest floor, now - 7 days), including files that only a detached version references. That is why graph tables must not use stock cleanup.

   Open Lance work does not take this cost over:

   • lance#8097 (Dataset::referenced_files, open) refuses a dataset that has detached versions, because their files would look like orphans. It also still reads every present manifest.
   • lance#9062 (open) is about orphaned staging manifests, not detached version graphs.
   • lance#9019 (merged before 13.0.0) skips manifest reads for retain_n_versions on the attached chain. It does not apply to detached pins.

   The useful Lance pattern is the one stock cleanup already uses for attached manifests: read each manifest once, concurrently, then delete. OmniGraph needs its own notion of which versions the graph keeps. Lance does not have that.

   Related work

   This issue stays open. Nothing else has the same scope.

   • #1094 is the file-then-manifest window: a present manifest whose files are already gone. Related, not the same bug. Manifest-first for unpinned versions avoids that window.
   • #1082 and #646 are maintenance beside a live server, and a served resumable operation. The report sets those aside. They do not remove these opens. A served cleanup with the same plan still dies at the same timeout.
   • #432 asks for --table / --exclude-table on optimize. The same selection is the right bound for cleanup, with the borrowed-file rule above.
   • #1093 is storage outcomes and restartable server ownership. It does not change the collector's read set.
   • #1068 is another cold cost that follows history (every merged branch's history block) on a different operation.
   • #1017 keeps the single-writer support boundary. It does not make this plan cheaper.
   • Draft RFC 0068 may later let pins be read without opening every historical __manifest version. It is not a fix for this issue, and it is still draft. Until then, the retained-commit scan (manifest_snapshots) remains.

   Test plan for a fix

   • Change collector_cost_is_bounded_by_retained_versions_and_garbage_examined so a 30-day plan's table_opens stays bounded by versions that can be removed, not by versions kept. Cover two sizes. Record manifest_snapshots beside table_opens.
   • Same graph, keep 1 and older-than: opens for kept versions are not repeated. keep 1 may still open versions it will remove.
   • cleanup.collector_post_snapshot: manifest set unchanged. A later run deletes. Add a subset run that deletes only its tables.
   • A borrowed-file case: a subset run does not delete a file an unfinished table still references. Owner: collector tests in maintenance.rs.
   • CLI: --json includes CollectorCost. Plan-only deletes nothing and does not take the writer gate. Owner: CLI maintenance tests.
   • If the read set of cleanup changes, update crates/omnigraph-dst/cost_table.txt and explain every changed line.
   • Keep the guard that production code does not call cleanup_old_versions.

   Limitations

   No S3 timing, no cluster lock, no 18-graph window. The interrupt is a failpoint before the table loop, not a kill on the Nth manifest GET. The single unpinned version in the 30-day plan was counted, not attributed to a specific write. Local manifest_snapshots dominated 30-day wall time. On S3, table-manifest GETs may dominate instead. Both terms scale with retained history.
4. blarghmatey

   added a commit that references this issue on Oct 21, 2026

   fix(omnigraph): only optimize in Production's nightly window (#6520)

   ...

   7e5ed14
5. aaltshuler

   added

   acceptedTriaged and validated; open for a PRTriaged and validated; open for a PR

   bugSomething isn't workingSomething isn't working

   P-highHigh priorityHigh priority

   and removed

   needs-triageAwaiting maintainer triageAwaiting maintainer triage

   on Oct 26, 2026