From 099ee1a21640573897895782cfa9d3e8883d2d73 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 4 Jul 2026 12:22:09 +0000 Subject: [PATCH] feat(raft-apply-decoupling): route DIVERGENT verdict through existing fence (task 5.2) Add bool apply_divergent to the existing recover_policy_inputs_t; persist_recover_decision() unconditionally short-circuits to the existing RECOVER_DECISION_FENCE_UNRECOVERABLE when true, reusing the same sticky recovery_unrecoverable flag and persist_recover_loud_fence_if_unrecoverable() -- no new fence mechanism, no ha.c changes needed (the existing promotion gate already reads the flag independently), no new self-termination path. Fixes a real bug found during review: the fence's operator-facing ERROR() log unconditionally asserted a "no healthy peer reachable... durable_floor_lsn < hwm" clause that was only truthful for the pre-existing structural-fence trigger. For the new apply_divergent trigger this produced literally false claims in test output ("100 < 100", "2 < 0"). Fixed by branching the log call itself on apply_divergent so it states only the verified reason. --- .kiro/specs/raft-apply-decoupling/tasks.md | 3 +- src/controller/persist.c | 100 +++++- .../test_apply_correctness_verification.c | 310 ++++++++++++++++++ src/include/persist.h | 28 +- 4 files changed, 417 insertions(+), 24 deletions(-) diff --git a/.kiro/specs/raft-apply-decoupling/tasks.md b/.kiro/specs/raft-apply-decoupling/tasks.md index 2c390a2f..168fc46e 100644 --- a/.kiro/specs/raft-apply-decoupling/tasks.md +++ b/.kiro/specs/raft-apply-decoupling/tasks.md @@ -115,7 +115,7 @@ - _Depends: 2.3_ - _Boundary: Restart Replay & Verification — persist.c replay/recovery path only_ -- [ ] 5.2 Route a verification mismatch through the existing non-promotable fence mechanism +- [x] 5.2 Route a verification mismatch through the existing non-promotable fence mechanism - Reuse the existing loud-fence function and sticky fence flag rather than introducing a new terminal or fencing path - Observable: an injected materialization mismatch produces a DIVERGENT verdict, a loud operator-visible log entry, and a non-promotable fenced state - _Requirements: 8.3, 8.4_ @@ -193,6 +193,7 @@ - **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. - **1.3**: added a dedicated per-commit-engine sweep thread (not the shared `delaymgr` — would violate boundary + delaymgr's 200ms poll granularity vs. `DEFAULT_SYNC_TIMEOUT_MS`=500ms would add timing jitter). Race rule: majority-satisfied always wins over an already-expired deadline when both are true under the same lock hold. **Round 1 REJECTED**: `commit_sweep_once()` computed the next-deadline snapshot under `ce->lock`, unlocked to dispatch, then slept on the *stale* snapshot — a `commit_submit_async()` registration/broadcast landing during that unlocked dispatch window was lost (no thread parked on the condvar to receive it), so the sweep thread could sleep on an absent/late deadline indefinitely (worst case: unbounded delay in a low-traffic quorum-loss scenario, violating the "identical failure timing" no-regression bar). **Fixed** by holding `ce->lock` continuously across scan→wait decisions (releasing only for the unlocked dispatch step) and always re-scanning under the lock immediately before ever calling `cond_wait`/`cond_timedwait` — the standard recheck-then-wait idiom. Round 2 reviewer independently reproduced the round-1 bug shape to confirm the fix actually closes it, not just narrows it. **Precedent for 1.4**: any future code that needs to interrupt the sweep thread's wait (e.g., a demotion/truncation handler resolving a pending LSN out-of-band) must go through the same lock+broadcast discipline `commit_submit_async()` uses, or it risks the identical lost-wakeup class of bug. - **1.4**: added `commit_resolve_pending_above(ce, lsn_floor, rc)` (snapshot-then-`commit_resolve()`-per-entry, no parallel bookkeeping). Truncation: a single new `on_truncate`/`truncate_cb_ctx` callback slot on `persist_t` (same inversion-of-control shape as the existing `on_durable_advance` hook) fires from `persist_truncate_wal_above` after releasing `p->lock`, only when a truncation actually removed entries — this ONE wrapper covers all three real truncation call sites (`raft_cb_become_leader`'s post-election truncate, the library's `on_truncate_from` conflict-reconcile, `ha_demotion_convergence_fn`'s post-demotion truncate), verified by the reviewer reading all three. Demotion: `raft_cb_become_follower` calls `commit_resolve_pending_above(&c->commit, 0, -XCE_ERR_STANDBY)` (lsn_floor=0 → fails everything — a demoted node can never authoritatively confirm anything pending; verified against the CP posture). **Known residual, non-blocking**: a fourth internal truncation path (`persist.c` inside `persist_wal_write_replicated`'s tip-conflict-repair branch, STANDBY replicated-apply only) calls `truncate_wal_above_locked()` directly, bypassing the new wrapper — safe today only because demotion resolves ALL pending continuations before role flips to STANDBY (so nothing can still be registered by the time that path runs), but this is an *implicit* ordering invariant, not an asserted one. Flag for whoever next touches the STANDBY replicated-apply path: add a defensive comment/assertion there, or route it through the wrapper too, before relying on it further. Also: bug-1407 was believed closed at this point (70/70 across 3 runs) — **see 1.5's note below, this was a false reading, not a real fix**. +- **5.2**: routed `APPLY_DIVERGENT` (task 5.1) into the EXISTING loud-fence mechanism as a new INPUT, not a new fencing path — added `bool apply_divergent` to the existing `recover_policy_inputs_t`; `persist_recover_decision()` now unconditionally short-circuits to the pre-existing `RECOVER_DECISION_FENCE_UNRECOVERABLE` enum value when it's true (correct and design-mandated: a divergent local materialization can't be trusted for a peer/catch-up comparison either, so it must always fence regardless of peer state); reuses the same sticky `recovery_unrecoverable` flag and the same `persist_recover_loud_fence_if_unrecoverable()` function — confirmed by both review rounds to be the identical pre-existing symbols, zero new fence mechanism. Confirmed the existing promotion-gate consultation in ha.c (~line 1078, `persist_recovery_unrecoverable(&c->persist)`) already independently reads this flag — zero ha.c changes needed. Zero new self-termination paths (verified by grep both rounds) — stays alive, keeps retrying, per the CLAUDE.md process-lifecycle invariant. Took 2 review rounds. **Round 1 REJECTED — real, test-reproduced defect, not documentation**: the operator-facing ERROR() log's fixed boilerplate clause ("no healthy peer reachable to catch up; durable_floor_lsn=X < cluster committed high-water hwm=Y") fired UNCONDITIONALLY for every fence decision, including the new apply_divergent trigger — but for that trigger the fields are either hardcoded placeholders or explicitly-set values that made the clause LITERALLY FALSE in test output ("100 < 100", "2 < 0" — numerically impossible claims presented as fact to an operator during a real incident). Before this task, `RECOVER_DECISION_FENCE_UNRECOVERABLE` was only reachable via the structural "no healthy peer" branch, so the clause was truthful by construction; the new unconditional short-circuit broke that invariant. This directly undermines Req 8.3's "loud, operator-visible" requirement — a log an operator can't trust during an incident is worse than no log. **Round 2 fixed** by branching the ERROR() call itself (not just the inner reason string) on `apply_divergent` — that branch now states only the real, verified reason with no peer/HWM claim; the structural-fence branch is byte-identical to before. New regression test uses a genuine (non-mocked) stdout-capture mechanism relying on `controller_unit_test` never calling `tlog_init()` (verified true across all ~85 files in the test binary — confirmed the vendored tinylog fallback path used is a real raw `write(STDOUT_FILENO,...)`, not a stub) — both review rounds independently reverted the fix and reproduced the exact false-clause failure before re-confirming GREEN. **Lesson, consistent with task 2.2's earlier pattern**: reusing an existing decision path correctly at the LOGIC level doesn't automatically make every SIDE EFFECT of that path (like a log message assuming context from the old, narrower set of triggers) still valid for a newly-added trigger — check accompanying log/error text for assumptions that were true only because of the old, narrower reachability, not just the returned enum/decision value itself. - **5.1**: added a new `apply_verdict_t` (`APPLY_CLEAN`/`APPLY_DIVERGENT`) computed alongside the EXISTING, untouched structural `recover_verdict_t` (CRC) verdict — a dual-check design: (1) an entity-count check comparing `WAL_JOB_SUBMIT` entries scanned during replay against the live `c->jobmgr` table's actual growth (`count_jobmgr_entries()`), genuinely independent since it queries live state rather than trusting `apply_committed_entry()`'s return code — verified real by both review rounds via a forced job-count-mismatch test; (2) a position check comparing an "expected" LSN against `p->last_applied_lsn` (new `_Atomic uint64_t` field, per design.md, forward-looking infrastructure for task 6's live pump). Took 2 review rounds. **Round 1 REJECTED — real, live-today bug, not documentation**: the position check's "expected" value was computed as `recovered_tip = current_lsn - 1` (the PHYSICAL WAL-scan tip, which `replay_wal()` advances for every entry including `WAL_CHECKPOINT` markers and unknown types) instead of the LOGICAL apply cursor. `persist_checkpoint()` durably fsyncs the checkpoint marker in one step, then persists the advanced `checkpoint_lsn` to the on-disk snapshot in a LATER step — a crash between those two steps (a real, unavoidable crash-safety window this codebase already engineers hard for, cf. bug-661/ha-commit-durability) leaves a trailing checkpoint marker as the highest-LSN entry on next boot, causing `recovered_tip` to exceed the true `last_applied_lsn` and falsely report `APPLY_DIVERGENT` on a perfectly clean recovery — a direct Req 8.2 violation, reachable TODAY on any node that crashes at exactly the wrong moment during a checkpoint. **Round 2 fixed** by reading `p->last_applied_lsn` directly (the value `replay_wal()` already correctly advances only for entries actually dispatched through `apply_committed_entry()`, skipping checkpoint-marker/unknown-type entries) instead of `recovered_tip`; `recovered_tip` itself is untouched and still correctly feeds `durable_lsn`, which does need the physical tip. New regression test genuinely reproduces the trigger condition (real `persist_checkpoint()`, then `unlink()`s the exact snapshot file it wrote, landing on-disk state in the identical place a real crash would) — both review rounds independently reverted the fix and reproduced the false-DIVERGENT failure before re-confirming GREEN. **Known, disclosed, accepted scope gap (not a defect, flagged for whoever extends this check)**: the entity-count check covers jobs only. Queues are correctly excluded (config-derived, `WAL_QUEUE_CTRL` never creates a new one, verified by both review rounds). **Workers are NOT covered but CAN diverge**: `worker_from_capnp()`/`restore_worker()` genuinely creates a new worker on a `WAL_WORKER_CTRL`/`HOOK_DETAIL_WORKER_JOIN` entry when absent from `workermgr` — a worker-creation divergence is currently invisible to this verification. Extend `persist_verify_apply_correctness()` with a worker-count check (mirroring `count_jobmgr_entries()`) if/when this needs to be closed. **Self-disclosed, accepted limitation**: because `replay_wal()`'s single-threaded caller in `persist_recover()` derives both the "expected" and "actual" position values from the same atomic within moments of each other on the same thread, the position check is diagnostically tautological FOR THIS SPECIFIC CALL SITE today (though the entity-count half remains genuinely independent) — this will change once task 6's live apply pump gives expectation and actual state a real chance to diverge across threads/time; task 5.1's boundary does not require solving that now. - **4.2**: pure test-only close-out of task-group 4 — one new test `test_fact_state_unchanged_throughout_quorum_loss_outage` in `test_commit_barrier_reply.c` (30→31), approved first round, zero production changes. Drives a genuine `repl_remove_standby`-based quorum-loss window through all 4 real fact-receipt handlers, polls live job state + `committed_lsn` at 5 time-spread points (real 20ms sleeps, ~100ms total) rather than once, asserts a real hook-spy never sees `AFTER_EVENT` fire, and confirms durable WAL presence via direct on-disk segment read (`cbrp_count_jobstate_entries`) — combining and strengthening two pre-existing tests that each covered only half (4.1's own single-point-in-time check under normal conditions; an older pre-4.1 test that proved WAL durability but never inspected live state). **Static/inspection half of Req 6.2 confirmed independently by the reviewer, re-reading 4.1's landed diff**: none of the four `record_*_fact` functions call `apply_committed_entry`/`restore_job`/mutate the real job object — `record_job_fail_fact` specifically operates on a throwaway `job_t scratch = *job` shallow copy. **Architecturally significant finding for task 6 planning**: facts have NO commit-timeout/failure concept at the receipt layer, unlike commands (task 3.2's `commit_submit_async`). `persist_submit_job_fact()` → `wal_hook_append_classified(WAL_MODE_RETAIN)` → `commit_submit(..., COMMIT_MODE_RETAIN, 0)`, and `COMMIT_MODE_RETAIN`'s branch in commit.c is an immediate, unconditional return with NO continuation registered (explicit comment: "RETAIN never honors the timeout") — `commit_resolve()` never fires for a retained fact. A fact's outage is therefore open-ended by construction; its eventual commit is discovered only when `commit_advance_locked()`/`commit_on_ack` notices `committed_lsn` can advance past it via the ACK-registry watermarks, purely. **Task 6's apply pump must rely entirely on `committed_lsn` advancement to discover newly-eligible retained facts — there is no per-fact signal to wait on.** Confirmed neither of 4.1's two flagged hazards (double-append, merge-staleness) is triggered by this test, since it never drives apply. **Task-group 4 (fact apply-after-commit split) is now complete**: 4.1 (receipt converted to durable-append-only, two critical hazards discovered and documented for task 6) → 4.2 (retention proven under sustained quorum loss, fact-vs-command architectural asymmetry documented). **Both task-groups 1-4 are now done; task 6 (ordered apply pump) is the critical next milestone that must resolve, before going live: the double-append hazard, the merge-staleness hazard, AND design around facts' lack of a per-entry commit signal (commit_advance/committed_lsn-driven discovery only).** Groups 5 (restart verification) and 6 (apply pump) remain; 5.1 depends only on 2.3 (already done) so could run in parallel with 6, per tasks.md's dependency graph — continuing in numeric order (5 next) unless reprioritized. - **4.1**: converted the four worker-fact receipt handlers (`on_job_start`, `on_job_finish`, `on_job_fail`, `on_job_postdone` — NOT literally named `on_job_update`, task text's naming was approximate) to validate-and-durably-append-only: BEFORE_EVENT validation hook unchanged, AFTER_EVENT (post-mutation) hook no longer fires at receipt, live state mutation removed. **Key discovery**: the durable WAL append was NOT a separate call — it lived INSIDE the very `persist_job_hook_fun` AFTER_EVENT subscriber this task needed to silence, so a new `persist_submit_job_fact()` (persist.c) was added that calls the same unmodified `wal_hook_append_classified(WAL_MODE_RETAIN, wal_jobstate_t{jobid,state}, ...)` directly — verified wire-format-identical to before, `apply_committed_entry()`'s existing switch (task 2.2) routes it unchanged. `mark_job_start`/`mark_job_finish` (single caller each, verified via diff against 41d3620) renamed in place to `record_job_start_fact`/`record_job_finish_fact`; `mark_job_fail`/`mark_job_update` (shared with `job_ctrl`'s OUT-OF-BOUNDARY client-command path, 2 and 3 additional call sites) left untouched, with new durable-append-only siblings `record_job_fail_fact`/`record_job_update_fact` added instead — `job_ctrl`/`do_jobcontrol_op` confirmed byte-identical to pre-task by the reviewer. Approved first round. **Real, permanent regression accepted, not fixed (Req 5.1 required the removal)**: `on_job_postdone`'s `finish_time = time(NULL)` anomaly backstop is gone with no wire-format-level equivalent — concretely, a job hitting this rare no-prior-exit-status path now has `finish_time` stuck at 0 forever, which `job_should_retain()` (persist.c) evaluates as immediately GC-eligible instead of retained for the configured window. Narrow, rare-path, but real. **TWO CRITICAL HAZARDS DISCOVERED BY REVIEW, MUST BE RESOLVED BEFORE TASK 6 WIRES apply_committed_entry() INTO A LIVE PATH — currently inert only because replay runs before persist_start() registers hooks (cluster.c: persist_recover() at line ~1451 vs persist_start() at ~1694)**: diff --git a/src/controller/persist.c b/src/controller/persist.c index 77c80521..e9bbca6b 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -3311,7 +3311,7 @@ int persist_recover(persist_t* p) }; p->apply_verdict = persist_verify_apply_correctness( p, &apply_exp, jobmgr_count_before_replay); - if (p->apply_verdict == APPLY_DIVERGENT) + if (p->apply_verdict == APPLY_DIVERGENT) { ERROR("persist_recover: APPLY_DIVERGENT — materialized state does " "not match the committed log tip (expected last_applied_lsn=%" PRIu64 " new_job_count=%" PRIu64 "); a hidden apply defect is " @@ -3320,7 +3320,32 @@ int persist_recover(persist_t* p) apply_exp.expected_new_job_count, (verdict == RECOVER_TRUNCATED) ? "RECOVER_TRUNCATED" : "RECOVER_CLEAN"); - else + + /* raft-apply-decoupling task 5.2 (Req 8.3/8.4): route the DIVERGENT + * verdict through the EXISTING non-promotable loud-fence mechanism + * (persist_recover_loud_fence_if_unrecoverable + the sticky + * recovery_unrecoverable flag) instead of a new fence path. + * apply_divergent=true fences UNCONDITIONALLY (see + * persist_recover_decision), so the peer/HWM fields below are + * inert placeholders here -- they are not yet known at this + * boot-time replay call site (unlike the structural verdict's own + * fence check, which stays deferred to promotion time in ha.c, + * where live peer/HWM context exists). verdict/recovered_tip are + * passed through only for accurate, consistent logging. The member + * stays ALIVE, non-promotable, and keeps retrying catch-up -- it + * never exits (controller-never-self-terminate invariant, + * CLAUDE.md). */ + recover_policy_inputs_t apply_fence_in = { + .verdict = verdict, + .durable_below_hwm = false, + .healthy_peer_reachable = false, + .catch_up_succeeded = false, + .durable_floor_lsn = recovered_tip, + .cluster_hwm_lsn = 0, + .apply_divergent = true, + }; + persist_recover_loud_fence_if_unrecoverable(p, &apply_fence_in); + } else INFO("persist_recover: APPLY_CLEAN — materialized state matches the " "committed log tip (last_applied_lsn=%" PRIu64 ")", apply_cursor); @@ -3366,6 +3391,20 @@ recover_decision_t persist_recover_decision(const recover_policy_inputs_t* in) if (in == NULL) return RECOVER_DECISION_ELIGIBLE; /* defensive: nothing to fence */ + /* raft-apply-decoupling task 5.2 (Req 8.3/8.4): an APPLY_DIVERGENT + * post-replay verdict is a NEW INPUT to this same decision table, not a + * new fence mechanism (design.md "Restart Replay & Verification"). It + * fences UNCONDITIONALLY -- every other field below is irrelevant once + * this is true. Unlike a structurally truncated/behind log (rows 2/3/5), + * where a healthy peer's missing entries can repair the gap and legitimately + * downgrade the outcome to a plain FENCE-then-catch-up, an apply-time + * defect means the LOCAL apply path itself produced the wrong + * materialized state from bytes it already had -- streaming more entries + * from a peer does not prove that path is now trustworthy, so this is + * never downgraded by peer reachability or catch-up success. */ + if (in->apply_divergent) + return RECOVER_DECISION_FENCE_UNRECOVERABLE; + bool truncated = (in->verdict == RECOVER_TRUNCATED); /* Rows 1 & 6: a structurally CLEAN log that is NOT behind the cluster @@ -3430,22 +3469,47 @@ persist_recover_loud_fence_if_unrecoverable(persist_t* p, if (decision != RECOVER_DECISION_FENCE_UNRECOVERABLE) return decision; - /* Loud, operator-visible alert: log the truncation boundary - * (durable_floor_lsn) and the cluster high-water mark so an operator sees - * exactly where the local log stops short of the cluster's committed state. - * The member STAYS ALIVE, stays non-promotable, and keeps retrying catch-up - * (Req 2.4) — it NEVER self-exits. */ - const char* why = (in->verdict == RECOVER_TRUNCATED) - ? "recovery-truncated log (structural corruption)" - : "log behind the cluster committed high-water mark"; - ERROR("persist_recover: UNRECOVERABLE — %s and no healthy peer reachable to " - "catch up; truncation boundary durable_floor_lsn=%" PRIu64 - " < cluster committed high-water hwm=%" PRIu64 ". Staying ALIVE and " - "PERMANENTLY non-promotable (refusing to serve divergent state) and " - "RETRYING catch-up indefinitely — will self-heal when a healthy peer " - "returns; operator attention may be needed " - "(ha-commit-durability Req 2.4)", - why, in->durable_floor_lsn, in->cluster_hwm_lsn); + /* Loud, operator-visible alert. The two triggers reaching this terminal + * are NOT logged with the same clause (round-2 fix, raft-apply- + * decoupling task 5.2 remediation): + * + * - apply_divergent: fences UNCONDITIONALLY (persist_recover_decision), + * evaluated at boot-replay time BEFORE peer/HWM context exists (see + * the call site in persist_recover()). `healthy_peer_reachable` and + * `cluster_hwm_lsn` are inert placeholders there, not real facts — + * asserting "no healthy peer reachable" or a durable_floor_lsn < hwm + * comparison for this trigger would report unevaluated-or-false + * information to the operator (round-1 review: the placeholders can + * make the comparison numerically impossible, e.g. "2 < 0"). This + * branch names the real reason (apply-correctness divergence) only. + * - structural (truncated / behind-HWM): reaches this terminal only + * via the pre-existing "no healthy peer reachable" branch in + * persist_recover_decision, so the peer-reachability and + * durable_floor_lsn/hwm comparison ARE guaranteed true here — that + * clause is unchanged from before this task. + */ + if (in->apply_divergent) { + ERROR("persist_recover: UNRECOVERABLE — post-replay apply-correctness " + "verification is DIVERGENT (materialized state does not match " + "the committed log tip, raft-apply-decoupling Req 8.3); a local " + "apply defect cannot be proven fixed by peer reachability or " + "catch-up alone. Staying ALIVE and PERMANENTLY non-promotable " + "(refusing to serve divergent state) and RETRYING catch-up " + "indefinitely — operator attention is needed to diagnose the " + "apply path (raft-apply-decoupling Req 8.3/8.4)"); + } else { + const char* why = (in->verdict == RECOVER_TRUNCATED) + ? "recovery-truncated log (structural corruption)" + : "log behind the cluster committed high-water mark"; + ERROR("persist_recover: UNRECOVERABLE — %s and no healthy peer reachable to " + "catch up; truncation boundary durable_floor_lsn=%" PRIu64 + " < cluster committed high-water hwm=%" PRIu64 ". Staying ALIVE and " + "PERMANENTLY non-promotable (refusing to serve divergent state) and " + "RETRYING catch-up indefinitely — will self-heal when a healthy peer " + "returns; operator attention may be needed " + "(ha-commit-durability Req 2.4)", + why, in->durable_floor_lsn, in->cluster_hwm_lsn); + } /* Set the sticky non-promotable flag so the promotion gate refuses * leadership as belt-and-suspenders until catch-up clears it (Req 2.4/2.6). diff --git a/src/controller/testsuite/test_apply_correctness_verification.c b/src/controller/testsuite/test_apply_correctness_verification.c index a32aa847..62db9eeb 100644 --- a/src/controller/testsuite/test_apply_correctness_verification.c +++ b/src/controller/testsuite/test_apply_correctness_verification.c @@ -508,10 +508,320 @@ static void test_verify_apply_correctness_clean_when_no_cluster(void) printf("test_verify_apply_correctness_clean_when_no_cluster: PASS\n"); } +/* ── raft-apply-decoupling task 5.2 (Req 8.3/8.4): route an APPLY_DIVERGENT + * verdict through the EXISTING non-promotable loud-fence mechanism + * (persist_recover_decision / persist_recover_loud_fence_if_unrecoverable + * / the recovery_unrecoverable sticky flag) rather than a new fence path. + * ────────────────────────────────────────────────────────────────────── */ + +/* + * An APPLY_DIVERGENT verdict is a NEW INPUT to the EXISTING Boot Policy + * Decision Table (persist_recover_decision), not a new decision path. Prove + * apply_divergent fences UNCONDITIONALLY: even when every OTHER input looks + * perfectly healthy (structurally CLEAN, at/above the cluster HWM, a healthy + * peer reachable, catch-up already succeeded — exactly the combination that + * otherwise decides ELIGIBLE), apply_divergent=true alone must still force + * RECOVER_DECISION_FENCE_UNRECOVERABLE. Unlike a structurally truncated or + * behind log, a healthy peer's missing entries do not prove a LOCAL apply + * defect is fixed, so this input is never downgraded to a plain FENCE the + * way truncated-with-a-peer is (rows 2/3/5) — it is not something catch-up + * alone resolves. + */ +static void test_apply_divergent_decision_forces_unconditional_fence(void) +{ + recover_policy_inputs_t healthy_but_divergent = { + .verdict = RECOVER_CLEAN, + .durable_below_hwm = false, + .healthy_peer_reachable = true, + .catch_up_succeeded = true, + .durable_floor_lsn = 42, + .cluster_hwm_lsn = 42, + .apply_divergent = true, + }; + TEST_ASSERT_EQUAL_INT_MESSAGE( + RECOVER_DECISION_FENCE_UNRECOVERABLE, + persist_recover_decision(&healthy_but_divergent), + "an APPLY_DIVERGENT verdict must fence UNCONDITIONALLY, even when " + "every structural/HWM/peer/catch-up input looks eligible"); + + /* Sanity: flipping apply_divergent back to false with the SAME other + * fields reverts to the pre-existing ELIGIBLE outcome -- confirms this + * is an ADDITIVE input, not a change to the pre-5.2 decision table for + * every other combination. */ + recover_policy_inputs_t healthy_and_clean = healthy_but_divergent; + healthy_and_clean.apply_divergent = false; + TEST_ASSERT_EQUAL_INT_MESSAGE( + RECOVER_DECISION_ELIGIBLE, + persist_recover_decision(&healthy_and_clean), + "sanity: without apply_divergent this exact combination is " + "unchanged from the pre-5.2 ELIGIBLE outcome"); + + printf("test_apply_divergent_decision_forces_unconditional_fence: PASS\n"); +} + +/* + * The ACTION helper (persist_recover_loud_fence_if_unrecoverable) must react + * to apply_divergent exactly like the pre-existing structural-corruption + * terminal: log LOUDLY, set the SAME sticky recovery_unrecoverable flag (no + * second/parallel flag), and RETURN -- the test simply continuing past the + * call is the proof the process stayed alive (no abort()/exit() anywhere on + * this path), mirroring test_boot_policy_row4_truncated_no_peer_loud_fence's + * verification style (test_commit_durability.c) -- the existing template + * for this exact assertion shape. + */ +static void test_apply_divergent_loud_fence_sets_sticky_flag_and_stays_alive(void) +{ + persist_t p; + memset(&p, 0, sizeof(p)); + + recover_policy_inputs_t in = { + .verdict = RECOVER_CLEAN, + .durable_below_hwm = false, + .healthy_peer_reachable = true, + .catch_up_succeeded = true, + .durable_floor_lsn = 100, + .cluster_hwm_lsn = 100, + .apply_divergent = true, + }; + + TEST_ASSERT_FALSE_MESSAGE( + persist_recovery_unrecoverable(&p), + "sanity: the sticky flag starts clear on a fresh persist_t"); + + /* Logs LOUDLY, sets the sticky flag, and RETURNS -- reaching the + * assertions below IS the proof the process stayed alive. */ + recover_decision_t d = persist_recover_loud_fence_if_unrecoverable(&p, &in); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + RECOVER_DECISION_FENCE_UNRECOVERABLE, d, + "an APPLY_DIVERGENT input must decide the loud, non-terminating " + "fence -- not process exit"); + TEST_ASSERT_TRUE_MESSAGE( + persist_recovery_unrecoverable(&p), + "an APPLY_DIVERGENT verdict must set the SAME sticky " + "recovery_unrecoverable flag the structural-corruption case uses -- " + "not a second, parallel fence flag"); + + printf("test_apply_divergent_loud_fence_sets_sticky_flag_and_stays_alive: " + "PASS\n"); +} + +/* + * End-to-end: a REAL materialization mismatch produced by an actual + * persist_recover() replay (not the pure functions tested above) must ITSELF + * set the sticky non-promotable fence, with no test-only shortcut faking the + * verdict. + * + * Forces a genuine mismatch: replay_wal's new_job_submits_out counts every + * RAW WAL_JOB_SUBMIT header it scans (persist.c), unconditionally -- it does + * NOT know whether apply_committed_entry()'s restore_job() actually created a + * new job or silently no-op'd on a duplicate jobid (restore_job is + * idempotent, Req 7.1). Appending the identical jobid twice therefore drives + * new_job_submits=2 while the live jobmgr table only grows by 1 -- a real, + * reproducible APPLY_DIVERGENT with nothing about + * persist_verify_apply_correctness() itself mocked or shortcut. + */ +static void test_real_replay_divergence_sets_unrecoverable_fence(void) +{ + char tmp[256]; + strcpy(tmp, "/tmp/xce_tacv_divergent_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(tmp)); + + /* Phase 1: append the SAME jobid twice as two separate WAL_JOB_SUBMIT + * entries -- a real replay counts both, but restore_job() only + * materializes the job once. */ + { + persist_t* p = tacv_make_fresh_persist(tmp); + TEST_ASSERT_NOT_NULL(p); + + for (int i = 0; i < 2; i++) { + xce_job_t jdef; + memset(&jdef, 0, sizeof(jdef)); + jdef.jobid = 72001; + jdef.state = JOB_STAT_PEND; + + job_submit_data_t submit; + memset(&submit, 0, sizeof(submit)); + submit.queue = (char*)"rq1"; + submit.user = (char*)"tester"; + jdef.submit = &submit; + + uint8_t* payload = NULL; + size_t plen = 0; + TEST_ASSERT_EQUAL_INT(0, tacv_encode_job(&jdef, &payload, &plen)); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(p, WAL_JOB_SUBMIT, payload, (uint32_t)plen)); + free(payload); + } + + TEST_ASSERT_EQUAL_INT(0, persist_flush(p)); + tacv_teardown_persist(p); + } + + /* Phase 2: fresh persist_t + fresh cluster_t, recover from disk. The + * duplicate-jobid replay must produce a genuine APPLY_DIVERGENT verdict, + * which persist_recover() must now route through the existing + * non-promotable loud-fence mechanism (task 5.2). */ + { + persist_t* p2 = tacv_make_fresh_persist(tmp); + TEST_ASSERT_NOT_NULL(p2); + + tacv_build_cluster(); + p2->cluster = &g_tacv_c; + + TEST_ASSERT_FALSE_MESSAGE( + persist_recovery_unrecoverable(p2), + "sanity: the sticky flag starts clear before recovery runs"); + + /* persist_recover() itself must RETURN normally (0) -- no + * abort()/exit() anywhere on this path (controller-never-self- + * terminate invariant). Reaching the assertions below is itself + * part of the proof the process stayed alive. */ + TEST_ASSERT_EQUAL_INT(0, persist_recover(p2)); + + TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_DIVERGENT, + persist_apply_verdict(p2), + "fixture sanity: the duplicate-jobid replay must genuinely " + "diverge (new_job_submits=2, jobmgr only grew by 1)"); + + TEST_ASSERT_TRUE_MESSAGE( + persist_recovery_unrecoverable(p2), + "a real APPLY_DIVERGENT verdict from persist_recover() must set " + "the sticky non-promotable fence via the existing loud-fence " + "mechanism (Req 8.4)"); + + tacv_teardown_cluster(); + tacv_teardown_persist(p2); + } + + char cmd[512]; + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp); + if (system(cmd) != 0) + fprintf(stderr, "cleanup warning: rm -rf %s failed\n", tmp); + + printf("test_real_replay_divergence_sets_unrecoverable_fence: PASS\n"); +} + +/* + * raft-apply-decoupling task 5.2 round 2 (Req 8.3 remediation): the + * apply_divergent ERROR log must NOT append the shared structural-fence + * boilerplate clause ("...and no healthy peer reachable to catch up; + * truncation boundary durable_floor_lsn=X < cluster committed high-water + * hwm=Y...") -- round-1 review found that clause is only guaranteed true for + * the pre-existing structural-fence rows (reached exclusively via the + * "no healthy peer" branch in persist_recover_decision). apply_divergent + * fences UNCONDITIONALLY, evaluated at boot-replay time before peer/HWM + * context exists, so `healthy_peer_reachable`/`cluster_hwm_lsn` are inert + * placeholders there -- emitting that clause reports unevaluated-or-false + * facts to the operator (round-1 repro: "durable_floor_lsn=2 < ... hwm=0", + * a numerically impossible claim). + * + * This test picks fixture inputs that make the false clause easy to catch if + * regressed -- a peer that WAS reachable (true) and durable_floor_lsn (100) + * NOT below cluster_hwm_lsn (100), the exact combination + * test_apply_divergent_loud_fence_sets_sticky_flag_and_stays_alive uses -- + * and asserts on the REAL ERROR() text. + * + * Log capture mechanism: this test binary (controller_unit_test) never calls + * mylog_init()/tlog_init() -- that only happens in the real daemon main()s + * (controller/main.c, scheduler/main.c) and the xce_test client tool's + * main(), never in controller_test.c. So tlog's internal root log handle + * stays NULL for this whole process, and every ERROR()/INFO() call falls + * through tlog's early-print path, which writes the fully formatted line + * directly to STDOUT_FILENO via a raw write() (vendored tinylog, tlog.c). + * Redirecting STDOUT_FILENO for the duration of the call under test + * therefore captures the EXACT text an operator would see on their + * terminal/log -- nothing about persist_recover_loud_fence_if_unrecoverable + * or ERROR() itself is mocked or shortcut. There is no pre-existing + * log-content-assertion helper elsewhere in this test file or its siblings + * (test_ha_startup.c/test_ha_role_lifecycle.c/test_commit_barrier_rollout.c + * only redirect a *forked child's* fds to /dev/null to silence noise, never + * capture-and-inspect content in-process) -- this is a new, self-contained, + * one-test-local mechanism, not a shared harness change. + */ +static void test_apply_divergent_loud_fence_log_omits_false_peer_hwm_clause(void) +{ + persist_t p; + memset(&p, 0, sizeof(p)); + + recover_policy_inputs_t in = { + .verdict = RECOVER_CLEAN, + .durable_below_hwm = false, + .healthy_peer_reachable = true, + .catch_up_succeeded = true, + .durable_floor_lsn = 100, + .cluster_hwm_lsn = 100, + .apply_divergent = true, + }; + + char tmp[] = "/tmp/xce_tacv_logcap_XXXXXX"; + int fd = mkstemp(tmp); + TEST_ASSERT_TRUE_MESSAGE(fd >= 0, + "fixture: mkstemp for stdout capture must succeed"); + + fflush(stdout); + int saved_stdout = dup(STDOUT_FILENO); + TEST_ASSERT_TRUE_MESSAGE(saved_stdout >= 0, + "fixture: dup(STDOUT_FILENO) must succeed"); + TEST_ASSERT_TRUE_MESSAGE(dup2(fd, STDOUT_FILENO) >= 0, + "fixture: dup2 redirect of STDOUT_FILENO must succeed"); + + recover_decision_t d = persist_recover_loud_fence_if_unrecoverable(&p, &in); + + fflush(stdout); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdout); + + TEST_ASSERT_EQUAL_INT_MESSAGE(RECOVER_DECISION_FENCE_UNRECOVERABLE, d, + "sanity: fixture inputs must still decide the unrecoverable fence"); + + off_t len = lseek(fd, 0, SEEK_CUR); + TEST_ASSERT_TRUE_MESSAGE(len > 0, + "fixture: the ERROR() call must have produced captured output"); + + char* buf = calloc(1, (size_t)len + 1); + TEST_ASSERT_NOT_NULL(buf); + TEST_ASSERT_TRUE_MESSAGE(lseek(fd, 0, SEEK_SET) == 0, + "fixture: rewind of the capture file must succeed"); + ssize_t nread = read(fd, buf, (size_t)len); + TEST_ASSERT_TRUE_MESSAGE(nread == len, + "fixture: reading back the full captured buffer must succeed"); + close(fd); + unlink(tmp); + + TEST_ASSERT_NULL_MESSAGE(strstr(buf, "no healthy peer reachable"), + "the apply_divergent fence log must not claim no healthy peer was " + "reachable -- healthy_peer_reachable is an unevaluated placeholder " + "for this trigger, and was actually true in this fixture"); + TEST_ASSERT_NULL_MESSAGE( + strstr(buf, "durable_floor_lsn=100 < cluster committed high-water " + "hwm=100"), + "the apply_divergent fence log must not assert a false/unevaluated " + "durable_floor_lsn < hwm comparison"); + + /* Positive: the log must still be loud, operator-visible, and name the + * real reason accurately (Req 8.3's "loud, operator-visible" mandate is + * a requirement to satisfy, not just a clause to strip). */ + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(buf, "UNRECOVERABLE"), + "the fence log must still be loud/operator-visible"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(buf, "apply-correctness verification is DIVERGENT"), + "the fence log must still accurately name the apply-divergence " + "reason"); + + free(buf); + printf("test_apply_divergent_loud_fence_log_omits_false_peer_hwm_clause: " + "PASS\n"); +} + void test_apply_correctness_verification(void) { RUN_TEST(test_clean_replay_produces_apply_clean_alongside_crc_verdict); RUN_TEST(test_checkpoint_tail_with_no_snapshot_is_apply_clean); RUN_TEST(test_verify_apply_correctness_detects_job_count_mismatch); RUN_TEST(test_verify_apply_correctness_clean_when_no_cluster); + RUN_TEST(test_apply_divergent_decision_forces_unconditional_fence); + RUN_TEST(test_apply_divergent_loud_fence_sets_sticky_flag_and_stays_alive); + RUN_TEST(test_real_replay_divergence_sets_unrecoverable_fence); + RUN_TEST(test_apply_divergent_loud_fence_log_omits_false_peer_hwm_clause); } diff --git a/src/include/persist.h b/src/include/persist.h index 7366da70..523617c3 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -85,6 +85,21 @@ typedef struct { * when a catch-up was attempted) */ uint64_t durable_floor_lsn; /* highest intact replayed LSN (truncation boundary) */ uint64_t cluster_hwm_lsn; /* cluster committed high-water mark, for logging */ + + /* raft-apply-decoupling task 5.2 (Req 8.3/8.4): the post-replay + * apply_verdict_t computed a semantic mismatch (APPLY_DIVERGENT) — the + * materialized state does not match the committed log tip even though + * the WAL bytes replayed cleanly. This is a NEW INPUT to the existing + * Boot Policy Decision Table, not a new fence mechanism: unlike a + * structurally truncated/behind log (which a healthy peer's missing + * entries can repair), an apply-time defect is not something streaming + * more entries from a peer proves fixed, so persist_recover_decision() + * treats this field as an UNCONDITIONAL fence trigger — set it true and + * every other field in this struct becomes irrelevant to the outcome + * (see persist_recover_decision's implementation). Defaults to false + * (an ordinary structural-only evaluation) when left unset by a + * designated initializer, matching every pre-existing call site. */ + bool apply_divergent; } recover_policy_inputs_t; /* ============================================================ @@ -108,9 +123,11 @@ typedef enum { * expectation (Req 8.2). */ APPLY_DIVERGENT = 1, /* materialized state does NOT match the log-derived * expectation (Req 8.3) — a hidden apply defect. - * Routing this into the existing non-promotable - * fence mechanism is raft-apply-decoupling task 5.2; - * this type only classifies, it does not fence. */ + * Routed into the existing non-promotable loud-fence + * mechanism via recover_policy_inputs_t.apply_divergent + * (raft-apply-decoupling task 5.2, Req 8.4) — this + * type itself still only classifies; persist_recover() + * is what acts on it. */ } apply_verdict_t; /* Lightweight post-replay expectation, derived purely from scanning the @@ -340,8 +357,9 @@ struct persist_s { * defaults to APPLY_CLEAN before recovery runs, mirroring * recovery_verdict's default-clean convention. Written single-threaded * during recovery; read by persist_apply_verdict/persist_last_applied_lsn - * thereafter (task 5.2 will additionally consume apply_verdict to route a - * DIVERGENT result into the existing loud-fence mechanism). */ + * thereafter. A DIVERGENT result is additionally routed by persist_recover() + * into the existing loud-fence mechanism (task 5.2, Req 8.4) via + * recover_policy_inputs_t.apply_divergent. */ _Atomic uint64_t last_applied_lsn; apply_verdict_t apply_verdict;