Repository navigation
fix(indexer): collect shared dictionary blobs, run the collector periodically, batch storage deletes - #1862
Conversation
Add StorageWrite::delete_many and ContentStore::release_many. The defaults loop over the single-object forms; FileStorage unlinks a batch in one blocking task, and S3Storage issues one DeleteObjects request per thousand keys under a single request permit. StorageContentStore folds every candidate address of every id, legacy ones included, into one batch. Wrappers (tiered, resolver, encrypted, proxy, metered) forward. The collector and the storage sweep released one artifact per request, 32 wide, which on S3 took the whole process-wide request cap for as long as a pass ran and starved readers. A 500-version tail was ~10,000 requests; it is ~10 now.
…on the periodic tick The collector skipped every dictionary blob a manifest named, because @shared/dicts is ledger-wide and a fork starts out reading the source's dictionaries. Only a storage sweep ever reclaimed them, and nothing ran one, so every incremental build leaked its rewritten reverse-dict leaves. On one deployment that was 34 GB under a 225 MB commit log. The collector now releases a dictionary blob unless another branch of the ledger, retracted branches included, still reaches it through its own chain (SharedBlobPolicy, computed by gc::siblings from a branch listing the worker refreshes every catch-up interval). A blob a sibling still reaches is deferred; when that sibling replaces it, its own manifest names it and its pass releases it. Passes are serialised per ledger name so two branches cannot both defer the same blob and both consume the manifests. A pass now plans first and releases in batches: nodes from every eligible manifest, then superseded manifests and roots oldest-first in bounded chunks. The sweep deletes in batches too. The catch-up tick, and start-up, now run a collector pass over every ledger whose chain may exceed retention. A ledger that stops publishing kept the versions its last burst left inside the age guard until its next write, which for a load-once dataset never comes.
…are reindex GC A branch drop deleted the branch prefix and left @shared/dicts alone, so the dictionary blobs only that branch referenced stayed forever. Once a sibling's collector pass has deferred a blob to the branch, nothing else names it. The drop now takes the dropped chain's dictionary refs before its roots go, subtracts what every surviving branch still reaches, and releases the difference last, under the collector lock where a worker exists. The reindex-spawned collector pass takes the same sibling-aware policy from a fresh branch listing, and holds the collector lock in Background mode. Docs: the collector reclaims dictionaries now; a sweep is for the backlog left by 4.2.0 and earlier, and for orphans.
bplatz
added
bug
… references A manifest names a CID relative to the one build that replaced it. Content addressing does not know that a later build can reproduce the identical bytes under the same CID, whatever an earlier manifest said. Reverse-dict leaves hit this routinely under a monotonic key pattern (ULID, UUIDv7, sequential ids): a leaf growing by one entry per build is re-hashed and its old CID garbaged every build, and the half a later split keeps is byte-identical to one of the leaf's own earlier states. Verified with update_reverse_tree directly: 4,000 synthetic builds with sequential keys resurrected an already-replaced leaf on every single split, 7-200 builds after the replacement. The identical simulation with scattered random keys produced zero resurrections in 4,000 builds, so this is a monotonic-key hazard, not a universal one. retained_refs now checks every candidate release against all_cas_ids() of every root this pass retains, before the shared-blob policy or the branch-local release decision runs, and skips anything still referenced. Applies to every artifact kind, not only dictionaries: the same content- identical-recreation shape is not ruled out for branch-local leaves. Residual, documented rather than closed here: a build publishing a new root during this pass, between the chain snapshot this check reads and the release call, can still lose a CID the check would have protected. Same single-process caveat the sweep and MaintenanceGuard already carry; closing it needs holding every branch of the ledger for the release step, which is a bigger, separate change.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
@bplatz I think I'm tracking/following what this PR accomplishes, both re: the dict leak repair and also the data-loss fix on still-referenced index data marked as garbage. As such (if I understand correctly) this seems great. Claude thought a bit more rigorously about the performance & possible (though much narrower) ongoing data-loss windows, so I'll include its entire review below:
This is a strong PR, and the two bugs it fixes are the right two. The leak number does the arguing on its own — 34 GB under a 225 MB commit log is a ~150× amplification, and "only a storage sweep ever reclaimed them, and nothing ran one" is the sentence that explains why it kept growing. But the resurrection fix is the one I'd have wanted most, because it's the data-loss half: a manifest names a CID relative to the build that replaced it, content addressing doesn't know a later build can reproduce identical bytes, and so a CID an old manifest called garbage can be live again on a retained root. Your characterisation of it is the best part of the change — sequential keys resurrecting a leaf on every split across 4,000 builds versus zero in 4,000 with random keys turns it from a theoretical hazard into "this bites every ULID/UUIDv7 application and is invisible to random-key testing," which also answers why nobody found it sooner. I checked the guard rather than trusting it: neutering retained_refs to return an empty set makes exactly a_garbage_named_cid_still_live_on_a_retained_root_is_not_released go red, 25 others green, so the scenario is genuinely pinned. Ordering the guard ahead of the shared-blob policy is right, and so is applying it to every artifact kind rather than just dictionaries. Two non-blocking notes: I'd like the residual publish-during-pass race tracked as an issue rather than living only in the fifth commit's body, since the next person will read the function and not the log; and I'd like one observed memory figure for the retained_refs set on your largest index, because all_cas_ids() walks reverse-dict leaves too and nothing here measures what that costs at scale.
Adherence to repo commitments:
- Patterns/abstractions: ✔ Extends the existing collector and
SharedBlobPolicyrather than adding a parallel mechanism; the newgc::siblingsmodule earns its separation;delete_many/release_manyare added as a proper storage primitive across all four backends rather than open-coded in the collector. - Performance (speed first, memory second): ✔ Net strongly positive. Turns an unbounded leak proportional to build count into bounded reclamation. The guard is hoisted to one
HashSetper pass with O(1) lookups — no per-candidate cost. New costs are background-only (periodic collector tick, a branch listing per catch-up interval) with no query/transact hot-path impact. The one unmeasured item is the resident size of the retained-refs set, noted inline. - Testing: ✔ 10 new tests including the mutation-proven safety pin, plus
drop_branch_releases_dictionary_blobs_only_the_branch_referencedand coverage for both new storage primitives. CI green across clippy, fmt, test, testsuite-sparql, sql-bridge, wasm32, wasm-smoke — the W3C suite has not regressed. - Conventions: ✔ Five well-factored commits in dependency order with thorough multi-line bodies; docs updated (
docs/cli/sweep.md,docs/indexing-and-search/background-indexing.md) to say the collector now reclaims dictionaries and what a sweep is still for.
Verified locally at branch HEAD 630ba485c: mutation of retained_refs → gc::collector 25 passed / 1 FAILED (the named test, panic at :1082), restored byte-identical with a clean worktree; read retained_refs/partition_nodes for per-candidate cost; traced all_cas_ids() through index_root.rs:1181+; counted 10 new tests; gh pr checks 1862 green including testsuite-sparql.
Approving so you can merge when ready — the residual-race issue is the only thing I'd like to see exist before this drops off everyone's radar.
| /// resurrected CID this pass must not delete is exactly one still directly | ||
| /// reachable from a surviving root — nothing behind a named-graph or | ||
| /// annotation branch manifest changes that. | ||
| fn retained_refs( |
There was a problem hiding this comment.
fluree-db-indexer/src/gc/collector.rs:104 — optional — the residual race deserves an issue, not just a commit body. Your own message is clear about it: "a build publishing a new root during this pass, between the chain snapshot this check reads and the release call, can still lose a CID the check would have protected."
I agree with the scoping — closing it needs holding every branch of the ledger for the release step, and that is a bigger change than this one should carry. And it's fair that the sweep and MaintenanceGuard already carry the same single-process caveat, so this PR narrows the window rather than opening it.
What I'd ask is that it live somewhere a reader will find it. Right now the only record is the body of the fifth commit on a five-commit PR, and the next person to touch retained_refs will be reading the function, not git log. A tracking issue referenced from a one-line comment here ("residual: see #NNNN") costs nothing and means the window is a known-open item rather than folklore. This is the case where I'd genuinely use the follow-up-issue escape hatch, and I'm naming why: it's a decision about concurrency scope across the whole indexer, not something to fold into a GC fix.
There was a problem hiding this comment.
Went further than a tracking issue, because on a second look this was not the sweep's caveat: the pass a publish spawns is a detached task and nothing kept the worker from building the same ledger again underneath it, so the race was reachable in one process, on exactly the busy monotonic-key ledgers where the revival happens (gap of 7-200 builds vs ~40 builds inside the age guard at that deployment's rate). And this PR is what would have exposed dictionaries to it.
Closed in 88a0932 + 67d44f9: the pass is split into plan_garbage (snapshot, releases nothing) and GarbagePlan::release, which the worker runs inside a ReleaseWindow over the ledger name. A build in flight on any branch is waited out, new builds defer and resume when the window closes, and inside it the head and every sibling's head are re-read so roots published since the snapshot join the retained set. A pass with nothing to release never opens one; the periodic pass gives up rather than wait behind a build; a reindex/sweep hold refuses the window and hold_quiesced waits one out. Branch drop had the same compute-then-release gap and goes through the same window.
What is left is a second process on the same storage, which #1635 already tracks, so I have pointed the docs there rather than filing a duplicate. Since this is a new concurrency mechanism added after your approval, I would appreciate another look at GcLockState / open_release_window in orchestrator.rs.
| index_chain: &[IndexChainEntry], | ||
| keep_count: usize, | ||
| ) -> std::collections::HashSet<ContentId> { | ||
| index_chain[..keep_count.min(index_chain.len())] |
There was a problem hiding this comment.
fluree-db-indexer/src/gc/collector.rs:108 — optional, and it's a number I'd want rather than a change. retained_refs builds one HashSet<ContentId> per pass from keep_count roots' all_cas_ids(), and I checked what that walks — index_root.rs:1181+ covers forward dict packs and the reverse-dict tree leaves (your own all_cas_ids_includes_leaves_and_sidecars pins that). So the resident set is O(keep_count × CIDs-per-root) for the duration of the pass.
The shape is right and I wouldn't change it: built once, O(1) lookups per candidate in partition_nodes, no per-candidate cost. And it's the inherent price — you can't know a CID is unreferenced without the reference set.
But on a large index with several retained generations that set isn't small, nothing in the diff measures it, and the collector has no bench (consistent with the repo — benches live in fluree-db-api, fluree-db-query, fluree-db-spatial only). Before this runs against the biggest deployments I'd like one observed figure for the largest index you have handy. If it turns out uncomfortable, a Bloom filter is a clean fallback here specifically because its false positives err toward retention, which is the safe direction for a collector — but I'd want the measurement before adding a second mechanism.
Praise
There was a problem hiding this comment.
Two things here.
The number: the node-release debug line now carries retained_refs (the set's size) and published_since, so it shows up on the first pass against a large index without a bench. For scale, the chain walk already holds every root of the chain decoded for the length of the pass; this set is the deduplicated CIDs of the roots the pass keeps, and consecutive roots share nearly all of them, so it is strictly smaller than what is already resident. If memory ever bites it will be the decoded chain, not this set, so I would hold off on a Bloom filter.
Looking at what the set covers turned up a real gap, though: it was index_chain[..keep_count], but under a live age guard a pass leaves many more roots than that in the chain. A CID revived on one of those, then replaced again by a manifest too young to act on, was released while a query against that root could still read it. It is now every root the pass does not release (67d44f9), pinned by a_cid_live_on_an_age_guarded_root_past_retention_is_not_released, which fails against the old bound. So the set is somewhat larger than the one you reviewed, which makes the logged figure more worth having.
| /// resurrected CID this pass must not delete is exactly one still directly | ||
| /// reachable from a surviving root — nothing behind a named-graph or | ||
| /// annotation branch manifest changes that. | ||
| fn retained_refs( |
There was a problem hiding this comment.
fluree-db-indexer/src/gc/collector.rs:104-126 — praise, and the ordering is the part I'd protect. Checking retained_refs before the shared-blob policy and the branch-local release decision is the right precedence for a collector: every ambiguity resolves toward retention. Applying it to every artifact kind rather than only dictionaries — because "the same content-identical-recreation shape is not ruled out for branch-local leaves" — is the conservative reading and the correct one. The doc comment on partition_nodes pointing back to retained_refs for why is what will keep someone from "optimising" the order later.
| @@ -0,0 +1,277 @@ | |||
| //! Which dictionary blobs a ledger's other branches still reach. | |||
There was a problem hiding this comment.
fluree-db-indexer/src/gc/siblings.rs — praise. Including retracted branches in the reach computation is the detail that's easy to miss and expensive to get wrong — a retracted branch that still names a blob is precisely the case where "nobody's using it" is wrong. Good instinct, and worth the +277 lines of its own module rather than being inlined into the collector.
delete_many was compile-checked only. Seeds 1,004 objects, deletes 1,003 of them plus a key that never existed, and checks that both DeleteObjects requests landed, the missing key is not reported as a failure, and the one object outside the batch survives.
…build A collector pass protected a CID a retained root still references, but read the retained roots from a snapshot taken when the pass began. The pass a publish spawns runs detached, and nothing kept the worker from building the same ledger again underneath it, so a build that revived a garbage-named CID (a reverse-dict leaf split under monotonic keys does, 7-200 builds after the CID was replaced) could publish a head naming a blob the pass then deleted. Before this branch the collector never released dictionary blobs, so the window only opened with it. A pass now plans against the snapshot (plan_garbage, releases nothing) and releases inside a ReleaseWindow over the ledger name: builds in flight on any branch are waited out, new ones defer and resume on the tick the window sends when it closes. Inside it the branch head is re-read and every root published since the snapshot joins the retained set, and the sibling refs are computed from a fresh listing. A pass with nothing to release never opens a window. The periodic pass gives up rather than wait behind a build; a branch held for maintenance refuses the window, and hold_quiesced waits out one already open. The sibling listing is no longer cached. Five minutes of staleness was argued safe against the age guard, which does not hold once a fork minutes old can revive a pre-fork CID; BranchDirectory is gone and the listing now costs one all_records() per releasing pass rather than one per pass. A branch drop had the same window between reading what the survivors reference and releasing the difference. It now reads the survivors and releases inside a window too. The node-release debug line reports the retained set's size and how many roots were published since the plan. What remains is a second process indexing the same storage, which #1635 tracks.
…hout a listing per pass Two things the release window's first cut got wrong, found checking it against each backend. retained_refs covered only the newest keep_count roots. Under a live age guard a pass leaves many more than that in the chain, and a CID revived on one of them, then replaced again by a manifest too young to act on, was released while a query against that root could still read it. The retained set is now every root the pass does not release. The release step listed all_records() to find siblings. That is O(ledgers) on every nameservice: a directory walk and a read per record on file storage, a LIST and a GET per record on the storage-backed one, and on DynamoDB a query per ledger behind a listing index that is only eventually consistent, so it was neither cheap nor reliably fresh. Siblings now come from the cached listing plus every branch this process has built or taken for maintenance, the only way a fork the listing predates can reference more than its source's head did, and each sibling's head is one consistent lookup made inside the window. The listing is taken fresh per pass only under the version ceiling or an age guard shorter than twice the listing's age, where the staleness argument does not hold. Tests: the age-guarded revival, a fork the listing predates, the lookup narrowing, and one end-to-end scenario (sequential keys, real builds under a running worker, passes collecting behind them, a fork, a fork drop, and a check after each stage that every dictionary a surviving chain references still exists) run on file storage with the file nameservice and on S3 with DynamoDB under LocalStack. Each new test fails with its fix disabled.
2 participants
Problem
The index collector skipped every dictionary blob a garbage manifest named, because
@shared/dicts/is ledger-wide and a fork starts out reading the source branch's dictionaries. Only a storage sweep ever reclaimed them, and nothing runs one automatically. Every incremental build rewrites a few 2 MB reverse-dict leaves, so the leak is per publish: on one deployment_systemheld 34 GB of orphaned dictionary blobs under a 225 MB commit log (5,584 builds in three days at a 10 KB publish threshold, ~8 MB of dictionaries leaked per build).Two smaller gaps compounded it:
ccv-chartson the same deployment held 102 roots and 5.2 GB of leaves for a 461 MB live index.@shared/dicts/alone, so the blobs only that branch referenced stayed forever.On I/O: the collector and the sweep released one artifact per request, 32 wide. The S3 backend caps requests process-wide at 32, so a pass over a long chain took the whole cap and starved readers for as long as it ran.
Change
Storage (
fluree-db-core,fluree-db-storage-aws):StorageWrite::delete_manyandContentStore::release_many. Defaults loop;FileStorageunlinks a batch in one blocking task;S3Storageissues oneDeleteObjectsper thousand keys under one permit.StorageContentStorefolds every candidate address (legacy ones included) into one batch. Wrappers forward.Collector (
fluree-db-indexer):SharedBlobPolicyonCleanGarbageConfig. A dictionary blob is released unless another branch of the ledger, retracted branches included, still reaches it through its own chain (gc::siblings). Sibling chains are walked through a flat per-branch store on purpose: a fork's copied root links into the source namespace, and a branch-aware walk would count the source's own history as referenced elsewhere. A deferred blob is named by the sibling's own manifest when it replaces it, so whichever branch drops it last deletes it.GcLocks,IndexerHandle::hold_gc). Two sibling passes could otherwise each defer the same blob and both consume the manifests that named it.all_records()is O(ledgers) on every nameservice (a directory walk plus a read per record on file storage, a LIST plus a GET per record on the storage-backed one, a query per ledger behind an eventually consistent index on DynamoDB), so it is not taken per pass. A fork the listing predates holds either a copy of its source's head root, whose blobs the source's own chain still protects inside the age guard, or roots built here, which is what the tracked set covers. Each sibling's head is never cached: one consistentlookup, made as the pass releases. Withgc_hard_max_old_indexesset, or an age guard under ten minutes, the listing is taken fresh per pass.API (
fluree-db-api): a branch drop takes the dropped chain's dictionary refs before its roots go, subtracts what every surviving branch still reaches, and releases the difference last, under the collector lock where a worker exists. The reindex-spawned pass uses the same sibling-aware policy from a fresh listing.Docs: the collector now reclaims dictionaries; a sweep is for the backlog left by 4.2.0 and earlier, and for orphans.
Fix: a manifest's "garbage" can still be live
A manifest names a CID relative to the one build that replaced it. Content addressing does not know that: a later build that happens to produce byte-identical output gets the same CID, whatever an earlier manifest said. Reverse-dictionary leaves hit this routinely under a monotonic key pattern (ULID, UUIDv7, sequential ids, timestamp suffixes) — a leaf receiving one new entry per build is re-hashed and its old CID garbaged every build, and the half a later split keeps is byte-identical to one of the leaf's own earlier states, reviving a CID an already-consumed manifest named.
Verified directly against
update_reverse_tree: 4,000 synthetic builds with sequential keys resurrected an already-replaced leaf on every single split, 7-200 builds after the replacement. The identical simulation with scattered random keys produced zero resurrections in 4,000 builds — a monotonic-key hazard, not a universal one.retained_refschecks every candidate release againstall_cas_ids()of every root the pass leaves in the chain, before the shared-blob policy or the branch-local release decision runs, and skips anything still referenced. That is every root the pass does not release, not just the newestmax_old_indexes + 1: under a live age guard a pass leaves many more, and a query that started against one of them can still read it. Applies to every artifact kind, not only dictionaries.Fix: a build can revive a CID while a pass is running
retained_refsreads the retained roots from the snapshot the pass took when it began. The pass a publish spawns is a detached task and nothing kept the worker from building the same ledger again underneath it, so within one process a build could revive a CID the pass had planned to release and publish a head naming it, before or after the release. With the revival gap measured at 7-200 builds and ~40 builds inside the age guard at the deployment's publish rate, that is an ordinary event on a busy monotonic-key ledger, and this PR is what would have exposed dictionaries to it.A pass is now two steps.
plan_garbagewalks the snapshot and releases nothing.GarbagePlan::releaseruns inside aReleaseWindowover the ledger name: a build in flight on any branch is waited out (it may already have uploaded the blob), new builds defer and resume on the tick the window sends when it closes. Inside the window the branch head is re-read and every root published since the snapshot joins the retained set, and the sibling refs are computed from each sibling's head as of that moment. A pass with nothing to release never opens a window; the periodic pass gives up rather than wait behind a build; a branch held for reindex or sweep refuses the window, andhold_quiescedwaits out one already open.A branch drop had the same gap between reading what the survivors reference and releasing the difference; it now does both inside a window.
What remains is a second process indexing the same storage, which #1635 already tracks.
Related issues
Follow-up: #1548 — closed by the chain-breaking-reindex fix and the sweep, but its own report flagged dictionaries as ~98% of the leak and asked for ingest-time reclamation to cover them; that part is what this PR does.
Follow-up: #1885 — found while adding the S3 + DynamoDB end-to-end test, unrelated to this change: the sync CAS read bridge times out on a current-thread runtime and panics inside a
LocalSetagainst remote storage. The server is unaffected; the new S3 test runs multi-threaded with a spawned worker for that reason.Not resolved here, left open on purpose:
reindex_min_bytesdefault #1636 (evaluate the 10 KBreindex_min_bytesdefault) — this PR reclaims what a build produces, not how often one runs. Commenting there with concrete per-build costs.GcLocksand the release window are both in-process only, the same limitation that issue already tracks for the sweep.Fluree::drop_branchalready did a full purge before this PR; this PR only makes it reclaim the branch's own dictionaries too. The issue wantsmaterialize's gate-fail path to call something branch-scoped instead of the name-scopeddrop_ledger, which this PR doesn't touch.DropMode::Hardsilently degrades toSoft#1596 (vector graph source drop never reclaims blobs) — different code path, untouched.Not covered
fluree indexandfluree reindexstill do not collect. Follow-up if wanted: await one collector pass before returning.Verification
Defer,Releasewith and without a sibling reference), the sibling union stopping at a fork's namespace, the per-ledger lock, the periodic pass on a chain no publish will reach, batch release at a legacy address, file batch delete, and a real fork drop that keeps the survivor's blobs and releases the fork's own. The fork-drop test fails with the release disabled. A further test reproduces the resurrection scenario directly (a CID named garbage by an old manifest, live again via a retained root) and fails with the check disabled; another has the reviving root published after the plan, and fails with the head-to-snapshot walk disabled. The window: defers builds on every branch, waits out a build in flight, is refused under a maintenance hold and waited out by one, and is honoured by the worker's admission path.cargo test -p fluree-db-indexer --lib(414),cargo test -p fluree-db-core --lib --features native storage(134),cargo test -p fluree-db-api --features native --test grp_ledger(187) and--test grp_index(93) all pass. Clippy clean on the touched crates; server and CLI build.grp_ledger) and on S3 + DynamoDB under LocalStack: sequential subject ids, real builds under a running worker with passes collecting behind them, a fork, builds on both branches, a fork drop, a build after the drop, and after every stage a check that every dictionary blob a surviving chain references still exists. It fails with the sibling check disabled.DeleteObjectspath now runs:cargo test -p fluree-db-api --features aws-testcontainers,native --test it_storage_s3_testcontainers(4) against LocalStack, including a new test that deletes 1,003 keys across two requests plus a key that never existed. Not part of CI, which has no testcontainers job.retained_refsset's size is now on the node-release debug line (retained_refs, withpublished_since), so the figure the review asked for shows up on the first pass against a large index.