fix: let a no-op republish complete its raft queue entry - #1716
Conversation
The commit worker's no-change arm completes a zero-change transaction by republishing the branch's current head unchanged — and apply_head's monotonicity guard rejected exactly that (commit_t <= current_t), pushing the entry back. Re-staging a no-op can only reproduce the same unchanged head, so the submission looped re-stage -> reject until the waiter timed out: every zero-change write through the raft committer stranded with 'submission stranded by leader transition'. Equal t AND equal commit id is now recognized as the designed no-op completion: the entry is consumed, the idempotency record is cached, and the ref is left untouched (the advance stamp keeps pointing at the real advance). A same-t publish with a DIFFERENT commit id is still a stale writer and still refused.
…e relay A follower's client_write refuses with ForwardToLeader, which pins every proposer to whichever node leads — a load balancer spreading requests across a group's nodes has no way to land writes from the others. The network router (peer-trusted, beside the raft RPCs) gains /propose: the leader decodes a JSON command, client_writes it locally, and answers the JSON application response. forward::propose_via_leader is the caller half — local client_write when leading, relay to the membership-recorded leader raft_addr otherwise, one retry with a fresh lookup when leadership moves mid-relay. Same SSRF posture as the request-forwarding middleware: no-redirect client, is_valid_leader_url on the membership-supplied address. The relay wire is JSON in both directions, deliberately unlike the raft RPCs' postcard: commands are constrained to postcard-safe shapes by the log, but application RESPONSES never ride the log and may carry serde_json::Value state postcard cannot decode. Proven on the three-node harness: a follower's direct client_write refuses, its relayed propose applies, and all three replicas converge.
BackgroundIndexerWorker::new returns (worker, IndexerHandle) and the handle owns the worker's ShutdownTrigger. The raft leader watcher's task closure bound it to _handle, dropping it at the end of the closure — which fires the shutdown oneshot, so worker.run() exits on its first select, before its first log line. A raft cluster therefore runs with NO indexer at all: nothing ever indexes, every read walks the commit chain from genesis, and read latency degrades unboundedly as commits accumulate, with no error anywhere. Move the handle into the worker's task so they live and die together; the leader watcher's abort on leadership loss still releases both. Found (and fixed identically) in fluree-solo's embedded raft host, where the symptom was 3.4s-and-growing unindexed upsert scans that took debug-log gap analysis to trace back to a '_' binding.
A deployment that lived in the single-node file posture had no way into raft mode: the replicated nameservice never reads ns@v2, so the first raft boot came up believing no ledgers exist while all their data sat on disk — the posture was reachable only with a fresh store. RaftBootstrapConfig grows adopt_file_registry (a store root; default None). The node that initializes the cluster calls RaftIntegration::adopt_file_registry() once a leader is known, and every non-retracted ns@v2 record replays into the machine as ordinary proposals: ledger init, commit head, index head, and the config and status values verbatim. Tombstones are skipped — they carry no live state. Once-only via a marker under the raft storage root, written after a successful replay. Crash-safe by idempotence rather than atomicity: a crash before the marker re-runs the replay, and every step tolerates its own prior success (AlreadyExists on init, a head the machine already holds at least as new, read-then-CAS for the index ref). Genuine divergence — the machine holding DIFFERENT state, which the marker should have made impossible — refuses loudly rather than guessing which side is authoritative. Joining nodes never adopt; they receive the registry by replication. Embedders should guard their non-raft boot path after adoption — the file registry beside the store goes stale on every raft write (the fluree-solo host stamps the store and refuses a later file-posture boot; this server can grow the same guard when its config wires the option). Integration tests: full replay fidelity (heads, tombstone skipped), marker once-only, and convergence when re-run over a partially adopted machine.
The raft nameservice deliberately keeps serving retracted records so admin tooling can read the flag — but LedgerState::load and Fluree::ledger_exists consumed lookup() without checking it, so on a tombstoning backend a DROPPED ledger kept loading and serving queries: the cache eviction on LedgerRetracted landed, and the very next query re-loaded the ledger from its still-referenced heads. Backends that hide the record outright (file, DynamoDB, memory) never showed this, which is how it survived — the divergence upstream #1670 describes. Filter retracted at both consumers: the loader treats it as not-found (matching the documented peer semantics — 'retracted reads identically to not-found for the query path'), and ledger_exists answers false. Demonstrated live on fluree-solo's raft-mode standalone: drop reported 'dropped', eviction logged, and the ledger answered queries indefinitely; with this, the drop-then-query probe refuses.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
Approving @bplatz but recommending some changes, even if narrowly, and only against the file-registry adoption commit. The titular fix is correct, and I'd approve fa0a541b4 on its own without hesitation.
The notes I'd want to focus on are largely from ea1d6972e. The config and status replay arms at integration.rs:490 and :499 discard read errors from get_config/get_status, and the function then writes its own once-only marker — so an unreadable record silently loses that ledger's config with no error, no warning, an info log claiming success, and no second chance for the operator. It's the only place in that function that doesn't follow the "refuse loudly rather than guess" rule you set for it, and the two arms have no test coverage at all despite the test module's doc claiming they do. Both are small fixes and the design around them is good — the idempotent-not-atomic replay, tolerating prior success on every step, and the partial-retry test are all well judged.
The optional items below the blocking ones are genuinely optional; the propose-relay retry note is about an API with no production caller yet, and I'd not hold anything for it.
db adherence checklist
| Axis | Assessment |
|---|---|
| Performance (speed-first) | Positive. One ContentId equality added per ApplyHead — once per committed write, inside a function already doing a map lookup and a VecDeque::pop_front. Removes an unbounded re-stage → reject → refresh loop that burned a staging pass plus a raft round-trip per iteration until waiter timeout on every zero-change write. Separately, b8bb981d3 recovers a raft cluster from running entirely unindexed, which is a large latent read-latency win. No hot-path query code touched. |
| SPARQL ↔ JSON-LD IR parity | Not applicable — no query, parse, or lowering code in the diff. |
| Shared-abstraction adherence | Good. The fix stays inside the existing guard rather than adding a parallel path; drop_queue_if_empty, the idempotency insert, and the HeadApplied response all run unchanged. The relay reuses is_valid_leader_url and the no-redirect client posture from the existing forwarding middleware instead of inventing a second SSRF story. adopt_file_registry replays through the ordinary NameServiceLookup/RefPublisher/ConfigPublisher traits rather than reaching into the state machine. |
| Tests / CI | CI ran and is green on all six real jobs (fmt, clippy, test 14m22s, testsuite-sparql, bench-paths, plan); the eight "skipping" checks are release-publishing jobs. The test job is --workspace --all-features, so the non-default raft-gated tests genuinely execute. I re-ran the targeted suites and mutation-checked the new test: reverting the guard exception turns apply_head_completes_a_no_op_republish_of_the_current_head red in 0.026s (fails fast, doesn't hang). Gap: the config/status adoption arms are untested — see the second blocking comment. |
Packaging note. The title and body describe one of four changes. The other three are a new peer-trusted HTTP write endpoint with its own SSRF surface, an indexer lifetime fix, and a 260-line one-shot migration feature with a marker file. The indexer fix in particular deserves better billing than it's getting — a raft cluster running with no background indexer at all, every read walking the commit chain from genesis, is a bigger operational finding than the no-op strand this PR is named for. Splitting ea1d6972e out would also let the adoption feature get reviewed as the migration it is.
| } | ||
| } | ||
| } | ||
| if let Ok(Some(config)) = file_ns.get_config(ledger_id).await { |
There was a problem hiding this comment.
The config and status replay arms discard a read error, and then the marker gets written anyway — so a one-shot migration can drop a ledger's config permanently and tell nobody.
if let Ok(Some(config)) = file_ns.get_config(ledger_id).await has no Err arm. FileNameService::get_config returns Err on a split_ledger_id failure and on any read_json_from_address I/O or JSON-parse failure (fluree-db-nameservice/src/file.rs:1598-1605), so a truncated or unreadable ns@v2 record surfaces as Err and lands in the discarded arm. The loop then continues, the per-record tracing::info!("file registry record adopted into the replicated nameservice") still fires, adopted still increments, and finish(adopted) writes <raft storage root>/file-registry-adopted. From then on the adoption.marker.is_file() early return at the top of the function makes every later call Ok(0).
That is unrecoverable from the operator's side by design: adoption is deliberately once-only, and the only way back is deleting the marker by hand — which they have no signal to do, because nothing was returned, nothing was warned, and the info log says the record adopted fine.
This is also the one place in the function that departs from the principle the commit message sets out for itself: "Genuine divergence … refuses loudly rather than guessing which side is authoritative." The two head arms honour that with FileRegistryAdoptionError::Diverged. These two swallow.
Failing scenario: transition boot on a store where one ledger's ns@v2 JSON was truncated by an earlier unclean shutdown. all_records() still yields the record, so existence and both heads adopt correctly; get_config errors; the ledger arrives in raft with its config silently absent, the marker is written, and the operator's first symptom is a ledger behaving with default config in production.
The fix is small: give both arms explicit Err handling and route it through the error type the publisher-side arms immediately below already use — FileRegistryAdoptionError::replay(ledger_id, "config", e). If you'd rather not fail the whole adoption over one unreadable optional record, then at minimum warn! it and count it, and have finish() refuse to write the marker when the count is non-zero, so a re-run is still possible.
There was a problem hiding this comment.
Addressed in 403812c.
Your finding was the visible half. Writing the test you asked for in the next comment turned up a second fault in the same two arms: they pushed with expected: None, and apply_versioned_push (state_machine.rs:2125) reads an absent value as VersionedValue::absent() — ConfigValue::unborn / StatusValue::initial — then requires expected.as_ref() == Some(¤t). None can never match, so every push returned Conflict and the Ok(_) => {} swallowed that too. Neither config nor status was ever carried, on any path. The // A conflict is a retried replay's own prior write comment was wrong: it was the first attempt failing, every time.
Both arms are now read-then-CAS in the shape of the index-head arm above them — read failure aborts through Replay, a real conflict refuses through Diverged, and a watermark at or above the registry's absorbs a retried replay's own write.
| @@ -0,0 +1,198 @@ | |||
| //! The transition boot: a store that lived in the single-node FILE | |||
| //! posture moves into raft, and `adopt_file_registry` carries every | |||
| //! registry record — existence, both heads, config, status — into the | |||
There was a problem hiding this comment.
The test module's own doc claims config and status coverage that isn't there — and those are exactly the two arms with the swallowed error above.
Line 3 reads "carries every registry record — existence, both heads, config, status — into the replicated nameservice". seed_file_registry (lines 37-70) calls init, fast_forward_commit, and compare_and_set_ref only; it never calls push_config or push_status. Neither test reads config or status back. I checked: grep -n "config\|status" over the file returns only this doc line and the name no_registry_and_no_config_both_answer_zero, where "config" means the bootstrap config, not a ledger config.
So both replay arms are entirely unexercised. The heads path is well covered — replay fidelity, tombstone skipping, marker once-only, and convergence over a partially adopted machine are all pinned, and the partial-retry case in particular is a nice piece of test design. Config and status just aren't in it, while the doc says they are.
Seeding a config and a status in seed_file_registry and asserting both come back through integration.nameservice() would cover the arms and make the doc true. It would also give the fix above something to fail against.
There was a problem hiding this comment.
Addressed in 403812c. Mutation-checked both ways: restoring expected: None fails with Diverged { machine holds v: 0, registry carries v=4 }, and restoring the original arm including the swallow fails the readback at left: 0, right: 4.
| // `commit_t == current_t` with `commit_id` equal to the stored | ||
| // head. That is not a stale writer — it is the designed way an | ||
| // entry that advances nothing leaves the queue. Rejecting it | ||
| // (as this guard once did) pushed the entry back and stranded |
There was a problem hiding this comment.
fluree-db-consensus/src/raft/commit_worker.rs:752 — The process_revert docstring still asserts the mechanism this PR just disproved — it says ApplyHead against the same head "surfaces via QueueDesync::WrongFront only if another transactor jumped ahead, which is exactly the race the queue already serializes against." Both halves turned out to be wrong: it surfaced as HeadNotMonotonic, not WrongFront, and it fired unconditionally rather than only under a jumped-ahead transactor. That belief is arguably why this sat latent since June. The code is fixed now, but the comment sits directly above one of the four arms that depends on the new behaviour, so the next reader gets the old model. 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 fluree-db-consensus/src/raft/commit_worker.rs is not in this diff.
| }; | ||
| match relay_propose::<C>(&node.raft_addr, &cmd).await { | ||
| Ok(data) => return Ok(data), | ||
| Err(e @ ProposeError::Relay(_)) => { |
There was a problem hiding this comment.
The relay's retry arm can double-apply. relay_propose maps a resp.bytes() failure to ProposeError::Relay(...) at line 1024, and this arm retries the whole client_write on any Relay error. By the time bytes() fails the leader has already answered a status line, so it has already applied the command — the retry lands it twice. The 503 arm is genuinely safe (the leader refused with ForwardToLeader, nothing applied), and the send() failure is honestly ambiguous, but the response-read failure is unambiguously post-apply. Since there's no production caller yet — only multi_node_group.rs:289 — this is a design note rather than a live defect, and I'd normally leave it. I'm raising it because the commit message frames this as the answer for a load balancer spreading writes across a group, so it will get wired to something, and at that point "at least once" becomes a property callers need told about. Either narrow the retry to the 503 and send() cases, or say plainly in the propose_via_leader doc that delivery is at-least-once and commands should be idempotent.
There was a problem hiding this comment.
Not addressed — taking your read that this shouldn't hold the branch. The analysis is right: a bytes() failure is unambiguously post-apply and the retry double-applies. Since the choice between narrowing the retry and documenting at-least-once depends on what ends up calling it, I'd rather make it with the caller in hand than guess now.
| .map_err(FileRegistryAdoptionError::Marker)?; | ||
| Ok(adopted) | ||
| }; | ||
| if !registry.is_dir() { |
There was a problem hiding this comment.
if !registry.is_dir() { return finish(0); } writes the marker when the configured root has no ns@v2 subtree. A typo in the operator's adopt_file_registry path therefore burns the one-shot adoption for that raft storage root, silently. Answering Ok(0) without writing the marker in this specific case would leave the door open for the corrected config, and you'd lose nothing — the adoption.is_none() case above already returns Ok(0) without a marker.
| /// a value at least as new; a config/status conflict). | ||
| /// | ||
| /// Returns how many ledger records were carried. | ||
| pub async fn adopt_file_registry(&self) -> Result<usize, FileRegistryAdoptionError> { |
There was a problem hiding this comment.
Nothing in adopt_file_registry checks that this node is the one that initialized the cluster; the "joining nodes must NOT call this" contract lives only in the doc comment. Given RaftBootstrapConfig is the kind of thing that gets templated across every node in a deployment, a cheap self.raft.current_leader() check (or an explicit is_initializer flag threaded from bootstrap) would make the contract enforceable rather than advisory.
There was a problem hiding this comment.
Not addressed — leaving as-is for now. Agreed the contract should be enforceable rather than advisory, but current_leader() doesn't actually separate the initializer from a joining node that later won an election, so the honest version is the is_initializer flag threaded from bootstrap. That's a bootstrap-surface change I'd rather not fold into this branch.
The adoption's config and status arms never carried anything. Two
faults compounded:
- The read discarded its error (`if let Ok(Some(..))`), so a record
whose `ns@v2` JSON was truncated by an unclean shutdown lost its
config with no error, no warning, and an info log claiming the
record adopted fine — permanently, since the once-only marker was
still written afterwards.
- The push passed `expected: None`, which `apply_versioned_push` can
never match: an absent value reads as the `unborn` / `initial`
watermark, not as missing. Every push conflicted, and the conflict
was swallowed by `Ok(_) => {}`.
Both arms now read-then-CAS like the index-head arm above them: a read
failure aborts through `Replay`, a genuine conflict refuses through
`Diverged`, and a watermark already at or above the registry's absorbs
a retried replay's own prior write.
Also: a configured registry root with no `ns@v2` subtree answers
`Ok(0)` without writing the marker, so a mistyped path no longer burns
the one-shot adoption silently, matching the unconfigured case.
`process_revert`'s docstring described the pre-fix `ApplyHead`
behaviour (`WrongFront` only under a jumped-ahead transactor); it now
describes the no-op completion the four arms below it depend on.
Tests seed config and status in the file registry and assert both
watermarks and payloads arrive through the replicated nameservice,
covering arms the module doc already claimed. Mutation-checked: the
old `expected: None` push fails the readback at v=0 vs v=4.
|
Heads up @bplatz — this one turned
// A soft-dropped ledger still has a (retracted) record: `exists`
// is "is there a record", not "is it live".
assert!(
fluree.ledger_exists("x:main").await.unwrap(),
"exists() reports the record, which a soft drop keeps",
);
Your commit message reads like the test is simply now wrong ("exists is a query-path question"), in which case updating the assertion and its comment is the whole fix. I didn't want to push that myself since it's your semantics call and the old assertion was deliberate enough to carry an explanatory comment. Happy to push it if you'd rather — just say which way you want One thing worth flagging while you're in here: this settles a sub-question of #1670, which is still open and carries the wider delete-semantics decision (init-reclaims vs reject, whether an un-retract path exists, branch-scoped hard purge). Your commit body already cites the same divergence. Might be worth a line on #1670 recording that the exists half is now decided, so whoever picks up the rest isn't re-deriving it — and so the two don't drift apart, which is the failure mode that issue is about in the first place. |
2 participants
Problem
Every zero-change write routed through the raft committer hung until the waiter timed out with
submission stranded by leader transition.The commit worker's no-change arm completes a zero-change transaction by republishing the branch's current head unchanged — same
t, same commit id.apply_head's monotonicity guard (which exists to refuse stale writers) rejected exactly that shape (commit_t <= current_t) and pushed the queue entry back for retry. Re-staging a no-op can only reproduce the same unchanged head, so the submission looped re-stage → reject forever.Fix
Equal
tand equal commit id is now recognized as the designed no-op completion:last_advanced_*stamps keep pointing at the last real advanceA same-
tpublish with a different commit id is still a stale writer and is still refused, so the guard's safety property is unchanged.Testing
apply_head_completes_a_no_op_republish_of_the_current_headpins the completion behavior (entry consumed, ref untouched, stale same-t/different-id still rejected)cargo test -p fluree-db-consensus