Make storage-sweep planning proportional to distinct manifests

[zonotope]

Problem

Sweep planning re-read the same data once per index root. live_addresses expanded every root of every branch chain independently and unioned the results, but consecutive roots share nearly all of their branch manifests — an incremental build rewrites only the branches whose leaves changed and carries the rest over by CID. Deduplication happened at the ContentId level after expansion, so the I/O was fully paid: O(roots × manifests) reads for O(distinct manifests) of routing information.

It also did its two independent halves in sequence, walked branch chains one after another, and materialized every decoded root of a chain in memory at once.

All of this runs while the ledger is held out of indexing, so planning latency is indexing downtime. On the ~3000-root deployment in #1637 the redundant reads alone were tens of thousands of round trips against remote storage.

Changes

Read each manifest once per sweep — new ChainCasIds in fluree-db-binary-index accumulates the reachable CAS set across a run of roots and remembers which manifests it has already expanded. collect_root_cas_ids_expanded becomes a wrapper over a single-root accumulation, so the strict path keeps exactly one traversal implementation. The already-expanded set is deliberately separate from the id set: a manifest's own CID lands in the latter via all_cas_ids() before anything routes through it, so the id set cannot answer whether it was read.

Overlap the independent reads — plan_sweep runs the chain walk and the prefix listing under try_join! (disjoint storage, neither informs the other), and branch chains run through buffer_unordered. A chain is a sequence of dependent reads and cannot overlap itself, so this only helps multi-branch ledgers; the bound is set accordingly.

Walk one root at a time — PrevIndexChainWalk replaces the collect-everything walk with a cursor. The sweep reduces each root to CIDs and drops it, so a long chain now costs its distinct refs rather than every decoded root simultaneously. walk_prev_index_chain_cs_cached is rebuilt on the cursor, so clean_garbage and drop keep the Vec they index into and slice by retention window.

Read roots through the artifact cache — plan_sweep takes a cache directory, sourced from IndexerHandle (the worker resolves it from its own config; IndexingMode::Disabled passes None, since reading through a directory no builder writes costs the writes and returns no hits). IndexerConfig::artifact_cache_dir() is now the single derivation, replacing a private helper in build/incremental.rs and an inline copy in the orchestrator.

Evict cached bytes when a blob is released — ContentStore::release now drops the CID from every disk cache the process holds open, after the deletes rather than before (evicting first leaves a window where a concurrent reader repopulates the entry from storage that still holds the blob).

The cache was landed, reverted, and re-landed — read this before reviewing that history

The history contains 013a1a7 and its revert 8aeb8ff. The first attempt followed the issue's sketch, which treats enabling the cache as a pure speed change. It isn't.

The walk detects the end of a chain by a root that storage no longer holds. A cached copy outlives its blob, so the walk read a root the collector had released and kept going; expanding that root then failed, because its branch manifests were released along with it, and strict expansion turned a truncated chain into a failed plan. On any ledger with named graphs, every sweep after a GC truncation aborted until the cache entry aged out — reclaiming nothing, not merely less.

The re-land adds the rule that makes it safe: when expanding a root fails, ask whether storage still holds that root. If it does not, the chain ends there — exactly where an uncached walk would have stopped. If it does exist, the failure is a root whose refs are unreadable rather than gone, and the plan still refuses, because a live set short of those refs would classify live artifacts as orphans. It is the same existence test the walk already uses, applied one level down.

Correctness does not rest on the eviction commit. That keeps the cache honest and saves the walk from descending into dead roots; the existence rule is what makes a stale entry harmless when eviction did not run.

Testing

• a_chain_reads_a_carried_over_manifest_once — two roots sharing an arena through a get-counting store: one read per manifest, and the skipped manifest's leaves are still in the set. Verified to fail (left: 2, right: 1) with the dedup disabled.
• a_released_root_ends_the_chain_even_when_its_cache_entry_survives — primes the cache, then deletes a root and its manifest without evicting, which is the state a crash between the two leaves behind. Verified to reproduce the original abort (cannot expand index root at t=1 ... refusing to sweep) with the existence rule removed.
• a_cached_plan_matches_an_uncached_one — cached and uncached planning agree while every walked root is still in storage.
• releasing_a_blob_evicts_its_cached_copy — end-to-end through StorageContentStore::release, which also guards the #[cfg(feature = "native")] wiring.
• Eviction unit tests for the entry removal, the byte accounting, and the absent-CID path (most released CIDs were never cached).
• Both cache tests call assert_cache_populated, which fails loudly when disk caching is disabled rather than passing vacuously — with FLUREE_DISK_CACHE_BUDGET_BYTES=0 every read falls through to storage and the cached and uncached paths become the same path.

Two fixture gaps are closed along the way. Sweep roots carried no named graphs, so expansion did no I/O and no sweep test could reach any of this; fir6_with_named_graph_for routes one, with minimal_fir6_for delegating to it. And the chain helper now writes a manifest per root, since roots sharing one manifest cannot model a superseded manifest — deleting the shared one breaks the live head instead.

Full workspace cargo check --all-targets, cargo clippy --workspace --all-targets, cargo fmt --check, and fluree-db-core --no-default-features are clean. Suites: core 782, indexer 378, binary-index 369, api grp_index 88.

Notes for reviewers

One behavior change with a real tail. Readers that previously survived GC releasing a blob out from under them — because their local cache still had it — now get a miss and a NotFound. Serving released data from cache was masking a latent bug rather than providing a guarantee, and the retention window is what actually prevents that race, so failing fast is arguably the improvement. But nothing here tests it. If something odd appears after deploy it will be NotFounds correlated with GC activity, and this is the mechanism.

Residual over-count, documented not fixed. A root contributes its direct refs before any manifest is read, so when expansion fails on a released root the accumulator already holds refs an uncached walk never saw. Whichever of those blobs still exist stay counted live and wait for a later run. That is the direction that costs a deferral rather than a live artifact. Likewise, on a ledger with only a default graph, expansion reads nothing, so a stale cached root extends the walk without failing — same benign over-count.

Breaking signature. fluree_db_indexer::plan_sweep takes an additional artifact_cache_dir: Option<&Path>. It is not re-exported from fluree-db-api, so the blast radius is direct consumers of the indexer crate.

A Send trap worth knowing about. Building the per-branch futures inside a .map() closure compiles in fluree-db-indexer but destroys Send for the whole future, and it surfaces only in fluree-db-server as implementation of 'Send' is not general enough pointing at the /sweep route and naming types that appear nowhere in the indexer. Collecting the futures into a Vec first avoids it. cargo check -p fluree-db-indexer does not catch this; the server crate has to be in the check.

Eviction is best effort, by design. It reaches this process's caches only, a crash between the delete and the call leaves an entry that survives restarts, an in-flight fetch can rewrite an entry after it is evicted, and it drops entries keyed by CID rather than the digest-plus-extension form (finding those needs a directory scan per call, which a release loop cannot afford). The doc comment says so, and the sweep tolerates a released object rather than treating eviction as a guarantee.

Not included

The remaining cost is O(roots) sequential root reads — one per root, chain-ordered, unavoidable without a durable watermark, and cold on the first sweep after a deploy. The "minute versus hour" figure in #1637 is the issue's estimate rather than an observation; this should be measured against the ~3000-root ledger before the issue is closed.

Cross-process safety is out of scope and tracked in #1635. Planning still assumes the caller holds index builds excluded, which IndexingMode::Disabled does not do.

Closes #1637

[aaj3f]

@zonotope -- approving with some notes below. Also, just FWIW I really appreciated the PR description. I'm trying to spend some human-time getting some human-understanding on these PRs (even if they're afield of code or logic I'm otherwise used to in the repo). The PR description "read this first" and "notes for reviewers" was much appreciated.

The one thing ask a bit about is fluree-db-indexer/src/gc/sweep.rs:303. The "failed expansion + absent root = end of chain" rule is applied at every position including the head, but PrevIndexChainWalk::end_of_chain_or_error (collector.rs:507) deliberately refuses to treat the head as an ending, and the sweep's copy of that test drops the guard.

I probed it: prime the cache, delete the head root and its manifest from storage without evicting, and the plan succeeds with every retained root and manifest classified as an orphan — where the uncached path refuses, and an_unreadable_head_aborts_the_plan pins that it should.

Admittedly, I couldn't find any way to construct an in-process path to that state under a normal config. The producers I can name are lifecycle rules, operator cleanup, a partial restore, or the cross-process case #1635 tracks. But in every one of them today's behavior is "refuse and surface an error," which is what you want when storage moved underneath you, and the new behavior is "silently delete the rest of the index."

Given this is GC, I'd be inclined to have the guard as it's one bool and a test releasing chain.last() instead of chain[0].

Separately, and more of a question than a blocker: it's worth a look at @christophediprima and you looking at each other's PRs possibly. #1637's ~3,000-root chain is the symptom #1601 looks to fix, and if retention is genuinely bounded at ~20 old versions then the dedup saves 20× rather than 3,000× and the disk cache is buying a smaller win. The dedup, the memory bound, and the parallelism I'd keep either way. The two PRs also overlap textually in collector.rs, config.rs, and orchestrator.rs, so there's a rebase coming regardless. If we merge this first we may just want to add a comment or comments to #1601

Adherence to repo commitments:

• Patterns/abstractions: ✔ Extends collect_root_cas_ids_expanded rather than forking it — the single-root helper becomes a wrapper over ChainCasIds, so there's still one traversal implementation. IndexerConfig::artifact_cache_dir() correctly collapses a private helper and an inline copy into one derivation. PrevIndexChainWalk keeps walk_prev_index_chain_cs_cached working for clean_garbage.
• Performance (speed first, memory second): ✔ Query engine and hot path untouched. Planning drops from O(roots × manifests) reads to O(distinct manifests), verified by the get-counting test rather than by the description; memory drops from every decoded root to one. The evict_cached_cid stat per released blob is blocking FS in async, but at microsecond scale against network deletes and consistent with best_effort_write's existing idiom. No regression risk.
• Testing: ⚠️ New tests are real, wired, and go red under mutation, and it_index_sweep.rs is properly declared in grp_index.rs:8-9. The gap is the head-position case above, which no test covers and where the cached and uncached paths silently disagree.
• Conventions: ✔ fmt and clippy clean against the 23 denied lints with --all-features --all-targets; doc comments updated thoroughly and the removed behavior is documented where it moved.

Verified locally at branch HEAD (9e0d66dd4): cargo check on fluree-db-indexer/fluree-db-binary-index --all-targets and on fluree-db-server --all-features (the check that catches the Send trap) — both clean; cargo fmt --check and cargo clippy --all-features --all-targets -D warnings on the three touched crates — clean; gc:: 36 passed, expanded_cas 5 passed, disk_cache 20 passed, storage::tests 5 passed; dedup mutation → test fails left: 2, right: 1; head-release probe → plan succeeds and orphans the live chain.

[aaj3f]

Blocking

fluree-db-indexer/src/gc/sweep.rs:303 — blocking. The new end-of-chain rule has no head-position guard, so a released-but-cached head root plans every retained root and manifest in the branch as an orphan.

Claude caught this, and I confirmed it with a repro. chain_cas_ids reads "expansion failed and storage no longer holds the root" as the end of the chain. That is the right rule for a root the collector truncated. But it is applied at every position including the first, and PrevIndexChainWalk::end_of_chain_or_error one level up deliberately refuses to do that — collector.rs:507 reads if !self.yielded || self.store.has(root_id)..., and the doc comment above it spells out why: "The head is never an ending — a walk that could not read its first root saw nothing at all." The PR describes this rule as "the same existence test the walk already uses, applied one level down." It is that test minus its most important guard.

When it fires the blast radius is the whole branch. add_root extends ids with root.all_cas_ids() before it reads any manifest, then fails on the first released manifest, and we break — before chain_ids.insert(entry.root_id). So live for that branch collapses to the head's direct refs, everything older is absent from the live set, and execute_sweep (sweep.rs:158-176) deletes every planned orphan with no sanity bound on how much of the ledger it is reclaiming.

I probed it directly: prime the cache over a three-root named-graph chain, then delete the head root and its manifest from storage without evicting. Result — plan SUCCEEDED. live=3 scanned=5 orphans=4, with both retained roots and both of their branch manifests classified as orphans. The uncached path in the same situation refuses, and there is already a test pinning that: an_unreadable_head_aborts_the_plan (sweep.rs:1002). So the cached and uncached paths diverge here, in the unsafe direction, and a_cached_plan_matches_an_uncached_one doesn't cover it.

I want to be straight about reachability: I could not construct an in-process, normally-configured path to "head absent from storage, cache entry alive." hold_ledger_for_maintenance re-reads the head after quiescing, and clean_garbage retains 1 + max_old_indexes and never releases the head. The producers I can name are all out-of-the-ordinary — an S3 lifecycle rule or operator cleanup on a bucket (not far-fetched on exactly the customer who has been fighting unbounded index growth), a partially restored bucket, or the cross-process case that #1635 tracks and this PR explicitly leaves out of scope. Worth noting too that execute_sweep deletes by raw address and so bypasses the new eviction, which makes a mis-plan self-amplifying: the sweep that deletes a root leaves its cache entry behind for the next sweep to walk through.

The reason it still might be worth addressing before merge is that in every one of those scenarios the current behavior is to refuse and surface an error, which is what an operator wants when storage has been tampered with underneath them — and the new behavior is to silently delete the rest of the index. That's a safety property being narrowed as a side effect, and the fix is one bool:

let mut chain_ids = ChainCasIds::new();
let mut walk = PrevIndexChainWalk::new(store, head, artifact_cache_dir);
let mut expanded_any = false;

while let Some(entry) = walk.next_entry().await? {
    if let Err(e) = chain_ids.add_root(store, &entry.root).await {
        // ... existing comment ...
        // The head is never an ending, for the same reason
        // `PrevIndexChainWalk::end_of_chain_or_error` refuses it: a chain
        // that ends before its first root leaves the live set empty, and
        // every artifact the branch reaches becomes an orphan.
        if expanded_any && !store.has(&entry.root_id).await.unwrap_or(true) {
            tracing::debug!(/* ... */);
            break;
        }
        return Err(/* ... */);
    }
    chain_ids.insert(entry.root_id);
    if let Some(garbage_id) = entry.garbage_id {
        chain_ids.insert(garbage_id);
    }
    expanded_any = true;
}

A test in the shape of a_released_root_ends_the_chain_even_when_its_cache_entry_survives but releasing chain.last() instead of chain[0], asserting the plan refuses, would pin it.

[aaj3f]

Worth discussing before merge

fluree-db-indexer/src/gc/sweep.rs:281 — design / merge-order question, not a blocker. If we accept #1601 then some of the premise motivating this PR changes a bit. I haven't yet looked at #1601, though, so the premise or the solution may not hold. What I describe below is only what's taken as assumptive context from the description of 1601.

#1637 justifies the work with a ~3,000-root chain. But an index chain of 3,000 roots is itself treated as a "bug" that #1601 ("bound retained index versions by count, not only by age") intends to resolve — its description reports 79 retained versions against a target of 5 because max_old_indexes and min_time_garbage_mins are ANDed, so the count bounds nothing. With #1601's hard ceiling at max_old_indexes * 4, steady-state chains are ~20 old versions, and the manifest dedup here saves roughly 20× rather than 3,000×.

Regardless of the tension above, this PR adds quite a bit — the memory bound, the branch-level parallelism, and the dedup all still earn their place, and a ledger that has already grown to 3,000 roots has to be swept down regardless. But it does change the calculus on the disk-cache piece specifically, which is the component that carries the entire new failure class (the revert, the re-land, and the finding above all trace to it). Twenty sequential root reads is not the bottleneck. So: is the cache still worth its risk if retention ends up being actually bounded, or does #1601 have issues that should keep it from merge (which then absolutely validates this), or is it worth splitting out and landing separately once #1601 has been battle-tested to confirm chains stay short?

[zonotope]

The cache is not a new risk surface. clean_garbage on main already reads roots through walk_prev_index_chain_cs_cached with the artifact cache directory, and main already has the "released root ends the chain, distinguished by existence" rule for it. This PR just reuses that walk for the sweep over the full chain instead of the retained window. The head guard you found is the one piece the collector's version had that the sweep's copy dropped, and that is now closed and pinned by a test that fails without it.

#1601 also doesn't bound disk size. It bounds chain length instead. After the ceiling fired, the ledgers from #1601 still held 370, 169, and 125 root files on disk, and artifacts that have fallen off the chain are outside anything retention can reach. A ledger that has already grown to 3,000 roots has to be walked once at that length no matter what retention does afterward, and that first walk is the one that holds the ledger out of indexing.

#1601 still has merge conflicts and requested changes, so I think we should merge this anyway and re-evaluate the cache after #1601 lands given the cache's risks aren't new and it still benefits the sweep.

[aaj3f]

Optional

fluree-db-core/src/storage.rs:738 — optional. The eviction doc at disk_cache.rs:592-611 lists what evict_cached_cid doesn't guarantee, and the list is genuinely good, but I think it's missing one: execute_sweep (fluree-db-indexer/src/gc/sweep.rs:163) deletes orphans through Storage::delete by address rather than through ContentStore::release, so the sweep's own deletions never evict. That makes the sweep the one in-process producer of the "blob gone, cache entry alive" state that this PR's existence rule exists to tolerate. I couldn't construct an actual failure from it — the roots a sweep deletes are ones no chain reaches, so no later walk goes looking for them — so this may well be fine as-is. But either routing those deletes through release, or adding the case to the doc's gap list, would stop the next reader from concluding the state is only reachable via a crash. (Commenting here because sweep.rs:163 isn't in this diff.)

[aaj3f]

Optional

fluree-db-core/src/disk_cache.rs:614 — optional, and I don't think this matters much. Reads don't touch mtime (try_read_cached_bytes is a plain fs::read) and evict_until sorts by modified, so the shared artifact cache evicts in write order rather than access order. The sweep is now a bulk writer of O(roots) root blobs into it, which on a long chain could push out leaf artifacts the read path is actively using — precisely the hot ones, since they've been resident longest. This only bites when the indexer's directory and the read path's coincide, which happens when IndexerConfig::data_dir is unset and both fall back to $TMPDIR/fluree_binary_cache; I couldn't establish what the deployed configuration does, so it may be a non-issue. Flagging it mostly so it's on the record if someone sees post-sweep query latency.

[aaj3f]

Praise (worth preserving)

fluree-db-indexer/src/gc/sweep.rs:499 — assert_cache_populated is the best thing in this diff. A cache test that silently passes when caching is disabled proves nothing, and with FLUREE_DISK_CACHE_BUDGET_BYTES=0 both paths would have collapsed into the same path. Failing loudly instead is exactly right, and it's the kind of regression-proofing that usually only gets added after the test has already lied to someone.

[aaj3f]

Praise (worth preserving)

fluree-db-binary-index/src/format/expanded_cas.rs:105-108 — keeping expanded_manifests separate from ids, with the reason written down (a manifest's own CID lands in ids via all_cas_ids() before anything routes through it, so ids can't answer whether it was read). That's the subtle bug this design would otherwise have had, caught in advance. Same for the :88-96 warning that subtracting callers need their own instance — I checked root_assembly.rs:168,171 and the contract holds.

[aaj3f]

Praise (worth preserving)

fluree-db-indexer/src/gc/sweep.rs:303 — the unwrap_or(true) fails safe: an existence check that itself errors treats the root as present and refuses. Easy to get backwards, and getting it backwards here would be data loss.

[aaj3f]

Backlog-sweep nudge: this is approved with green CI and covers #1637 completely (dedup, cache, concurrency, streaming walk) — it seems the only thing between it and merged is the one head-position guard from review (the bool + chain.last() release test; the reviewer's probe showed the cached path silently orphaning a live chain when the head root is missing from storage), which has been pending since 8/13.

Two mechanical notes for the landing: "Closes #1637" currently lives only in a PR comment, so the merge won't auto-close the issue — worth moving into the body (or we close manually on merge). And per the body's own gate, the planning measurement on the ~3,000-root deployment should get run before we call #1637 done.

One coordination flag: #1601 (retention bounding) has textual overlap in collector.rs/config.rs/orchestrator.rs — whichever merges second rebases, so probably worth agreeing on the order now.