fix(consensus): extract fluree-raft-core and add a replicated kv fragment; fix two waiter-map defects - #1678
Conversation
…entity, ownership)
First step of splitting the generic raft substrate out of
fluree-db-consensus so the workflow and entity-resolution groups can
reuse it instead of forking transport/storage/admin code.
Moved verbatim into the new crate:
- storage.rs + storage/{fs,memory}.rs — the durable log/vote/snapshot
traits and backends. Already payload-agnostic (opaque Vec<u8>), so
the only edit is one `use` path.
- http.rs — hop-by-hop header classification.
- NodeId / ClusterNode — the raft+client address pair carried through
membership.
The crate has no fluree-db-* dependency and, deliberately, no openraft
dependency: storage payloads are opaque bytes and ClusterNode satisfies
openraft's blanket Node bound through its derives alone. openraft
arrives with the log/network/admin adapters in the next step.
Genericized:
- ownership.rs splits into `digest_parts` + `owner_for_digest` in core,
with the RefKey-shaped digest staying in consensus as a wrapper.
Rendezvous ownership is decided locally and independently on every
node, so a digest change lets two nodes claim the same branch — the
hash is a wire format and a rolling upgrade is what breaks. A new
integration test pins the live implementation against a frozen copy
of the pre-extraction algorithm over 3,606 key pairs plus the cases
a fold could plausibly get wrong (separator inside a field, empty
fields, multi-byte UTF-8), and both crates carry golden constants.
Added:
- GroupId, a validated single path/route component for the coming
multi-group hosting. Charset makes traversal and separators
unrepresentable rather than filtered; storage-layout names (log,
snapshots, vote, committed, last_purged) are reserved because the
nameservice group keeps its historical unprefixed root.
No behavior change: every moved file is a 0-line diff, consensus
re-exports storage/http/NodeId/ClusterNode at their old paths, and the
raft feature still gates openraft for monolithic users. Verified with
409 consensus+core tests, the 9-test 5-node HTTP cluster suite
(failover, liveness demotion, concurrent writes), raft_integration, and
peer-mode tests; workspace clippy and fmt clean.
…erence seed Two review findings from the extraction. GroupId accepted `con`, `nul`, `aux`, `prn`, `com0`-`com9`, and `lpt0`-`lpt9`, which Windows reserves as character devices. The filesystem backend supports Windows (fsync_dir has a documented non-Unix branch), so `storage_root()` could hand back a path that cannot be created. Rejected on every platform rather than under cfg(windows): a group id is operator-supplied config replicated through membership, so a mixed-OS cluster has to agree on what is valid — accepting `aux` on Linux and failing on Windows turns a config typo into a node that cannot start. The check is exact, not a prefix match; `console` and `com10` stay usable. The ownership stability test imported RENDEZVOUS_SEED from the implementation, so changing the production seed would have moved the reference with it — the comparison would keep passing while every existing cluster's ownership map silently shifted. The reference seed is now a literal, with a separate test asserting the published constant still equals it.
…apters
Second step of the fluree-raft-core extraction. The four openraft-facing
adapters move to the core crate, parameterized by the type config
instead of monomorphized against the nameservice's.
New: `FlureeRaftConfig`, the constrained openraft profile. It pins the
five associated types every Fluree group shares — NodeId=u64,
Node=ClusterNode, Entry=Entry<C>, SnapshotData=Cursor<Vec<u8>>, and
Responder=OneshotResponder<C> — so each adapter carries one bound
instead of five. Applications still write their own
`declare_raft_types!`; a blanket impl covers whatever it produces.
The Responder pin is not cosmetic: openraft gates its blocking
client-write API on that exact type, so without it `add_learner` and
`change_membership` do not exist on `Raft<C>` and the whole admin
surface fails to resolve. `declare_raft_types!` already defaults to it.
Moved and genericized:
- log_adapter — LogAdapter<C, S>. Log-id and vote converters stay
concrete since NodeId is pinned.
- network — RaftTransportConfig + client + relative RPC router.
- admin — RaftAdmin<C> + relative admin router.
- forward — now generic over a `LeaderView` source rather than
`Raft` directly. This is what the routing decision actually needs
(current leader + membership addresses), and it turns seven cases
that previously required a live 5-node cluster into unit tests:
election-in-progress, self-is-leader, leader-missing-from-
membership, and the SSRF guard's loopback and link-local arms.
NetworkConfig splits along the same line it was already fused across.
The generic transport keeps timeouts and the body caps for openraft's
three routes plus leader forwarding; cross_node_propose_timeout and the
apply_staged_commit / apply_queue_poison caps stay in consensus, since
they configure RPCs a crate that knows nothing about staged commits has
no business describing. Consensus's NetworkConfig now embeds the
transport half; the ~6 call sites are updated rather than papered over
with a Deref. Its Default is hand-written because a derived one would
silently zero three real tuning values.
`build_client` becomes a free function: it never used the type config,
and one client is meant to be shared across every group in a process.
Routers stay relative — no prefix of their own — so the host nests them
at the legacy /raft and /cluster or under a group id. The two SSRF-guard
helpers become public because the nameservice's own cross-node forward
validates the same membership-sourced URLs, and a second drifting copy
of that check is the thing to avoid.
openraft is gated behind a new `raft` feature on the core crate, wired
from the consensus `raft` feature. Verified openraft is still absent
from the default build, so monolithic users — who reach this crate only
for http::is_hop_by_hop via peer mode — are unaffected.
423 core+consensus tests, the 14-test server raft suite including the
5-node HTTP cluster (failover, liveness demotion, concurrent writes),
workspace clippy and fmt all clean.
Splits what a replicated state machine owes the cluster into two traits. AppStateMachine is deterministic state reduction only: apply is a pure function of (state, command, log_index), with the usual rules — no clocks, no RNG, no IO, timestamps carried in commands rather than read inside apply. StateMachineObserver is everything else. The original sketch had a hook handed &State after each apply; that cannot work, because effects must run outside the state write lock so a slow subscriber can't stall apply, and once the guard drops there is no &State left to hand anyone without cloning the whole thing. So the observer runs in two phases: on_command and its siblings run while the state is borrowed and may only push owned values into a buffer, then publish gets the whole batch after the lock is released. Ordering between categories of effect is the observer's business — it has the entire batch and can make as many passes as it needs, which is what the nameservice's manager-then-bus-then-waiters sequence requires. Membership gets a first-class path. Those entries never travel through Command, so without apply_membership an application cannot maintain state derived from the voter set — which is what the nameservice's worker-eligibility demotions are. The view handed over is a plain MembershipView, not openraft's type. Snapshots are application-owned, with codec:: supplying a versioned postcard framing. A bare postcard(State) is a trap: the first field anyone adds makes every existing snapshot undecodable with no way to distinguish an old snapshot from a corrupt one. The framing carries magic + an explicit u16 so peek_version gives a real migration branch, and the toy state machine exercises a v1 -> v2 migration to prove it. apply takes the command by reference. That lets the adapter hand the same value to the observer without the clone the nameservice adapter does today, and costs nothing: apply clones only what it retains, which is never more than cloning the whole command would have been. Also adds a conformance fixture behind a `testing` feature. Two adapters will coexist for a while — this one and the nameservice's bespoke one — and running both through the same checks is what keeps them from quietly diverging. It covers what every adapter owes openraft: tracking last_applied, boot restore resuming after the snapshot, point-in-time snapshot builds, persist-before-swap on install, a refused install leaving published state untouched, membership surviving a restart, one response per entry including blank and membership, and path-safe snapshot ids. The counter state machine in tests/ proves the seam is implementable and pins the two properties the design turns on: publish observes a released lock (checked with try_read from inside publish, which fails if the write guard were still held), and a live install is distinguishable from a boot restore.
RaftGroup::bootstrap assembles one group — storage, log adapter, state machine, transport — and hands back the Raft handle, the advisory read model, an admin surface, and two relative routers. Generic only over the application; the observer and storage types live on the bootstrap method so callers write RaftGroup<MyApp>. Routers stay relative on purpose. A host nests them at bare /raft and /cluster for a group whose ClusterNode addresses already say so, or under /raft/<group_id> for a new one — which is what lets several groups share a process without the existing one's replicated membership going stale. default_raft_config and the election-timeout check move across from the server. The invariant is election_timeout_min > rpc_timeout: openraft's stock 150-300ms window is shorter than our 500ms RPC timeout, so a candidate re-elects before its own vote RPCs resolve, every vote lands on a stale term, and a failover livelocks with every survivor climbing to the same term and no leader. openraft's validate() cannot catch this because it does not know the transport timeout. A test pins that openraft's stock defaults *would* violate it, so nobody simplifies back to them. The leader watcher gains bounded graceful shutdown. Previously tasks were aborted outright, which drops their futures mid-await and skips cleanup. Now each leadership term gets a CancellationToken, and on leadership loss the token is cancelled, tasks get a shared grace deadline to wind down, and only stragglers are aborted — then all are awaited. That await is the point: without it a rapid leader flap starts a second generation of "leader-only" tasks while the first is still running. run_periodic generalizes the eviction scheduler's shape: sleep, tick, and exit promptly on cancellation rather than at the end of the next sleep. The liveness monitor deliberately stays in consensus — 1,034 lines tied to worker-eligibility semantics, with no second consumer yet. tests/multi_node_group.rs is the acceptance bar: three real HTTP nodes running the toy counter group with filesystem storage, formed by single-voter bootstrap growing to three via add-learner and change-membership. It touches no fluree-db-* crate, which is the point — it shows a second consumer can stand up a group without forking anything. It also covers what the forwarder's stub tests cannot: that a live Raft<C> reports leadership and resolves a leader's *client* address from replicated membership, the reason ClusterNode carries an address pair at all.
Three review findings. openraft drives network and storage futures on C::AsyncRuntime, which the profile left unpinned — but everything this crate hands openraft is Tokio-bound: reqwest for the transport, tokio::fs for the filesystem backend, tokio::spawn for leader tasks. A config naming a different runtime compiled fine and would have panicked on first IO. Pinned to TokioRuntime, which is what declare_raft_types! defaults to anyway. A different runtime is a real thing to want, but it needs its own transport and storage backends first; this pin is the right place to relax when that happens. connect_timeout lived on RaftTransportConfig, but it is baked into the reqwest::Client at construction and one client is meant to serve every group in a process. Two groups with different values silently got whichever one built the shared client. Moved it, with pool_idle_timeout, onto a new process-level HttpClientConfig that only build_client consumes — so the mismatch is now unrepresentable rather than documented. RaftTransportConfig keeps what genuinely is per-request: timeouts applied per call, and the route body caps. Docs were two PRs stale: the crate and Cargo comments still said openraft was absent and the adapters were forthcoming, and config.rs claimed four pins while listing five. Also fixed every broken intra-doc link — cargo doc is now warning-free — and removed nameservice concepts (AdvanceRef, /api/transact) that rode along into the generic crate's forward module docs. Dropped xxhash-rust from fluree-db-consensus; the rendezvous hash moved to fluree-raft-core in the first extraction commit and nothing in consensus has used it since.
…rdering test Six review findings. shared_state() handed out Arc<RwLock<State>>, so a consumer could mutate replicated state outside the log. A later snapshot clones that state under an unchanged last_applied and ships it to peers as though the log had produced it — replica divergence with no failed write to point at. Now returns a ReadOnlyState wrapper exposing only read/try_read. The with_state / open_with_state constructors are gone with it: they took a caller-supplied handle, which by definition was writable. A reader that needs the same state now takes it from shared_state() after construction, which is the same wiring in a different order. bootstrap_with_client accepted any reqwest::Client while RaftGroup.client is documented as the one to hand LeaderForwarder. Reqwest follows redirects by default, and the SSRF guard only validates the URL a request is *sent* to — so an honest-looking peer address could 302 a forwarded request at an instance-metadata endpoint. Added a RaftHttpClient newtype that only build_client can produce, and required it everywhere a membership-supplied URL is dialed: the forwarder, the network factory, and the nameservice's cross-node propose forward. The guarantee is now carried by the type rather than by a doc comment. install_persists_before_it_swaps only covered the success path, which a swap-first implementation also passes; the failure test rejected bad bytes before storage was reached. It now injects a snapshot-store write failure with an otherwise-valid snapshot and asserts state and last_applied are untouched — the only way to tell the two orderings apart. Verified by temporarily inverting the adapter's order: the test fails, and passes again when restored. The success path moved to its own install_is_durable check. The fixture hardcoded StateMachineAdapter despite documenting a "both adapters" purpose, so the nameservice's bespoke adapter could never have run it. ConformanceHarness::Adapter is now any RaftStateMachine, with the harness supplying open/probe/probe_snapshot. Corrected the profile pin count (six, not five) and dropped the blanket #![allow(dead_code)] from the test support module — it turned out not to be load-bearing at all; removing it produces no warnings. Also added the coverage gap noted separately: a leader task that ignores its cancellation token must be aborted after the grace period rather than hanging shutdown. The test pins both bounds — shutdown waits at least the grace period, and returns well inside a multiple of it.
Every node ran resolve_applied / resolve_aborted for every terminal apply, and resolve_with's vacant arm buffered an outcome when no waiter was registered. QueuedTransactor refuses submissions on followers, so a follower never registers — and buffered one outcome per transaction, forever. Only install_snapshot cleared them, which a healthy follower rarely does, so growth was effectively unbounded on every non-leader node plus the former leader after each stepdown. The buffering existed for a real race: the proposer cannot register until client_write returns its queue_id, by which time a fast worker may already have landed ApplyHead. But a buffer cannot distinguish a late proposer from an absent one, and on a follower the proposer is always absent. So interest is armed before proposing, keyed by the submission's request_cid — which the proposer knows and the command already carries. When a node applies that EnqueueCommand, the adapter binds the interest to the queue_id the state machine just assigned. Because binding happens during apply it strictly precedes any later ApplyHead for the same id, which closes the race structurally and leaves nothing to buffer: an unmatched resolve is simply dropped. A follower arms nothing, so it tracks nothing. The bind is ordered alongside the other waiter resolutions rather than done inline, because one apply batch can carry both the enqueue and its terminal command and the bind has to land first. WaiterTicket replaces the bare receiver and cleans up on Drop — an abandoned submission (timeout, cancellation, a panic on the propose path) can no longer leave an entry behind, bound or not. wait() borrows the receiver instead of consuming it, so a timed-out ticket stays valid: the retry re-proposes the same request_cid, rejoins the same entry as InFlight, and waits on the binding it already has. Two regression tests. The unit test drives 1,000 terminal applies with no local interest and asserts nothing accumulates. The cluster test asserts every node's map is empty after 25 completed submissions across 5 nodes; reintroducing the buffering makes it fail with "node 2 tracks 25 waiters after 25 completed submissions". install_snapshot now abandons armed interests as well as bound waiters — the snapshot replaced the state wholesale, so a pending enqueue is no more trustworthy than a pending head advance. An admin queue-clear still leaves unbound interests alone: their enqueue has not applied, so it lands against post-clear state and resolves on its own terms.
…ules The component map and submission-flow walkthrough still described the waiter as a oneshot registry keyed by queue_id, registered after `client_write` returns. It is now armed by request_cid before proposing and bound during apply, which is what keeps followers from tracking anything. Also adds the modules from the state-machine and runtime work to the crate map — `state_machine`, `runtime`, and the `testing` conformance fixture — and records the sixth profile pin (`AsyncRuntime`).
…rmance fixture The extraction leaves two openraft state-machine adapters in the tree. The generic one is held to the adapter contract by `fluree_raft_core::testing::run_all`; the bespoke nameservice one was held to it only by inspection. Implements `ConformanceHarness` for it, behind a new test-only `testing` feature, with a snapshot store whose writes can be made to fail — without that the persist-before-swap check cannot distinguish the two orderings and silently skips. Verified each check can fail against this adapter by mutating it: swapping state before persisting, dropping the snapshot restore from `open`, returning no response for a membership entry, and rebuilding the snapshot from live state rather than the builder's instant. All four were caught. A companion test pins the fixture's precondition that distinct commands reach distinct probes, so a later edit to the harness cannot weaken every check at once.
A state fragment plus a pure reduction an application embeds in its own state machine — not a service and not its own group. A lease fences the work it guards only if both are ordered by the same log, so putting it in a second group would just reintroduce two independent clocks. The load-bearing decisions: - An entry's version is the Raft log index of the write that created it, not a per-key counter. Per-key counters reset on delete or expiry, and a fencing token that can repeat is not a fence. - Expiry is logical absence to every observer, so whether the sweep has run is invisible and takeover never depends on ticker timing. - Every CAS failure returns the current record, which saves each consumer a racing follow-up read and is the recovery path for a lost response: a holder recognizes its own committed renewal in what comes back. - TTLs are rejected, never clamped. Silent truncation turns "why did my entry vanish" into an incident, and clamping ttl_ms would not bound now_ms + ttl anyway. - Eviction ranges a secondary expiry index rather than scanning, is capped per apply by policy and again by a hard maximum, and reports whether work remains. - Size limits are enforced by the same function client-side and at apply time, so a proposer that skipped the pre-check cannot diverge the cluster. Gated behind a `kv` feature independent of `raft`: it is pure state and pure logic, so a consumer can depend on the semantics without linking openraft. Verified the tests can fail: reordering two KvCommand variants is caught by the wire-format pin, leaking an expiry-index entry is caught by the invariant check every mutation runs through, and making the expiry boundary exclusive is caught by four tests including both named regressions.
… cluster Two gaps between "the kv algebra is right" and "kv is usable". Nothing drove eviction. `Evict` is bounded on purpose, so something has to notice `more_expired` and come back, and the retry protocol has two details that only show up later as a backlog that never drains: re-propose immediately with the *same* cutoff — a fresh clock read per round lets a steadily-expiring fragment outrun the sweep — and propose nothing when nothing has expired, since an idle ticker that still writes grows the log on every node forever. Both now live in `kv::sweep` rather than in each consumer. `sweep_once` is separated from the ticker so the protocol is testable at chosen instants, and bounds one tick's rounds so a backlog cannot monopolize the leader. And nothing proved the fence survives replication. Every kv test was a single-process reduction of a command vector, which pins the algebra but not the property kv exists for: a fence is only a fence if the refusal comes from the replicated log. `tests/kv_group.rs` runs three real HTTP nodes whose state machine embeds tenanted fragments — the composition the module documents but could not demonstrate — and drives acquire, renew, lapse, and takeover through `client_write`, with followers answering from their own replicated copy. It also pins the `Tenant` discriminants, which are a snapshot wire format. Test support is now split per concern and included per binary with `#[path]`, so a test binary is never compiled with code it does not reference. That removes six dead-code warnings without suppressing anything, and `multi_node_group` picks its follower after waiting for an election instead of assuming one had happened. Verified the new claims can fail: sourcing the version from anything but the log index breaks both fence tests, dropping the renewal's version precondition breaks the takeover test, advancing the cutoff mid-drain breaks the same-cutoff test, and removing the idle pre-check breaks the no-write-when-idle test.
The PR-2 split moved per-request settings to RaftTransportConfig and client settings to HttpClientConfig, leaving both docs attributing all three timeouts to NetworkConfig. The operations table also called the per-route body limits hard limits when they are configurable defaults, and omitted pool_idle_timeout and the follower forward cap entirely.
…napshots Five defects from review, two of them pre-deployment. Reclamation destroys information: once the record expiring at 100 is gone, nothing distinguishes "expired" from "never existed". Because `Evict` is bounded, partial sweeps are the normal case, so a later command carrying a rolled-back `now_ms` saw the reclaimed records as absent and the survivors — same expiry, same batch — as live. Which records a caller could see depended on how far the last sweep got. `KvFragment` now carries a logical-time floor that every reclamation raises and every decision reads through, so time within a fragment cannot run backwards: a record already treated as expired can never be treated as live again, and a lapsed lease cannot be renewed back into existence by a slow clock. It moves only on reclamation, so one far-future clock cannot mass-expire a fragment with nothing to reclaim. What it does not do — make a rolled-back clock harmless in wall-clock terms — is documented, since no state machine can without a trusted clock. openraft batches up to 300 entries per append-entries RPC and 0.9 does not shrink a batch the peer refuses. Against kv's default 1 MiB value limit that is ~300 MiB into a 64 MiB body cap: a follower far enough behind gets a 413, the transport reports a network error, and openraft retries the same oversized batch forever. The failure is silent, total, and only reachable from a node already degraded. `RaftGroupConfig` now takes an optional `max_command_bytes`, and bootstrap caps `max_payload_entries` so a full batch fits. Also: a fragment's expiry index and byte total are no longer serialized — a snapshot carries records and the floor, and both are rebuilt on decode, so a corrupt or wrongly-migrated snapshot cannot arrive able to underflow the byte total or arm a stale index entry that deletes a live record. Sweep configs with a zero interval or zero round budget are raised to a usable floor rather than spinning or silently sweeping nothing. And the key size limit is enforced for `Delete` and `TakeOnce`, not only `Put`. Each fix is mutation-verified: reverting any one of the five fails at least one test naming the property it protects.
The rationale for fluree-raft-core lived only in module docs and an untracked working file, so nothing shipped with the merge explained why the fence is a log index, why ownership is a wire format, or what the six type-config pins buy. `docs/design/raft-core.md` carries it, aimed at someone about to stand up a second group. Also corrects two things the review fixes made stale. The crate-map said expiry makes sweep timing invisible full stop; that holds only with the logical-time floor, since `Evict` is bounded and a partial sweep would otherwise split the expired set. And the operations guide called the per-route body limits hard-coded and described only the oversized-single- request cause of a 413 — the more insidious cause is a catch-up batch that cannot fit, where openraft retries the same oversized payload forever and one follower silently never rejoins. Indexed in SUMMARY.md and design/README.md. Verified every identifier the page names exists in the crate, that its numeric claims match the code, and that its cross-document anchors resolve.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
This is carefully built, @bplatz. Approving with a few small notes.
The one thing I'd encourage definitely folding in before merge is in waiter.rs:157: WaiterTicket::drop removes by key with no identity check, so a displaced ticket's drop deletes the live occupant's binding for the same queue_id (probe-verified; the existing displacement test only passes because it keeps first alive). I could not reach it through the shipped server — CachingCommitter's in-flight claim blocks the trigger — so it's a footgun in the primitive rather than a live bug, but the primitive is what #1679 builds on and the fix is a remove_if on ticket identity.
Two smaller notes: the server still carries its own copy of default_raft_config and the election-timeout check that runtime.rs now owns, and the title/body under-sell that this closes a real follower WaiterMap leak (release notes will read it as a pure refactor).
Adherence to repo commitments:
- Patterns/abstractions: ✔ Extends the existing raft plumbing into a reusable crate behind a blanket-implemented type-config bound; re-exports every historical path; no parallel construct.
⚠️ minor: the server keeps a duplicatedefault_raft_config/ election-timeout check. - Performance (speed first, memory second): ✔ No hot path touched. Nameservice apply path is unchanged except one
DashMap::removeper applied enqueue that replaces an unbounded per-follower insert;LeaderView's boxed future sits under an HTTP round-trip. No performance-degradation risk. - Testing: ✔ 176/176 (
fluree-raft-core) and 320/320 (fluree-db-consensus) under--all-features; 9/9 in the server'sraft_multi_nodesuite; all 83 moved tests came along (now 93 + 9 residual); mutations on the ownership seed (7 red) and waiter buffering (2 red) both bite.⚠️ binding_a_second_interest…should alsodrop(first)to pin the Drop-eviction fix. - Conventions: ✔ Multi-line, mechanism-first commit bodies throughout; clippy
-D warningsclean against the workspace lints;[lints] workspace = true,version.workspace,license.workspaceon the new crate; docs (raft-core.md, crate map, ops guide) updated and spot-checked against the defaults.⚠️ Title reads as a pure refactor for a PR that ships a leak fix.
Verified locally at branch HEAD fa4e130: cargo fmt --all -- --check clean; cargo clippy -p fluree-raft-core --all-features --all-targets -- -D warnings and -p fluree-db-consensus clean; cargo nextest run -p fluree-raft-core --all-features 176 pass; -p fluree-db-consensus --all-features 320 pass; cargo check -p fluree-db-server --all-features --all-targets clean; cargo nextest run -p fluree-db-server --all-features --test raft_multi_node 9 pass.
Approving so you can merge when ready, but I'd really like the waiter.rs remove_if in first — it's a one-liner and it's the seam #1679 lands on.
| /// would time out on the still-empty receiver — silently re-proposing | ||
| /// under a fresh `request_cid` and producing a duplicate commit on | ||
| /// retry. | ||
| impl Drop for WaiterTicket { |
There was a problem hiding this comment.
🟡 Medium (latent in the shipped server, but a real defect in the new primitive's contract).
A displaced WaiterTicket's Drop evicts the live occupant of the same queue_id. bind (waiter.rs:225-237) documents that a second interest binding to an occupied queue_id displaces the first and the first's receiver errors — but impl Drop for WaiterTicket at :157-166 then removes by key alone: self.map.waiters.remove(queue_id) with no check that the entry it is removing is its own. So the displaced ticket, on its way out, deletes the binding the current holder is waiting on.
Concrete scenario: two keyed same-body submissions for one request_cid alive on the same node; both bind to queue_id 5, the first's wait returns Displaced, the transactor's retry loop burns through attempts_allowed (each wait now returns Displaced immediately because receiver is None, and each retry is a real client_write), returns stranded_error, and drops the ticket → waiters.remove(5) → the second holder's outcome is dropped on the floor when ApplyHead(5) lands, and it times out on a commit that actually applied. I reproduced this by appending a test that mirrors binding_a_second_interest_to_one_queue_id_displaces_the_first (:514) with a drop(first) before resolve_applied(5, …): assertion failed: second's binding must survive first's drop (map.len() is 0, expected 1). That existing test only passes because it keeps first alive.
I want to be fair about reach: in the server as shipped I could not construct the trigger. CachingCommitter always wraps QueuedTransactor (fluree-db-server/src/lib.rs:848-855) and refuses a concurrent same-key submission with AlreadyInFlight; anonymous submissions never get InFlight (the queue scan at state_machine.rs:2255 is inside if let Some(key)); and a ticket's own retry re-proposes the same request_cid, so bind is a no-op rather than a displacement. So today this is a footgun rather than a live bug. But the WaiterMap is exactly the primitive #1679 and every future consumer will build on, and its own doc comment describes the displacement path as supported behavior.
Fix is small: give Waiter (and Interest) an identity the ticket can match on and remove conditionally. The bound: Arc<OnceLock<u64>> is already shared between Interest and the ticket, so the cheapest version is to carry it into Waiter too and use DashMap::remove_if:
impl Drop for WaiterTicket {
fn drop(&mut self) {
match self.bound.get() {
Some(queue_id) => {
self.map
.waiters
.remove_if(queue_id, |_, w| Arc::ptr_eq(&w.bound, &self.bound));
}
None => {
self.map
.interests
.remove_if(&self.request_cid, |_, i| Arc::ptr_eq(&i.bound, &self.bound));
}
}
}
}…and add drop(first) to binding_a_second_interest_to_one_queue_id_displaces_the_first before the resolve_applied(5, …) so the property is pinned. Related, and worth a line while you are in there: after Displaced, WaiterTicket::wait returns Err(Displaced) instantly forever (receiver is None), so the retry loop in queued_transactor.rs:207-216 spends its remaining attempts on proposals that cannot be waited on — it may be right to treat Displaced as terminal rather than retryable.
There was a problem hiding this comment.
Addressed in 3982a2a — remove_if on Arc::ptr_eq, exactly as you laid it out, plus the drop(first) assertion in binding_a_second_interest_to_one_queue_id_displaces_the_first.
Thank you for being precise about reach rather than just filing it as a bug — the CachingCommitter / anonymous-submission / same-request_cid-is-a-no-op analysis saved me re-deriving whether this needed a hotfix. It didn't, and it's now described that way in the body.
The fix went wider than the finding. The same defect is on the interests map: re-arming a request_cid before either ticket binds displaces the first, and the first's drop was removing the second's armed interest — so nothing is left to bind when the enqueue applies and the terminal apply has nowhere to go. Your snippet already covered it (you wrote both arms), but the probe was only on the queue_id side, so I added dropping_a_displaced_interest_leaves_the_live_one_armed to pin it. Removing by key alone fails both tests.
I audited the rest of the map before calling it done: Waiter is constructed in exactly one place (inside bind, carrying interest.bound), so no path can produce an entry whose Arc doesn't match its ticket's; and the other three removals — bind's interests.remove, resolve_with's, and drain_all_with's — are map-level operations that should take whatever occupies the slot, so they stay unconditional. The only case remove_if now declines is a slot owned by a live ticket that will remove it itself, so nothing is stranded; a_follower_accumulates_nothing still passes.
On the related Displaced point: filed as #1688 rather than folded in, because I think it's a closer call than it looks. The retry re-proposes the same request_cid and SmResponse::IdempotencyHit is a separate arm that returns successfully — so after a displacement the retries aren't futile, they're a poll for the other holder's completion, and one can legitimately succeed. Making Displaced terminal loses that. What's actually wrong is narrower: the loop's only pacing is ticket.wait(), which returns instantly once displaced, so the budget fires back-to-back. Re-arming isn't a third option — bind only fires when this node applies the enqueue, and a retry returning InFlight applies nothing. The issue lays out the three options; happy to take your read on it there.
| @@ -409,7 +413,7 @@ impl RaftBootstrapConfig { | |||
| /// at runtime so an override of *either* config that breaks it is | |||
| /// caught even when this constructor isn't used. | |||
| fn default_raft_config(network: &NetworkConfig) -> RaftConfig { | |||
There was a problem hiding this comment.
optional. This is more of a question than a suggestion. Commit b81683c says default_raft_config and the election-timeout check "move across from the server", and they did land in fluree-raft-core/src/runtime.rs:116 and :183 — but the server kept its own default_raft_config(network: &NetworkConfig) here and its own inline copy of the election_timeout_min <= rpc_timeout_ms warn at :254-262, now reading network.transport.rpc_timeout. So the livelock invariant is spelled out in two places, and the next person to retune one will not know about the other. I think the server versions can just become fluree_raft_core::runtime::default_raft_config(&network.transport) and check_election_timeout(&raft_cfg, &config.network_config.transport) with no behavior change (same 2× rpc, 750 ms floor, 2× spread). Minor and non-blocking — but if you agree it's right, I'd rather see it folded in now than lost in the backlog.
There was a problem hiding this comment.
Addressed in 11b9b3a — you were right, and "moved across" was the wrong word in that commit body; they were copied.
Both server copies are gone. fluree_db_consensus::raft now re-exports runtime alongside storage, admin, forward and log_adapter, so the server reaches it the same way it reaches the rest of the moved surface: runtime::default_raft_config(&network_config.transport) and runtime::check_election_timeout(&raft_cfg, &config.network_config.transport). Net −52/+9.
I diffed the arithmetic and the warn text before deleting — identical, as you expected, so no behavior change. The core's doc comment already carries the full rationale including the openraft-stock-150–300 ms narrative, so nothing was lost with the server's copy. I kept one short comment at the call site for the only thing the core docs can't say: that this is the place holding both configs, so an override of either is caught even when the constructor wasn't used to build it.
Server raft_multi_node 9/9 and consensus + raft-core 497/497 after the change.
| @@ -0,0 +1,2196 @@ | |||
| //! A replicated key/value fragment an application embeds in its own | |||
There was a problem hiding this comment.
comment, for Andrew to rule on rather than a finding. kv and kv::sweep (~2.7k lines) are new functionality with no consumer in the workspace — the only state machine that embeds a KvFragment is tests/support/leases.rs. The design is careful (log-index versions so a fence never repeats, the monotone time floor, denormalizations rebuilt on decode, append-only postcard enums pinned by golden bytes) and I have no correctness finding against it; it also has its own feature flag, so it costs nothing to non-users. It would review more cleanly as its own PR, but bplatz has standing to bundle it, and the commit bodies name the intended consumers (workflow and entity-resolution groups). Flagging the scoping question, not blocking on it.
There was a problem hiding this comment.
Ruling: it stays. There is already a consumer upstream using kv in anger, which is the thing that would have made it a separate PR if it hadn't existed — the code isn't speculative, it has a caller today that this crate is being extracted to serve.
Appreciate you flagging the scoping question rather than either blocking on it or letting it pass silently, and for reviewing it on the merits anyway.
| //! Buffered/async impls that violate this break Raft's safety. | ||
|
|
||
| use crate::raft::NodeId; | ||
| use crate::node::NodeId; |
There was a problem hiding this comment.
fluree-raft-core/src/storage/fs.rs:1 — observation. There is no test that opens a log/snapshot directory written by the pre-PR binary through the new crate. I am comfortable that compatibility holds — storage/fs.rs, storage/memory.rs, and http.rs are 100%-similarity renames, the entry payload is still postcard::to_allocvec(entry) of an unchanged Entry<TypeConfig>, and the ownership digest is pinned against a frozen copy — so this is cheap insurance rather than a gap. If a small golden on-disk fixture is easy to drop into fluree-raft-core/tests/, it is the kind of thing that catches the next refactor.
Commenting here because fluree-raft-core/src/storage/fs.rs is not in this diff.
There was a problem hiding this comment.
Filed as #1689 rather than folded in — your framing that it's insurance against the next refactor rather than a gap in this one is right, and building the fixture properly is more work than it looks.
I carried your compatibility argument into the issue verbatim (100%-similarity renames, unchanged postcard::to_allocvec(entry) of Entry<TypeConfig>, ownership digest pinned against the frozen copy) so whoever picks it up knows what is already established and doesn't re-litigate it.
| /// implementation: changing the production seed would change most of | ||
| /// what this file expects, and the comparison tests would keep passing | ||
| /// while every existing cluster's ownership map silently moved. | ||
| const REFERENCE_SEED: u64 = 0x6661_6566_5246_4252; |
There was a problem hiding this comment.
the frozen reference with a literal seed is exactly right for a wire-format hash. I bumped RENDEZVOUS_SEED by one and 7 tests went red across both crates (published_seed_matches_the_frozen_reference, digest_and_owner_match_the_reference_implementation, owner_matches_the_reference_across_voter_set_shapes, digest_is_stable, owner_for_digest_is_stable, and consensus's two *_pre_extraction_* tests).
There was a problem hiding this comment.
Thanks for actually perturbing the seed rather than reading the test — seven reds across both crates is the answer I wanted and couldn't have claimed on my own.
| }; | ||
| use fluree_raft_core::testing::{run_all, ConformanceHarness}; | ||
|
|
||
| /// Memory storage whose snapshot writes can be made to fail. |
There was a problem hiding this comment.
the fault-injecting FaultySnapshotStore is what makes install_persists_before_it_swaps (testing.rs:193) a real test rather than one that passes for either ordering. And the fixture genuinely runs against the bespoke nameservice adapter, which is the thing that keeps two adapters honest until #1679 lands.
There was a problem hiding this comment.
Thanks — the persist-before-swap ordering was the one I was least sure a test could actually catch, so confirming FaultySnapshotStore makes it bite rather than pass either way is the check I wanted.
| /// Arming first is what closes the race: by the time this node | ||
| /// applies the enqueue there is already somewhere to bind, so no | ||
| /// terminal apply can arrive with nowhere to go. | ||
| pub fn arm(self: &Arc<Self>, request_cid: ContentId, ref_key: RefKey) -> WaiterTicket { |
There was a problem hiding this comment.
the arm-before-propose / bind-during-apply design is the correct structural close of the race, and reintroducing buffering in resolve_with makes a_follower_accumulates_nothing and dropping_a_ticket_releases_a_bound_waiter fail, so the leak fix is pinned. The 5-node server suite's empty-map assertion (raft_multi_node.rs:832-856) passes.
There was a problem hiding this comment.
Appreciated. Worth noting the seam moved slightly since you reviewed it: Waiter now also carries the bound Arc, so a ticket's Drop can tell its own entry from one that displaced it. resolve_with is unchanged, so the buffering mutation you ran still bites the same two tests.
Both `WaiterMap` maps are keyed by something a second submission can collide on — `request_cid` for interests, `queue_id` for waiters — and `bind` documents that collision as supported: a duplicate joining an in-flight entry displaces the earlier waiter, whose receiver errors so the caller retries under its idempotency key. `WaiterTicket::drop` removed by key alone. A displaced ticket still names the slot it briefly held, so its cleanup deleted the binding the *current* holder was waiting on: the terminal `ApplyHead` then found no waiter, and the live submission timed out on a commit that had actually applied. The `bound: Arc<OnceLock<u64>>` is already shared between a ticket and its interest, so it is an identity the ticket can match on. Carry it into `Waiter` too and remove conditionally on `Arc::ptr_eq`. Not reachable through the server as shipped — `CachingCommitter` refuses a concurrent same-key submission with `AlreadyInFlight`, anonymous submissions never get `InFlight`, and a ticket's own retry re-proposes the same `request_cid` so `bind` is a no-op rather than a displacement. This is a defect in the primitive's contract rather than a live bug, but the primitive is what the workflow and entity-resolution groups build on. Both maps are pinned: the displacement test now drops the displaced ticket before resolving, and a companion covers the interests side. Removing by key alone fails both.
…' into feature/raft-core-extraction
The extraction moved `default_raft_config` and the `election_timeout_min > rpc_timeout` check into `fluree-raft-core::runtime`, but the server kept its own copies rather than calling them. The livelock invariant — 2× `rpc_timeout`, 750 ms floor, 2× spread, and the warn when an override breaks it — was therefore spelled out in two places, and retuning one would not surface the other. Delete both copies and call the crate's. `fluree_db_consensus::raft` re-exports `runtime` alongside the other moved modules, so the server reaches it the same way it reaches `storage` and `admin`. No behavior change: the arithmetic and the warn are identical, and the core's docs carry the full rationale the server's comment did.
bplatz
changed the title
…ck-order-and-minio Conflict in `fluree-db-server/src/raft.rs`: this branch moves the assembly into `fluree-db-consensus::raft::integration` and leaves a re-export shim, while main gained a change inside the file — the server's duplicate `default_raft_config` and inline election-timeout check collapsed onto `fluree_raft_core::runtime` (#1678). Kept the shim, and re-applied the collapse in the file's new home. `integration.rs` was moved verbatim before that fix landed, so taking this side alone would have silently reintroduced the duplicated livelock invariant the move was meant to consolidate.
2 participants
Extracts the application-agnostic Raft substrate out of
fluree-db-consensusinto a newfluree-raft-corecrate, so other apps can use the raft substrate without forking and copying it.Stacked on #1677.
Why
fluree-db-consensus::raftmixes two things: generic Raft plumbing (durable log and snapshot storage, node identity, HTTP transport, membership admin, follower→leader forwarding, rendezvous ownership) and nameservice-specific behavior (the command queue, staged receipts, waiter resolution, ledger-cache watermarks). Only the second is about naming ledgers. A second consumer today would have to copy the first.What moved
fluree-raft-core— nofluree-db-*dependencies, by design:Vec<u8>payloadsNodeId,ClusterNode, and a validatedGroupIdnewtypeRefKey/raftfor one group or under aGroupIdfor severalFlureeRaftConfig, a blanket-implemented profile pinning the six openraft associated types that are genuinely fixed, leavingDandRto the applicationAppStateMachine/StateMachineObserverseamRaftGroup::bootstrap, the leader-task lifecycleEverything is re-exported at its historical path, so
fluree_db_consensus::raft::storage::…,::NodeId, and::ClusterNodekeep resolving.runtimeis re-exported the same way, and the server callsruntime::default_raft_config/runtime::check_election_timeoutrather than keeping its own copies, so the election-timeout livelock invariant has one home. No nameservice behavior, wire format, or route path changes.openraftsits behind a default-offraftfeature on the new crate, verified absent from the default dependency graph — monolithic builds, which reach this crate only forhttp::is_hop_by_hop, do not compile or link it.The state-machine seam
An adapter does two things, and mixing them is where the bugs are.
AppStateMachineis the pure reduction — no clocks, no RNG, no IO.StateMachineObservercaptures effects while the state write lock is held and publishes them after it drops, so a subscriber reading state back cannot re-enterapply, and never sees a half-applied batch.The application owns its snapshot format: the adapter stores whatever
encode_snapshotreturns, verbatim. The versionedcodechelper is offered, not imposed.The
kvfragmentA key/value fragment an application embeds in its own state machine — not a service, not its own group. A lease fences the work it guards only if both are ordered by the same log; a second group reintroduces the two-independent-clocks problem it exists to solve.
ttl_mswould not boundnow_ms + ttlanyway.BTreeMap<Tenant, KvFragment>keyed by an append-only enum, because postcard is positional: appending a struct field breaks every deployed snapshot, appending an enum variant does not. Discriminants are pinned by golden-bytes tests.kvis gated independently ofraft: it is pure state plus a pure reduction, so a consumer can hold the semantics without linking openraft.kv::sweep(needs both) is the leader-only eviction driver, and owns the two details that otherwise get reimplemented wrong per consumer — re-propose immediately with the same cutoff, and propose nothing when nothing has expired.Catch-up batch sizing
openraft batches up to
max_payload_entries(300 by default) per append-entries RPC and, in 0.9, does not shrink a batch the peer refuses. Againstkv's default 1 MiB value limit that is ~300 MiB into a 64 MiB body cap: a follower far enough behind gets a 413, the transport reports a network error, and openraft retries the same oversized batch forever. One node silently never rejoins while the cluster looks healthy.RaftGroupConfig::max_command_bytesis a declaration, and bootstrap caps the batch to fit rather than warning — the failure is silent, total, and only reachable from an already-degraded node. The nameservice group's commands are uniformly small and leave it unset.Bug fixes shipped here
Not a pure refactor — three defects are fixed alongside the extraction, two of them in the waiter path.
Follower
WaiterMapleak. A follower accumulated a waiter entry per applied queue command, forever. The root cause was buffering: it cannot distinguish a late proposer from an absent one. Waiters are now tracked by local interest — armed byrequest_cidbefore proposing, bound to aqueue_idwhen this node applies the enqueue. Because the bind happens during apply it strictly precedes any later terminal command, so the race closes structurally and the buffering is deleted rather than bounded. A follower arms nothing and tracks nothing;WaiterTicketcleans up on drop, so an abandoned submission leaves nothing behind.Displaced
WaiterTicketevicted the live holder. Both waiter maps are keyed by something a second submission can collide on —request_cidfor interests,queue_idfor bound waiters — andbinddocuments that collision as supported: a duplicate joining an in-flight entry displaces the earlier waiter, whose receiver errors so the caller retries under its idempotency key. ButWaiterTicket::dropremoved by key alone, so a displaced ticket deleted the binding the current holder was waiting on — the terminalApplyHeadthen found no waiter, and the live submission timed out on a commit that had actually applied. Thebound: Arc<OnceLock<u64>>is already shared between a ticket and its interest, so it serves as an identity; both maps now remove conditionally onArc::ptr_eq. Not reachable through the server as shipped —CachingCommitterrefuses a concurrent same-key submission withAlreadyInFlight, anonymous submissions never getInFlight, and a ticket's own retry re-proposes the samerequest_cidsobindis a no-op rather than a displacement — so this is a defect in the primitive's contract rather than a live bug, but the primitive is what the workflow and entity-resolution groups build on.NetworkConfigsplit. Per-request settings (timeouts, body caps) areRaftTransportConfigand may differ per group; client settings (connect and pool-idle timeouts) areHttpClientConfig, because they are baked into the sharedreqwest::Clientand cannot vary per request. Previously a co-hosted deployment would silently get whichever group's connect timeout happened to build the client. The no-redirects SSRF guarantee is now carried by theRaftHttpClientnewtype, so an injected client cannot quietly reinstate them.Verification
The acceptance bar for "a second consumer can do this without forking anything" is two integration tests that depend on
fluree-raft-corealone, nofluree-db-*crate:multi_node_group.rsstands up three real HTTP nodes of a counter group, andkv_group.rsstands up three nodes whose state machine embeds tenantedkvfragments and drives lease acquire / renew / lapse / takeover throughclient_write, with followers answering from their own replicated copy.Both adapters in the tree — the generic one and the nameservice's bespoke one — now run the same conformance fixture, which is what keeps them from quietly diverging. The persist-before-swap check needs a storage backend whose snapshot writes can be made to fail; without one it silently passes for either ordering.
The rendezvous algorithm is a wire format, so a frozen copy of the pre-extraction implementation is checked against the live one over thousands of key pairs — nodes that disagree can both claim the same key.
Where a test asserts a property, I confirmed it fails when the property is removed: sourcing a kv version from anything but the log index, dropping a renewal's version precondition, removing the time floor, trusting serialized denormalizations, advancing an eviction cutoff mid-drain, removing the idle pre-check, swapping snapshot state before persisting it, dropping the snapshot restore from
open, returning no response for a membership entry, and reintroducing waiter buffering.Not in this PR
Migrating the nameservice's own adapter onto the generic seam. It is feasible —
encode_snapshotoutput is stored verbatim, so the snapshot format is preserved, and the seam covers every effect — but it rewrites the path handling every production write, and running both adapters through one fixture already buys most of the anti-drift benefit. It gets its own PR.Docs
docs/design/raft-core.mdis new and indexed inSUMMARY.md. The crate map and the Raft operations guide are updated; the latter previously called the per-route body limits hard-coded and described only the oversized-single-request cause of a 413.