fix(storage): clamp FileStorage ranged reads to the object, and sweep orphaned .tmp staging files

[aaj3f]

Two storage fixes that both live in fluree-db-core/src/storage/file.rs, so they're riding together rather than as two one-file PRs.

Fixes #1671
Fixes #1620

#1671 — ranged reads now clamp, like the trait already said they would

The issue frames this as FileStorage and ProxyStorage disagreeing. Having gone looking, I think it's actually a bit stronger than that: the trait's own doc comment at fluree-db-core/src/storage.rs:212-214 already promises the clamp —

Returns the bytes within the range, which may be shorter than requested if the object is smaller than range.end.

— and the default impl (storage.rs:219-229), MemoryStorage (storage/memory.rs:94-107), and the proxy (which has no override, so it inherits the default) all honour it. FileStorage was the only StorageRead implementation that sized its buffer from range.end - range.start rather than from the object, so this is a lone contract violation rather than a sibling divergence. I think that also answers the open question at the end of the issue — there isn't really a doc decision to make, the doc was already right and the code just wasn't meeting it.

Mechanism, and it's worth being precise that this was never only about open-ended ranges: let len = (range.end - range.start) as usize ran before any I/O, so the allocation was sized from the request for any range. 0..40_000_000_000 on a 10-byte file allocated 40 GB of address space and handed the caller a Vec with len 10 and capacity 40,000,000,000 — I reproduced that shape directly and got exactly len=10 capacity=40000000000 at ~1.7 MB max RSS, because vec![0u8; len] gets lazy zero pages that never fault in. So oversized ranges "worked" while quietly reserving whatever the caller asked for, and only a width past Vec's capacity limit actually tripped: mid..u64::MAX — the established "read to the end" spelling, asserted against the proxy at fluree-db-server/tests/proxy_integration.rs:1821 — became vec![0u8; usize::MAX], whose capacity-overflow panic spawn_blocking caught and relabelled Io("spawn_blocking failed: task panicked ..."). A slightly unkind touch: the one backend that couldn't serve the call was also the one that couldn't say why.

The fix is the one suggested in the issue. #1599 already stats the open handle in this same function for its zero-length guard, so the size was sitting right there — I hoisted that single metadata() call above both guards and clamped with it:

let len = requested.min(file_len.saturating_sub(offset)) as usize;

The read loops already stopped at EOF, so an in-range request is byte-identical to before.

One deliberate behaviour change worth flagging, since it's the bit I'm least certain about: the zero-length guard used to be file.metadata().map(|m| m.len() == 0).unwrap_or(false), i.e. it tolerated a stat failure and carried on. It now returns Io instead. My reasoning is that the buffer size is derived from that stat, so without it there's nothing to size the read against, and an error naming the failure beats either guessing a length or falling back to trusting the caller's u64. In practice this is fstat on a descriptor the thread owns and doesn't fail — but if the permissive form is preferred, I'm happy to put it back.

I checked the other implementations for the same class while I was in there and none of them have it: the default / memory / proxy-filtered paths slice after a full read, Arc<dyn Storage> and the two routers in fluree-db-api/src/lib.rs are pure delegation, and S3 and proxy-raw hand the range straight to a remote as an HTTP Range header, where the server does the clamping per RFC 7233.

One boundary over, though, the backends still don't fully agree, and the trait doc now names both known divergences rather than letting one stand as if it were exhaustive — a contract paragraph with an unnamed exception is exactly the shape #1671 grew from. First: at start >= object length, FileStorage and MemoryStorage both return Ok(vec![]) — this PR's own test asserts it at 99..u64::MAX — while S3 returns 416. That predates this change, isn't in this diff, and is tracked as #1712; fixing it wants someone who can exercise real S3 rather than my reading of RFC 7233. Second: a zero-length object reads as NotFound from FileStorage — the #1599 debris guard treats an empty blob as absent on every read surface, this one included — where MemoryStorage and the default implementation return Ok(vec![]). That one is deliberate and stays deliberate; it's still a divergence in this method's observable behaviour, so the doc says so.

#1620 — sweeping staging files a crash left behind

Confirmed neither candidate coverer actually covers this. #1612 cleans up on its own error paths in stage_bytes / write_atomic / create_new_atomic, and none of those run for a SIGKILL, an OOM kill or a power loss. #1614's sweep plans over list_prefix, which continues on is_tmp_artifact (file.rs:561 at the merge base) — so it structurally can never enumerate one of these files, let alone reclaim it. That filter is exactly what makes an orphan harmless and immortal at the same time.

The sweep is FileStorage::sweep_orphaned_staging(), an explicit startup action — deliberately not mounted on the constructor. That placement is load-bearing, not taste. The parser below tolerates one residual (a foreign <name>.<decimal>.<decimal>.tmp still parses as a legacy orphan), and the argument for tolerating it leans entirely on the sweep being a deliberate, visible, disable-able startup step. Hung off new, that argument collapses, because FileStorage::new is reachable from any unit test or tool pointed at a directory the process doesn't own — fluree-db-connection has a Debug-formatting test that constructs FileStorage::new("/tmp/test"), and with the sweep on the constructor, running that one test unlinks a 90-day-old mybackup.json.1.2.tmp an operator left under /tmp/test. Each decision is defensible alone; composed, they silently delete operator data. So constructing a storage now touches nothing on disk — which also takes the env reads, the canonicalize and the global claim-mutex back out of the constructor — and the layers that actually know it's startup make the call:

• create_sync_connection / create_async_connection's file arms in fluree-db-connection
• FlureeBuilder::build, build_encrypted_internal, build_client_file, and the address-identifier storages in build_local_storage_from_config in fluree-db-api

That covers every production entry to a file-backed instance — the server and CLI arrive through those builders, and FileNameService::new shares the very tree the builder already sweeps, with the once-per-canonicalized-path claim making the layered calls idempotent. fluree-search-httpd constructs read-only FileStorage handles and deliberately does not sweep: a reader has no business unlinking anything in a writer's tree, and no released build ever swept there.

The part I'd most like a second opinion on is the concurrency stance, so it's worth being explicit about what it does and doesn't promise. The tree is shared — between processes in a multi-instance deployment, and between subsystems even in one process — and a sweep that guesses wrong pulls a live writer's file out from under it. Three rules:

0. Never a file this backend's staging writer didn't name. .tmp is a suffix, not a namespace: the indexer's vocab merge, the disk cache, the nameservice tracking file and the Raft log all stage under it, and the nameservice shares this exact tree — FileNameService::new builds a FileStorage on the path FlureeBuilder::build also hands to storage. So a file is only the sweep's business once its name parses as one this writer produced, with every segment shape-checked rather than just the dot count. Rules 1 and 2 apply only to names that pass. Predicating on ends_with(".tmp") instead — the obvious cheap version — would leave the age heuristic as the only thing standing between vocab_merge's index-build temporaries and an unlink, and a long-running index build is precisely the shape rule 2 is not competent to judge.
1. Never a staging file bearing this process's token. tmp_sibling embeds a 64-bit token drawn once per process, so a name carrying ours is ours. In-flight and already-leaked are indistinguishable from a directory entry, so both are left alone. This rule is exact — for the current name format. A legacy <name>.<pid>.<seq>.tmp name carries a pid where the token now sits, and a pid can never equal a 16-hex token, so for legacy names rule 1 is inert and rule 2 stands alone. That case isn't a corner, it's the upgrade path itself: a rolling upgrade has a still-running v4.1.5/v4.1.6 process staging into the shared tree in the old format, protected by the age heuristic and nothing else. Fine in practice — a staging write is open for milliseconds against a one-day threshold — but the rules don't layer there, and the docs now say so plainly instead of presenting 1 and 2 as stacked defenses.
2. Never a file modified inside 24 hours. This one is a heuristic and I want to name it as one: it reads another host's clock through ours, and it assumes no legitimate staging write stays open that long.

Anything the sweep can't classify — an unparseable name, an entry it can't stat, an mtime in the future — is kept. Every unknown resolves toward leaving the file alone.

Two notes on that parser, since it's the load-bearing bit. Zero-padded numbers are rejected — nothing here emits one, and it keeps date- and offset-stamped foreign names (backup.2026.08.tmp, wal.000001.000002.tmp) out of the legacy branch. One residual remains, deliberately: a foreign name shaped <non-empty>.<decimal>.<decimal>.tmp — dump.1.2.tmp — still parses as legacy. I checked all four in-tree .tmp producers against the parser and none emits that shape, so it's an operator's own file rather than a collision with anything we ship. Closing it properly means gating legacy reclaim behind an opt-in, which is an upgrade-behavior decision rather than a parser one — and with the sweep now reachable only from an explicit startup call an operator can see coming and switch off, rather than from any construction, I think recording it on staging_token is the right resting place.

And the legacy branch is load-bearing rather than politeness: staging writes arrived in 85183c9ca, which v4.1.5 and v4.1.6 both contain, so every store written by a currently released build produces orphans in that format. Dropping it would strand them on disk permanently.

On why an age threshold is defensible here when #1635 argues (correctly, I think) that it isn't for the index-build sweep: the objection there is that a reindex replays the whole commit chain, so its duration grows with the ledger and no constant can bound it. A staging file doesn't have that shape — its entire life is one write_all of one already-in-memory buffer, followed immediately by a rename. There's no long-running case to get wrong. I believe that's a genuinely different situation rather than the same heuristic reused, but if it reads as the same heuristic wearing a different hat, I'd rather hear that before this ships than after.

What it explicitly does not guarantee: it is not a lease, and it does not coordinate with other processes. A foreign staging write that somehow stayed open for over a day would be unlinked. Even then I think the blast radius is small — on POSIX the writer keeps its open descriptor, so its write_all and sync_all still succeed against the now-unlinked inode and only the final rename fails, surfacing as a write error rather than anything torn or wrong landing under a content address. But it is a spurious write error, and I'd rather say that than imply the sweep is airtight. If/when #1635 lands a real lease, this is a natural thing to hang off it.

Practicalities: the walk is handed to the blocking pool when there's a runtime to hand it to (the sweep is called from create_async_connection and the API's async client builds, so a recursive read_dir on the caller would park a runtime worker — ~1s warm on local APFS for the default budget), and runs inline only for the genuinely synchronous caller (create_sync_connection). It runs at most once per base path per process however many startup layers call it, keyed on the canonicalized path so that's a property of the directory rather than of how someone spelled it.

It's also bounded, at 100k directory entries. Exhausting that budget is not a deferral, and it would be easy to describe it as one: the walk restarts from the top with no cursor and never removes content, so if the first 100k entries it meets are content, every later start re-walks those same entries and never reaches the orphans past them. That case logs at warn (reclaiming logs at info — different events), and FLUREE_STORAGE_TMP_SWEEP_BUDGET raises the cap, because a warning an operator can't act on is just noise. FLUREE_STORAGE_TMP_SWEEP=0 turns the sweep off entirely, using the same falsey spellings FLUREE_STORAGE_FSYNC already accepts. The budget is a separate variable rather than an overload of that one on purpose — FLUREE_STORAGE_FSYNC=1 means "on", so reading FLUREE_STORAGE_TMP_SWEEP=1 as a one-entry budget would be "off" wearing a disguise.

Tests

For #1671 the guard test fails at the parent commit with the exact error from the issue and passes after. There's also a differential test that runs nine ranges — three of them open-ended — through both FileStorage and MemoryStorage and asserts they agree, since backend agreement is the property that was actually broken.

For #1620, both directions the issue asked for, plus both directions of the mounting decision — the constructor must not sweep, and no startup path may lose its sweep:

• construction_is_pure_and_the_explicit_sweep_reclaims_an_orphan — two pins in one: an aged orphan survives FileStorage::new, then the explicit sweep reclaims it
• file_connection_startup_sweeps_orphaned_staging (fluree-db-connection) and building_a_file_instance_sweeps_orphaned_staging (fluree-db-api) — the flip-side regression guards: opening a file connection and building a file-backed client each still reclaim a planted stale orphan. Both run without a runtime, so the walk is inline and the assertion can't race it.
• the_sweep_leaves_a_live_looking_staging_file_alone — a fresh foreign-token file survives
• our_own_staging_file_is_never_reclaimed_however_old — rule 1 beats rule 2 even at a zero threshold
• the_sweep_ignores_tmp_files_other_subsystems_wrote — the seven real foreign .tmp shapes, plus an arity case, all aged past the threshold and swept at a zero threshold so age offers no cover and rule 0 is the only thing on trial
• the_shape_check_still_reclaims_our_own_orphans — the tightening didn't cost us the orphans the sweep exists for, current format and legacy alike, sitting next to a foreign file that must survive
• the_walk_is_handed_to_the_blocking_pool_when_a_runtime_exists / the_walk_runs_inline_without_a_runtime — the hand-off both ways; the async one awaits the returned JoinHandle rather than polling for the file
• a_future_mtime_is_treated_as_live, only_files_past_the_threshold_are_reclaimed, the_sweep_never_touches_content, the_walk_is_bounded_by_its_entry_budget, a_base_path_is_swept_at_most_once_per_process

And the concrete shape that motivated the mounting: with a 90-day-old mybackup.json.1.2.tmp planted in /tmp/test, both hardcoded-path unit tests (test_connection_handle_file in fluree-db-connection/src/lib.rs, test_create_file_storage in fluree-db-connection/src/storage.rs) run green and the file survives. The one test that pointed a real startup path at /tmp/test — test_fluree_builder_file — now builds in a tempdir, since a startup path sweeping is exactly its documented behaviour.

I mutation-checked the mounting as well as the rules this time — ten mutants, ten killed, each by the test that owns it: disabling the shape check reddens the_sweep_ignores_tmp_files_other_subsystems_wrote; disabling the token rule reddens our_own_staging_file_is_never_reclaimed_however_old and only that; disabling the age rule reddens three; admitting zero-padded decimals reddens the parser test with Some("2026") for backup.2026.08.tmp; a spawned walk that never runs reddens the blocking-pool test; a gutted inline walk reddens two; letting every claim win reddens the once-per-path test; hanging the sweep back on new reddens the purity pin; and deleting the wiring call from either create_sync_connection or FlureeBuilder::build reddens the corresponding startup pin. The assertions check the reclaimed/kept counts and not just file existence, so a file that was never examined can't pass as one that was deliberately kept.

Gates

• cargo test -p fluree-db-core --all-features — 815 lib tests green (46 of them in storage::file), plus integration and doc-tests
• cargo test -p fluree-db-connection --lib (39), cargo test -p fluree-db-api --lib (776), cargo test -p fluree-db-nameservice --lib (163) — the crates whose startup paths now own the sweep call
• cargo check -p fluree-db-connection --features aws — the async (S3-capable) connection path compiles with its file-arm sweep
• cargo check -p fluree-db-core --no-default-features --features native --lib — clean, since Handle::try_current needs tokio's rt and I didn't want that quietly supplied by a dev-dependency
• cargo clippy -p fluree-db-core --all-features / -p fluree-db-connection / -p fluree-db-api — --all-targets --no-deps, clean
• cargo fmt --all --check — clean

Docs: the "Staging files left by a crash" section in docs/operations/storage.md now says the sweep is an explicit startup step (constructing a storage handle never deletes anything) and carries the legacy-format carve-out in operator terms, next to the durability table.

[bplatz]

Approving. I verified both issues on pristine main (fe3c198c8) rather than taking them as given, and both are real: 0..u64::MAX gives Err(Io("spawn_blocking failed: … capacity overflow")) on FileStorage against Ok(10) on MemoryStorage, and planted orphans (current and legacy format, aged) survive construction untouched. Your "lone violator" reading holds too — default impl, memory and proxy all clamp.

The clamp fix is clean: on the branch 0..MAX→10, 5..MAX→5, 99..MAX→0, 0..40e9→10, 3..7→4, identical to MemoryStorage across the board. I also probed the staging parser directly and every claim in the description held — uppercase hex, zero-padded segments and a.b.c.tmp all rejected, dump.1.2.tmp admitted as the stated residual.

One note for the #1671 write-up: it isn't only open-ended ranges. The allocation was sized from the request for any range — 0..40_000_000_000 on a 10-byte file allocated 40 GB of address space and returned a Vec with len 10 and capacity 4e10. Lazy zero pages mean no panic and negligible RSS, so only usize::MAX actually trips capacity overflow, but the oversized reservation was handed to the caller. Your fix covers all of it; worth a line so it isn't later read as narrowly about u64::MAX.

Three things below. The first is a demonstrated defect and I'd want it settled before merge; the other two are a doc sentence each.

On the concurrency stance you asked about: the rules themselves I think are right, and the argument for why an age threshold is sound here but not for #1635 holds up — a staging file's life really is one write_all plus a rename, with no long-running case to get wrong. My concern isn't the rules, it's where they're mounted.

[bplatz]

This is the one I'd change before merge: a destructive, environment-mutating sweep hangs off a bare constructor, and it composes badly with the parser residual you documented on staging_token.

fluree-db-connection/src/lib.rs:400 is a unit test whose whole body asserts a Debug string, and it constructs FileStorage::new("/tmp/test") — a hardcoded shared path. I created /tmp/test, planted mybackup.json.1.2.tmp (exactly the <name>.<decimal>.<decimal>.tmp shape you describe as "an operator's own file rather than a collision with anything we ship"), stamped it 90 days old, and ran that single test:

planted, mtime 90d old: True
test result: ok. 1 passed
ls /tmp/test -> empty

The file was gone. Each decision is defensible alone — the residual is tolerable because the sweep is a deliberate startup action, and sweeping at startup is reasonable — but together they mean constructing a FileStorage silently unlinks operator data.

I'd move it out of new: an explicit call from the connection/startup layer, which is the layer that actually knows it's startup, or a builder step next to with_durability. That also takes env reads, canonicalize and a global mutex back out of a constructor, and makes the /tmp/test test harmless again.

If you'd rather keep it here, then I think the residual has to close rather than stay recorded, since new being reachable from tests and tooling is what turns it from theoretical into the repro above.

[bplatz]

Worth saying in the doc that rule 1 is exact only for the current format. A legacy name puts a pid in the token slot, so it can never equal a 16-hex own_token:

staging_token("real.json.<our pid>.0.tmp") -> Some("45116")
own_token                                  -> "3d7b301d75f9a910"   // never matches

So for legacy names rule 1 is inert and rule 2 is the only thing standing. The case where that matters is a rolling upgrade — a v4.1.5/v4.1.6 process staging into a shared tree is covered by the heuristic alone. In practice fine, since a staging write won't stay open a day. But the doc presents 1 and 2 as layered, and on the branch that's load-bearing for exactly the deployment shape it was written for.

[bplatz]

There are two gaps here, not one. For a zero-length object FileStorage returns NotFound (the #1599 debris guard) where MemoryStorage returns Ok(vec![]):

ZERO file 0..10 -> Err(NotFound("z/empty: … (zero-length blob, treated as absent)"))
ZERO mem  0..10 -> Ok(0)

Deliberate, and I think correct. But this paragraph is here specifically to make the contract authoritative, and naming only S3 reads as exhaustive — which recreates in miniature the thing #1671 was. A clause pointing at the zero-length rule as the other exception would settle it.

[aaj3f]

Thanks @bplatz — the /tmp/test demonstration settled it, and I took your preferred fix: the sweep moved out of new, not the residual closed around it.

FileStorage::new is pure again — no filesystem access, no env reads, no claim-mutex. The sweep is an explicit sweep_orphaned_staging(&self) keeping the exact Handle::try_current()→spawn_blocking / inline split, wired at every file-backed startup entry: the connection layer's sync and aws-async file arms, and the four builder paths in fluree-db-api (build, build_encrypted_internal, build_client_file, build_local_storage_from_config) — the server and CLI arrive through those. Deliberately not wired: FileNameService::new (it shares the tree the builder already sweeps, and the once-per-canonicalized-path claim dedupes), the bare create_file_storage factory, and fluree-search-httpd's read-only handles.

Re-running your repro post-fix: planted mybackup.json.1.2.tmp stamped 90 days old in /tmp/test, ran that unit test — green, file survives, mtime intact. Chasing the shape further turned up two more of the same class worth knowing about: test_create_file_storage also constructs against hardcoded /tmp/test (harmless now for the same reason), and test_fluree_builder_file in fluree-db-api calls the real build() path on /tmp/test — which means with the sweep wired into build() it would have become the next deleter. It builds in a tempdir now.

Both regression directions are pinned: construction_is_pure_and_the_explicit_sweep_reclaims_an_orphan asserts an aged orphan survives new and then dies to the explicit call, and two runtime-free wiring pins (connection and builder) each fail if their sweep call is deleted — so a startup path can't silently lose the sweep, which was the regression I most wanted a guard against after un-mounting it. Full mutation matrix is 10/10, including the two new wiring mutants.

Both doc sentences landed essentially as you framed them: rule 1's legacy carve-out (a pid can never equal a 16-hex token, so rule 2 stands alone for exactly the rolling-upgrade shape), and the contract paragraph now names two known divergences — the S3 416 (#1712) and the zero-length NotFound from the #1599 guard.

And your allocation finding reproduced exactly — the pre-fix shape on a 10-byte file gives len=10, capacity=40000000000 at 1.7 MB max RSS, so the reservation was request-sized for any range and only usize::MAX widths ever tripped. The body now says so rather than reading as narrowly about u64::MAX.