fix: index storage growth — stats sketch GC, disk cache for local storage, drop of blob/ by bplatz · Pull Request #2018 · fluree/db

bplatz · 2026-10-05T16:27:19Z

A local ledger grew to 3.6 GB in eight minutes: a 232-commit bulk load followed by an app writing two commits per message, with a background index build after each commit (the default reindex_min_bytes = 100). Three independent defects account for it. One commit fixes each.

1. Superseded stats sketches were never garbage collected

IncrementalRootBuilder::set_sketch_ref replaced the root's sketch_ref without adding the old CID to the replaced set. The collector therefore never released it, and every incremental build left an index/stats/*.hll behind. On the reporting ledger these were 2.4 GB of the 3.6 GB. A second ledger with 6 retained roots held 845 sketches.

The old sketch now enters the garbage manifest unless the new root still references it, the same pattern set_dict_refs and set_annotation_index use. Full rebuilds were already correct because superseded_cids diffs the roots.

Ledgers indexed before this fix need no migration. A sketch whose root the collector has released is unreachable from the chain, so the sweep reclaims it (a sweep plan on the 845-sketch ledger listed 839 of them).

2. The disk artifact cache served local storage

Readers already preferred a store's own local file over the cache. The indexer, however, copied every artifact it wrote into the cache, gated only on permits_plaintext_cache(), which is an encryption property, not a locality one. On file storage those copies were never read back. Memory storage has no local paths, so it was cached on reads and on writes.

The reporting server had copied about 2.5 GB of sketches into $TMPDIR/fluree_binary_cache. That directory reached 14 GB on the same machine, shared with every test run and bounded only by a budget of 90% of free disk.

StorageRead and ContentStore gain a required is_remote(), with no default for the same reason permits_plaintext_cache has none: a wrapper that forgot to delegate would be silently wrong.
- File and memory: false.
- S3, IPFS, the storage proxy and the browser store: true.
- Tiered and address-routing stores: true if any tier is remote.
- Branched store: true if any ancestor is remote.
- Encrypted and metered wrappers: delegate.
disk_cache::uses_disk_cache, needs_disk_copy and seed_disk_cache are the single rule every reader and writer of the cache goes through. Every reader gate that tested permits_plaintext_cache() now calls uses_disk_cache. best_effort_cache_bytes_to_path is removed, so seeding cannot bypass the rule. Outside the trait implementations, the only remaining direct use of permits_plaintext_cache() is the build-staging check in rebuild.rs, which is a separate encryption question.
MemoryStorage::simulating_remote() returns a view of the same data that reports its reads as remote. Tests of remote-only behavior use it; six existing tests that exercised the cache through plain memory storage now do.

Remote storage, including S3 on Lambda, behaves as before.

fetch_cached_bytes and fetch_cached_bytes_cid return the shared body's future instead of awaiting it from an async fn. The extra layer pushed it_absent_subject_scan_narrowing and it_exists_semijoin_correlation past rustc's layout query depth under cargo check --workspace --all-targets; those fixture futures are close to the limit.

3. Hard drop left `{branch}/blob/` behind

Storage kinds with no layout of their own fall through to {branch}/blob/. Today that means the four edge-annotation arena kinds. drop_artifacts listed only commit/, txn/, index/ and config/, so a dropped ledger that had annotations kept its arenas.

Drop now lists blob/ too. The name-registry .lock files under ns@v2/ are deliberately left: they are flock targets, and unlinking one that a process still holds lets a second process lock a fresh inode at the same path.

4. Memory storage read its index through the remote lane

Fix 2 takes memory storage off the disk cache, which was right, but the native readers had only two cheap lanes: a local file, or a disk-cache file, both mapped. Memory storage has neither, so it fell through to the remote lane. A leaf's first open made range reads, and every open after that copied the whole blob out of the store and kept it nowhere. Dictionary leaves and packs were copied out on every query too.

StorageRead::resolve_local_bytes is the in-memory counterpart of resolve_local_path, defaulting to None. StorageContentStore::resolve_cached_bytes falls back to it at the current and legacy addresses, in the order resolve_local_path uses.
MemoryStorage stores Arc<[u8]> and hands its bytes out shared. A simulating_remote() view holds nothing resident.
Each native reader takes resident bytes right after the local file: leaf handles (SharedBlobLeafHandle, with the directory shared through the leaflet cache), whole-leaf bytes, range reads, dictionary leaves and forward packs. SharedLeafBytes::Shared and SharedBlobLeafHandle are no longer residency-only.
MeteredContentStore now delegates resolve_cached_bytes. Encrypted storage keeps the None default, since its bytes at rest are ciphertext.

Loading the ledger before every query, a warm query read 16 artifacts from memory storage before this fix and reads 8 after. The 8 are the store rebuild itself: the root, the branch manifests and two small dictionaries, the same as on main. With the ledger loaded once, warm queries read nothing.

This adds another hook that readers consult around the trait. The storage refactor in the follow-up below replaces it, with get returning a shared handle that memory storage serves as an Arc clone.

Memory-backed benches

The memory-backed benches that query an indexed ledger, run on an m7a.4xlarge with FLUREE_BENCH_PROFILE=full (100 samples) at small scale. Each variant was built in its own tree with its own target directory, and each bench binary hashes differently across the three. The figures are criterion medians from one round of a planned two; the run was stopped after round 1.

base is ac26b463e, this PR's merge base.
fixes 1–3 is 70ad15aee, before this section.
this PR is 0b8ae3e60.

bench	base	fixes 1–3	this PR
`fulltext_scan_all_indexed/1000`	372 µs	438 µs (1.18×)	360 µs (0.97×)
`fulltext_scan_filtered_indexed/1000`	309 µs	439 µs (1.42×)	302 µs (0.98×)
`vector_scan_all_indexed/1000`	308 µs	383 µs (1.25×)	315 µs (1.02×)
`vector_scan_filtered_indexed/1000`	340 µs	466 µs (1.37×)	337 µs (0.99×)
`annotation_hydration/arena/10000`	129 ms	160 ms (1.23×)	139 ms (1.07×)
`annotation_hydration/scan/100`	1.52 ms	2.08 ms (1.37×)	1.69 ms (1.11×)
`annotation_hydration/scan/10000`	173 ms	300 ms (1.73×)	205 ms (1.18×)
`non_annotation_hydration/baseline/100`	2.43 ms	2.70 ms (1.11×)	2.40 ms (0.99×)

The unindexed fulltext_scan_all and vector_scan_filtered are within 2% across all three. vector_scan_all/5000, also unindexed, read 1.08× and 1.13×. The smallest annotation_planner cases run 13–29% faster than base on both later variants, consistent with base reading dictionary blobs back from cache files.

The annotation_hydration/scan residual of 1.11× to 1.18× is not explained. That bench calls fluree.ledger(), the uncached load, on every iteration, so it measures rebuilding the store on each query as much as reading the index. One round is not enough to separate it from noise. Follow-up: #2030

During a single small-scale pass, base left 29 MB in 138 files in its TMPDIR. Both later variants left nothing.

Tests

set_sketch_ref_retires_the_superseded_sketch and set_sketch_ref_keeps_an_unchanged_sketch_out_of_garbage: unit tests on the builder.
collected_index_versions_leave_no_stats_sketches_behind: five real incremental builds under a running worker that keeps one old version. Afterwards a sweep plan must find no orphaned sketch. It fails with exactly three leaked sketches when the fix is reverted.
sweep_reclaims_leftover_stats_sketches_and_keeps_the_heads: the sweep reclaims a leftover sketch and keeps the head's. It fails when sketch_ref is dropped from all_cas_ids.
it_disk_cache_locality, four end-to-end tests:
- file storage leaves the cache empty after builds and a query;
- memory storage leaves it empty;
- remote storage still seeds what its builds wrote (checked on the new sketch, which only the next build reads, because the build's own read-through fills the cache regardless);
- encrypted remote storage leaves it empty.
Four mutations of the rule are each caught by exactly the test they target: ignoring is_remote, restoring the old plaintext-only rule, disabling seeding, and ignoring the plaintext flag.
Unit tests on uses_disk_cache / needs_disk_copy / seed_disk_cache, and on fetching from a local store.
hard_drop_removes_annotation_arenas_under_blob: seals a real arena, then hard-drops. It fails with the arena files listed when blob/ is removed from the drop list.
it_memory_storage_reads: 20 warm queries over memory storage on a loaded ledger read nothing from the store. It fails with two leaf reads per query when memory storage stops answering resolve_local_bytes.
Unit pins on each resident arm: resident_leaves_open_without_fetching_or_copying (leaf handle, shared and owned leaf bytes, range reads), resident_dict_leaves_are_borrowed_not_fetched (with and without the global cache), resident_packs_load_without_fetching_or_copying, plus resolve_local_bytes_shares_the_stored_bytes and resolve_cached_bytes_borrows_memory_storage_bytes_at_any_candidate_address in core. Removing each of the seven arms fails exactly the pin that targets it.
build_twice_and_query in it_disk_cache_locality now builds through FlureeBuilder with a private ledger cache directory, so the encrypted and memory tests see what query readers write, not only the indexer. Making the readers' gate ignore the plaintext flag fails the encrypted test; restoring the old plaintext-only rule fails the memory test.
cargo nextest run --workspace --all-features, cargo clippy --all --all-features --all-targets -- -D warnings, and cargo check --workspace --all-targets.

Not addressed here

Sketch size. The sketch blob holds one entry per (graph, property), each with two dense 256-register HLLs hex-encoded in JSON, and is rewritten whole on every build. At 873 named graphs that is 13 MB per build. The median entry has one non-zero register; a sparse binary encoding measures 0.6 MB on the same data.
Storage abstraction. is_remote, permits_plaintext_cache, resolve_local_path, resolve_cached_bytes and resolve_local_bytes each let callers ask the store how to read, and branch on the answer. A follow-up will have get return a shared handle and move the disk cache into a CachedContentStore wrapper, which removes all five. Follow-up: Storage: return shared handles from get and move the disk cache into a store wrapper #2029
Sweeping blob/. The sweep cannot reclaim orphans under blob/, because files there are named only by hash. Giving the annotation kinds an explicit layout under index/ would fix that.
Out-of-tree implementations. is_remote() is a required trait method, so a StorageRead or ContentStore implementation outside this repository needs one line.

Readers already took a store's local file before the cache, but the indexer copied every artifact it wrote into the cache, gated only on permits_plaintext_cache(). On file storage those copies were never read back; memory storage was cached on both reads and writes. A file-backed server collected a copy of every index version in $TMPDIR/fluree_binary_cache, bounded only by a budget of 90% of free disk. - StorageRead and ContentStore gain a required is_remote(). File and memory storage answer false; S3, IPFS, the storage proxy and the browser store answer true; tiered and routing stores answer true when any tier is remote; wrappers delegate. - disk_cache::uses_disk_cache, needs_disk_copy and seed_disk_cache are the one rule every reader and writer of the cache goes through. Every reader gate that checked permits_plaintext_cache() now calls uses_disk_cache; best_effort_cache_bytes_to_path is removed. - MemoryStorage::simulating_remote() gives tests of remote-only behavior a view of the same data that reports its reads as remote.

IncrementalRootBuilder::set_sketch_ref replaced the root's sketch_ref without recording the old CID as garbage, so the collector never released it: every incremental build left an index/stats/*.hll behind. With a build per commit and a sketch of one entry per (graph, property), a ledger of a few hundred commits held 2.4 GB of them. The old sketch now joins the garbage manifest unless the new root still references it. Sketches left by earlier builds stay reachable to the sweep, which reclaims them once the collector has released their roots.

Storage kinds with no layout of their own land under {branch}/blob/, which today means the edge-annotation arenas. Drop listed only commit/, txn/, index/ and config/, so a dropped ledger that had annotations left its arenas on disk.

aaj3f

@bplatz these are nice finds and nice fixes. I started writing this review w/ some questions/notes re: the GC orphan pattern and the decision to make is_remote() required with no default, but the more I consider both the more I'm on the side of your design/implementation decisions. I'll include the full review below (where they are mentioned) so you can see my notes, but I've qualified that they're not recommendations so much as observations:

This is a really nice find: three independent leaks, one commit each, and each with a test that goes red on exactly the thing it fixes (I reverted each fix and watched them fail). The sketch fix follows set_dict_refs' pattern to the letter, and putting every reader and writer of the cache behind uses_disk_cache / needs_disk_copy / seed_disk_cache makes the rule one place instead of a dozen call sites, which is what made the old permits_plaintext_cache()-only gate easy to get wrong in the first place.

Must address before merge:

fluree-db-api/tests/it_disk_cache_locality.rs:125 — build_twice_and_query creates the instance with Fluree::new, so its query readers cache into $TMPDIR/fluree_binary_cache, not the private cache_dir every assertion inspects. encrypted_remote_storage_writes_nothing_to_the_disk_cache therefore can't see a query reader spilling plaintext: with the binary-index reader gate mutated to ignore the plaintext flag, it stays green while a plaintext leaf and two packs land in $TMPDIR/fluree_binary_cache. The production code is right; the test this PR now points to for that guarantee doesn't watch it. Building through FlureeBuilder::memory().with_ledger_cache_config(..).build_with(..) fixes it, and I checked that it catches the mutation and stays empty on this PR's code (snippet inline).

The rest:

🟠 fluree-db-binary-index/src/read/binary_index_store.rs:1138 — memory storage now opens leaves through the remote path (range reads, then a whole-blob copy per open) and re-reads dictionary blobs from the store each query. I measured roughly 520 KB per query copied out of the store at 20k subjects (2.9 MB at 100k) against 12 KB on main, with no wall-clock change I could see in a debug build. vector_query, fulltext_query, annotation_hydration and annotation_planner are the memory-backed benches that would show it, and bench.yml's compare_only doesn't run them, so I'd like to see a before/after on those in the body before merge.
🟠 docs/cli/sweep.md — "need no migration" depends on an operator running fluree sweep, since nothing runs it on its own; the page's list of historical leak sources could name the per-build stats sketches (anchored at it_index_sweep.rs:325).
🟡 Entries earlier versions copied into $TMPDIR/fluree_binary_cache are never read or evicted again on file- or memory-backed hosts, since eviction runs only on writes; a sentence saying the old directory can be deleted would help (docs/security/encryption.md:466).
❓ fluree-db-core/src/storage.rs:258 — whether a default of true for is_remote() is the better trade than a required method: unlike permits_plaintext_cache, a forgotten delegate defaulting to true reproduces today's behavior exactly, and it would spare every out-of-tree implementor the compile break. Your call.
🟡 fluree-db-api/src/admin.rs:1487 — drop_artifacts' doc comment still lists four subprefixes.

On the "Not addressed here" list: I agree the sketch size is separable, since it's a new on-disk encoding for a blob that existing ledgers already hold, and with this PR the superseded sketches are collected, so what remains is write amplification per build rather than growth. Sweeping blob/ is separable for the same kind of reason, and whether those kinds deserve a layout under index/ probably depends on how long the arena kinds are around. The out-of-tree note is the ❓ above.

Adherence to repo commitments:

Patterns/abstractions: ✔ The sketch fix extends IncrementalRootBuilder's replaced-CID pattern rather than special-casing the collector; the cache rule is one function family every gate calls; is_remote sits beside permits_plaintext_cache with the same delegation shape in every wrapper (tiered, routing and branched answer "any tier remote"; encrypted and metered delegate).
Performance (speed first, memory second): ⚠️ File-backed: better, because the indexer stops writing a copy of every artifact and every reader already took the local file first (unchanged). Remote and S3: unchanged, since is_remote() is true and the gates reduce to the old ones. Memory: leaves and dictionary blobs now come from the store each query instead of the cache copy; no regression I could measure, but no release-mode numbers either (see above). The build path gains one Vec push per build.
Deployment targets: ✔ Lambda (S3, the S3+S3 tiered store, optional encryption): every store answers remote, so caching is exactly as before. File-backed hosts (fluree-db-server on file, the embedded file builder): stop seeding the cache; reads unchanged. wasm32: the browser store answers remote and residency mode bypasses the disk cache anyway; wasm32 and wasm-smoke are green. The change only removes disk writes, so no host fence is in play.
Testing: ⚠️ Every new test ran in CI by name (test job: 13,803 passed), and each commit's tests go red when its fix is reverted; the encrypted-remote test's reader half is the gap above.
Conventions: ✔ One commit per defect with thorough bodies; clippy and fmt green; docs/security/encryption.md updated; docs/cli/sweep.md is the one page I'd add to.

Verified locally at branch HEAD 7bba01a97: the new tests by name in grp_index, grp_ledger and the core and indexer lib suites (all pass); one mutation per commit, each caught by its tests; a store-read-counting probe over memory storage on main and on this branch; reader-cache probes over simulated remote storage, with and without encryption. I reviewed at 7bba01a97. The rebase onto #2003 (00eaf44e8) leaves commits 2 and 3 byte-identical (git range-diff), and commit 1 only moves its is_remote onto #2003's HookedStorage, so everything above still holds. CI is green on the new head.

Cross-PR note: merge order. Now that #2017 is on next: since is_remote() has no default, the first main → next merge after this lands needs a one-line is_remote() on #2017's test wrapper RootReadCounting (fluree-db-api/tests/it_index_root_reads.rs:41), and #2017's kind_name kept in disk_cache.rs. Git carries #2017's two io_stats::record(|| kind_name(id), …) calls into fetch_through_cache on its own, but kind_name itself sits in the conflict hunk. Neither break is near a marker: on a local merge of #2017 and #2020 they were an E0046 and two E0425s, and with both fixed your locality tests, #2020's sketch tests and #2017's stats and triple-term suites all pass. #2017's FLUREE_FORCE_REMOTE_READS will also only keep simulating S3 if StorageContentStore::is_remote() reports true while it's set. Since this needs no reindex, it belongs on main, and it's a good 4.2.x candidate alongside #2013.

Approving now so you can merge without waiting on another pass from me — just be sure the test-helper fix is in before you do, and the memory-bench numbers would be great to have alongside it.

aaj3f · 2026-10-06T10:30:35Z

+    let indexer_config = IndexerConfig::small().with_data_dir(tmp.path().join("data"));
+    let cache_dir = indexer_config.artifact_cache_dir();
+    let nameservice = MemoryNameService::new();
+    let mut fluree: Fluree = Fluree::new(


🔴 Must address before merge — encrypted_remote_storage_writes_nothing_to_the_disk_cache can't see what the query readers write, so its "nor what readers fetched" half isn't tested.

Fluree::new here leaves the instance without a ledger manager, so its readers take their cache directory from binary_store_cache_dir() (fluree-db-api/src/lib.rs:3926), which falls back to LedgerManagerConfig::default().cache_dir, i.e. $TMPDIR/fluree_binary_cache. Only the indexer uses cache_dir. So the comment at :110 ("the disk cache of both its indexer and its readers in one private directory") doesn't hold, and the three tests that assert on built.cache_dir only see the indexer's writes: its seeding, plus its own read-through of the previous version.

I checked it two ways. Running this same flow over MemoryStorage::new().simulating_remote() with a fresh TMPDIR, the indexer's private directory got 31 files and $TMPDIR/fluree_binary_cache got 8 that none of these tests look at. Then I mutated the query readers' gate (in fluree-db-binary-index/src/read/artifact_cache.rs only, uses_disk_cache → cs.is_remote(), so the indexer's seeding and read-through keep the real rule): encrypted_remote_storage_writes_nothing_to_the_disk_cache stayed green while three plaintext artifacts, an FLI3 leaf and two FPK1 packs with no FLU\0 envelope, landed in $TMPDIR/fluree_binary_cache. The two binary-index unit tests on stale cache entries did go red, so those gates have unit coverage; but the integration test this PR points to from it_storage_encrypted.rs doesn't watch the reader path, and the one it replaced (build_client_memory_honours_key_and_bypasses_disk_cache) did, through with_ledger_cache_config.

The fix is small: build through the builder so the readers share the private directory (illustrative):

let mut fluree: Fluree = FlureeBuilder::memory() .with_ledger_cache_config(LedgerManagerConfig { cache_dir: cache_dir.clone(), ..LedgerManagerConfig::default() }) .build_with( storage.clone(), NameServiceMode::ReadWrite(Arc::new(nameservice.clone())), );

I tried exactly that against the same mutation: the private directory caught the three plaintext files, and on this PR's code it stays empty. memory_storage_writes_nothing_to_the_disk_cache gets the same benefit, since today it can't see a memory-storage reader writing to the cache either.

aaj3f · 2026-10-06T10:30:35Z

-        // Skipped for a store that forbids a plaintext copy outside it.
-        if cs.permits_plaintext_cache() {
+        // Skipped for a store the cache does not serve.
+        if uses_disk_cache(cs.as_ref()) {


🟠 Should address — memory storage now opens leaves through the remote path, and nothing in CI measures it.

This is more of a measurement ask than a correctness point. Before this PR, memory storage answered permits_plaintext_cache() == true with no local path, so an incremental build seeded the cache and this fast path mmapped leaves from it. Now memory skips it, so a leaf opens through note_remote_leaf_open: range reads for the header and directory first, then from the second touch (HOT_REMOTE_LEAF_PROMOTION_TOUCHES = 2) a whole-blob get_leaf_bytes_sync on every open, which no longer leaves anything behind for the next one. Dictionary blobs that came out of the cache file now come from the store on each query too.

I counted it with a read-counting wrapper around MemoryStorage (20k subjects, a full build then an incremental one, two warm-up queries, then 20 identical queries; debug build). Per query, main reads about 12 KB from the store in four small reads; this branch reads about 520 KB: three leaf blobs (197 KB) and four @shared/dicts blobs (310 KB), plus roots and branches. At 100k subjects it's 2.9 MB per query against 12 KB. Wall clock didn't move across three interleaved runs (113–124 ms per query on both sides), but that's a debug build on a busy machine, so I wouldn't read much into it in either direction.

The memory-backed benches that query an indexed ledger are vector_query, fulltext_query, annotation_hydration and annotation_planner, and bench.yml's compare_only runs only file-backed ones, so a before/after on those four would settle it. I don't think it will matter much (the dictionary blobs were read out of the cache file before, so that half is a different source rather than new work), but since performance is the axis we gate hardest on, I'd like to see those numbers in the PR body before this merges. If they do move, maybe a store that is neither remote nor file-backed could skip the range-read stage and open a full-blob handle directly; I haven't tried that.

aaj3f · 2026-10-06T10:30:35Z

+    );
+}
+
+/// Ledgers indexed before the collector retired superseded sketches still hold


🟠 Should address — existing ledgers only get their sketches back when someone runs the sweep, and docs/cli/sweep.md doesn't say these exist.

The body says ledgers indexed before this fix "need no migration" because the sweep reclaims their sketches, and this test proves the sweep does. But nothing runs the sweep on its own: it's fluree sweep and POST /fluree/sweep (fluree-db-cli/src/commands/sweep.rs:69-78, fluree-db-server/src/routes/mod.rs:128-129), and the collector never reaches what is already off the chain (fluree-db-indexer/src/gc/mod.rs says as much). So a ledger holding 2.4 GB of old sketches keeps them after the upgrade until an operator sweeps it.

docs/cli/sweep.md already names the two historical sources the sweep exists for ("a reindex published by Fluree 4.1.4 or earlier", "dictionary blobs whose manifests were consumed on Fluree 4.2.0 or earlier"). A third, "one stats sketch per incremental build under index/stats/, on Fluree 4.2.3 or earlier", would tell operators to run it. Since the PR leans on the sweep for its upgrade story, I recognize this is minor and non-blocking, but if you agree it's right, I'd rather see it folded in now than lost in the backlog.

Commenting here because docs/cli/sweep.md is not in this diff.

aaj3f · 2026-10-06T10:30:35Z

+  storage (S3, a peer's upstream) keep a read-through disk cache of index
+  artifacts (`$TMPDIR/fluree_binary_cache` by default, or
+  `LedgerManagerConfig::cache_dir`), and the indexer seeds it with artifacts it
+  just built; storage on local disk or in memory never uses it. With


🟡 Optional — what earlier versions already copied into the cache stays there after an upgrade.

On file or memory storage this PR stops every read and write of the cache, and that also stops its eviction: the budget is enforced when something writes to the cache (best_effort_write, fluree-db-core/src/disk_cache.rs:489), and evict_cached_cid only reaches directories this process opened through DiskArtifactCache::for_dir. So on a host with only local storage, the 14 GB the reporting machine had in $TMPDIR/fluree_binary_cache is never read again and never reclaimed by Fluree. Maybe one sentence, here or in the storage docs, that a file- or memory-backed deployment can delete its old cache directory after upgrading? Minor and non-blocking, but if you agree it's right, I'd rather see it folded in now than lost in the backlog.

aaj3f · 2026-10-06T10:30:35Z

+    /// `true` when any of them is remote. Required for the same reason as
+    /// [`Self::permits_plaintext_cache`]: a wrapper that forgot to delegate
+    /// would quietly cache local reads, or stop caching remote ones.
+    fn is_remote(&self) -> bool;


❓ Question — would a default of true be the safer trade for is_remote()?

This is more of a question than a suggestion. I follow the parallel with permits_plaintext_cache, but the failure modes seem asymmetric to me. For permits_plaintext_cache either default can be wrong in a way that hurts (a plaintext spill one way, a silently uncached store the other), so making every implementor answer is the only safe choice. For is_remote, a wrapper that inherited true and forgot to delegate would behave exactly as every store does today, copying into the cache; only false could quietly uncache S3. A true default would make this change non-breaking for every StorageRead / ContentStore implementor outside the workspace, at the cost of the compile-time nudge you get now. I don't feel strongly about it, and it's your call; I mostly wanted to name the trade, since the body's "needs one line" lands on each downstream implementor at its next upgrade.

aaj3f · 2026-10-06T10:30:35Z

            format!("fluree:{storage_method}://{branch_prefix}/txn/"),
            format!("fluree:{storage_method}://{branch_prefix}/index/"),
            format!("fluree:{storage_method}://{branch_prefix}/config/"),
+            format!("fluree:{storage_method}://{branch_prefix}/blob/"),


🟡 Nit — the doc comment on drop_artifacts (fluree-db-api/src/admin.rs:1487) still lists four subprefixes.

It reads "Enumerates the per-branch subprefixes (commit/, txn/, index/, config/)". Adding blob/ there keeps it in step with this list. So minor, but worth folding in with the rest if you agree.

aaj3f · 2026-10-06T10:30:35Z

+    ///
+    /// **GC note**: Records the old sketch CID as replaced unless the new
+    /// root still references it.
    pub fn set_sketch_ref(&mut self, cid: Option<ContentId>) {


👍 This is the set_dict_refs / set_annotation_index pattern to the letter, and it composes with the collector the way it should: retained_refs checks every manifest entry against all_cas_ids() of each retained root, which includes sketch_ref (fluree-db-binary-index/src/format/index_root.rs:1240-1243), so a sketch any retained version still points at can't be released early; and BranchedContentStore::release touches only the branch's own namespace (fluree-db-core/src/storage.rs:1426), so a fork's first build naming its parent's sketch releases nothing of the parent's. Reverting just these lines makes collected_index_versions_leave_no_stats_sketches_behind fail with exactly the three leaked sketches the body describes, and set_sketch_ref_retires_the_superseded_sketch with it.

zonotope · 2026-10-07T15:47:07Z

@bplatz I'm 👍🏾 for merging this pr because it fixes a problem we're running into now, but I'm in the middle of writing a longer review with some medium term suggestions that I think would both simplify the code and avoid issues like this in the future.

…ollow-ups build_twice_and_query built its instance with Fluree::new, which has no ledger manager, so query readers cached into $TMPDIR/fluree_binary_cache rather than the private directory every assertion inspects. The encrypted and memory tests saw only the indexer's writes. Building through FlureeBuilder with a ledger cache config puts the readers in the same directory. - docs/cli/sweep.md names the per-build stats sketches left by 4.2.3 and earlier, which only a sweep reclaims. - docs/security/encryption.md notes that cache entries earlier releases copied from local or memory storage are never read or evicted again. - drop_artifacts' doc comment lists blob/.

With the disk cache limited to remote storage, memory storage fell through to the remote read lane: range reads on a leaf's first open, then a whole-blob copy on every open after it, kept nowhere. Dictionary leaves and packs were copied out of the store on every query too. StorageRead gains resolve_local_bytes, the in-memory counterpart of resolve_local_path, defaulting to None. StorageContentStore's resolve_cached_bytes falls back to it at the current and legacy addresses, in the order resolve_local_path uses. MemoryStorage now stores Arc<[u8]> and hands its bytes out shared; a simulating_remote view holds nothing resident. Each native reader takes resident bytes right after the local file: leaf handles (SharedBlobLeafHandle, with the directory shared through the leaflet cache), whole-leaf bytes, range reads, dictionary leaves and forward packs. SharedLeafBytes::Shared and SharedBlobLeafHandle are no longer residency-only. MeteredContentStore now delegates resolve_cached_bytes. Encrypted storage keeps the None default, since its bytes at rest are ciphertext. On a loaded ledger, warm queries over memory storage now read nothing from the store, against 16 reads per query before.

zonotope

I'm approving here because I think the fix is sound, but I think is_remote and how it's used here is adding additional complexity which would make the system less maintainable going forward. I think we could simplify the storage layer in a way that would make things like the disk cache bug less likely in the future, and make issues like that easier to fix when they do show up.

I try to avoid adding boolean methods to traits because they usually exist to facilitate control flow outside of the trait definition. Code outside of this trait will inevitably use this method to provide some sort of polymorphism by hand (if is_remote() { do(this)} else {do(that)}), and doing dispatch by hand defeats the purpose of the trait.

Ideally, nothing that reads from or writes to storage would care whether or not they're writing to a local or remote back end, whether that back end is cached, or even whether the back end might be encrypted. It should only care about storing some data in a way that it can access it later. Local vs remote, caching, and encryption are implementation details that nothing outside of the trait boundary should be concerned with.

I think we're here because of two reasons: the trait is at the wrong level of abstraction, and we aren't fully encapsulating storage functionality as we should. Right now we already have a permits_plaintext_cache trait method that leaks implementation details outside of the trait, and this pr is adding is_remote. It looks like both of those methods exist so that the independent caching functions can find out whether or not they can or need to get involved. Instead, the cache should be part of the store in the first place, and implementations that aren't helped by caching just wouldn't implement caching.

The cache is here so that the query-side readers can optimize reads by not fetching index artifacts more than once. It's a directory of local files so that those readers can memory map them. That forces them to know about paths, whether data is stored locally or remote, and it has to know about a separately managed cache dir.

The cache has to sit outside of the trait implementations to be useful because the trait sits at the wrong abstraction level. Right now ContentStore::get returns the whole blob as a Vec<u8> copied on the heap, asynchronously. get doesn't fit for accessing leaves because we'd copy the entire leaf on the heap every time we open one, and mapping the file wouldn't save anything at that point.

We could fix this by both adding a CachedContentStore<S> wrapper that cached what's read out of its inner content store, and changing get to return a shared, read-only handle that would deref to [u8] instead of returning a fully instantiated Vec<u8>. File storage would return a memory map directly instead of what we do now by forcing outside code to call resolve_local_path so that they could access the raw data outside of the storage trait and make a memory map from that. Memory storage would return an Arc clone, and the cached wrapper would return a map of its cache file. The cached wrapper would also implement get_range by reading from its cache file.

In each of these cases, callers would just read bytes from storage. They wouldn't need to care about where those bytes came from, whether or not they could or should be cached, or how to apply particular optimizations. All of the nuances of the individual implementations are kept in the implementations themselves.

These changes would also lead to similar simplifications around encryption. Because caching sits outside of the storage stack and encryption is inside it, the caching layer can only see plaintext. permits_plaintext_cache exists so that the caching layer can find out if it's allowed. If caching was just another wrapper around a generic storage impl, then we could implement encrypted caching by paying attention to the wrapping order to hold ciphertext in the cache.

I think this pr does fix a problem that users are running into right now, and the changes I'm proposing would be a pretty big refactor, so I'm fine with not holding up this bug fix for a storage refactor if you prefer. In that case, I think we should open another issue and continue the discussion about these changes there.

bplatz · 2026-10-07T17:58:47Z

Thanks both.

@aaj3f:

Test helper: fixed in 70ad15a, which builds through FlureeBuilder with a private ledger cache dir. Making the readers' gate ignore the plaintext flag now fails the encrypted test, and the old plaintext-only rule fails the memory test.
Docs: sweep.md now names the stats sketches from 4.2.3 and earlier. encryption.md now says the old cache directory can be deleted. The drop_artifacts comment now lists blob/.
Memory benches: your read count was right, and it shows up in time: up to 1.73× on the indexed memory benches. 0b8ae3e gives memory storage a zero-copy read path and brings them back to roughly base. The numbers are in section 4 of the description. One residual, annotation_hydration/scan at 1.11–1.18×, is tracked in annotation_hydration/scan is 11–18% slower than #2018's merge base over memory storage #2030.
is_remote default: keeping it required for now, since the follow-up removes it entirely.

@zonotope: agreed on the direction. Filed as #2029 with a staged plan. 0b8ae3e adds one more hook of the kind you describe, as a stopgap the refactor deletes.

bplatz added the bug Something isn't working as expected label Oct 5, 2026

bplatz requested review from aaj3f and zonotope October 5, 2026 16:27

bplatz added 3 commits October 5, 2026 14:37

fix(api): hard drop removes the branch's blob/ prefix

00eaf44

Storage kinds with no layout of their own land under {branch}/blob/, which today means the edge-annotation arenas. Drop listed only commit/, txn/, index/ and config/, so a dropped ledger that had annotations left its arenas on disk.

bplatz force-pushed the fix/index-storage-growth branch from 7bba01a to 00eaf44 Compare October 5, 2026 18:41

bplatz mentioned this pull request Oct 5, 2026

perf(indexer): sparse binary + zstd stats sketches; 15-minute GC default #2020

Merged

bplatz added this pull request to stack #2021 October 5, 2026 19:51

aaj3f approved these changes Oct 7, 2026

View reviewed changes

bplatz added 2 commits October 7, 2026 12:23

zonotope approved these changes Oct 7, 2026

View reviewed changes

This was referenced Oct 7, 2026

Storage: return shared handles from get and move the disk cache into a store wrapper #2029

Open

annotation_hydration/scan is 11–18% slower than #2018's merge base over memory storage #2030

Open

Merge remote-tracking branch 'origin/main' into fix/index-storage-growth

c380788

bplatz merged commit 95f99ac into main Oct 7, 2026
9 checks passed

bplatz deleted the fix/index-storage-growth branch October 7, 2026 18:14

aaj3f mentioned this pull request Oct 8, 2026

fix(query): evaluate grouped SELECT expressions once per group #2006

Open

Navigation Menu

Repository navigation

fix: index storage growth — stats sketch GC, disk cache for local storage, drop of blob/ - #2018

fix: index storage growth — stats sketch GC, disk cache for local storage, drop of blob/#2018
bplatz merged 6 commits into
mainfrom
fix/index-storage-growth

bplatz commented Oct 5, 2026 •

edited

Loading

Uh oh!

aaj3f left a comment

Uh oh!

aaj3f Oct 6, 2026

Uh oh!

aaj3f Oct 6, 2026

Uh oh!

aaj3f Oct 6, 2026

Uh oh!

aaj3f Oct 6, 2026

Uh oh!

aaj3f Oct 6, 2026

Uh oh!

aaj3f Oct 6, 2026

Uh oh!

aaj3f Oct 6, 2026

Uh oh!

zonotope commented Oct 7, 2026

Uh oh!

zonotope left a comment

Uh oh!

bplatz commented Oct 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bplatz commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Superseded stats sketches were never garbage collected

2. The disk artifact cache served local storage

3. Hard drop left {branch}/blob/ behind

4. Memory storage read its index through the remote lane

Memory-backed benches

Tests

Not addressed here

Uh oh!

aaj3f left a comment

Choose a reason for hiding this comment

Uh oh!

aaj3f Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

aaj3f Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

aaj3f Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

aaj3f Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

aaj3f Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

aaj3f Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

aaj3f Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

zonotope commented Oct 7, 2026

Uh oh!

zonotope left a comment

Choose a reason for hiding this comment

Uh oh!

bplatz commented Oct 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bplatz commented Oct 5, 2026 •

edited

Loading

3. Hard drop left `{branch}/blob/` behind