fix(controller): bug-1433 async-thread fdatasync block + bug-1434 committed_lsn CAS race
bug-1433: raft_cb_become_leader runs synchronously off peer_ctrl_dispatch_to_pool, which hardcodes VTASK_LIVENESS for every inbound peer-control packet including vote replies. The bug-1431 round-2 fix (flusher_flush_once force-flush in persist_promotion_advance_committed) was reachable from that never-blocks task class -- same bug family as bug-1198. Removed the force-flush; round-3 (durable-capped only) plus the already-shipped bug-1432 immediate-first-heartbeat fix close the visibility-latency gap non-blockingly. TRACKED RESIDUAL: persist_apply_pump_run_sync is still reachable on the same VTASK_LIVENESS path and does bounded pread() -- lower risk (page-cache-hot) but strictly still blocking I/O; not safe to remove outright (reopens bug-1043-family job loss), needs its own follow-up (reclassify peer_ctrl_classify's winning-vote-reply case or dispatch promotion tail onto a fresh VTASK_LIVE_BLOCK vtask). bug-1434: persist_promotion_advance_committed and ha_standby_advance_committed_and_apply raced on persist_t.committed_lsn via an unlocked load-then-conditional-store during the promotion window (before ha->role flips to CTRL_PRIMARY) -- a losing racer could regress committed_lsn. Added persist_committed_lsn_advance_atomic (CAS retry loop, lock-free) and routed both call sites through it. Validated: 19/19 unit test suites, 6-round chaotic soak (0 loss/divergence) + 6-round sequential+leadership-coverage soak, both PASS. Also folds in raft-apply-decoupling and raft-ha-core-safety validate-impl phase bumps to "implemented" and clears config-upgrade-snapshot's now-satisfied entity-bearing-snapshots block.
This commit is contained in:
parent
ea0ac81c92
commit
40f510fb14
25 changed files with 1617 additions and 226 deletions
|
|
@ -99,6 +99,12 @@
|
|||
- (2026-06-14, reserve-lsn-0 TEST FALLOUT) Base-shift broke tests that did a "priming append so the entry lands at LSN >= 1" — that priming was ONLY to escape the old lsn-0 special-case and is now redundant. Fix = REMOVE the priming append so the real entry lands at lsn 1 (test_commit_engine tests 32/34, test_commit_barrier_rollout fact-hook, test_commit_barrier_reply cbrp_setup, test_p2_integration single_node) OR bump a hard-coded `current_lsn==1` assertion to `==2` if the priming must stay (test_egress_filter ef_setup). The ENGINE-LESS walAck path only "worked" via `repl_wait_majority_ack`'s `entry_lsn==0 → return 0` hatch (repl.c:437); with the first entry now lsn 1 it routes to `commit_submit(ce=NULL)` → -EINVAL, so engine-less commit tests MUST wire a commit engine (test_p2_integration test 9: added commit_engine_init(n_peers=3) + repl_set_commit_engine + commit_engine_fini). Tests asserting RELATIVE lsn (lsn_before+1, current_lsn>0, h1.lsn>h0.lsn) or byte-offset truncation math are base-shift-immune.
|
||||
- (2026-07-03, raft-apply-decoupling task 3.3) `test_commit_barrier_reply.c`'s `cbrp_setup` fixture wires job submission's commit continuations through a SEPARATE standalone `commit_engine_t g_hce` (via `commit_engine_init(&g_hce, &g_hc.persist, ...)` + `repl_set_commit_engine`), NOT through `cluster_t`'s own embedded `c->commit` field. This matters because `raft_cb_become_follower`'s demotion branch (ha.c:1395) calls `commit_resolve_pending_above(&c->commit, 0, -XCE_ERR_STANDBY)` — i.e. it operates on `c->commit`, the field `g_hce` deliberately bypasses. So `raft_cb_become_follower_with_cluster_for_test(&g_hc, term)` CANNOT be used to force a job-submission continuation FAILED in this file's harness — it would touch the wrong (never-initialized, all-zero) engine. `persist_truncate_wal_above(&g_hc.persist, cutoff)` has no such problem: `persist_set_truncate_cb` is wired directly by `commit_engine_init` onto whichever engine you pass it (here, `g_hce`), so truncation is the ONLY viable way to force a deterministic, explicit non-timeout FAILED outcome (`-ENOENT`) for a job-submit continuation in this specific fixture. Added 4 tests exercising this (single + repeated truncation-forced FAILED, repeated timeout-forced FAILED, array mixed-outcome) — all GREEN on first run (no bug found; task 3.2's failure-path guarantee holds). Generic demotion-resolve-continuations-FAILED behavior already has its own dedicated coverage in `test_demotion_convergence.c` (task 1.4/1.5), which uses `ha_promote_to_primary` + the cluster-aware `_for_test` seam against a REAL `c->commit`-shaped fixture — reuse THAT file, not this one, for anything demotion-specific.
|
||||
- (2026-06-13, kiro-impl process) A RED-only test pin is only valid if its OWN impl task can turn it green via the SAME seam the test drives. Misfires caught in review: boot-replay pin RED on an lsn-0 `replay_wal` skip artifact; hooks pin via the `c==NULL` raft shim where `ha_register_persist_hooks(&c->hooksys)` can never run (re-point to the real-cluster `ha_promote_to_primary` seam). Deleting machinery → DELETE the machinery-subject tests (design "no shims, rewritten or deleted not stubbed"), never add file-local no-op stubs. Unity runner prints the PREVIOUS passing test's file:line as a stale prefix on FAIL lines → attribute failures by test NAME.
|
||||
- (2026-07-10, persist_apply_pump_run_sync build+test validation) A bare `grep -i TIMEOUT testlog.txt` is NOT a reliable single-shot hang detector in this suite: it returns 100+ matches every clean run because "timeout" appears in test names (`test_wait_ack_timeout`, `test_heartbeat_timeout_fires_failover`, ...) and in expected WARN-level log lines from tests that deliberately exercise timeout paths (`commit: WAIT deadline expired without commit ... timeout_ms=250`), every one of which is immediately followed by `PASS`. Same false-positive shape as the already-documented `grep -c FAIL` overcount from expected "FAILED" fault-injection log lines (see the `:FAIL:`-anchor entry above) — a plain substring match on an English word that's also load-bearing test vocabulary in an HA/timeout-heavy codebase will always overcount. The AUTHORITATIVE signal for a real meson-level hang is the per-test `result:` field in testlog.txt (`result: exit status 0` = clean; a real hang shows `result: TIMEOUT` or "killed by signal") plus a case-SENSITIVE word-boundary check `grep -noE '\bTIMEOUT\b'` (zero hits = no real timeout verdict anywhere). This is a DIFFERENT axis from the pre-existing 2026-06-13 learning two entries up ("meson wrapper's OWN 300s `Timeout: 1` summary field is not fatal, use the direct binary") — that one is about meson's outer test-runner timing out the whole suite; this one is about naively grepping the word "timeout" inside a clean log. Both must be checked via their authoritative field, never via a bare case-insensitive substring grep.
|
||||
- (2026-07-10, bug-1431/1432) `delay_task_ms(mgr, interval_ms, -1, fn, arg)` fires its FIRST invocation only after `interval_ms` elapses — it is a periodic timer, not "run now then repeat." A comment claiming a delaymgr-scheduled task starts "immediately" is not proof it does; verify against this semantics before trusting it. `ha_start_primary_heartbeat` had exactly this stale claim, costing a full heartbeat interval (3000ms default) of promotion latency before a reconnecting standby's true log position was learned via the heartbeat-reply stateReport. Fix pattern: call the task function synchronously once before arming the periodic `delay_task_ms`, but ONLY if that function does purely non-blocking work (verified here — `primary_heartbeat_fn` only issues async `xce_ha_heartbeat_req` sends).
|
||||
- (2026-07-10, bug-1431) `persist_truncate_wal_above`/`truncate_wal_above_locked` only sets `p->committed_lsn` on branches that perform a REAL truncation. Its common no-op early-return (nothing above cutoff_lsn — the case when a newly-promoted leader's own log already covers the just-computed `safe_committed_lsn`) leaves `committed_lsn` untouched. Any caller that derives a new commit floor and expects `persist_truncate_wal_above` to have applied it (a comment in `raft_cb_become_leader` claimed exactly this) must NOT assume that — use a dedicated advance function instead (`persist_promotion_advance_committed`, durable-capped, monotonic via a CAS loop). **UPDATED 2026-07-10 (bug-1433, controller-invariant-audit):** an earlier version of this fix also force-flushed (`flusher_flush_once`, a blocking `fdatasync`) when durable_lsn lagged — REMOVED, because the real multi-node winning-vote-reply path reaches this function on a `VTASK_LIVENESS`-classified vtask (never expected to block — same bug class as bug-1198). The visibility gap is closed non-blockingly by bug-1432's immediate-heartbeat fix alone; no force-flush needed.
|
||||
- (2026-07-10) A chaotic-HA-soak "COMMITTED JOB LOSS" verdict with `committed_divergence=0` and all jobs present at final settlement is a VISIBILITY-LATENCY gap, not a durability violation: the job was legitimately committed by the old primary before it died, but the new leader's post-election `safe_committed_lsn` (computed only from THIS election's granted-voter sample, which can be conservative when a second voter is mid-restart) is lower until fresh quorum reconfirmation — correct Raft Figure-8 safety, not a bug by itself. Per this project's test philosophy (skill xce-test-ha: "a committed job an elected primary cannot list IS a real loss regardless of eventual reappearance") this is still a hard soak FAIL and must be fixed — but ONLY by shrinking the reconfirmation window (e.g. sending the first post-promotion heartbeat immediately), never by unsafely exposing unconfirmed old-term entries early just to make the check pass.
|
||||
- (2026-07-10, bug-1433, controller-invariant-audit) `peer_ctrl_classify` (ha.c ~5935) hardcodes `VTASK_LIVENESS` for EVERY inbound peer-control packet (vote replies, PreVote replies, WAL/config ACKs) — a task class documented as "never admission-gated," i.e. never expected to block. `raft_cb_become_leader` (the raft library's `.on_become_leader` callback) is invoked synchronously from THIS dispatch path whenever a real multi-node winning vote reply arrives — NOT from the separate `VTASK_LIVE_BLOCK`-dispatched election-*start* path (`ha_dispatch_election_start`, which only covers RDHUP-failover/single-node self-grant). Any blocking call added inside `raft_cb_become_leader` (or anything it calls) is reachable on a worker class the design guarantees never blocks — same bug class as bug-1198 (closed by moving `persist_checkpoint`'s snapshot-send fdatasync off VTASK_LIVENESS entirely). Before adding ANY blocking call to the promotion path, trace peer_ctrl_classify's actual dispatch, don't trust an existing nearby comment claiming "already on VTASK_LIVE_BLOCK" — one was WRONG (see bug-1433 in buglog.json for the full trace). `persist_apply_pump_run_sync` still has this residual (bounded `pread`, lower risk than `fdatasync`, not fixed this session — see buglog.json bug-1433 for why it can't simply be removed).
|
||||
- (2026-07-10, bug-1434, controller-invariant-audit) `persist_promotion_advance_committed` and `ha_standby_advance_committed_and_apply` both did an unlocked `load; if(target<=cur) return; store` on `p->committed_lsn` — safe ONLY if truly single-writer, but `raft_cb_become_leader` calls the former BEFORE setting `role = CTRL_PRIMARY` (~170 lines later), so a still-in-flight WAL catch-up entry on another thread can call the latter concurrently during that window — classic lost-update race that can regress `committed_lsn` (monotonicity violation). General lesson: when two independent call sites do "monotonic-max store to a shared atomic, no lock" and you can't prove they're strictly serialized by role/state gating (gating checked AT CALL TIME can still race with a WINDOW where the gate value itself is about to change), use a CAS retry loop (`persist_committed_lsn_advance_atomic`), not a bare load-then-store — it's safe under ANY interleaving with zero lock-ordering risk.
|
||||
|
||||
## Do-Not-Repeat
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue