fix(raft-apply-decoupling): prevent apply-divergence fence from being live-cleared (task 5.3)

Add explicit test coverage for the DIVERGENT fence's stay-alive and
restart-only-clear guarantees (task 5.3), which surfaced a critical,
pre-existing safety gap: the sticky recovery_unrecoverable flag is
trigger-agnostic, and two pre-existing live call sites in ha.c
(promotion_fence_timeout_fn's periodic timer, and
raft_cb_become_leader's promotion-eligibility gate) clear it based
purely on structural conditions, never checking the apply verdict.

A node whose local apply dispatch diverges from the committed log,
but whose WAL bytes are otherwise structurally healthy, would have
its fence silently released by the live timer, then be eligible for
promotion to primary on a later election -- serving genuinely
divergent state.

Fix: gate both clear-call-sites on persist_apply_verdict(&c->persist)
!= APPLY_DIVERGENT in addition to their existing structural checks.
An apply-divergence-triggered fence can now only clear via a fresh
process restart's clean persist_recover() replay.

New tests: test_promotion_fence_apply_divergent_survives_structural_catchup
(test_commit_durability.c) covers the timer site; and
test_promotion_refused_when_apply_divergent_even_if_eligible
(test_convergence_verification.c) covers the promotion-gate site,
structurally isolated so it cannot pass for the wrong reason.

Closes out task-group 5 (restart apply-correctness verification).

.kiro/specs/raft-apply-decoupling/tasks.md

@@ -106,7 +106,7 @@
   - _Requirements: 6.1, 6.2_
   - _Depends: 4.1_
 
-- [ ] 5. Restart apply-correctness verification
+- [x] 5. Restart apply-correctness verification
 - [x] 5.1 (P) Compute a post-replay expectation from the committed log tip and compare it against materialized state
   - Derive a lightweight expectation (e.g., last-applied position and a count or checksum of materialized entities) from the recovered log
   - Compare the expectation against the actual state produced by replay through the shared apply dispatch function
@@ -121,7 +121,7 @@
   - _Requirements: 8.3, 8.4_
   - _Depends: 5.1_
 
-- [ ] 5.3 Unit test the DIVERGENT verdict path stays alive and keeps retrying
+- [x] 5.3 Unit test the DIVERGENT verdict path stays alive and keeps retrying
   - Force a DIVERGENT verdict and confirm the process does not exit or abort
   - Confirm the fence clears once a subsequent recovery reaches a consistent state
   - Observable: the test process remains running and retrying after a forced DIVERGENT verdict, never terminating
@@ -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.3**: closes task-group 5. Took 3 review rounds — the heaviest task in the entire series, and the only one that surfaced a genuine CRITICAL production safety bug (not a doc/log issue like 5.1/5.2's rounds). **Round 1 (test-only) REJECTED**: while writing tests for "the fence stays alive" and "the fence clears on a subsequent consistent recovery," review discovered the sticky `recovery_unrecoverable` flag is trigger-agnostic — it has ZERO provenance about WHY it was set. Task 5.2 correctly reused this flag for the new `apply_divergent` trigger, but TWO PRE-EXISTING, live, running-process call sites in ha.c already clear this SAME flag based purely on STRUCTURAL conditions (never checking the apply verdict): `promotion_fence_timeout_fn` (a self-re-arming periodic timer) and the promotion-eligibility gate in `raft_cb_become_leader`. **Concrete, reachable attack path**: a node's local apply dispatch has a defect (materialized state diverges from the committed log) while its WAL bytes are otherwise fine (structurally healthy — orthogonal to the apply defect by construction). The apply-divergent fence sets correctly at boot. But WHILE THE PROCESS KEEPS RUNNING, the periodic timer sees the structural conditions were always fine and silently calls `persist_clear_recovery_unrecoverable()` — releasing a fence that was never actually resolved. If this node then wins a later election, the promotion gate now sees `unrecoverable==false` and promotes it to primary — **a genuinely divergent node serving authoritative state**, a direct Req 8.4 / CLAUDE.md "never serve divergent state" violation. **Round 2 fixed the code**: added `apply_divergent = persist_apply_verdict(&c->persist) == APPLY_DIVERGENT` as an additional guard ANDed into BOTH clear-call-site conditions — `promotion_fence_timeout_fn` (line ~876) and `raft_cb_become_leader`'s gate (line ~1130) — so an apply-divergence-triggered fence can now ONLY be cleared by a fresh process restart's clean `persist_recover()` (since `apply_verdict` has exactly two production write sites: `persist_init()`'s reset-to-`APPLY_CLEAN` and `persist_recover()`'s boot-time computation — no live path resets it). Verified via full trace + RED→GREEN for site 1. **Round 2 REJECTED anyway**: the fix was logically sound, but review PROVED site 2's guard had ZERO test coverage — by deleting only that guard and re-running the ENTIRE 298-test regression sweep across 13 groups, every single test still passed. An unverified defense-in-depth guard on promotion/election-safety code doesn't meet the bar. **Round 3 (test-only) closed it**: added a test that structurally isolates site 2 from site 1 — proven not to be exercising the already-covered sticky-flag path, since `ha_promotion_gate_decision()` never populates `apply_divergent` in its own inputs, so ONLY the round-2-added standalone guard at the call site can catch this scenario. RED→GREEN independently reproduced by two separate reviewers reverting the guard and confirming the exact predicted test failure. **Final closed picture, verified end-to-end across all 3 rounds**: (a) apply-divergence sets the fence correctly (5.2); (b) survives the structural timer's periodic re-check (round 2); (c) survives the promotion gate's re-check even in the hypothetical where the sticky flag itself somehow reads false (round 3); (d) only clears via a fresh process restart replaying cleanly — confirmed by round 1's own two-separate-`persist_t`-instances test. **Forward note for task 6, already flagged in-repo (test_apply_correctness_verification.c comments), not a defect today**: this entire safety picture rests on `apply_verdict` only ever being recomputed once, at boot. If task 6's live apply pump ever adds a live re-verification of apply-correctness (not currently planned, but worth checking), the "restart-only" assumption these 3 rounds of guards depend on would need re-examination at that time. **Lesson for the whole series**: reusing an existing mechanism correctly at the logic/decision level (5.2's `apply_divergent` short-circuit) does not automatically make every OTHER consumer of that same mechanism's output (ha.c's two independent, pre-existing live-clear call sites, written before this feature existed and with no reason to have considered a future trigger) still behave safely — a shared, trigger-agnostic sticky flag is exactly the kind of implicit coupling that needs an explicit audit of ALL its readers/writers whenever a new trigger is added, not just the one you're actively wiring.
 - **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.