fix: never delete raw-txn blobs inline; tolerate dangling txn refs; add fluree verify - #1677
Conversation
…xn refs; add `fluree verify` Raw-txn blobs are content-addressed (identical body → identical CID) but `ContentStore::release` is an unconditional delete with no refcount. The commit path released the blob on every failure branch — the pending-upload Drop guard (detached task), `build_commit` errors, and publish failures — so any transaction whose body matched an already-committed one could delete that commit's blob. Seen in production: an in-process commit-conflict retry re-spawned the same upload while attempt 1's detached delete landed after, leaving a commit with a permanent dangling `txn` pointer that broke clone. - Remove all inline releases of raw-txn blobs; orphans are left for reachability GC. Regression test fails on the old guard, passes now. - Export, pack, push-validate, merge, and CLI push/clone now warn and continue on a missing txn blob instead of failing the whole chain. `ExportCommitsResponse` gains `missing_blobs`. - New `Fluree::verify_ledger` + `fluree verify`: walks the commit DAG and reports missing commits/parents, undecodable commits, t gaps, missing txn blobs, and a missing index root. Exits non-zero on problems. - INFO-level progress lines through the commit window (head temporal resolved, raw txn upload finished, commit record built, commit head published) so a future commit-phase hang localizes from logs alone.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
This is careful work, and the diagnosis was very nicely described, @bplatz
Two things to consider before merge. First, the PR body and the two doc comments (commit.rs:762, raw_txn_upload.rs:22-25) say orphans are "reclaimed by reachability GC", but I'm not sure what this reachability GC would be as there there is no collector for txn blobs so far as I can tell. gc/sweep.rs:20-24 excludes transactions by design, and nothing else releases ContentKind::Txn. Because the upload is spawned before staging, every distinct failed body (SHACL/policy/parse, novelty cap, exhausted retries) is now a permanent blob, and Fluree AI / solo opts every transaction in. The direction is right — there's no safe inline delete without a refcount — but I'm not following the claim of the comments.
Minimally, update the comments; the real completion would be something like a commit-chain-rooted sweep of txn/, which verify_commit_chain now makes possible, and I'd genuinely like your call on whether that lands here or immediately behind.
Second, clone (sync.rs:1617-1622) tolerates every fetch error — MultiOriginFetcher::fetch folds 404/auth/5xx/network into one FetchFailed and IntegrityFailed is caught too — which contradicts the body's own "only NotFound is tolerated" and turns a corrupt origin blob or a mid-clone network blip into a silent provenance gap. A typed not-found in nameservice-sync matched here (and only here) fixes it; every other tolerant site in the PR already matches NotFound specifically.
The rest is optional though I'd advocate for seeing it folded in rather than punted: a declared missing_blobs on the push request so a client that forgot blobs still gets a 400 (commit_transfer.rs:904-913), distinct exit codes for verify so it can actually be the automation gate the docs describe (commands/verify.rs:66-72), and walking the primary lineage first in verify_commit_chain (verify.rs:151, one-line .rev()).
Adherence to repo commitments:
- Patterns/abstractions:
⚠️ ExtendsContentStore/ExportCommitsResponsecleanly and addsFluree::verify_ledgerin the api crate's idiom; but it introduces "orphan for GC" as a storage contract without connecting it to the existinggc::plan_sweepmechanism that is the repo's actual reclaim abstraction. - Performance (speed first, memory second): ✔ No hot query path touched; commit success path gains four
info!events per commit and nothing per flake; failure paths lose an await. Disk growth from unreclaimed txn blobs is the one resource concern (above). No performance-degradation risk. - Testing:
⚠️ The two incident tests are wired (grp_transact.rs:8-9), run, and go red under mutation; no coverage of push-side tolerance, pack/merge/clone tolerance, or the CLIverifyexit path. - Conventions: ✔ Multi-line conventional commit;
cargo fmt --all -- --checkclean;cargo clippy -p fluree-db-transact -p fluree-db-api -p fluree-db-cli --all-targets -- -D warningsclean; docs added (docs/cli/verify.md,endpoints.md, README/SUMMARY rows).
Verified locally at branch HEAD (06de3ed99, base f41e33b2b): fmt clean; nextest -p fluree-db-transact 350/350; nextest -p fluree-db-api --test grp_transact -E 'test(raw_txn)' 3/3 with both new tests seen by name; both mutations red then green on restore; clippy clean at package scope. Note this head has had no CI run (stacked on #1674, which is itself stacked; ci.yml only fires on PRs into main), so please retarget or wait for the stack to fold before relying on green checks.
Happy to talk through any of these
| /// [`build_commit`] is deliberately left in place: it is | ||
| /// content-addressed and may be shared with a commit that did | ||
| /// publish, so deleting it here could dangle that commit's | ||
| /// `txn` pointer. Orphans are reclaimed by reachability GC. |
There was a problem hiding this comment.
🟠 High / blocking as written — also applies to the module doc at fluree-db-transact/src/raw_txn_upload.rs:22-25.
🟠 The reclaim path these comments (and the PR body) name does not exist — nothing sweeps orphaned txn blobs today, so this PR converts a racy delete into a permanent, unbounded leak.
The only garbage collection in the workspace is fluree-db-indexer/src/gc/ — the manifest-driven collector plus the storage sweep from #1614 — and the sweep's own scope note excludes this content kind explicitly: "Only index artifacts — every branch's index/ prefix plus the ledger's shared dict namespace. Commits, transactions, and config blobs are deliberately excluded: they are reachable through the commit chain, not the index chain, so this sweep cannot prove them unreferenced" (fluree-db-indexer/src/gc/sweep.rs:20-24). After this PR the grep of .release( across fluree-db-*/src finds no caller that ever touches a ContentKind::Txn blob.
Concrete consequence: the upload is spawned before staging (fluree-db-api/src/tx_builder.rs:640 and every maybe_spawn_txn_upload call at :1218-1295), so every distinct body that fails SHACL/policy/parse, hits NoveltyAtMax, or exhausts the retry loop now leaves a blob forever — and solo opts every transaction into store_raw_txn (fluree-lambda-transact/src/handler.rs:1314), so on our own product that is every failed write. Growth is proportional to failed-transaction volume with no bound and no operator lever. To be clear, I agree the old inline deletes had to go — there is no safe inline delete of a content-addressed key without a refcount, and a cross-process SQS redelivery can't be refcounted in-process — so the direction is right; it's the second half of the rule that's missing.
Fix, minimally: change these two comments and the body to say plainly that no reclaim exists yet (a future maintainer reading "reclaimed by reachability GC" will assume it's handled). The real completion is a commit-chain-rooted sweep of the txn/ prefix — plan_sweep already has the enumerate-minus-live shape (sweep.rs:85-120) and verify_commit_chain from this PR is the root-set walk the sweep module said it lacked. Whether that sweep lands in this PR or right behind it is a genuine cost/scheduling decision (it's an O(commits) read over the chain, and someone has to decide when it runs), which is the one case where I'd say a follow-up is legitimate — but the comments can't ship claiming it's already there.
| )) | ||
| })?; | ||
| } | ||
| Err(e) => { |
There was a problem hiding this comment.
🟡 Medium / blocking on the body's own stated rule.
🟡 clone now tolerates every fetch failure, not just not-found — including an integrity failure from the origin — which contradicts "only NotFound is tolerated; other storage errors still propagate."
MultiOriginFetcher::fetch (fluree-db-nameservice-sync/src/origin.rs:300-345) folds a 404 together with 401/403, 5xx, and connection errors into a single SyncError::FetchFailed, and separately returns SyncError::IntegrityFailed when an origin serves bytes that don't hash to the requested CID (:318-321). The new Err(e) => arm here catches all of them, prints a warning, and continues. Every other tolerant site in this PR (export_commit_range, pack, merge, CLI push/publish) matches fluree_db_core::Error::NotFound(_) specifically — clone is the one that doesn't.
Failing scenario: a transient network blip (or an expired token) mid-clone now yields a "successful" clone with a provenance gap and one warning line scrolled past, where before it failed loudly and the user re-ran; worse, a corrupt or tampered txn blob on the origin becomes "continuing without it" instead of an integrity error.
Fix: give nameservice-sync a typed not-found — e.g. SyncError::NotFound(cid) surfaced from fetch_object's 404 arm (origin.rs:118-134) and propagated by fetch only when every origin returned it — and match on that here; leave IntegrityFailed and the rest fatal as they were. --no-txns already exists as the explicit "I know, skip them" opt-out.
| txn_cid = %addr, | ||
| "pushed commit references a txn blob the client did not provide; accepting commit without it" | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
🟡 Medium, optional.
🟡 Push now accepts an undeclared gap, so a client that simply forgot to attach blobs silently creates dangling refs on the receiver where it used to get a 400.
This is more of a question than a suggestion. validate_required_blobs is now a function that always returns Ok and logs, so the receiver can no longer tell "the source honestly has a provenance gap" from "this pusher has a bug" — and the export side of this same PR does declare its gaps (missing_blobs), so the two halves of the protocol are asymmetric. I wonder if we want a mirror-image missing_blobs: Vec<String> (#[serde(default)], additive on the wire) on PushCommitsRequest (:193-201), with the receiver tolerating only declared CIDs; the CLI push already knows exactly which CIDs it couldn't read (sync.rs:776-782) and could populate it. That keeps the tightened invariant for buggy or third-party pushers at no cost to the intended case. It's minor and non-blocking — but if you agree it's right, I'd rather see it folded in now than lost in the backlog.
| if report.is_healthy() { | ||
| Ok(()) | ||
| } else { | ||
| Err(CliError::Config(format!( |
There was a problem hiding this comment.
🔵 Low, optional.
🔵 verify's exit status can't distinguish "provenance gap, still clonable" from "chain broken" from "couldn't run", which undercuts the automation-gate pitch in docs/cli/verify.md:34-35.
Everything funnels through CliError::Config → EXIT_ERROR = 1 (error.rs:7, 150-151) — the same code a nameservice lookup failure or a bad --limit would produce. MissingTxnBlob is, per this very PR, tolerable by clone/pack/push/merge; MissingCommit/UnreadableCommit are not. Automation gating a clone on fluree verify && fluree clone would refuse exactly the clone this PR made work. CliError::ExitCode(n) exists for typed exits; perhaps 0 / 1 (runtime error) / 3 (provenance-only problems) / 4 (chain problems), documented in the table. Minor — but if you agree, I'd rather see it in this PR than tracked-and-forgotten.
| } | ||
| } | ||
| for (idx, parent) in env.parents.iter().enumerate() { | ||
| frontier.push((parent.clone(), Some((cid.clone(), env.t, idx == 0)))); |
There was a problem hiding this comment.
🔵 Low, optional.
🔵 Parents are pushed primary-first and the frontier is a stack, so on a merge DAG the walk descends the merge-parent lineage to genesis before touching the primary lineage.
Two effects: --limit N (documented as "newest first" at docs/cli/verify.md:21) spends its budget on the wrong lineage whenever there's a merge near the head, and a shared ancestor first reached via a non-primary edge is marked visited, so the TGap check on its primary edge never runs. I don't think this is common in practice, and export_commit_range has the same DFS shape (pre-existing, commit_transfer.rs:1427-1429), but the fix is a one-liner:
| frontier.push((parent.clone(), Some((cid.clone(), env.t, idx == 0)))); | |
| for (idx, parent) in env.parents.iter().enumerate().rev() { |
| Err(fluree_db_core::Error::NotFound(_)) => { | ||
| let (referenced_by, referenced_by_t) = referenced_by | ||
| .map(|(id, t, _)| (id, t)) | ||
| .unwrap_or_else(|| (cid.clone(), -1)); |
There was a problem hiding this comment.
⚪ Nit. When the head itself is missing, referenced_by falls back to the missing CID and -1, so the CLI prints "missing commit X (parent of t=-1 X)". Probably an Option on referenced_by (or a dedicated MissingHead variant) reads better.
| // Cancel an upload still in flight. A blob that already landed stays | ||
| // in storage — see the module docs for why it must not be deleted. | ||
| if let Some(handle) = self.handle.take() { | ||
| handle.abort(); |
There was a problem hiding this comment.
praise. The cancel-only Drop is genuinely safe, and I traced it rather than trusting it: each attempt spawns an independent put with no shared in-flight state in any store impl; FileStorage::write_bytes_durable runs write_atomic inside spawn_blocking (fluree-db-core/src/storage/file.rs:645-660), so an abort can't leave a truncated file (blocking work runs to completion, rename is atomic); S3 PUT is atomic. So an aborted upload either landed whole or not at all, and can't disturb a concurrent attempt's put of the same CID — which closes the exact interleaving from the incident (the stale-snapshot continue at tx_builder.rs:1637 dropping an unfinished CommitOpts). Both regression tests go red under mutation (details in the verdict).
Review follow-ups on the raw-txn dangling-ref fix. - The "orphans are reclaimed by reachability GC" claim was false: nothing sweeps txn blobs, so every distinct body that fails staging leaks permanently. Say so plainly in `raw_txn_upload` and `commit.rs`, and record why a naive `txn/` prefix sweep is unsafe — the raft queued transactor writes live QueuedRequest envelopes under the same ContentKind::Txn prefix and releases them itself. - `clone` tolerated every fetch error, including an origin serving bytes that fail their hash and an expired token mid-clone, turning a transient fault into a silent provenance gap. Add `SyncError::NotFound`, raised from `fetch_object`'s 404 arm and returned by `MultiOriginFetcher::fetch` only when every eligible origin 404s (a skipped-for-auth origin is not evidence of absence). Clone matches that and nothing else. - `verify` pushed parents primary-first onto a stack, so a merge near the head sent the walk down the merge lineage to genesis first — `--limit` spent its budget on the wrong lineage and a shared ancestor reached by a merge edge never got its t-contiguity check. Walk parents in reverse. - A missing head printed "parent of t=-1 <itself>"; give it its own `MissingHead` problem. - `verify` collapsed every outcome onto exit 1, so `fluree verify && fluree clone` refused exactly the clone this change made work. Classify problems as provenance-only vs chain-broken (`VerifySeverity`) and exit 0/1/3/4. - `PushCommitsRequest` gains `missing_blobs`, mirroring the export side, so a sender declares a gap instead of leaving the receiver to infer it. Undeclared gaps stay accepted — a sender predating the field cannot declare one, and refusing them would re-break the incident ledger — but warn instead of logging at debug. - The commit path gained four INFO events per commit in a crate that had none; keep one (`commit head published`) and demote the intra-phase breadcrumbs to debug. Tests: push accepts declared and undeclared txn-blob gaps (red when the receiver rejects undeclared); all-404 vs 404-plus-401 vs 404-with-skipped origin classification; severity assertions on the existing gap test.
|
Thanks — all four substantive points confirmed and addressed in GC claim. You're right, and it was the only actually-wrong thing here. The comments and the PR body now say plainly that nothing reclaims txn blobs and that every distinct failed body leaks. I also wrote down a constraint your sketch and I both need to respect: the raft queued transactor writes live clone. Fixed properly: verify. push Also demoted the commit-path INFO events — Test gap you flagged: added push-side tolerance coverage (declared and undeclared, red when the receiver rejects undeclared). pack/merge tolerance still uncovered. CI: #1674 landed, GitHub retargeted this to |
2 participants
Problem
Raw-txn blobs (
store_raw_txn) are content-addressed — a byte-identical body maps to the same CID and storage key — butContentStore::releaseis an unconditional delete with no reference count. The commit path released the blob on every failure branch: thePendingRawTxnUploadDrop guard (a detached task),build_commiterrors (incl.EmptyTransaction), and publish failures. Any transaction whose body matched an already-committed one could therefore delete that commit's blob.Seen in production: two chunks in flight concurrently; the loser hit the in-process commit-conflict retry in
tx_builder, which re-spawns the same upload (same CID). Attempt 1's Drop-guard delete landed after attempt 2's no-op re-put, leaving commit2e9b2dcd…with a permanent danglingtxnpointer. The ledger's state was fine (flakes live in the commit), butfluree clone/ export hard-failed on the missing blob, and the ledger was effectively unreplicable. The deterministic variant — an SQS redelivery of an already-committed body → zero flakes → release — needs no race at all.Changes
Root cause — no inline deletes of raw-txn blobs (
fluree-db-transact)build_commiterrors (release_raw_txn_after_build_errandBuildState.txn_id_for_releaseare gone). Orphans are left for reachability GC.Readers tolerate a dangling
commit.txn— warn and continue instead of failing the chain:export_commit_range(newmissing_blobsonExportCommitsResponse),pack, push-sidevalidate_required_blobs(unprovided blob accepted; provided blobs still hash-verify),merge, and CLIpush/clone. OnlyNotFoundis tolerated; other storage errors still propagate.fluree verify [LEDGER] [--limit N] [--json]backed byFluree::verify_ledger: walks the commit DAG via a branch-aware store and reports missing commits/parents, undecodable commits,tgaps (primary-parent edges only), missing txn blobs, and a missing index root. Non-zero exit on problems.Commit-window logging (INFO):
head commit temporal resolved→raw txn upload finished→commit record built→commit blob stored→commit head published, so a commit-phase stall localizes from logs alone.Tests
duplicate_body_failure_keeps_first_commits_txn_blob— fails 3/3 against the old Drop guard, passes with the fix (multi-thread runtime so the upload completes during staging, as in production).verify_and_export_tolerate_missing_txn_blob— verify report names the commit/t/CID; export succeeds withmissing_blobspopulated.EmptyTransactionunit test inverted to assert the blob survives.Docs:
docs/cli/verify.md,docs/api/endpoints.md(missing_blobs, push tolerance), README/SUMMARY rows.