Materialization audit, Elle cert hardening, formation election-stall fix, replayable Accord log - #308
Open
bkearns wants to merge 22 commits into
Open
Materialization audit, Elle cert hardening, formation election-stall fix, replayable Accord log#308bkearns wants to merge 22 commits into
bkearns wants to merge 22 commits into
Conversation
…t (t_2487eeb7) The audited-crate set was a hand-maintained list, which made OOM-guard coverage opt-in — and opt-in coverage rots. `ferrosa-graph` and `ferrosa-sparql` both grew query-sized serving paths, and shipped real materialization OOMs, while sitting entirely outside the gate. The audit reported "no findings" the whole time. Coverage is now exhaustive: every workspace member must be either audited or listed in NON_SERVING_CRATES with the reason it owns no query-sized path. A crate in neither set produces an `unclassified-crate` finding, so adding a crate fails the audit instead of silently widening the blind spot. The check runs in the binary rather than only in a unit test, and an unreadable workspace manifest is itself a finding — unknown coverage must never read as a clean run. Also teaches `returns-vec-partition-or-row` about `Vec<VirtualRow>`. `VirtualTable::read` returns a whole table while `visit_rows` streams, and the trait docs already tell large/live tables to override the visitor; this is what holds them to it. Audited crates: 6 -> 15. Findings: 0 -> 82, all pre-existing materialization the gate could not previously see. 21 of them are in crates it was ALREADY auditing, hidden purely by the missing VirtualRow shape. One is confirmed class A (t_250fa355: read_anchor_partitions range_read arm, which its own doc comment already called "the unbounded materialization"). The 82 are baselined in oom-audit-allow.toml marked NOT INDIVIDUALLY TRIAGED, with a short expiry (2026-10-01) so they fail the gate rather than becoming a permanent rubber stamp. The reasons deliberately do not claim these sites are safe. Triage is t_a49d88c3. Known gap: entries omitting `symbol` suppress a whole (file, rule) pair, so a new violation inside an already-allowlisted file is not caught (verified). Tracked as t_e1d5f83c. Verified: enforce exits 0 today and 1 after the baseline expiry; a new materializing fn in a newly-audited crate is caught (exit 1).
…findings The t_2487eeb7 baseline marked all 52 entries "NOT individually triaged". That was true when written; it is no longer true for ferrosa-graph and ferrosa-sparql, and an allowlist that understates what is known is as misleading as one that overstates it. 14 entries now carry the actual bound: schema-arity with_capacity, single-row clones, max_result_rows-bounded copies, the fail-loud max_message_size guard, and two genuine rule misfires (.cloned() over an Option; SET/REMOVE loops whose push targets a per-column vec, not the stream). 5 entries are re-marked CONFIRMED REAL rather than merely untriaged — the range_read call sites in expand.rs, leapfrog.rs, varpath.rs and reconcile.rs. Their expiry moves in to 2026-09-15 so known bugs surface before unknown ones. Root cause and fix plan: t_bc5f0e6f (WritePath::range_read is a materializing wrapper over an already-existing stream, so each is an in-place swap). Notable corrections to earlier belief: - sparql DEFAULT_MAX_ROWS is NOT a silent result cap. Crossing it raises SparqlError::Execution — a loud error, never a truncation. It prevents OOM rather than causing it, so the entry records that instead of implying a debt. - The Bolt codec with_capacity sites are bounded for query data, but the count is wire-declared and decoded pre-auth. Filed separately as t_3e665492 (allocation amplification), since it is a different failure mode than materializing query results. 33 entries in the remaining crates stay honestly marked untriaged (t_a49d88c3). Verified: enforce exits 0 today; the confirmed-real entries fire as expired on 2026-09-16; 53 xtask tests green.
…s its bound Finishes what 8e08f17 started. All 52 baseline entries now carry a real classification: 44 TRIAGED (bounded or rule misfire, each citing the specific bound) and 9 CONFIRMED REAL (expiring 2026-09-15, ahead of the rest). Zero entries remain marked "NOT individually triaged". Three structural patterns explain nearly all of the 47 newly-classified findings: - Virtual tables: every read() collects an in-memory registry into Vec<VirtualRow> while the streaming twin visit_rows exists. The class is the REGISTRY's bound, not the read's — connections, peers, in-flight queries, alert rules, MVs, tables and indexes are all server-resource bounded. - ferrosa-sql is a materializing executor whose blocking operators cannot be streamed at all (t_50d99192). - sstable/postgres/flight/view are bounded by schema arity, one flush-sized SSTable, or one Arrow RecordBatch. TWO SUSPECTED class-A findings REFUTED by reading the code rather than trusting the pattern: - billing.rs buckets looked unbounded (tenant x keyspace x minute) but are capped by MAX_BUCKETS with evict_oldest() at :79-80. - query_fingerprints.rs looked attacker-amplifiable via distinct query shapes but is capped at MAX_ENTRIES=10_000 with evict_lowest() at :74-75. Both drop to bounded. Class-A count is 19, not the 21 first reported. Two rule misfires are worth recording because they are INVERTED — the flagged constants are memory-BOUNDING, the opposite of the risk the rule describes: PARTITION_TOKEN_SUMMARY_MAX_ENTRIES downsamples so reader memory does not scale with table size, and MAX_NODE_READ is a fixed single-trie-node buffer. New fix tasks: t_f348ba0b (postgres full-table SELECT materialization, client-facing, highest), t_50d99192 (ferrosa-sql executor memory bound). Verified: enforce exits 0 today; confirmed-real entries fire as expired on 2026-09-16; 53 xtask tests green.
Owner decision, reversing part of the t_a49d88c3 triage: any bound on a result is a smell that the path is not streaming, and is a bug. Ferrosa has streaming and spill-to-disk temp-table sort, so a cap is a workaround for missing streaming rather than a safety feature. Failing loud beats truncating, but both are worse than answering the query. I had justified five entries on exactly the reasoning this rejects. Reclassified as CONFIRMED REAL: - graph engine.rs / http.rs — sized by config.max_result_rows - graph executor/stream.rs — collect_to_graph_result BREAKS at max_rows and returns Ok with no truncation flag. GraphResult carries no such flag, so a short answer is indistinguishable from a complete one. Silent data loss, which the fail-loud rule forbids outright. - sparql executor.rs DEFAULT_MAX_ROWS + property_path.rs — refuse a legitimate query the engine could answer by spilling. The repo already decided this and I contradicted it. spill.rs:23 records "Owner decision (2026-07-25): the graph path SPILLS rather than caps, and `max_result_rows` is then removed outright", and expand.rs/varpath.rs already removed their truncation — varpath.rs:305 says it "SILENTLY TRUNCATED the result — no error, no flag". leapfrog.rs (5 sites) and collect_to_graph_result did not. The distinction that matters is not bounded-vs-unbounded but WHAT is bounded (varpath.rs:310): RESULT buffer -> bug; stream or spill WORK (timeouts, visited ceilings) -> legitimate DoS control ONE structure (a protocol frame, a single trie node buffer) -> not a result bound Two sstable entries are marked NEEDS OWNER DECISION rather than re-judged, since my judgement on this exact question was already overruled once: whether a sampled index summary and a fixed single-node read buffer fall inside the rule. New tasks: t_27ca9f59 (graph silent truncation, p93), t_0cf21483 (sparql spill, p86). t_50d99192 reframed from "cap and/or spill" to spill. t_61c4e020 (weaken the rule) archived WON'T FIX — the rule is right. Verified: enforce exits 0 today; 53 xtask tests green.
…outright Graph queries could return a short answer with no error and no flag, so a client could not tell a partial result from a complete one. Finishes the migration spill.rs:23 already recorded: "Owner decision (2026-07-25): the graph path SPILLS rather than caps, and `max_result_rows` is then removed outright." expand and varpath had already dropped their truncation; leapfrog, the virtual-table anchor, and the collect bridge had not. REMOVED (each returned Ok with fewer rows than matched): - collect_to_graph_result's `max_rows` parameter. Both production callers already passed usize::MAX, so the cap was dead in production and lived on only as a hazard — and as a test asserting it. - leapfrog: 4 `result_rows.len() >= max_result_rows` breaks, plus the `max_results` cap inside leapfrog_join. - the virtual-table anchor projection break in expand.rs. - `GraphEngineConfig::max_result_rows` itself, now unused. The analysis leapfrog.rs asked for in its own NOTE: the leapfrog_join cap looked like a JOIN-WORK bound, but dropping an intersecting binding drops an ANSWER — a result bound wearing a work bound's name, truncating silently. What genuinely bounds work there is check_timeout(query_timeout), which fails loud. TESTS — three replaced, because they asserted the defect: - collect_honors_the_max_rows_cap -> collect_never_truncates_the_result - leapfrog_join_respects_max_results -> leapfrog_join_returns_the_complete_intersection - execute_virtual_table_respects_max_rows -> execute_virtual_table_returns_every_row The virtual-table one fed in 100 rows and asserted 5 came back. Each replacement drives a result larger than the old cap and asserts every row survives. Verified the fix rather than assuming it: deleting the stream.rs allowlist entry and re-running the audit showed what actually remains — the drain-into-Vec MATERIALIZATION, which is the documented boundary until the transport increment sends rows straight to HTTP/Bolt. The entry is restored saying exactly that instead of overclaiming. Two other entries citing the now-deleted config field were corrected. ferrosa-graph 490 tests green, clippy clean, OOM audit enforce exits 0.
… (t_d3b2dec1) `ferrosa_sql::execute` is synchronous and CPU-bound — scan, filter, sort, hash-aggregate, hash-join over a materialized row set — and both PG query paths called it directly inside an async handler (query.rs:705 simple, server.rs:654 extended). There was no spawn_blocking anywhere in ferrosa-postgres, so one PG-wire SELECT over a large table occupied an async worker for the whole sort/join. That is the shape PR #131 fixed on the CQL keepalive path, where sync work inline on an async worker starved keepalives and raft heartbeats into a "pool is broken" / CheckQuorum step-down. Same mechanism, heavier per call. Adds `offload::execute_offloaded`, which moves the statement, catalog, schema and params onto a blocking thread. The move is cheap: `MapCatalog`'s tables are `Arc<dyn TableProvider + Send + Sync>`, so it is a refcount bump, not a copy of row data. Why spawn_blocking and not ferrosa_sched: `submit_scan` is built for chunked streaming producers — it hands back a `ScanSlot` to yield against and wires channel-close cancellation. A relational `execute` is one opaque call with no yield points, so it has nothing to do with a `ScanSlot`. spawn_blocking is not unbounded here either: the data and background runtimes set explicit max_blocking_threads ceilings (t_88223ad0) so blocking work cannot oversubscribe the cores and starve consensus. A panic in the executor is re-raised with its original payload rather than flattened into an `ExecError`. Every `ExecError` variant is a user-facing SQL condition; a panic is an engine bug, and dressing it up as a query error would hide a defect behind a message the client cannot act on. TESTS: `executor_does_not_run_on_the_async_worker` observes the executor's thread through a `Catalog::resolve` that records `thread::current().id()` — resolve runs inside `execute`, so it reports the thread the executor actually ran on. Verified the test has teeth by temporarily inlining the call: it fails with "the synchronous executor ran on the async worker". A second test asserts offloading is behavior-preserving, error case included. This is also the prerequisite for t_f348ba0b: bridging the async storage stream into TableProvider::scan's synchronous Iterator needs a bounded channel whose consumer may block on recv, which is only safe off the runtime. ferrosa-postgres 139 tests green, clippy clean, fmt clean.
The dispatch arm for BEGIN/COMMIT/ROLLBACK said transactions were not yet implemented. They are: server::execute_simple intercepts them and drives the session's buffered write-set through Accord (commit_txn), and the extended protocol does the same — FMEA PG-1 records write atomicity as Implemented over both protocols. That arm is only reachable by a caller that enters dispatch directly with no session state, so it now says what is actually true: transaction control needs a session. A reader following the old message would have concluded ferrosa-postgres has no transaction support at all, which is wrong and would misdirect anyone scoping the Elle certification work (t_27672375).
…ON.md The note predated the CI job. `.github/workflows/elle-strict-serializable-nightly.yml` exists and runs both the fault-free and nemesis certifications nightly against a 3-node RF=3 compose cluster, opening an issue on failure. Understating coverage is its own hazard: a reader scoping the Postgres certification (t_27672375) would have concluded Elle has no regression guard and rebuilt one. Records the division of labour — CI covers regression, the Fly run remains the manual reference certification on real multi-host infrastructure.
…t blind-abort
The unified-transaction-manager architecture said of crash-recover: "the temp
table is dropped (NVMe reclaimed) ... a coordinator restart can enumerate orphan
temp tables and abort/GC them".
That is a data-loss bug. The NVMe per-transaction temp tables are
commit-log-equivalent durable state, not scratch: a temp table may hold the only
local record of a transaction Accord has ALREADY agreed to commit. If the
coordinator dies after the commit decision but before promotion runs locally,
blind abort silently discards an acked commit — the standard in-doubt case for
any consensus-committed transaction.
Replaces it with the resolution obligation: enumerate orphans on restart and
resolve each against that transaction's Accord status —
Committed -> promote (idempotent; promotion may have partially run)
Aborted -> drop
Undecided/in-doubt -> drive Accord recovery to a decision, then act;
never unilaterally abort
Never proposed -> drop
and records that promotion must be idempotent and restartable so a crash during
promotion resumes without double-applying.
Also reframes "Durable/recoverable txn state" in the future-seams list: it was
written as an availability concern (don't lose a long interactive transaction).
It is a CORRECTNESS requirement — without durable transaction identity a node
cannot ask Accord what was decided, which forces exactly the blind abort that
drops the commit.
Implementation and test plan: forge t_7b5788a3. This gates the Elle
certifications, since the nemesis schedule restarts nodes and would surface
blind-abort as lost commits.
… (t_7b5788a3) First slice of the restart-recovery obligation the spec fix (c412b6f) established: the NVMe per-transaction intent temp tables are commit-log-equivalent durable state, so a restart must resolve each orphan against its Accord decision instead of blind-aborting it. Adds `resolve_orphan_intent(phase, past_deadline, replay_was_lossy)` returning Promote / Drop / Recover / Quarantine. Pure and total, so every input combination is exhaustively testable without any storage machinery — the correctness core lands before the storage that will depend on it. Owner decisions encoded: - THE ACCORD DECISION ALWAYS WINS OVER THE DEADLINE. A committed transaction is promoted however far past `transaction_open_timeout` the restart happens; expiring it instead would silently drop an acked commit. The deadline governs only transactions Accord never decided. - Applied -> Drop (already materialised; re-promoting risks a double-apply). - Undecided within deadline -> Recover; never a unilateral abort on one node's opinion. `replay_was_lossy` turned out to be load-bearing and is the non-obvious part. `CrashRecoveryReplay::replay` SKIPS entries that fail CRC — partial writes from a crash mid-write — counted in `skipped_count()`. So once anything was skipped, "absent from txn_states" no longer proves "never proposed": the skipped record could have been this transaction's Committed entry. Both the absent case and the undecided-past-deadline case therefore escalate to Recover rather than Drop when replay was lossy. Dropping on known-incomplete evidence is precisely how a commit would be lost. TESTS — 9, verified to have teeth by mutation rather than assumed: - reintroducing the old spec's behaviour (Committed -> Drop) fails 2 tests with "committed must never be dropped"; - deleting the lossy-replay guard fails 3 with "a skipped entry may have been its Commit record" / "absence is not proof of never-proposed". Two are exhaustive sweeps over all input combinations rather than point cases. Note `CrashRecoveryReplay` is still exported but never called in production — wiring replay into startup is the next slice, and is what makes this reachable. ferrosa-storage 1029 lib tests green, clippy clean, fmt clean.
…7b5788a3) The Accord protocol log was durable but UNREADABLE, which is why nothing ever replayed it and Accord protocol state was lost on every restart. Three components existed and did not fit: - `FileSyncWriter` appended serialized entries as raw concatenated bytes, with no length prefix and no delimiter; - `AccordProtocolEntry::deserialize` needs an EXACT record boundary — it reads the CRC from the final four bytes of whatever slice it is handed; - `CrashRecoveryReplay::replay` takes records already split apart. So the file accumulated entries no reader could delimit. (`ProtocolLog`, the type that looks like the log, is in-memory only — its own docs say disk persistence is "a later optimization" — and is unwired in production.) Adds `accord::framed_log`: `[8-byte magic][u32 LE len][record]...`, with `FileSyncWriter` stamping the magic once on creation and framing every record thereafter. Prefix and payload go out in a single `write_all`, so a crash can tear the tail but cannot interleave a prefix with another record's payload. The magic is not decoration. Without it the first four bytes of a pre-framing file would be read as a length, silently yielding plausible-looking garbage records. With it, a legacy file is reported as `LegacyUnframed` and the caller must decide — "no records" and "records I cannot read" are different, and conflating them would quietly discard recoverable transaction state. A torn tail is expected and benign (that entry never completed its fsync, so no protocol reply depended on it) and is reported rather than hidden. An absurd length is treated as a torn tail instead of driving the allocation. TESTS — 8, including the end-to-end proof that the three components now compose: drive the REAL `FileSyncWriter`, read the file back, replay it, and assert a `Committed` transaction survives the simulated restart. That is precisely the state whose loss would force the blind abort of its intents. Two existing writer tests failed on the format change and were UPDATED, not weakened — one asserted `b"entry-1entry-2"`, which was the bug itself: concatenated bytes with no boundary cannot be split back into records. They now assert individually-delimited records plus magic-stamped-once. Next slice: consume this at startup to seed the conflict index and applied set. ferrosa-storage 1037 lib tests green, clippy clean, workspace builds.
…bcc) Every existing nemesis fault is a NETWORK fault — isolate, isolate-coordinator, netem latency. None of them ever discards local state, so the passing strict-serializability certification says nothing about process death. That gap matters here specifically. Nothing replays the Accord protocol log at startup, so a restarted node returns with an EMPTY ConflictIndex — no memory of in-flight transactions — and begins serving PreAccept immediately. A replica answers PreAccept with the dependencies it knows about, so an amnesiac replica reports an INCOMPLETE dep set, which is the shape that produces real-time ordering anomalies (:strong-PL-1-cycle-exists / :G-nonadjacent-item-realtime). Whether quorum covers that, or whether a restarted node must be fenced until recovery completes, is unanswered — and untested. Adds `inject_restart_replica`: SIGKILL node3, restart it, wait for it to serve CQL again. Design notes worth keeping: - SIGKILL rather than a graceful restart. A clean shutdown could flush state a real crash would not, testing the easy case and hiding the one we care about. - node3, NOT the seed. The generator runs ON node1 and writes its history to node1's /tmp; restarting node1 would kill the generator mid-run and take the history with it. A non-coordinator restart still exercises the amnesia question. Coordinator restart needs the generator off-cluster and is separate. - The fault verifies it BIT (machine state after SIGKILL) and that node3 REJOINS and serves CQL. A node that never rejoins silently degrades the run to two nodes, which is a weaker test, not a pass — so that case logs a loud warning telling the reader to distrust the verdict. Either outcome is worth having: green converts an assumption into evidence; red finds a real consensus bug in a path nothing currently covers. The likely fix if red is fencing, NOT storing write keys in the protocol log (see t_4b91be79 for why that lever was rejected). Harness change only — no certification run performed.
…f its target Extends the restart fault (20dbc80c) to cover the COORDINATOR, which is the more interesting case: the coordinator holds the in-flight Accord state for the transactions it is driving, so its amnesia on restart is what most plausibly produces an incomplete dep set. That required moving the generator. It previously ran ON the seed against `localhost:9042`, which cannot survive a coordinator restart — SIGKILLing that machine would destroy the generator process AND the history in its /tmp. The generator now runs on node2 (never restarted) and drives node1 over the internal network, so node1 can be killed while the run and its evidence continue. This is safe by construction: each worker is pinned to one target with NO retry failover, and a failed BEGIN/UPDATE classifies `:fail` while a failed COMMIT classifies `:info` — never `:fail` — so a write that did land cannot masquerade as one that did not. Verified in elle_list_append.rs before relying on it. Two guards against a falsely-green run: - A generator preflight (node2 -> node1:9042) fails BEFORE the ~40min build+run rather than producing an empty history from a bad address. - The schedule roughly doubled in length, because each restart blocks until the node serves CQL again. A fault that fires after the generator has finished hits an IDLE cluster and proves nothing, so GEN_ARGS is scaled up (8/1800/8/3) and the script now REPORTS whether the generator actually outlasted the schedule. An under-provisioned run is visible instead of silently degrading the new faults into no-ops while still printing `valid? true`. Harness only — no certification run performed yet.
…ed quotes The first run of the restart-fault cert aborted at the generator preflight with GEN_FAIL. That was not a real connectivity failure — both of my new checks were broken, in two independent ways: 1. `/dev/tcp/host/port` is a BASH builtin. `nem` runs `sh -lc`, and /bin/sh on debian:trixie-slim is dash, which has no /dev/tcp — so the test could only ever fail, regardless of whether the path was reachable. 2. `nem` wraps its argument in single quotes (`sh -lc '$2'`), so the nested single quotes in `sh -c '</dev/tcp/...'` terminated the string early and mangled the command. Both checks now use curl, which is installed in the image and is already the readiness probe `wait_ready` trusts, with no nested quotes. The preflight targets node1's WEB port rather than raw CQL: CQL binds `[::]:9042` with the same wildcard as web's `[::]:9090`, so a reachable web port proves the DNS + cross-node IPv6 path the generator needs. THE MORE SERIOUS HALF was the rejoin check inside `inject_restart`, which had the identical defect. It would have reported "did NOT rejoin within 300s" for BOTH restart faults no matter what actually happened — turning the guard that exists to detect a degraded 2-node run into a permanent false alarm, and undermining exactly the evidence these faults were added to produce. The preflight caught this before the workload ran: the run aborted and tore down cleanly (machines, app, and bucket all destroyed) rather than producing an empty history. That is the guard working as designed — it just happened to fire on its own bug first.
…nate faults TWO CORRECTIONS TO THE PREVIOUS RUN. 1. THE REJOIN TIMING WAS NOT A MEASUREMENT. The loop reported `i*10`, counting iterations while ignoring the several seconds each `flyctl ssh` round-trip costs. So it under-reported real latency, AND its 10s granularity made two restarts appear to land on an identical figure — which I then read as "matches formation_timeout exactly". That inference was not supported by the instrument. Now measured as real elapsed wall-clock and logged as REJOIN_SECONDS alongside the configured timeout, so the correlation can be judged instead of guessed. Poll interval tightened to 5s. 2. THE WORKLOAD WAS TOO SMALL TO MEAN ANYTHING. The run produced 1026 :ok / 5 :info / 13369 :fail — 93% failure — and Elle returned `valid? true` over roughly a thousand successful appends. That is a VACUOUS pass: no anomalies found because almost nothing succeeded, not because ordering held. (The repo has hit this before: a 2026-07-20 run was green on a mostly-rejected-write history and was correctly discarded as weak.) Cause is structural, not a product bug. Each worker pins to one target with no failover (deliberate — a transaction must not split across nodes), that target is node1, and node1 is SIGKILLed for ~2 minutes. Every op in that window fails. Two restarts plus the coordinator isolation account for ~11.5k of the 13.4k failures at the observed ~41 ops/sec. Fixed by sizing the run so the successful portion dominates: GEN_ARGS 8/8000/8/3 (~64k ops, ~26min) against a schedule of roughly 475s. The outlast guard added with the restart faults did its job here — it reported that the generator finished at 349s while the schedule was still running, which is what revealed the coordinator restart had no post-rejoin traffic. Keeping that guard honest is what makes a future green verdict worth citing.
…cross nodes Three cert runs produced vacuous verdicts. Root cause found in the generator, not the database. === 1. RECONNECT WAS GATED ON THE WRONG CONDITION === The session replacement lived inside `if let Some(id) = txn_id_opt`, so a broken session was only ever replaced when BEGIN had already SUCCEEDED. That is exactly backwards: when the connection dies, BEGIN is the statement that fails, so there is no txn id, the branch is skipped, and the worker spins its entire remaining op budget against a corpse. Measured on run 3: 38,543 of 61,564 failures were "BEGIN: No connections in the pool: The pool is broken". One network disruption ~100s in permanently killed all eight workers, leaving Elle to check a history that was 96% connection errors — 2,418 :ok out of 64,000 ops. `valid? true` on that is meaningless. Reconnect is now driven by session HEALTH via `is_session_dead`, independent of which statement failed. Deliberately narrow: pool/connection errors replace the session, while server-side rejections (invalid query, insufficient replicas) leave it alone, since needlessly rebuilding a session would discard the transaction state the pinned-connection model depends on. The READ path had the same defect AND swallowed its error entirely (`Err(_)`), so read failures never reached the tally that exists to explain a run. Both fixed. Why earlier certifications never hit this: the generator ran ON the coordinator against localhost, which survives peer-level ip6tables rules, so the pool never broke. Any off-node generator — required to fault-inject the coordinator itself — depends on this reconnect. === 2. ALL WORKERS PINNED TO ONE NODE === `min_by_key(host_id)` gave every worker the same target, so one node coordinated 100% of the workload. Workers now round-robin across all nodes (schema setup stays on a single session; each worker remains pinned to ITS node, preserving the one-transaction-one-connection invariant that FallthroughRetryPolicy protects). Two things this fixes: killing a node no longer removes all traffic, so a rejoining node faces real load — the condition the restart faults need; and transactions are now coordinated by every node, so cross-coordinator Accord ordering is exercised, which single-target runs never covered. Corrects a wrong diagnosis of my own along the way: I had claimed the generator was dying with its target, on the strength of two runs ending at similar wall-clock despite a 4.4x op count. The 64,000 :invoke count refutes that — the generator completed every op. Failing ops are simply fast, so a mostly-failing run races through its budget.
Local end-to-end validation of the reconnect fix was attempted and could NOT be completed — recorded here so the gap is not mistaken for coverage. The local 3-node compose cluster runs an 8-day-old nightly image that predates server-minted transaction ids, so every append is rejected instantly with "nested transactions are not supported" (15,997 of 16,000 failures). With no network round-trip, 24,000 ops complete in SEVEN SECONDS, so no mid-run node kill can land while the generator is alive. Validating the reconnect end-to-end needs an image built from current main. What the attempt DID verify: the worker-spreading fix works — the generator reports "pinning 6 workers across 3 node(s)" against the live cluster. So the classification that drives the reconnect is unit-tested instead, which needs no cluster: - the five failure texts that actually dominated the fault-injected run are classified dead (they were the 38,543 "pool is broken" BEGIN failures that previously never triggered a reconnect); - server-side rejections — insufficient replicas, invalid query, client timeout, internal error — are classified LIVE. Reconnecting on those would be actively harmful: rebuilding a session mid-run discards the transaction state the pinned-connection model depends on; - classification is case-insensitive, since driver error casing is not a stable contract. Verified the tests bite by inverting `is_session_dead` to always return false: 2 of 3 fail with "must replace the session for: BEGIN: No connections in the pool...". Restored. This tests the DECISION, not the reconnect wiring. The end-to-end path still needs a current-image run before the next certification is trusted.
…aft state timeline The formation test installed no tracing subscriber, so a CI failure printed only the final metrics line. Worse, RUST_LOG=openraft=debug HIDES the formation race (20/20 pass with it, ~3% failures without): synchronous formatted writes on the raft path shift the timing window. So the durable diagnostic is a metrics-watch timeline (25ms sampling, transitions only, one shared epoch across nodes), dumped on failure alongside voters/learners membership — the fields that separate 'votes rejected' from 'elections stopped' (t_b0aac0d3).
…ph_result Rebase repair: main's graph-http-streaming (#306) added two call sites passing max_rows=usize::MAX while this branch's truncation fix removed the cap parameter entirely (a mid-drain cap silently truncates with no partial flag). usize::MAX meant 'no cap', so dropping the argument preserves semantics exactly.
…ed for the pre-vote stall) candidate_re_campaigns_while_peers_are_down: the seed initializes a 3-voter membership with both peers down and must keep firing elections — current_term advancing past 1 is the oracle (election-timer liveness, not leadership; a lone seed can never lead a 3-voter cluster). Fails on the unfixed tree with term frozen at exactly 1: the fork's tick election path gates elect() behind a pre-vote round that FerrosRaftNetwork provides no transport for, so every probe is an automatic NO and no election after initialize()'s can ever fire (t_b0aac0d3).
…exists raft_enable_pre_vote defaulted true per ADR-012, but the pinned openraft fork's tick election path hard-gates elect() behind run_pre_vote_round() while FerrosRaftNetwork never overrides pre_vote — the default trait impl returns an unimplemented NetworkError counted as a NO vote. A pre-vote quorum was structurally impossible, so after the seed's single initialize()-driven election at term 1, no election could ever fire again: a transient first-round vote loss became a permanent formation stall (~3% under CPU starvation, CI job 89743146291). Default the gate OFF so the tick path elects directly and a lost round self-heals on the next timeout, exactly like upstream. The FERROSA_RAFT_ENABLE_PRE_VOTE override remains for when the transport lands (t_32cb5ad3). ADR-012 assert inverted with rationale; election-storm mitigation remains the election_guard watchdog, now documented as load-bearing (CL-2/CL-15). Spec: specs/implemented/bug-cluster-formation-pre-vote-election-stall.md (t_b0aac0d3).
bkearns
force-pushed
the
chore/materialization-lint-coverage
branch
from
July 28, 2026 19:33
4b8fbf0 to
60af610
Compare
bkearns
changed the title
Contributor
Author
|
Reviewer coverage statement (honest scope, per-commit): Deep line review (by me, this cycle):
Reviewed as a set (Elle hardening, 6 commits Pre-existing from original PR scope (not re-reviewed this cycle): the materialization-audit/xtask, Accord framed-log/crash-recovery, and postgres offload commits predate this convergence effort and rode through the rebase unchanged (3-way, no content conflicts outside the graph repair noted above). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope grew substantially since opening — this PR now carries four related workstreams, rebased onto e6c768c:
1. Materialization audit coverage + replayable Accord protocol log (original scope)
Exhaustive-by-default materialization lint (NON_SERVING_CRATES + unclassified-crate findings), graph truncation fix (capless
collect_to_graph_result— a mid-drain cap silently truncates with no partial-result flag; bounds belong upstream in the stream), framed Accord protocol log + crash recovery.2. Elle certification hardening (6 commits)
The 2026-07-26 nemesis cert was vacuous: the generator pinned all workers to one node, so killing the coordinator stopped 100% of traffic (1026 :ok / 13369 :fail,
valid? trueover ~1000 single-coordinator appends). Fixes: workers spread across nodes (each pinned to its node, preserving the one-txn-one-connection invariant), dead sessions replaced on any failure, rejoin latency measured properly, workload sized to dominate faults, coordinator-restart fault added, + a local podman certification harness (deploy/local-elle/) so harness breakage is caught before a cloud run.3. Formation diagnosability (2 commits)
The formation test installed no tracing subscriber (CI failures printed nothing usable), and
RUST_LOG=openraft=debugHIDES the formation race (20/20 pass with it) — so the durable diagnostic is a metrics-watch timeline recorder (25ms sampling, shared epoch, voters/learners dump on failure).4. Formation election-stall fix (2 commits: red test + fix) — the bug that failed this PR's own Test + Coverage
Root cause (t_b0aac0d3):
raft_enable_pre_votedefaulted true, but the pinned openraft fork hard-gates the tick election path behind a pre-vote round whose transportFerrosRaftNetworknever implements — every probe is an automatic NO, so after the seed's singleinitialize()-driven election at term 1, no election can ever fire again. A transient first-round vote loss became a permanent stall (~3% under CPU starvation; CI job 89743146291 was exactly this).Fix: default the gate OFF until the transport lands (t_32cb5ad3); tick path then re-elects and self-heals, like upstream. Regression guard:
candidate_re_campaigns_while_peers_are_down(term must advance past 1 with peers down; deterministic red on the unfixed tree).Verification: 250 consecutive CPU-starved formation runs, zero failures (two runs of 200+50, both externally killed mid-run by machine pressure, zero failures in every completed iteration), vs pristine main's measured ~3% (1/30 control + 5/85 across instrumented runs).
Spec:
specs/implemented/bug-cluster-formation-pre-vote-election-stall.md. FMEA updated: CL-2 no longer claims PreVote as a live mitigation (the election_guard watchdog is load-bearing); new CL-15 row.Pre-existing red-check note for reviewers: Test + Coverage may still hit two OTHER catalogued load-sensitive families unrelated to this diff — compaction bounded-wait (t_283f37df) and fair_admit share-capture (t_ffb53cf9). Attribute against those before assuming this PR.