diff --git a/.kiro/specs/raft-apply-decoupling/tasks.md b/.kiro/specs/raft-apply-decoupling/tasks.md index 54d81a69..103fa01d 100644 --- a/.kiro/specs/raft-apply-decoupling/tasks.md +++ b/.kiro/specs/raft-apply-decoupling/tasks.md @@ -158,7 +158,7 @@ - _Depends: 6.1, 4.2_ - [ ] 7. Conformance, regression, and existing-behavior preservation -- [ ] 7.1 Extend the deterministic conformance harness with apply-ordering assertions +- [x] 7.1 Extend the deterministic conformance harness with apply-ordering assertions - Add an assertion that no entry's state mutation or reactions occur before that entry commits - Add an assertion that re-applying an already-committed entry is idempotent - Observable: the conformance harness run reports both new assertions passing alongside its existing safety checks @@ -188,6 +188,9 @@ ## Implementation Notes +- **7.1**: correctly identified `src/lib/raft/raft_sim.c` (pre-existing `#ifdef XCE_TESTING` TLA+-conformance simulator, driven by `src/lib/testsuite/test_raft.c` in the `xcelib_test` Unity binary — a different binary/library boundary than every other task in this spec, which use `controller_unit_test`) as the intended "deterministic conformance harness," confirmed verbatim against design.md's Testing Strategy line 442 ("Extend `raft_sim.c`'s conformance harness with 'no reaction before commit' and 'idempotent re-apply' assertions layered on its existing apply-after-`on_commit_advanced` loop"). Added two fault-injectable checker functions (`raft_sim_check_apply_after_commit`, `raft_sim_check_idempotent_reapply`) plus `raft_sim_replay_commit`, gated by two new flags (`RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT`, `RAFT_SIM_BREAK_REAPPLY_GUARD`) and a new `apply_reaction_count[]` field on `raft_sim_node_t`. Wired into `sim_persist_entry`/`sim_conf_on_commit_advanced`, both no-ops (byte-identical to prior behavior, confirmed by reviewer) when the flags are unset — zero regression risk to the 5 pre-existing scenario functions. RED phase independently reproduced by reviewer for both checks (`Expected 39338 Was 0`, `Expected 2 Was 1`); GREEN 287/0/0 confirmed twice (implementer + reviewer), ASan-clean. + **Scoping decision, independently upheld by reviewer**: the two new checks run only via their own 2 dedicated tests, NOT wired into `raft_sim_check_all`/`sim_scen_check` (the umbrella the 5 pre-existing scenario functions use). Reviewer traced this to `raft-sim-conformance` spec's Requirement 3 Criterion 5 (fixed four-invariant, fixed-order, halt-on-first-failure contract) — an already-shipped contract owned by a *different* spec that 7.1 has no charter to modify. Also: these checks operate on the harness's synthetic `applied[]`/`apply_reaction_count[]` model, not real production `apply_committed_entry()`/`persist_apply_pump_run()` — real production coverage of this ordering already exists via tasks 6.1-6.4's `test_apply_pump_integration.c`. Confirmed legitimate scoping, not a corner-cut. + **Residual, non-blocking documentation nitpick**: the `applied[k-1] != 0` "has-been-applied" sentinel is not structurally guaranteed non-zero the way log-entry term 0 is (raft.tla guarantees terms start at 1; arbitrary client payloads have no such guarantee) — the header comment's "same zero-means-absent convention as log-entry terms" analogy slightly overclaims precedent (the pre-existing `raft_sim_check_state_machine_safety` bounds by `commit_index` directly, doesn't use a zero-sentinel at all). Currently harmless because the only two call sites use hardcoded non-zero literals (`0x99AA`, `0x77BB`). **Fast-follow flag for whoever later widens this harness to PRNG-drawn payload values**: `apply_reaction_count[]` cannot substitute as the "ever-applied" indicator for `check_apply_after_commit`, since the `BREAK_APPLY_BEFORE_COMMIT` fault only touches `applied[]` — a real generalization needs a third parallel "ever-written" flag array, not a reuse of either existing array. - **1.1**: `commit_submit_async()` reuses the exact `wait_admit_locked`/`wait_release_locked`/`inflight_waits` admission-cap discipline as `COMMIT_MODE_WAIT` — task 1.2's `commit_resolve()` MUST call `wait_release_locked()` when resolving an async-registered entry, or the shared admission cap leaks. Continuation registry is `calloc(n_vcpus, ...)`, sized so `n_continuations >= wait_cap(ce)` always holds; `lsn == 0` is the free-slot sentinel. - **1.1 review**: bare `controller_unit_test` (no `--run` filter) segfaults deterministically in this environment (SSL_CTX_free/iomgr_fini crash, `test_ha_vep_teardown.c`) — reproduced identically on the pre-task base commit, confirmed pre-existing and unrelated. Logged as bug-1406. Also `test_commit_advance_retry_clears_push_before_commit_fence` (`test_config_repl.c`) fails independently of this feature, also reproduced on base — logged as bug-1407, flagged because it's adjacent to `commit_advance_locked`, which task 1.2 modifies. **Always validate with a filtered `--run ` invocation or the tightly-coupled commit-engine cluster, not the bare unfiltered binary, until 1406/1407 are separately fixed.** - **1.2**: implemented `commit_resolve()` + wired it into all 6 commit.c-internal paths that can advance `committed_lsn`/`writable_lsn` (the COMMITTED/majority-ack outcome). Chose `VTASK_LIVE_BLOCK` for dispatch (never admission-gated, unlike `VTASK_REQ_BLOCK` which can fast-reject — unacceptable for "exactly-once, never unresolved"). **Reviewer caught a real task-graph gap, not a defect in 1.2 itself**: converting `COMMIT_MODE_WAIT` away from a blocking wait also removes the *only* mechanism that ever detected a commit timing out (the caller's own `pthread_cond_timedwait` deadline) — nothing yet arms an equivalent timeout for async-registered continuations, and nothing yet wires demotion/log-truncation detection to call `commit_resolve(FAILED, ...)`. Without this, a pending async command during quorum loss would hang forever instead of failing — worse than today's behavior. Split out as new tasks **1.3** (timeout arming) and **1.4** (demotion/truncation wiring), inserted before the renumbered **1.5** (was 1.3, concurrency test — now exercises the real detection paths from 1.3/1.4 instead of calling `commit_resolve()` directly). Task 3.2 now also depends on 1.3/1.4, since its own "failure/timeout outcome replies" observable is meaningless without them. diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h index 1d9b8465..a5b751af 100644 --- a/src/lib/raft/raft.h +++ b/src/lib/raft/raft.h @@ -467,9 +467,24 @@ void raft_snapshot(const raft_state_t *s, raft_snapshot_t *snap); * supplied term (emulates G1: entries carry term 0 so on_get_entry_term cannot * distinguish same-index/different-term entries). * - RAFT_SIM_BREAK_TRUNCATE: on_truncate_from is a no-op (emulates G3: a divergent - * suffix is never deleted). */ + * suffix is never deleted). + * - RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT (raft-apply-decoupling task 7.1): + * on_persist_entry ALSO eagerly decodes and writes the entry's value into + * applied[] at persist time, before the entry ever commits — emulating an + * apply-before-commit regression (the defect class raft-apply-decoupling + * fixes). Lets the new raft_sim_check_apply_after_commit oracle be proven + * sensitive, not merely vacuously passing. + * - RAFT_SIM_BREAK_REAPPLY_GUARD (raft-apply-decoupling task 7.1): + * on_commit_advanced (conformance variant) reprocesses the FULL committed + * prefix on every call instead of only the newly-committed suffix, so a + * forced replay (raft_sim_replay_commit) re-fires an already-applied + * entry's reaction — emulating a duplicate/retried commit notification + * that is not idempotent. Lets the new raft_sim_check_idempotent_reapply + * oracle be proven sensitive. */ #define RAFT_SIM_BREAK_TERM_STAMP 0x1u #define RAFT_SIM_BREAK_TRUNCATE 0x2u +#define RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT 0x4u +#define RAFT_SIM_BREAK_REAPPLY_GUARD 0x8u /* Buffered RPC entry: one pending vote-request to route. * last_log_term/last_log_index capture the CANDIDATE's advertised last-log tuple @@ -614,6 +629,15 @@ typedef struct { * satisfies the design's `log_len` requirement for the in-memory log length; * no separate log_len field is added (it would duplicate log_count). */ uint64_t applied[RAFT_SIM_MAX_LOG]; + /* apply_reaction_count[k-1] counts how many times committed index k's + * apply-time reaction has fired (raft-apply-decoupling task 7.1). Under + * correct behavior this is always 0 or 1 per index — a value > 1 means the + * SAME committed entry's mutation/reaction ran more than once (a genuine + * duplicate application), which raft_sim_check_idempotent_reapply asserts + * never happens (Req 10.3). Incremented alongside applied[] inside + * sim_conf_on_commit_advanced only; the legacy (non-conformance) adapter + * never touches it, so it stays all-zero for raft_sim_init clusters. */ + int apply_reaction_count[RAFT_SIM_MAX_LOG]; /* Persisted-(term, votedFor) shadow surviving a Restart, mirroring raft.tla's * `Restart` (keeps currentTerm/votedFor/log, resets volatile state). The * conformance restart action restores from this shadow rather than re-reading @@ -855,6 +879,41 @@ int raft_sim_check_leader_completeness(const raft_sim_cluster_t *c); */ int raft_sim_check_state_machine_safety(const raft_sim_cluster_t *c); +/* ---- additive apply-ordering assertions (raft-apply-decoupling task 7.1) ---- + * + * The two checks below are layered ADDITIONS to the conformance harness's existing + * apply-after-on_commit_advanced loop (sim_conf_on_commit_advanced): they verify + * Req 10.1 and 10.3 against the harness's own apply model (applied[] / + * apply_reaction_count[]). They are deliberately NOT members of raft_sim_invariant_t + * and are NOT evaluated by raft_sim_check_all — that enum/function's four-invariant + * FIXED order is the raft.tla safety-property contract (Req 3.5) and is left + * untouched. Callers assert on these return values directly, same 1=holds/0=violated + * convention as the four checks above. */ + +/* + * raft_sim_check_apply_after_commit (Req 10.1): assert that no entry's state + * mutation/reaction — the harness's apply model, node->applied[] — occurs before + * that entry commits (node->commit_index reaching its index). applied[] uses the + * same zero-means-absent convention already established for log-entry terms + * elsewhere in this file: an untouched slot reads exactly 0, and every value this + * harness's tests/scheduler ever apply (PRNG draws or hand-authored client values) + * is non-zero, so "applied[k-1] != 0" reliably means "index k has been applied". + * Returns 1 when, for every node and every index k, applied[k-1] != 0 implies + * k <= commit_index; 0 on the first violation. Pure read-only over c. + */ +int raft_sim_check_apply_after_commit(const raft_sim_cluster_t *c); + +/* + * raft_sim_check_idempotent_reapply (Req 10.3): assert that re-applying an + * already-committed entry — whether a genuine duplicate on_commit_advanced fire or + * a forced raft_sim_replay_commit retry — never re-executes that entry's apply-time + * mutation/reaction. Backed by node->apply_reaction_count[k-1], incremented once + * per genuine apply of committed index k. Returns 1 when, for every node and every + * index k, apply_reaction_count[k-1] <= 1; 0 on the first index whose reaction + * fired more than once. Pure read-only over c. + */ +int raft_sim_check_idempotent_reapply(const raft_sim_cluster_t *c); + /* * raft_sim_invariant_t: identifier for the four Raft safety invariants checked by * the conformance harness, in the FIXED evaluation order used by @@ -952,6 +1011,20 @@ void raft_sim_export_trace(const raft_sim_cluster_t *c, FILE *fp); */ void raft_sim_advance_commit(raft_sim_cluster_t *c, int node_idx); +/* + * raft_sim_replay_commit (raft-apply-decoupling task 7.1; Req 10.3): re-invoke + * node_idx's OWN registered on_commit_advanced callback with its CURRENT + * commit_index — simulating a replay or retry of an already-processed commit + * notification. Bypasses the raft library's own fire-once-per-strict-advance + * contract (raft_advance_commit never re-fires on a held index) so the callback's + * OWN idempotency can be exercised directly, independent of whether the library + * would ever naturally re-invoke it. Works for clusters from either + * raft_sim_init or raft_sim_conformance_init, since both wire on_commit_advanced + * through node->cbs. No-op if node_idx is out of range or the node has no + * registered callback. + */ +void raft_sim_replay_commit(raft_sim_cluster_t *c, int node_idx); + /* * raft_sim_restart_node: model a process restart of node_idx (Req 8.3). * diff --git a/src/lib/raft/raft_sim.c b/src/lib/raft/raft_sim.c index 4dd54d88..208e8ef3 100644 --- a/src/lib/raft/raft_sim.c +++ b/src/lib/raft/raft_sim.c @@ -37,6 +37,11 @@ static void sim_noop_start_election_after_delay(void *ctx, uint32_t delay_ms) /* ---------- wired callbacks ---------- */ +/* Forward declaration: sim_conf_decode_value is defined further below (conformance + * harness adapter section) but sim_persist_entry's RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT + * fault branch (raft-apply-decoupling task 7.1) needs it earlier in the file. */ +static uint64_t sim_conf_decode_value(const raft_sim_entry_t *slot); + /* * sim_send_vote_request: buffer a pending RPC into the cluster queue. * @@ -125,6 +130,15 @@ static int sim_persist_entry(void *ctx, uint64_t index, uint64_t term, memcpy(slot->bytes, entry, n); slot->len = n; + /* Deliberately-broken-build fault switch (raft-apply-decoupling task 7.1): when + * RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT is set, eagerly decode and write this + * entry's value into applied[] right here at persist time — BEFORE it has ever + * committed — emulating an apply-before-commit regression (the defect class + * raft-apply-decoupling fixes). Purely additive — DEFAULT OFF, so legacy and + * conformance runs that never set the flag are byte-identical to before. */ + if (c && (c->fault_inject & RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT)) + node->applied[index - 1] = sim_conf_decode_value(slot); + if ((int)index > node->log_count) node->log_count = (int)index; @@ -314,12 +328,29 @@ static uint64_t sim_conf_decode_value(const raft_sim_entry_t *slot) static void sim_conf_on_commit_advanced(void *ctx, uint64_t commit_index) { raft_sim_node_t *node = (raft_sim_node_t *)ctx; + raft_sim_cluster_t *c = (raft_sim_cluster_t *)node->ctx; + + /* Deliberately-broken-build fault switch (raft-apply-decoupling task 7.1): when + * RAFT_SIM_BREAK_REAPPLY_GUARD is set, widen the apply range to the FULL + * committed prefix (1..commit_index) on EVERY call instead of only the + * newly-committed suffix (prev+1..commit_index) — emulating a regression where + * a retried/duplicate commit notification re-applies entries that were already + * applied. Purely additive — DEFAULT OFF, so a normal call (including a forced + * raft_sim_replay_commit retry) remains exactly as before: a no-op when + * commit_index has not strictly advanced. */ + int reapply_guard_broken = c && (c->fault_inject & RAFT_SIM_BREAK_REAPPLY_GUARD); uint64_t prev = node->commit_index; - if (commit_index > prev) { - for (uint64_t k = prev + 1; k <= commit_index; k++) { - if (k >= 1 && k <= (uint64_t)node->log_count && k <= RAFT_SIM_MAX_LOG) + if (commit_index > prev || reapply_guard_broken) { + uint64_t from = reapply_guard_broken ? 1 : prev + 1; + for (uint64_t k = from; k <= commit_index; k++) { + if (k >= 1 && k <= (uint64_t)node->log_count && k <= RAFT_SIM_MAX_LOG) { node->applied[k - 1] = sim_conf_decode_value(&node->log[k - 1]); + /* Reaction-fire mirror (Req 10.3): counts genuine applies of + * committed index k, so raft_sim_check_idempotent_reapply can + * assert it never exceeds 1. */ + node->apply_reaction_count[k - 1]++; + } } } @@ -797,6 +828,22 @@ void raft_sim_advance_commit(raft_sim_cluster_t *c, int node_idx) (void)raft_advance_commit(&c->nodes[node_idx].state); } +/* + * raft_sim_replay_commit (raft-apply-decoupling task 7.1; Req 10.3): re-invoke + * node_idx's own registered on_commit_advanced callback with its CURRENT + * commit_index, simulating a replay/retry of an already-processed commit + * notification. See raft.h for the full contract. + */ +void raft_sim_replay_commit(raft_sim_cluster_t *c, int node_idx) +{ + if (!c || node_idx < 0 || node_idx >= c->n) + return; + + raft_sim_node_t *node = &c->nodes[node_idx]; + if (node->cbs.on_commit_advanced) + node->cbs.on_commit_advanced(node, node->state.commit_index); +} + /* Two log entries are identical iff their term, length, and payload bytes * (over the recorded length) all match. */ static int sim_entry_identical(const raft_sim_entry_t *a, @@ -929,6 +976,45 @@ int raft_sim_check_state_machine_safety(const raft_sim_cluster_t *c) return 1; /* holds */ } +int raft_sim_check_apply_after_commit(const raft_sim_cluster_t *c) +{ + /* Additive check (raft-apply-decoupling task 7.1, Req 10.1) — NOT one of the + * four raft.tla invariants and NOT evaluated by raft_sim_check_all; see raft.h. + * applied[k-1] != 0 is treated as "index k has been applied" (the same + * zero-means-absent convention this file already uses for log-entry terms); + * every value this harness ever applies is non-zero in practice. */ + for (int i = 0; i < c->n; i++) { + const raft_sim_node_t *node = &c->nodes[i]; + int limit = node->log_count; + if (limit > RAFT_SIM_MAX_LOG) + limit = RAFT_SIM_MAX_LOG; + + for (int k = 1; k <= limit; k++) { + if (node->applied[k - 1] != 0 && (uint64_t)k > node->commit_index) + return 0; /* applied ahead of commit — Req 10.1 violated */ + } + } + return 1; /* holds */ +} + +int raft_sim_check_idempotent_reapply(const raft_sim_cluster_t *c) +{ + /* Additive check (raft-apply-decoupling task 7.1, Req 10.3) — NOT one of the + * four raft.tla invariants and NOT evaluated by raft_sim_check_all; see raft.h. */ + for (int i = 0; i < c->n; i++) { + const raft_sim_node_t *node = &c->nodes[i]; + int limit = node->log_count; + if (limit > RAFT_SIM_MAX_LOG) + limit = RAFT_SIM_MAX_LOG; + + for (int k = 1; k <= limit; k++) { + if (node->apply_reaction_count[k - 1] > 1) + return 0; /* re-applied — Req 10.3 violated */ + } + } + return 1; /* holds */ +} + raft_sim_invariant_t raft_sim_check_all(const raft_sim_cluster_t *c) { /* Evaluate the four invariants in the FIXED order required by Req 3.5 and diff --git a/src/lib/testsuite/test_raft.c b/src/lib/testsuite/test_raft.c index def2b223..083059ef 100644 --- a/src/lib/testsuite/test_raft.c +++ b/src/lib/testsuite/test_raft.c @@ -8832,6 +8832,184 @@ static void test_sim_7_2_negative_both_directions(void) * (broken_rc < 0, Log Matching) AND the correct build passes (correct_rc == 0). */ } +/* + * test_sim_apply_decoupling_apply_after_commit_ordering + * (raft-apply-decoupling task 7.1; Req 10.1) + * + * New conformance-harness assertion, layered on the existing apply-after- + * on_commit_advanced loop: no entry's state mutation/reaction (the harness's apply + * model, node->applied[]) may occur before that entry commits (node->commit_index + * reaching its index). Proven in BOTH directions within one test, mirroring the + * task 7.1/7.2 fault-injection idiom used above for the four raft.tla invariants: + * + * (1) CORRECT (fault OFF, the shipped sim_conf_on_commit_advanced adapter): + * a persisted-but-not-yet-committed entry is NOT applied; once replicated + * to a quorum and committed, it IS applied — raft_sim_check_apply_after_commit + * holds throughout. + * (2) BROKEN (RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT set): the SAME persist step + * eagerly marks the entry applied while commit_index is still 0 — the SAME + * check now FAILS, proving the assertion has real teeth, not a vacuous pass. + */ +static void test_sim_apply_decoupling_apply_after_commit_ordering(void) +{ + /* ---- (1) CORRECT: fault OFF — apply never precedes commit ---- */ + { + raft_sim_cluster_t c; + memset(&c, 0, sizeof(c)); + TEST_ASSERT_EQUAL_INT(0, raft_sim_conformance_init(&c, 3, 2000, 0xA0715ULL)); + + raft_start_election(&c.nodes[0].state); + for (int g = 0; g < 64 && c.bag_count > 0; g++) + raft_sim_bag_deliver(&c, 0); + TEST_ASSERT_EQUAL_INT(RAFT_LEADER, c.nodes[0].state.role); + + uint64_t value = 0x99AAULL; + uint64_t out_index = 0; + TEST_ASSERT_EQUAL_INT(RAFT_OK, + raft_log_append(&c.nodes[0].state, &value, sizeof(value), &out_index)); + TEST_ASSERT_EQUAL_UINT64(1, out_index); + + /* Persisted but not yet replicated/committed: nothing applied yet, and the + * ordering check holds (vacuously — nothing to violate it with). */ + TEST_ASSERT_EQUAL_UINT64(0, c.nodes[0].applied[0]); + TEST_ASSERT_EQUAL_UINT64(0, c.nodes[0].commit_index); + TEST_ASSERT_EQUAL_INT(1, raft_sim_check_apply_after_commit(&c)); + + /* Replicate to one follower (2 of 3 = quorum) and commit. */ + TEST_ASSERT_EQUAL_INT(RAFT_OK, + raft_send_append_entries(&c.nodes[0].state, 1, 0)); + for (int g = 0; g < 64 && c.bag_count > 0; g++) + raft_sim_bag_deliver(&c, 0); + raft_sim_advance_commit(&c, 0); + TEST_ASSERT_EQUAL_UINT64(1, c.nodes[0].commit_index); + + /* Now applied, and ordering still holds. */ + TEST_ASSERT_EQUAL_UINT64(value, c.nodes[0].applied[0]); + TEST_ASSERT_EQUAL_INT(1, raft_sim_check_apply_after_commit(&c)); + } + + /* ---- (2) BROKEN: RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT — persisted-but-uncommitted + * entry is nonetheless marked applied; the SAME check now fails. ---- */ + { + raft_sim_cluster_t c; + memset(&c, 0, sizeof(c)); + TEST_ASSERT_EQUAL_INT(0, raft_sim_conformance_init(&c, 3, 2000, 0xA0715ULL)); + raft_sim_set_fault_inject(&c, RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT); + TEST_ASSERT_EQUAL_UINT(RAFT_SIM_BREAK_APPLY_BEFORE_COMMIT, c.fault_inject); + + raft_start_election(&c.nodes[0].state); + for (int g = 0; g < 64 && c.bag_count > 0; g++) + raft_sim_bag_deliver(&c, 0); + TEST_ASSERT_EQUAL_INT(RAFT_LEADER, c.nodes[0].state.role); + + uint64_t value = 0x99AAULL; + uint64_t out_index = 0; + TEST_ASSERT_EQUAL_INT(RAFT_OK, + raft_log_append(&c.nodes[0].state, &value, sizeof(value), &out_index)); + TEST_ASSERT_EQUAL_UINT64(1, out_index); + + /* Never replicated, never committed — commit_index is still 0 ... */ + TEST_ASSERT_EQUAL_UINT64(0, c.nodes[0].commit_index); + /* ... yet the corrupted adapter already marked it applied. */ + TEST_ASSERT_EQUAL_UINT64(value, c.nodes[0].applied[0]); + + /* The oracle has teeth: this deliberately-broken build FAILS the SAME check + * that passed throughout the correct-code block above. */ + TEST_ASSERT_EQUAL_INT_MESSAGE(0, raft_sim_check_apply_after_commit(&c), + "BROKEN config (apply-before-commit) did not trip the ordering check — " + "the assertion lacks teeth"); + } +} + +/* + * test_sim_apply_decoupling_idempotent_reapply + * (raft-apply-decoupling task 7.1; Req 10.3) + * + * New conformance-harness assertion: re-applying an already-committed entry + * (whether a genuine duplicate on_commit_advanced fire or a forced + * raft_sim_replay_commit retry) is idempotent — its apply-time reaction fires + * exactly once per index, never twice. Proven in BOTH directions: + * + * (1) CORRECT (fault OFF): commit one entry, then force a REPLAY of the SAME + * commit event via raft_sim_replay_commit — applied[]/commit_index stay + * byte-identical and raft_sim_check_idempotent_reapply holds. + * (2) BROKEN (RAFT_SIM_BREAK_REAPPLY_GUARD set): the SAME replay re-executes + * the apply-time reaction a second time — the SAME check now FAILS. + */ +static void test_sim_apply_decoupling_idempotent_reapply(void) +{ + /* ---- (1) CORRECT: fault OFF — replay of an already-applied commit is a no-op ---- */ + { + raft_sim_cluster_t c; + memset(&c, 0, sizeof(c)); + TEST_ASSERT_EQUAL_INT(0, raft_sim_conformance_init(&c, 3, 2000, 0xB0715ULL)); + + raft_start_election(&c.nodes[0].state); + for (int g = 0; g < 64 && c.bag_count > 0; g++) + raft_sim_bag_deliver(&c, 0); + TEST_ASSERT_EQUAL_INT(RAFT_LEADER, c.nodes[0].state.role); + + uint64_t value = 0x77BBULL; + uint64_t out_index = 0; + TEST_ASSERT_EQUAL_INT(RAFT_OK, + raft_log_append(&c.nodes[0].state, &value, sizeof(value), &out_index)); + TEST_ASSERT_EQUAL_INT(RAFT_OK, + raft_send_append_entries(&c.nodes[0].state, 1, 0)); + for (int g = 0; g < 64 && c.bag_count > 0; g++) + raft_sim_bag_deliver(&c, 0); + raft_sim_advance_commit(&c, 0); + TEST_ASSERT_EQUAL_UINT64(1, c.nodes[0].commit_index); + TEST_ASSERT_EQUAL_UINT64(value, c.nodes[0].applied[0]); + TEST_ASSERT_EQUAL_INT(1, c.nodes[0].apply_reaction_count[0]); + TEST_ASSERT_EQUAL_INT(1, raft_sim_check_idempotent_reapply(&c)); + + /* Force a REPLAY of the already-processed commit (simulated retry/duplicate + * notification), bypassing the library's own fire-once gate. */ + raft_sim_replay_commit(&c, 0); + + /* No residual mutation: applied[]/commit_index unchanged, reaction still 1. */ + TEST_ASSERT_EQUAL_UINT64(1, c.nodes[0].commit_index); + TEST_ASSERT_EQUAL_UINT64(value, c.nodes[0].applied[0]); + TEST_ASSERT_EQUAL_INT(1, c.nodes[0].apply_reaction_count[0]); + TEST_ASSERT_EQUAL_INT(1, raft_sim_check_idempotent_reapply(&c)); + } + + /* ---- (2) BROKEN: RAFT_SIM_BREAK_REAPPLY_GUARD — replay re-fires the reaction ---- */ + { + raft_sim_cluster_t c; + memset(&c, 0, sizeof(c)); + TEST_ASSERT_EQUAL_INT(0, raft_sim_conformance_init(&c, 3, 2000, 0xB0715ULL)); + + raft_start_election(&c.nodes[0].state); + for (int g = 0; g < 64 && c.bag_count > 0; g++) + raft_sim_bag_deliver(&c, 0); + TEST_ASSERT_EQUAL_INT(RAFT_LEADER, c.nodes[0].state.role); + + uint64_t value = 0x77BBULL; + uint64_t out_index = 0; + TEST_ASSERT_EQUAL_INT(RAFT_OK, + raft_log_append(&c.nodes[0].state, &value, sizeof(value), &out_index)); + TEST_ASSERT_EQUAL_INT(RAFT_OK, + raft_send_append_entries(&c.nodes[0].state, 1, 0)); + for (int g = 0; g < 64 && c.bag_count > 0; g++) + raft_sim_bag_deliver(&c, 0); + raft_sim_advance_commit(&c, 0); + TEST_ASSERT_EQUAL_UINT64(1, c.nodes[0].commit_index); + TEST_ASSERT_EQUAL_INT(1, c.nodes[0].apply_reaction_count[0]); + TEST_ASSERT_EQUAL_INT(1, raft_sim_check_idempotent_reapply(&c)); + + /* NOW arm the fault and replay — the corrupted adapter re-fires the reaction. */ + raft_sim_set_fault_inject(&c, RAFT_SIM_BREAK_REAPPLY_GUARD); + TEST_ASSERT_EQUAL_UINT(RAFT_SIM_BREAK_REAPPLY_GUARD, c.fault_inject); + raft_sim_replay_commit(&c, 0); + + TEST_ASSERT_EQUAL_INT(2, c.nodes[0].apply_reaction_count[0]); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, raft_sim_check_idempotent_reapply(&c), + "BROKEN config (reapply guard disabled) did not trip the idempotency " + "check — the assertion lacks teeth"); + } +} + #define RAFT_SIM_SWEEP_M 32 /* configured minimum number of randomized seeds (R6.2) */ #define RAFT_SIM_SWEEP_STEPS 400 /* per-run step cap — bounded for the suite budget (R6.4) */ @@ -9129,6 +9307,8 @@ void test_raft(void) RUN_TEST(test_sim_5_5_stale_rejoin); RUN_TEST(test_sim_7_1_fault_switch_mechanism); RUN_TEST(test_sim_7_2_negative_both_directions); + RUN_TEST(test_sim_apply_decoupling_apply_after_commit_ordering); + RUN_TEST(test_sim_apply_decoupling_idempotent_reapply); RUN_TEST(test_sim_6_1_seed_sweep); /* Group A: basic election correctness (task 5.1) */