Reclaim superseded and orphaned index artifacts - #1614
Conversation
|
Is the count ceiling from #1601 meant to land inside this branch, or alongside it? Asking because they look complementary rather than overlapping, and I want to know whether to expect one PR or two. I grepped this branch for
The reason I don't think the sweep subsumes it: Context for why we care about the pair specifically: we independently hit and patched the severed-chain half in production (carrying the same |
|
1601 is a separate branch that still needs review. If and when it's approved and merged, it will land in main alongside this one. |
| // in a chain share nearly all of their CAS refs, so deriving per root | ||
| // would rebuild the same handful of addresses once per root. | ||
| let mut reachable: HashSet<ContentId> = HashSet::new(); | ||
| for entry in walk_prev_index_chain_cs(&store, head).await? { |
There was a problem hiding this comment.
The error here isn't inspected — on any root past the head, walk_prev_index_chain_cs breaks and returns Ok with a truncated chain rather than propagating the failure. That shortens the live set, and the difference goes straight into orphans.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
This is great, it seems there might be potential GC loss across branches too but I did not fully trace that, worth a quick look to confirm branching workflows are fully included in the logic. If not a follow-up issue is probably warranted unless an easy fix.
Other than that I just noted the one issue inline.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
A few things worth considering, though approving regardless in case you want to fold into the stacked branches or follow-ups.
The sweep's central safety claim doesn't appear true as written, and I verified it rather than reasoning about it. sweep.rs:28-31 promises that a root which cannot be read aborts the plan — as do docs/cli/sweep.md and the swagger description — but live_addresses gets its chain from walk_prev_index_chain_cs, which is documented at collector.rs:347-349 as tolerant: a read failure breaks and returns a truncated chain at debug!, and only errors when the failure lands on the very first root. I built a 3-root chain with a dict referenced only by t=1, removed t=2's blob, and plan_sweep returned Ok with both the t=1 root and that dict in orphans (live = 4, scanned = 4). Your an_unreadable_head_aborts_the_plan proves the identical condition aborts one link earlier. Since ContentStore::get already distinguishes Error::NotFound from a transient failure, "GC released this root" and "S3 threw a 503" reach the walk as different variants and get collapsed into the same break — and on a 3,019-root sweep against remote storage, that second case is routine. What it deletes is exactly the set your comment at sweep.rs:171-174 says the full-chain walk exists to protect.
The second one is the same shape one layer up. hold_ledger_for_maintenance is right to enumerate through all_records — I checked, and lookup genuinely doesn't filter retracted records, so the soft-drop reasoning holds. But all_records itself accumulates with if let Ok(Some(record)) = … (storage_ns.rs:571), so a transient read on one branch's ns record silently drops that branch. records.is_empty() won't catch it, that branch's chain never gets walked, and because swept_addresses lists the shared dicts/ prefix unconditionally, the dictionaries only that branch references get planned as orphans and deleted. That tolerance is pre-existing and not yours — but this is the first caller where under-counting deletes data. A count cross-check against a list_prefix of the ns prefix would cover it without touching the nameservice trait.
On merge order, since IX-Erich asked and the answer affects three PRs: I'd land this one first, then rebase #1601 on top of it. This is a superset of #1600's fix — same chain link, plus the (cid, t) pair, plus a real garbage manifest, plus both collector stalls — so #1600 can close in its favour, though I'd lift examples/root_graph.rs out of it first, because that diagnostic is what turned six wrong hypotheses into one measurement and it isn't carried here. It is not a superset of #1601: I grepped for hard_max_old_indexes / hard_keep and found nothing, and the age guard is still live at collector.rs:101, so the AND that lets time override the count target survives.
Two things make that rebase worth doing in this order rather than the reverse — this branch removes the missing-manifest break I named as defeating #1601's ceiling, so #1601 inherits a materially stronger version of its own claim; and gc/test_support.rs:67's cid_and_addr_for derives addresses through content_address, which is exactly what #1601's two harness tests got wrong by hand-writing the prefix. Worth flagging that the rebase won't be textual: #1601 put its ceiling right after the inline age check at the old collector.rs:205, and that age check now lives inside release_manifest_nodes, which knows nothing about chain position.
Adherence to repo commitments:
- Patterns/abstractions: ✔ Extends
CleanGarbageConfigand the existing truncation loop in place, reuseswalk_prev_index_chain_csandcollect_root_cas_ids_expandedrather than re-deriving reachability, mirrorsreindexexactly for the HTTP handler and CLI, and deletes two#[expect(dead_code)]items instead of leaving them. - Performance (speed first, memory second):
⚠️ No query-path or engine change, and GC releases now run 32-way concurrent, which is a real improvement on the backlog-draining first pass. Butlive_addressesre-reads shared branch manifests once per root (O(roots × branches) round trips) on the uncached walk, while the ledger is held out of indexing — that's the ledger this PR targets, so it's worth the memo. No performance-degradation risk to the engine. - Testing: ✔ 377 pass in
fluree-db-indexer; all 13 new tests observed by name; six mutations all red on the right tests;it_index_sweep.rsis correctly wired intogrp_index.rsand its e2e case queries after sweeping. The two blocking findings are both uncovered, which is the gap.
Verified locally at branch HEAD (f4fc5f4ec): cargo nextest run -p fluree-db-indexer --all-features → 377 passed / 0 failed / 2 skipped, 184 compile units, all new test names present by name; cargo nextest run -p fluree-db-api -p fluree-db-server -p fluree-db-cli --all-features --no-fail-fast → 4280 passed / 6 failed / 18 skipped, all six being testcontainers suites failing on a missing /var/run/docker.sock (environmental, not this PR), with all five it_index_sweep cases green; cargo clippy over the six changed packages --all-features --all-targets -- -D warnings → clean; cargo fmt --all -- --check → clean; six mutation checks each red on the expected tests only; the interior-unreadable-root probe reverted and worktree clean; all_records / lookup / load_record retraction behaviour traced in fluree-db-nameservice/src/storage_ns.rs; all_cas_ids audited for live-set category gaps (none found); incremental.rs publish-block unification diffed for lost behaviour (none).
This should land, and land soon — three deployments are paying for it and one of them is carrying a patch waiting on this. It's really just the two fail-open seams I'd want closed first, and both are small: narrow the chain walk's tolerance to NotFound, and make the branch enumeration refuse to proceed on a partial list. Given this is a delete path pointed at customer index data, I'd rather have those two be strict for the same reason you made everything else in plan_sweep strict.
| // in a chain share nearly all of their CAS refs, so deriving per root | ||
| // would rebuild the same handful of addresses once per root. | ||
| let mut reachable: HashSet<ContentId> = HashSet::new(); | ||
| for entry in walk_prev_index_chain_cs(&store, head).await? { |
There was a problem hiding this comment.
The "aborts the plan" guarantee holds at the head root and nowhere else; an interior root that fails to read silently truncates the live set and the tail gets planned for deletion.
live_addresses builds the live set from walk_prev_index_chain_cs, and that walk is explicitly tolerant — collector.rs:347-349 documents it, and collector.rs:384-395 implements it: a read failure returns a truncated chain rather than an error, at debug!, and only propagates Err when the failure lands on the first root (if chain.is_empty()). So the promise in this module's own doc at :28-31 — "A root that cannot be read or expanded, a prefix that cannot be listed, or a CID whose codec is unrecognised aborts the plan" — is true for the head and false for every root behind it. The same sentence appears in docs/cli/sweep.md and in the /v1/fluree/sweep swagger description, so it is a promise made to operators in three places.
I verified this rather than inferring it. I built a 3-root chain where t=1 references a dict nothing else references, removed the t=2 root's blob, and ran plan_sweep:
PROBE orphans = ["fluree:memory://mydb/@shared/dicts/1201ee42…dict",
"fluree:memory://mydb/main/index/roots/d0b6b951…fir6"]
PROBE live = 4, scanned = 4
It returns Ok, and both the t=1 root and the dict only it references are in orphans — execute_sweep would delete them. The test immediately above it, an_unreadable_head_aborts_the_plan, proves the identical condition aborts when it lands on the head. That asymmetry is the finding.
Why this is not theoretical: ContentStore::get (fluree-db-core/src/storage.rs:585-618) cleanly distinguishes Error::NotFound from every other error, so "prior GC released this root" (a legitimate chain end) and "S3 returned a 503" (transient) arrive at the walk as different variants and are collapsed into the same break. On IX-Erich's ledger a sweep walks 3,019 roots of remote storage; a transient failure somewhere in the middle of that is an ordinary Tuesday. And what gets deleted is precisely the set the sweep goes out of its way to protect — your own comment at :171-174 says the walk covers the full chain rather than the retained window because "a root past retention is still referenced until the collector truncates it, and treating it as orphaned here would race that decision."
The fix is small because the distinction already exists: match NotFound for the chain-end case and propagate everything else.
let bytes = match get_cached_or_remote(store, ¤t_id, cache_dir).await {
Ok(b) => b,
Err(e) if is_not_found(&e) => { /* released by prior GC — chain ends here */ break; }
Err(e) => return Err(e),
};That does change clean_garbage's behaviour too, and I think favourably — a transient read there currently ends the pass silently, whereas propagating means GC simply retries next tick. If you'd rather not touch the shared walk, a strict wrapper used only by live_addresses is equally fine.
(The real location is gc/collector.rs:384-395, which this PR doesn't modify, so I've anchored to the call site that makes it load-bearing.)
| ) -> Result<(Vec<MaintenanceGuard>, Vec<BranchIndexHead>)> { | ||
| let records: Vec<_> = self | ||
| .nameservice() | ||
| .all_records() |
There was a problem hiding this comment.
Branch enumeration is silently lossy, and a branch that drops out of it takes its dictionaries with it.
The reasoning in the doc comment above this is right, and I want to say so first: going through all_records rather than list_branches so soft-dropped branches participate is exactly the correct call, and sweep.rs:47-50 states the invariant it protects clearly — "dict blobs are shared across branches, so a live set missing one branch would orphan dicts that branch still reads." I checked the retracted half and it holds: lookup at storage_ns.rs:495-500 goes through load_record with no retracted filter, unlike list_branches at :524-528, so the re-read after quiescing does return soft-dropped branches.
The problem is one layer further down. StorageNameService::all_records (fluree-db-nameservice/src/storage_ns.rs:571) accumulates with if let Ok(Some(record)) = self.load_record(ledger_name, branch).await — an Err reading any single branch's ns record is silently discarded from the result.
Concretely: ledger mydb with branches main and feature. A transient read error on ns@v2/mydb/feature.json during enumeration leaves records = [main]. That is non-empty, so the records.is_empty() guard at :2023 does not fire and nothing aborts. feature's chain is never walked, so dicts only feature references are absent from live. But swept_addresses lists the shared dicts/ prefix unconditionally (sweep.rs:248-251) regardless of which branches were enumerated — so those dicts are in scanned, land in orphans, and get deleted. feature's index is then reading through dangling dict refs.
The tolerance in all_records is pre-existing and you didn't introduce it — but this PR is the first caller where an under-count deletes data, so it becomes load-bearing here. Two ways out, either fine by me: a strict enumeration path for maintenance that propagates load_record errors, or a cheap consistency check in hold_ledger_for_maintenance comparing the record count against a list_prefix of the ledger's ns prefix and aborting on a mismatch. I'd lean toward the second since it doesn't require touching the nameservice trait.
(Real location fluree-db-nameservice/src/storage_ns.rs:571, not in this diff; anchored to the call that makes it consequential.)
| // would rebuild the same handful of addresses once per root. | ||
| let mut reachable: HashSet<ContentId> = HashSet::new(); | ||
| for entry in walk_prev_index_chain_cs(&store, head).await? { | ||
| let expanded = collect_root_cas_ids_expanded(&store, &entry.root) |
There was a problem hiding this comment.
Optional, performance. collect_root_cas_ids_expanded runs once per root, and it issues a store.get per named-graph branch and per annotation branch (expanded_cas.rs:73-127). Consecutive roots share nearly all of their branch manifests — which is the same observation your CID-dedup comment at :191-193 makes about addresses, just applied to reads rather than derivations. On 3,019 roots × a handful of graph orders that's tens of thousands of round trips, taken while the ledger is held out of indexing. A HashSet<ContentId> of already-expanded branch CIDs threaded through the loop would make it near O(distinct manifests) instead of O(roots × branches).
Related and cheaper still: :195 calls the uncached walk_prev_index_chain_cs, though walk_prev_index_chain_cs_cached exists and clean_garbage feeds it config.artifact_cache_dir — the sweep re-reads every root from remote storage even where a warm artifact cache is sitting right there. Neither is a blocker for an operator-invoked path, but the first one is the difference between a sweep that takes a minute and one that takes an hour on the ledger this PR is aimed at.
| // would rebuild the same handful of addresses once per root. | ||
| let mut reachable: HashSet<ContentId> = HashSet::new(); | ||
| for entry in walk_prev_index_chain_cs(&store, head).await? { | ||
| let expanded = collect_root_cas_ids_expanded(&store, &entry.root) |
There was a problem hiding this comment.
Nit. fluree-db-binary-index/src/format/expanded_cas.rs:33-42 keeps an explicit "Use sites (must all stay in sync)" list and asks callers to maintain it. gc/sweep.rs::live_addresses is a fifth strict use site and doesn't get added. Small, but the list only works if it's kept.
Commenting here because expanded_cas.rs is not in this diff — this is the new use site.
| pub async fn sweep_index_storage(&self, ledger_name: &str) -> Result<SweepResult> { | ||
| // Reject a branch-qualified alias rather than silently sweeping the | ||
| // whole ledger: a sweep is ledger-wide because dict blobs are shared. | ||
| let ledger_name = parse_whole_ledger_input(ledger_name)?; |
There was a problem hiding this comment.
Optional, operator UX. parse_whole_ledger_input is shared with drop_ledger, and its rejection message at :415-418 is hardcoded to that operation. So fluree sweep mydb:main returns "drop_ledger drops the whole ledger and does not accept a branch suffix 'main'. Pass "mydb" to drop the whole ledger, or use drop_branch("mydb", "main") to drop a single branch." — which tells an operator running a sweep to go use drop_branch. Given this is a destructive-adjacent surface, that's a confusing thing to read. Threading an operation name through the helper would fix both call sites.
| // Holds are per-branch, so a ledger-wide sweep must hold them all. | ||
| // Guards release on drop, so an early return frees whatever was taken. | ||
| let mut guards = Vec::with_capacity(records.len()); | ||
| if let IndexingMode::Background(handle) = &self.indexing_mode { |
There was a problem hiding this comment.
More of a question than a suggestion. When indexing_mode is Disabled, guards stays empty and the sweep runs with no exclusion at all. That's safe today because a Disabled mode means no in-process indexer exists to race — but it means the entire concurrency story for that configuration rests on the single-process caveat, and nothing in the code says so at the point where the guard is skipped. A one-line comment would save the next reader the walk I just did. Happy to punt this one entirely.
| .map_err(|e| IndexerError::StorageWrite(e.to_string()))?; | ||
| cache_artifact_bytes(&cache_dir, &root_id, &root_bytes, "index_root"); | ||
| let mut final_root = new_root; | ||
| super::root_assembly::attach_garbage_manifest( |
There was a problem hiding this comment.
Optional, and I think the trade is right. Writing an empty manifest unconditionally is the correct invariant — "replaced nothing" genuinely needs to be expressible — but it does mean one extra small PUT per index build, and your own follow-up note says reindex_min_bytes defaults to 100 bytes, i.e. roughly a build per commit. They are reclaimed at collector.rs:305-313 so it isn't a leak, just churn. Worth a sentence in the follow-up issue you're drafting for reindex_min_bytes, since the two interact.
| // first genuinely supersedes nothing, so an empty manifest is accurate. | ||
| // The second is unknown, and recording it as empty would let GC release | ||
| // the prior root while leaving behind every blob it referenced. | ||
| let garbage_cids = match (inputs.prev_index.as_ref(), prev_root.as_ref()) { |
There was a problem hiding this comment.
Praise, and please keep the comment. The three-arm match separating "no prior index" (Some(vec![]), accurate) from "prior index unreadable" (None, unknown) is the sharpest thing in the PR, and the comment above it explains why conflating them would produce a larger leak than the one being fixed. This is also the thing that resolves the composition hazard I flagged between #1600 and #1601 — #1600's fallback produced an empty diff that then skipped the manifest write, manufacturing exactly the garbage: None entry that walls the collector. Making the empty case expressible and making the collector step over the undeterminable case fixes both ends.
| let mut indexes_cleaned = 0; | ||
| let mut unnameable_indexes = 0; | ||
|
|
||
| for i in (keep_count..index_chain.len()).rev() { |
There was a problem hiding this comment.
Praise, with numbers. I mutation-checked six of the guards here and every one goes red on the right test: reverting the prev_index assignment, reinstating the break in the missing-manifest arm, reverting the zero-timestamp condition, narrowing release back to the canonical address, dropping the legacy entries from candidate_addresses, and limiting live_addresses to the first branch. That is a genuinely unusual result — the two neighbouring PRs in this series both had headline tests that stayed green with the fix reverted — and the end-to-end test earning it most is fluree-db-api/tests/it_index_sweep.rs:82, which sweeps a real index and then queries and counts rows, because a CID missing from the live set can only be observed by deleting and then reading.
| /// the root replaced nothing, which a root with no manifest cannot express. | ||
| /// The collector stops its oldest-first walk at the first absent manifest, so | ||
| /// a publisher that skipped the write would strand every newer version. | ||
| pub(crate) async fn attach_garbage_manifest( |
There was a problem hiding this comment.
Praise, whole-file. Deleting GarbageContext and the #[expect(dead_code)] encode_and_write_root closes the note I left on #1600 that the footgun shape still existed in the file even after the signature change. And collapsing incremental.rs's two near-identical ~130-line publish blocks into one path is the kind of cleanup that usually gets deferred forever; I diffed both branches and nothing was lost in the merge (cache_artifact_bytes, reconcile_ns_at_publish, and the IndexStats shape all survive).
There was actually a real bug here that existed before this branch in code that never ran because of the bug that this branch fixes. We could have locked the full ledger for any branch gc to fix this, but right now with about a background index per commit, that would effectively serialize commits across ledger for each branch, making a throwaway experiment slow down commits on the main production branch. |
…ng it Two self-corrections to the ceiling introduced earlier in this branch, both found by mutation-testing the rebase onto fluree#1614 rather than by review. **The clamp was dead code.** `hard_max_old_indexes` was clamped up to `max_old_indexes`, documented as stopping a misconfigured ceiling collecting inside the retention target. It cannot: the retention loop starts at `keep_count = 1 + max_old_indexes`, so every index it visits is already outside the retention promise. Whenever the clamp changed `hard_keep` it moved it between two values both at or below `keep_count`, leaving `i >= hard_keep` true either way. Deleting it changed no test — which is the definition of a guard that reads as load-bearing and is not, so it is gone and the comment now says where the real guarantee lives. **The test for it was vacuous**, in the way that is easiest to miss: it used `max_old_indexes: Some(5)` against a 3-entry chain, so `index_chain.len() <= keep_count` returned early and the retention loop never ran. It asserted `indexes_cleaned == 0` and passed because nothing was attempted, not because anything was protected. It stayed green with the entire ceiling removed. Rewritten with a chain LONGER than `keep_count` so the loop actually runs, and asserting both halves: the two eligible versions are collected despite garbage records well inside the age guard (the ceiling working), and the two retained versions survive at the most hostile setting, `hard_max_old_indexes: Some(0)` (the retention promise holding). It now fails under two independent mutations — starting the loop at 0, and removing the ceiling override — where before it survived both.
4 participants
A reindex published its index root with no
prev_indexlink and no garbage manifest, severing the GC chain. Every earlier root — and every leaf, branch, and dictionary blob only those roots referenced — became unreachable from any chain walk, so retention could never truncate past it and a reindexed ledger accumulated index artifacts without bound. The reporter measured 3.0 GB on disk for 52 MB of commit data (~60×, growing super-linearly), with 216 index roots and ~2,400 dictionary artifacts, none ever reclaimed.This branch fixes the chain, removes the two other conditions that could stall garbage collection permanently, and adds a storage sweep that reclaims artifacts already orphaned by the old behaviour — because nothing that walks the chain can find blobs that no root references any more.
All three of the issue's suggestions are implemented, including its third ("expose a manual GC/compaction admin endpoint... or a CLI subcommand").
Fixes #1548
What was wrong
The reindex root severed the chain.
encode_and_write_root_v6constructed its root withprev_index: None, garbage: Noneand only ever populated them from aGarbageContextthat the rebuild path passed asNone. The prior index head reached the function asFir6Inputs::prev_index_root_idbut was used solely to recover an annotation arena — it never reachedroot.prev_index. A chain walk from a reindexed root therefore returned a single entry, trippedindex_chain.len() <= keep_count, and returned having collected nothing.Two conditions stopped the collector forever. The chain walk runs oldest-first and
breaks on the first root with no garbage manifest, and again on the first manifest whosecreated_at_msis zero. Either one pins every version newer than it, on every subsequent pass. Manifest-less roots were routine: both publish paths wrote a manifest only when the garbage set was non-empty, conflating "replaced nothing" with "unknown". A zero timestamp identifies a record written before the field existed (created_at_msis#[serde(default)]), which is by construction older than any retention window.Nothing could reclaim what was already orphaned. Chain-based GC reclaims artifacts by name — a manifest lists them. Blobs orphaned by a severed chain are named by nothing and referenced by nothing, so no amount of fixing the chain recovers them.
What changed
The GC chain
prev_indexlink, sourced as a(cid, t)pair fromNsRecordso it needs no extra fetch and cannot silently go missing.collect_root_cas_ids_expanded(strict — the tolerant variant would misclassify still-reachable leaves).Absent and empty manifests mean different things, and the code keeps them distinct:
Some(vec![])means "superseded nothing",Nonemeans "could not determine". Recording the latter as the former after a full rebuild would let GC release the prior root while leaving behind every blob it referenced — a larger leak than the one being fixed.The storage sweep
plan_sweep/execute_sweepinfluree-db-indexer/src/gc/sweep.rsenumerate what storage holds, subtract everything reachable from a live index chain, and release the remainder. Planning and reclaiming are separate operations, so a dry run is available and the destructive half is reviewable on its own.all_recordsrather thanlist_branchesso retracted branches participate — a soft drop is reversible until purge, and omitting a soft-dropped branch would make it unrecoverable.Storage addressing
ContentStore::getresolves a CID through a fallback chain — current layout, then the pre-@shareddict location, then the pre-.fir6.jsonroot. That chain was open-coded in each read method, so anything deciding a blob was unreferenced had no way to agree with it.candidate_addressesnow names every address a CID's blob could occupy, and tests assert the coupling by planting a blob at only its legacy address, confirminggetfinds it, and confirming the same address is listed as a candidate.This also fixed a pre-existing leak:
releasedeleted only the canonical address, so a legacy-located dict named in a garbage manifest was never reclaimed whilereleasereturnedOk.Concurrency control
MaintenanceGuardexcludes index builds for a ledger while an admin operation writes index artifacts. Reindex and the sweep both hold it — a build publishes its artifacts before the root that references them, so a sweep running alongside would classify them as orphans and delete them, after which the build would publish a root pointing at deleted blobs.wait_for_idlealone is not sufficient: it reports that a ledger was idle, and a build can start again the instant it returns.Operator surface
POST /v1/fluree/sweepandPOST /v1/fluree/sweep/plan, on the admin-protected writes router (auth as the outer layer, peer-mode forwarding, and leader coordination — the guard only excludes the in-process indexer, so the sweep must run where indexing does).fluree sweep <ledger> [--dry-run] [--remote <name>], routing local / server / remote likefluree reindex./swagger.json, with request and response schemas and the409conflict case.Follow-ups, not included
reindex_min_bytesdefaults to 100 bytes (server_defaults.rs:31), which is a background build per commit. The reclamation work bounds the accumulation but not the churn. Drafted as a separate issue./swagger.jsonremains a hand-written stub covering 8 of ~69 routes (/swagger.jsonis a hand-written stub covering 6 of ~67 routes #1594); this branch added its two and no more.