fix(ha): close permanent catch-up wedge from duplicated replication-target formula (bug-1478)

Two independent call sites duplicated target = max(committed_lsn,
last_written): ha.c's heartbeat/stateReport dispatch (repl_ack_decode_and_
dispatch) and config_repl.c's catchup_derive_target (used by the ACK-driven
catch-up continuation path). Once config-upgrade-snapshot's marker-triggered
checkpoint advances last_written one LSN past an already-committed
committed_lsn (the checkpoint's own consumed, never-replicated LSN slot),
the unconditional max() permanently wedges every standby's catch-up target
at an unreachable value -- the primary re-streams an empty range every
heartbeat forever.

Extracted the shared, pure ha_compute_repl_target_lsn(committed_lsn,
last_written) (ha.h/ha.c), which only falls back to last_written on genuine
committed_lsn==0 bootstrap, and pointed both call sites at it so they can
never disagree again.

Fixing the second call site exposed three pre-existing tests in
test_convergence_verification.c that hardcoded the old buggy formula's
output as their expected target; updated their fixture values (committed_lsn
4->9, matching appended_lsn) to decouple them from the now-fixed edge case,
which already has dedicated pure-function coverage in test_ha_role.c.

GREEN: full controller_unit_test suite = 1040 tests, 0 failures, clean
across multiple runs. Live-verified via run-ha-soak.sh
--config-upgrade-wal-trim (18 jobs, zero job loss, ctrl-1 reconverged 3-of-3
using the preserved floor segment) -- unblocks config-upgrade-snapshot
task 8.9.
This commit is contained in:
Rongsong Shen 2026-07-28 09:53:20 +00:00
parent 71223ec712
commit 102e263cce
9 changed files with 850 additions and 20 deletions

View file

@ -563,5 +563,8 @@ Prior entry (2026-07-25) noted `repl_remove_standby` correctly sets the `last_di
## Key Learning (2026-07-25) — bug-1472 FIXED: `worker_from_capnp` now decodes `hook_detail` and applies UPDATE in place to an existing worker
`worker_from_capnp` (`src/controller/worker.c`) previously returned `-EEXIST` unconditionally whenever the decoded worker's name was already registered, silently discarding the decoded fields regardless of whether the WAL entry was a JOIN, UPDATE, or DEAD. Fixed by reading the WAL_WORKER_CTRL payload's leading `hook_detail` discriminator (already decoded by the caller at `persist.c:3728-3729` but never passed down or re-read by `worker_from_capnp` itself) and branching: `HOOK_DETAIL_WORKER_JOIN` keeps the original -EEXIST no-op (regression-guarded); `HOOK_DETAIL_WORKER_UPDATE` now REPLACES (bitwise, not OR — see worker_ctrl's assignment-style `&= ~(...)`/`|= (...)` semantics, an OR-based fix would make HOST_OPEN unable to ever clear the closed bits again) `XCE_WORKER_ADMIN_MASK` bits on the existing worker under its own write lock, then returns 0 so `persist.c`'s existing generic "0 ⇒ fire the hook_detail-keyed notification" fallthrough (persist.c:3737-3785, unmodified) fires the correct `HOOK_DETAIL_WORKER_UPDATE` hook; `HOOK_DETAIL_WORKER_DEAD` is deliberately left as -EEXIST/no-op (see the two Key Learning entries above for why — filed as bug-1472-remainder, not silently ignored). `persist.c` needed NO changes — confirmed its WAL_WORKER_CTRL apply case already treats any non-EEXIST, non-negative `worker_from_capnp` return generically. TDD: RED confirmed by temporarily reverting just `worker.c` (via `git stash push -- <2 files>` / `git apply` round-trip, keeping the new tests) and re-running — exactly the 2 new UPDATE tests failed, the JOIN-duplicate regression-guard test already passed (proving it exercises the untouched path). GREEN: full suite 1030→1033 tests (this worktree's immediate pre-fix baseline, not a stale prior-session number), 0 failures, net +3 = exactly the 3 new tests (`test_ace_worker_ctrl_update_applies_to_existing_worker`, `test_ace_worker_ctrl_update_replaces_not_ors_admin_bits`, `test_ace_worker_ctrl_join_duplicate_existing_is_noop`), zero regressions.
## Key Learning (2026-07-28, config-upgrade-snapshot task 8.9, bug-1478 FIXED) — a function's own doc comment can describe a NARROWER behavior than the code actually implements; the gap between the two is where a real bug hides
`ha.c`'s `repl_ack_decode_and_dispatch` computed the primary's replication/convergence target as `target_lsn = max(committed_lsn, last_written)` (`last_written = persist.current_lsn - 1`, the RAW physical WAL tip), with a comment saying this exists "so standbys can catch up even when committed_lsn is stuck at 0" — but the code applied the `max()` UNCONDITIONALLY, not only in that `committed_lsn==0` case. This was harmless until config-upgrade-snapshot task 5's marker-triggered checkpoint (a local-only, never-replicated `WAL_CHECKPOINT` entry that `persist_write_checkpoint_marker` appends by bumping `current_lsn` directly, bypassing `raft_log_append`/the commit engine — the same architectural gap already flagged, unfixed at its root, as bug-1466) started interleaving a real, mid-operation local-only entry: `last_written` then permanently sat one LSN past the REAL, majority-gated `committed_lsn` (confirmed via `commit_advance_locked`'s own log never advancing past 21, while `ha_dispatch_repl_action_for_standby`'s log reported "committed_lsn=22" forever) — and since a checkpoint's own consumed LSN slot is BY DESIGN never streamed/applied by any standby, `HA_REPL_CONVERGED` became permanently unreachable, forever re-streaming an empty range every heartbeat. **Ground-truthed via `xce_test waldump <wal_dir>`** (an existing REPL command in the `xce_test` binary, `src/controller/testsuite/waldump.c` — prints `LSN | TERM | TYPE | PLEN | PCRC | INFO` for every entry in a WAL directory, plus a gap/tip summary) directly on the preserved live-soak deploy dir, which is what turned "I think lsn=22 is the checkpoint" into "confirmed: lsn=21 is WAL_CONFIG_MARKER (type UNKNOWN/40B), lsn=22 is WAL_CHECKPOINT (0B)" — use this tool FIRST whenever a WAL-position/LSN-identity question comes up in a live soak investigation, instead of inferring entry types from log lines alone. **Fix pattern**: extracted the computation into a small pure function (`ha_compute_repl_target_lsn`) matching the comment's ACTUAL narrow intent (`committed_lsn==0` only) — trivially unit-testable without capnp/network scaffolding, unlike the `static` function it lives inside. **Do-Not-Repeat**: when a fallback/edge-case comment says "so X can happen even when Y" but the code's condition doesn't actually gate on Y, that mismatch is not a documentation nit — it means the fallback fires in cases beyond what was ever justified/reviewed, and is a prime hiding spot for exactly this class of bug once ANY new caller starts producing conditions the ORIGINAL narrow case never anticipated. Full finding + fix in `.wolf/buglog.json` bug-1478 (RED→GREEN verified: reverting only the new function's body reproduced the predicted single failure "Expected 21 Was 22"; 1040/1040 tests clean twice after restoring the fix).
## Do-Not-Repeat (2026-07-28, bug-1476) — a buglog "FIXED... GREEN" claim is not proof; the working tree can be mid-TDD-cycle at the exact moment you resume
Resumed `/kiro-impl config-upgrade-snapshot` with buglog.json's bug-1476 entry claiming "FIXED (2026-07-28)... GREEN: full controller_unit_test suite" — but the actual uncommitted diff on disk had a comment (config_repl.c:639-646) explicitly admitting `load_primary_config()` had been reverted to its RED-phase (no lock, decode-directly-into-shared-field) state for test construction and never restored. Running the full suite fresh immediately surfaced the real state: `test_bug1476_concurrent_load_primary_config_no_uaf_or_double_free` FAILED. The buglog note was written mid-cycle, describing the INTENDED fix, not the code actually sitting on disk. Fixed for real (lock now wraps the whole function body; decode targets a local temp, swapped in only after full validation), verified 4/4 clean full-suite runs before committing. **Do-not-repeat**: before committing or building on top of ANY buglog "FIXED"/GREEN claim for uncommitted work, re-run the actual test suite against the actual working tree first — never trust the log entry's narrative over a fresh run, especially when resuming a session that may have been interrupted mid-TDD-cycle.