refactor(consensus): reduce the nameservice through the generic state-machine seam - #1679
Conversation
Deletes the second openraft adapter. The nameservice's state machine was ~450 lines of last-applied bookkeeping, membership storage, and snapshot persistence that `fluree-raft-core` already owns generically, wrapped around the part that is actually about naming ledgers. That part now lives in `raft::app`, split the way the seam splits it: `NameServiceApp` is the pure reduction, `NameServiceObserver` is the effects — event bus, waiter resolution, staged receipts, content-store releases, ledger-cache watermark. `raft::state_machine_adapter` is their composition, kept at its historical path so imports resolve. Two orderings were structural in the old `apply` and are now explicit, because both are easy to lose and neither had a test. `publish` runs in two phases: every commit-head watermark reaches the ledger cache before any event reaches the bus. The watermark is a synchronous lossless write and the bus is bounded and lossy, so a cache lookup racing an apply must already compare against the new head. Doing it per-event would order each ledger against its own event but let one ledger's event overtake another's watermark. Within phase two, effects run in the order they were recorded, so a waiter bind lands before the terminal command that resolves it — a single apply batch can carry both, and a pass that handled terminals first would strand the proposer on work that had already succeeded. The snapshot format is unchanged: `encode_snapshot` returns bare postcard and the generic adapter stores it verbatim, so deployed clusters read and write the same bytes. `codec`'s versioned envelope is offered, not imposed, which is what makes that possible. Test scaffolding that wrote state directly now goes through the real paths — a demotion through `SetWorkerEligibility`, a seeded head through storage — since `apply` is the only writer. `ReadOnlyState::view_of` lets a test own state and hand out the read-only view consumers see; it only ever narrows. Verified both orderings fail when broken: binding after terminals strands the waiter, and dropping event-carried heads from phase one leaves the cache stale.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
This looks good and makes sense after #1678, @bplatz. The split lands exactly where the seam draws it, and the part I was most worried about (a delete-and-rewrite of the apply path for the cluster's source of truth) checks out: I walked the old apply / install_snapshot / restore_from_snapshot against the generic adapter plus NameServiceApp + NameServiceObserver hunk by hunk and every side effect is there in an order that's either identical or provably safe (the one real ordering change — effects interleaved per command in log order rather than batched per category — I couldn't construct a consumer that distinguishes, and it's the shape that makes binds-before-terminals a local property). Snapshot bytes and the postcard membership meta are byte-identical to what deployed nodes write, so the rolling-upgrade claim holds; the nameservice conformance harness hands the fixture a FaultyStorage, so persist-before-swap is actually exercised rather than skipped; and all 24 old adapter tests survive by name with 2 added.
The one thing you'd may want to tighten is a wording issue rather than a code one: the "watermarks before events" test pins that both head-carrying effect variants reach the cache, not the ordering itself (a mutation that reports the watermark after bus.notify stays green), so the body and doc framing over-claim slightly. Inline details below.
Two smaller optional notes inline as well (the whole-command clone the seam was designed to avoid, and a per-ref allocation at boot that goes nowhere); neither changes the verdict.
One process note, said once: this is stacked on #1678 (which is itself stacked), and ci.yml only runs on PRs targeting main, so the green checks here are the Release plan job and seven skipped jobs — nothing compiled or tested this branch in CI. I ran the fmt/clippy/nextest work locally at package scope (below); just be sure the stack gets retargeted or landed in order so a real CI pass happens before this reaches main.
Adherence to repo commitments:
- Patterns/abstractions: ✔ First real consumer of #1678's
AppStateMachine/StateMachineObserverseam; deletes the parallel adapter rather than keeping two;ReadOnlyStatenarrowing is the existing mechanism, not a new one. The only raft-core change is a narrowing constructor. - Performance (speed first, memory second): ✔ Apply path only, no query-engine crate touched. Per-command clone count unchanged from BASE; one
Vec<Effect>per batch replaces three; one O(refs) allocation at boot that's discarded. No performance-degradation risk. - Testing: ✔ 322/322 consensus tests pass with
--all-features(including the conformance fixture and the six single-node round trips); the binds-before-terminals test goes red under mutation; the watermark test goes red under the author's stated mutation but not under an ordering swap — see the inline note.fluree-db-servercompiles with--features raft --all-targets(the 9-test cluster suite builds; I didn't execute it since nothing in its delta changed). - Conventions: ✔ Multi-paragraph commit body that matches the diff; fmt clean; clippy clean under
--all-features --all-targets -D warningson both touched crates; three design docs updated and accurate against the code.
Verified locally at branch HEAD d6b1daef1: cargo fmt --all -- --check clean; cargo clippy -p fluree-db-consensus -p fluree-raft-core --all-features --all-targets -- -D warnings clean; cargo nextest run -p fluree-db-consensus --all-features → 322 passed / 0 failed; cargo check -p fluree-db-server --features raft --all-targets clean; three mutation checks on publish (per-event-after-notify → all green; drop LedgerCommitPublished arm → 2 red; terminals-before-binds → 1 red on the waiter timeout).
Approving so you can merge when ready (in stack order), but maybe worth folding the test-framing wording in first.
| /// head, or a cache keyed on the old value never revalidates. The | ||
| /// failure mode is a new event variant carrying a head that nobody | ||
| /// remembers to add here. | ||
| #[tokio::test] |
There was a problem hiding this comment.
optional — the "watermarks before events" test pins coverage, not ordering. I may be reading the intent too literally, but the body and the module docs (app.rs:23-33) present every_head_advancing_effect_reaches_the_watermark as the test for "every commit-head advance reaches the ledger cache before any event reaches the bus," and it isn't that: it calls publish once and reads the watermarks afterwards, so it is green regardless of which phase ran first.
Claude caught this and I confirmed it by mutation: I deleted phase one entirely and reported the watermark per-event after bus.notify (the exact ordering the module doc calls wrong) — all seven watermark/waiter tests stayed green, including this one. The mutation the body actually describes for it (dropping the LedgerCommitPublished arm from phase one) does go red, along with apply_reports_commit_head_advances_to_ledger_manager, so what the test really pins is "both head-carrying variants reach the cache" — which is worth having, and its own doc comment (app.rs:1448-1451) says exactly that.
I don't think a true ordering test is cheap here — the watermark is a memory write and the bus is a channel, so the order is only observable to a concurrent reader, and LedgerEventBus isn't something we can intercept — so I'd lean towards just making the claim match the test: keep the two-phase structure and the module doc as the thing that holds the ordering, and adjust the PR body / test framing so it says "coverage of head-carrying effects" rather than "ordering." The binds-before-terminals test, by contrast, is a real ordering test (my terminals-first mutation strands the waiter and it fails on the 5 s timeout). Minor and non-blocking — but if you agree it's right, I'd rather see the wording folded in now than have the next person trust a test for an invariant it doesn't hold.
| fn apply(state: &mut NameServiceState, command: &Command, log_index: u64) -> Response { | ||
| // `state_machine::apply` consumes the command — it moves fields | ||
| // out of the payloads rather than cloning them per arm. | ||
| state_machine::apply(state, command.clone(), log_index) |
There was a problem hiding this comment.
optional — the seam's no-clone benefit isn't realized here. This is more of a question than a suggestion. AppStateMachine::apply takes &Command specifically so the adapter can hand the same value to on_command "without cloning it" (fluree-raft-core/src/state_machine.rs:206-212), but NameServiceApp::apply clones the whole command because state_machine::apply still takes it by value (state_machine.rs:1365). Your code didn't introduce this — the old adapter cloned at the same spot (cmd.clone(), old :483) for the same reason — so it is perf-neutral and I'm not asking for it in this PR; changing the reducer to borrow is a wide edit across the arms that move fields out of the payload. I just wonder if it's worth a one-line comment at app.rs:80 saying the clone is there because the reducer consumes, so nobody later reads the seam doc and assumes the nameservice is clone-free. If you'd rather leave the comment as-is, that's fine too.
| ) { | ||
| // A snapshot advances heads in bulk without per-entry applies, | ||
| // so the per-apply watermark report never fires for them. | ||
| out.extend(state.refs.iter().map(|(key, entry)| Effect::HeadAdvance { |
There was a problem hiding this comment.
optional — boot restore now records a HeadAdvance per ref that goes nowhere. Not a big deal, but it's a small behavior delta from the old open, which did nothing at boot: on_snapshot_loaded(BootRestore) builds one (String, i64) per refs entry, and in production the ledger-manager cell is still empty when open runs (fluree-db-server/src/raft.rs:234-243 takes the cell before open; assembly fills it later), so phase one discards all of them. It's O(refs), once per process, and harmless — note_head_advance is monotonic-max so even a filled cell would be fine. If you want to keep boot allocation-free you could guard the sweep on self.ledger_manager.get().is_some() (same check publish makes), but I genuinely don't think it matters and would understand leaving it for symmetry with the live-install path.
| /// state is visible without going through openraft's RPC surface. | ||
| /// | ||
| /// Read-only by type: the only writer is `apply`. | ||
| pub type SharedState = ReadOnlyState<NameServiceApp>; |
There was a problem hiding this comment.
praise. SharedState = ReadOnlyState<NameServiceApp> is the right narrowing: at BASE the only writers through the old Arc<RwLock<_>> alias were the two test helpers (nameservice.rs:2000, :2969), and now the type makes the "test seeds state, snapshot ships it" hazard impossible rather than merely unlikely. The rewritten scaffolding — a real SetWorkerEligibility for the demotion, storage + open for a seeded head, view_of for owned test state — is genuinely better test design than the direct writes it replaces.
|
|
||
| async fn open(&self, storage: Arc<Self::Storage>) -> Self::Adapter { | ||
| StateMachineAdapter::open(storage) | ||
| StateMachineAdapter::open(storage, NameServiceObserver::new()) |
There was a problem hiding this comment.
fluree-db-consensus/tests/it_adapter_conformance.rs:102 — praise. The nameservice harness supplies a FaultyStorage, so install_persists_before_it_swaps actually runs against this composition instead of skipping the way the fixture allows. That's the check that matters most for a "the generic adapter now owns persist-before-swap" refactor, and it's live.
The watermarks-before-events ordering has no test. `publish` is synchronous, so once it returns both phases have run and the order is no longer observable — `every_head_advancing_effect_reaches_the_watermark` stays green with the watermark reported per-event after `bus.notify`. What it pins is coverage: every head-carrying effect variant updates the cache, so a new variant is not silently missed. Say so in the module docs and on the test, and name binds-before-terminals as the one that is a real ordering test. A refactor folding the phases together will not be caught by the suite, and the next person should know that before trusting it. Also note at the `command.clone()` that the seam's `&Command` exists so an app can skip this clone, and the nameservice does not — the reducer consumes — so the seam's contract is not read as a claim that this path is clone-free.
|
Both folded in — Test framing. You're right, and I've made the code say it rather than just fixing the body. The module docs now carry a "what the tests do and do not hold" section: binds-before-terminals is a real ordering test; watermarks-before-events is structural and untested, because Clone. There was already a line at Boot sweep — I'd rather leave it. The guard would put a second 322/322 consensus tests, fmt and clippy clean at On the stack: #1674 landed, GitHub retargeted #1677 to |
2 participants
Deletes the second openraft adapter. The nameservice now reduces through the generic seam
fluree-raft-coreprovides, like any other consumer.Stacked on #1678.
Why
The nameservice's adapter was ~450 lines, and most of it was not about naming ledgers: last-applied bookkeeping, membership storage, snapshot read/write, the persist-before-swap ordering.
fluree-raft-coreowns all of that generically. Keeping a second copy meant two implementations of the same contract drifting apart — a conformance fixture catches divergence, but only after it happens.What changed
raft::appholds the nameservice's actual contribution, split the way the seam splits it:NameServiceApp— the pure reduction. Routes aCommandthroughstate_machine::apply, mirrors membership into replicated state (preserving surviving demotions), owns the snapshot codec.NameServiceObserver— the effects. Event bus, waiter resolution, staged receipts, content-store releases, ledger-cache watermark.raft::state_machine_adapteris now just their composition with the generic adapter, kept at its historical path so imports resolve.SharedStateis a type alias forReadOnlyState<NameServiceApp>, so the ~60 read sites did not change.Two orderings that are easy to lose
Both were implicit in statement order inside the old
applyand both are easy to lose in a refactor. One of them now has a test; the other cannot cheaply have one, and is called out below.Watermarks before events.
publishruns in two phases: every commit-head advance reaches the ledger cache before any event reaches the bus. The watermark is a synchronous, lossless memory write; the bus is bounded and lossy. A cache lookup racing an apply has to already be comparing against the new head. Handling this per-event would order each ledger against its own event but let one ledger's event overtake another ledger's watermark — and a subscriber that reacts to the first by reading the second would see a stale head.This ordering is structural, not tested.
publishis synchronous, so by the time it returns both phases have run and the order they ran in is no longer observable — only a reader racing the call could tell, which no deterministic test can arrange against abroadcastbus we cannot intercept.every_head_advancing_effect_reaches_the_watermarkpins coverage (every head-carrying effect variant updates the cache, so a new variant is not silently missed) and stays green if the watermark is reported per-event afterbus.notify. The phase split is the only thing holding the order; the module docs now say so, and so does the test.Binds before terminals. Within phase two, effects run in the order they were recorded. A single apply batch can carry both an enqueue and the
ApplyHeadthat retires it, so the waiter bind has to land first; a pass that handled terminals first would resolve aqueue_idnothing is listening on, and the proposer would wait out its timeout on work that had already succeeded.Snapshot format is unchanged
encode_snapshotreturns bare postcard and the generic adapter stores whatever it returns verbatim —codec's magic-plus-version envelope is offered, not imposed. Deployed clusters read and write the same bytes, so this is not a rolling-upgrade break.Test scaffolding
Some tests wrote adapter state directly to set up a scenario. Since
applyis the only writer, those now go through the real paths: a mid-life demotion through theSetWorkerEligibilitycommand the liveness monitor actually proposes, and a seeded branch head through storage, so an adapter opened over it starts there.ReadOnlyState::view_oflets a test own state and hand out the read-only view consumers see — it only ever narrows, never grants write access to a live adapter's state.Verification
Binds-before-terminals is mutation-checked: binding after terminals strands the waiter until it times out. The watermark test is mutation-checked for what it claims — dropping the
LedgerCommitPublishedarm from phase one leaves the ledger cache stale — but not for ordering, which it does not hold. The nameservice still passes the shared conformance fixture, and the 9-test server cluster suite and 314 consensus tests pass unchanged.The conformance fixture stays generic over any openraft
RaftStateMachinerather than being narrowed to the seam's adapter — a future consumer that needs its own adapter should be held to the same nine properties.