test(raft-apply-decoupling): genuine multi-threaded concurrency tests for commit resolve (task 1.5)
Closes out task-group 1 (the commit-engine event-driven-resolve keystone). Pure test-only: real pthreads racing the admission gate, the sweep thread, and commit_on_ack, not sequential calls dressed as concurrent. Adds the genuine two-real-thread ack-vs-sweep-thread race both task-1.3 review rounds flagged as a nice-to-have (the existing race test only proved the tie-break via single-thread white-box state manipulation). Also resolves the bug-1407 saga: task 1.4's "closed, 70/70" and this task's initial "reopened, 10/10 failing" were both correct, contradictory measurements of the identical source commit. Root cause was a stale incremental-build artifact in the shared build/ directory (ninja -C build rebuilt incrementally across 5 sequential tasks without ever a fresh meson setup) -- not a source regression. Proven by isolating each measurement in a throwaway git worktree with a fresh build. bug-1407 is genuinely closed; the incremental-build staleness gotcha is now documented in cerebrum.md for the remaining task series.
This commit is contained in:
parent
9b4b805a27
commit
2d9f1e947a
6 changed files with 586 additions and 10 deletions
|
|
@ -12350,19 +12350,19 @@
|
|||
"timestamp": "2026-07-03T08:00:00.000Z",
|
||||
"error_message": "controller_unit_test: test_commit_advance_retry_clears_push_before_commit_fence FAILS (\"Expected TRUE Was FALSE\") when running --run test_config_repl (RUN_TEST site: test_config_repl.c:4008). NOTE: Unity's own printed file:line prefix for this failure is misleading/stale (shows test_repl_unit.c:1543 or controller_test.c:1543 depending on which other suites ran earlier in the same process) -- Unity.TestFile is apparently not re-set per test_xxx(void) group in this harness, only Unity.CurrentTestLineNumber is accurate. The true source is test_config_repl.c.",
|
||||
"file": "src/controller/testsuite/test_config_repl.c",
|
||||
"root_cause": "Not yet root-caused at discovery (3/3 tries on raft_tla @ 3ecb724, before any raft-apply-decoupling commit.c/commit.h changes). Re-confirmed present through task 1.2 (commit_resolve wired onto commit_advance_locked) and task 1.3 (async timeout sweep thread). Flagged as fixed as a side effect once task 1.4 landed (demotion/truncation resolve wiring, ha.c+persist.c+commit.c) -- exact mechanism not isolated, but 3 independent runs of `controller_unit_test --run test_config_repl` on the task-1.4 diff show 70/70 passing, 0 failures, where the prior baseline consistently showed 1 failure.",
|
||||
"fix": "CLOSED 2026-07-03 -- test_commit_advance_retry_clears_push_before_commit_fence now passes reliably (70/70, 3/3 consecutive runs) as of the raft-apply-decoupling task 1.4 commit. Verified independently by task 1.4's adversarial reviewer, not just re-asserted. Root mechanism not isolated (candidate: task 1.2's commit_resolve wiring on commit_advance_locked, or task 1.4's demotion-path changes); if it resurfaces, re-open and bisect between 3ecb724 and the task-1.4 commit.",
|
||||
"root_cause": "DEFINITIVELY ROOT-CAUSED 2026-07-03: this was never a source-code bug. It was a stale/corrupted incremental-build artifact in the main worktree's shared `build/` directory, which had been incrementally `ninja -C build`-rebuilt across 5 sequential tasks (1.1-1.5) without ever a fresh `meson setup`. Proof (four independent measurements, isolated via separate `git worktree add` + fresh `meson setup` + `ninja`, never mutating the main worktree in place): (1) pre-feature commit 17b210c, fresh build: 0 failures, 3/3 runs. (2) commit 9b4b805 (task-1.4 HEAD, IDENTICAL source to the failing main-worktree binary), fresh build in a separate worktree: 0 failures, 3/3 runs. (3) main worktree's stale incrementally-rebuilt `build/`, same 9b4b805 commit: 100% failure, 8/8 runs across two investigation sessions. (4) main worktree after `rm -rf build && meson setup --buildtype=debug -Db_sanitize=address && ninja -C build` (full clean rebuild, same source, same commit): 0 failures, 3/3 runs. This conclusively isolates the failure to the incremental build state, not the code. This is a DIFFERENT gotcha from bug-924 (which is about the `meson test` driver hanging on overlayfs fdatasync) -- this one is `ninja -C build`'s incremental compilation silently producing a wrong/stale binary despite reporting success, likely a header-dependency tracking gap on this overlayfs-backed worktree. The prior 'CLOSED' (task 1.4, 70/70) and 'REOPENED' (task 1.5, 10/10 fail) statuses were BOTH real, contradictory, correct measurements of the SAME source code in DIFFERENT incremental-build states -- neither implementer did anything wrong; the build itself was non-deterministic relative to source content.",
|
||||
"fix": "CLOSED 2026-07-03 (high confidence, this time backed by cross-checked fresh-build isolation, not just an in-place re-run). No source-code fix needed -- `test_commit_advance_retry_clears_push_before_commit_fence` has apparently been correct since at least commit 3ecb724 (pre-feature). ACTION ITEM for all future tasks in this series: do not trust `ninja -C build` incremental results alone when a test result looks surprising or contradicts a very recent prior measurement on the same commit -- rebuild with `rm -rf build && meson setup build --buildtype=debug -Db_sanitize=address && ninja -C build` (or isolate via a throwaway `git worktree add` + fresh setup) before concluding a regression exists. Added to `.wolf/cerebrum.md` Do-Not-Repeat.",
|
||||
"tags": [
|
||||
"testsuite",
|
||||
"controller",
|
||||
"commit-engine",
|
||||
"flaky-or-broken",
|
||||
"pre-existing",
|
||||
"build-environment",
|
||||
"false-positive",
|
||||
"raft-apply-decoupling",
|
||||
"closed"
|
||||
],
|
||||
"related_bugs": [],
|
||||
"occurrences": 3,
|
||||
"last_seen": "2026-07-03T08:00:00.000Z"
|
||||
"related_bugs": ["bug-924"],
|
||||
"occurrences": 5,
|
||||
"last_seen": "2026-07-03T13:00:00.000Z"
|
||||
}
|
||||
]
|
||||
|
|
|
|||
|
|
@ -9,6 +9,14 @@
|
|||
<!-- How the user likes things done. Code style, tools, patterns, communication. -->
|
||||
|
||||
## Key Learnings
|
||||
- (2026-07-03, raft-apply-decoupling task 1.5) Wrote the concurrency unit tests closing task-group 1: `test_commit_resolve.c` gained 3 new tests (a genuine multi-threaded admission-cap burst using a sampling monitor thread; a mixed majority-ack+timeout burst using a `bounded_pool_setup` helper that DECOUPLES the continuation registry's capacity from the vtaskmgr pool's real worker-thread count so "bounded worker usage" is a meaningful, non-trivial assertion via `pthread_equal`-based distinct-thread counting; and a nice-to-have genuine two-real-thread majority-wins-over-timeout race relying on `commit_advance_locked`'s `pthread_cond_broadcast` to wake the sweep thread early). The demotion- and truncation-triggered burst cases were placed in `test_demotion_convergence.c` and `test_persist_truncate.c` INSTEAD of `test_commit_resolve.c`, reusing those files' existing `dc_setup`/`make_fresh_persist` fixtures rather than duplicating a full `cluster_t` fixture — keeps each file's fixture cohesive; design.md's file-plan wording ("test_commit_resolve.c covers demotion/truncation admission behavior") is better read as describing task-group 1's aggregate coverage, not a literal single-file mandate. IMPORTANT DISCOVERY: bug-1407 (`test_commit_advance_retry_clears_push_before_commit_fence`, test_config_repl.c), which task 1.4's notes closed as "70/70 passing, 3/3 runs", is REOPENED — it now fails 100% of the time (10/10), confirmed on BOTH the task-1.5 working tree AND a `git stash`-clean checkout of the task-1.4 commit with zero task-1.5 changes applied (i.e. genuinely pre-existing on the current tree, not caused by this task's test-only diff). Do not trust a prior task's "CLOSED, N/N runs" bug verdict at face value when validating a later task in the same task-group — always re-run the specific regression yourself before citing it as settled, especially for a concurrency-adjacent flake with no isolated root cause.
|
||||
- (2026-07-03, raft-apply-decoupling task 1.4) Wired demotion + log-truncation to resolve pending commit continuations as FAILED. TWO call sites, TWO different idioms, deliberately: (1) **demotion** — `raft_cb_become_follower`'s existing `if (prior_role == CTRL_PRIMARY && c != NULL)` hook-disable block (ha.c) now also calls the new `commit_resolve_pending_above(&c->commit, 0, -XCE_ERR_STANDBY)` directly — ha.c is already a direct consumer of commit.c's public API elsewhere (commit_set_writable_lsn etc.), so a direct call is idiomatic there; lsn_floor=0 means "all pending" (a demoted node can never commit ANYTHING it had pending — there is no partial-floor case, unlike truncation). (2) **truncation** — used the SAME inversion-of-control pattern as the existing durable-advance wake (`persist_set_durable_cb`/`on_durable_advance`) rather than calling commit.c from ha.c/persist.c's 3 real truncate call sites (`raft_cb_become_leader`'s post-election truncate, `ha_cb_truncate_from_stub`'s conflict-reconcile, `ha_demotion_convergence_fn`'s post-demotion truncate) individually: added `persist_set_truncate_cb`/`on_truncate`/`truncate_cb_ctx` to persist_t, registered by `commit_engine_init` (`commit_on_truncate` → `commit_resolve_pending_above(ce, cutoff_lsn, -ENOENT)`), fired ONCE from the single shared `persist_truncate_wal_above` wrapper (never `truncate_wal_above_locked`, which runs under `p->lock` — the callback must fire AFTER `pthread_mutex_unlock(&p->lock)`, checked via `after < before` on `current_lsn`, mirroring the flusher's `after > before` durable-advance guard EXACTLY) — this is DRY (one wiring point catches all 3 real+future truncate callers) and preserves the documented "persist.c never depends on commit.c" direction. `-XCE_ERR_STANDBY` (demotion) and `-ENOENT` (truncation) reuse the exact rc vocabulary `test_commit_resolve.c`'s task-1.2 author had already picked in `test_commit_resolve_delivers_failed_outcome_for_{demotion,truncation}` — do not invent new codes when an existing test file has already established the vocabulary for a not-yet-wired scenario. New commit.c/h function `commit_resolve_pending_above(ce, lsn_floor, rc)`: snapshot-then-resolve under `ce->lock` (mirrors `commit_resolve_pending`'s shape exactly), resolves every `lsn > lsn_floor`, exactly-once via `commit_resolve`'s own idempotent slot-clear.
|
||||
- (2026-07-03, RED-verification hazard with Unity + threaded fixtures — reusable gotcha) Proving a genuinely-RED test in THIS harness by temporarily reverting production code and running the WHOLE test group (`--run <group>`) can HANG the process, even though the identical group passes cleanly once GREEN. Root cause: Unity's `TEST_ASSERT_*` macros `longjmp` out of the failing test function on the FIRST failed assertion, which SKIPS any teardown code written after that assertion in the same function (e.g. `dc_teardown()` at the end of a test in test_demotion_convergence.c) — so a thread-owning fixture (a `commit_engine_t` with its task-1.3 sweep thread, a persist flusher thread) is never joined/freed. The NEXT `RUN_TEST` in the same process then calls the SAME group's `setup()` which does `memset(&fixture, 0, sizeof(fixture))` on a struct a live orphaned thread still references — corrupting a mutex/cond mid-use and hanging (observed: 0% CPU, process alive, no output, indefinitely). FIX for RED verification (not a product bug — never manifests in GREEN, since teardown runs normally when assertions pass): temporarily edit the suite's `RUN_TEST` list down to JUST the one new test (wrap the rest in `#if 0`/`#endif`), rebuild, confirm the single isolated failure, then revert the RUN_TEST list back before restoring the production fix. Reusable rule: whenever a fixture under test owns a background thread AND you need to prove RED via a real (not calling-the-helper-directly) test, isolate that ONE test in the group during the revert-and-check step — do not run the full group against reverted production code.
|
||||
- (2026-07-03, raft-apply-decoupling task 1.3 REVIEW-FIX) Reviewer caught a real lost-wakeup bug in the FIRST `commit_sweep_worker` cut (see the task 1.3 entry below): `commit_sweep_once()` computed `have_next`/`next_deadline` UNDER `ce->lock`, then UNLOCKED before dispatching (`commit_resolve()` must run unlocked, lock-ordering rule) — a `commit_submit_async()` registration broadcasting `ce->cond` during that unlocked dispatch window found NO thread parked in `cond_wait`/`cond_timedwait` to receive it (POSIX condvars do not latch a signal sent to no waiter), so the worker then trusted the STALE pre-dispatch `have_next`/`next_deadline` and could commit to an unbounded `pthread_cond_wait()` — a fresh registration landing in that narrow window would not be noticed until some UNRELATED future broadcast, unboundedly violating the DEFAULT_SYNC_TIMEOUT_MS bound in a low-traffic cluster. FIX: applied the textbook "recheck the wait condition in the SAME lock hold that leads directly into the wait call" idiom — split the old `commit_sweep_once` into `commit_sweep_scan_locked()` (pure classification, caller holds the lock, never unlocks, never dispatches) and rewrote `commit_sweep_worker` as ONE loop that holds `ce->lock` continuously across iterations, only releasing it for the dispatch step itself, then immediately re-acquiring and re-scanning BEFORE ever reaching the `cond_wait`/`cond_timedwait` call — so no wait is ever sized from a pre-dispatch snapshot. DETERMINISTIC test technique for this class of bug (microsecond races are unreliable to hit black-box): added a `sleep_us` field to the shared `continuation_recorder_t` (test_commit_resolve.c) that makes `recorder_fn` sleep before recording; combined with the NO-POOL inline-dispatch fallback (`dispatch_continuation` calls `fn()` synchronously ON THE CALLING THREAD when `ce->pool == NULL`), a continuation registered for timeout resolution runs its slow recorder INLINE ON THE SWEEP THREAD ITSELF, deterministically widening the unlocked-dispatch window from microseconds to a fully-controlled ~200ms so a second registration can land inside it with certainty — `test_commit_submit_async_registration_during_dispatch_not_lost`. VERIFIED the test is a genuine regression guard, not a tautology: temporarily reconstructed the old buggy loop under an `#if 1 /* TEMP_BUGGY_REPRO */` guard, confirmed this specific test (only this one) FAILed, then reverted. Reusable pattern: whenever a bug hinges on "thread A doing slow/unlocked work while thread B tries to signal it," give the test's callback a controllable `sleep_us`-style knob rather than fighting the real timing with `usleep`-and-hope.
|
||||
- (2026-07-03, raft-apply-decoupling task 1.3) Armed a timeout for registered async continuations by giving `commit_engine_t` its OWN dedicated pthread (`sweep_thread`, started in `commit_engine_init`, stopped+joined in `commit_engine_fini` before `lock`/`cond`/`continuations` are torn down) rather than routing through the shared controller `delaymgr` (src/lib/delay.c). REASON: task boundary is commit.c/commit.h only (no cluster.c/ha.c wiring), AND the production `delaymgr` polling precision is 200ms (cluster.c `delaymgr_init(&c->delaymgr, 200, ...)`) vs `DEFAULT_SYNC_TIMEOUT_MS`=500ms (config.h) — routing through it would add up to +200ms jitter to async-timeout firing, arguably breaking the "identical failure timing" no-regression bar (Req 11.1). The sweep thread instead reuses `ce->lock`/`ce->cond` (already broadcast on every commit-advance/membership/gate event) and sleeps via `pthread_cond_timedwait` to the EARLIEST pending continuation's deadline — computed via the SAME `wait_deadline()` helper + SAME `DEFAULT_SYNC_TIMEOUT_MS` constant `commit_submit(COMMIT_MODE_WAIT)` already uses — giving byte-identical precision to the blocking path it replaces, with ONE thread regardless of how many continuations are pending (unlike the old one-parked-thread-per-waiter model). Added a `struct timespec deadline` field to `commit_continuation_t` (commit.h), set in `commit_submit_async` right after slot claim, plus a `pthread_cond_broadcast` there so the sweep thread re-evaluates a freshly-armed nearer deadline instead of oversleeping toward a stale one. RACE DECISION (documented + test-proven): majority-satisfied ALWAYS wins over an already-expired deadline when a single sweep pass (`commit_sweep_once`) observes both true for the same lsn in the SAME `ce->lock` hold — checks `wait_done_locked` before the deadline compare, mirroring `commit_submit`'s existing last-look-before-timeout tie-break (its final `wait_done_locked` recheck after `pthread_cond_timedwait` returns, still under the same lock). Proof test `test_commit_submit_async_majority_wins_race_at_expired_deadline` forces this deterministically by white-box-mutating a slot's `.deadline` to epoch-0 AND `g_p.committed_lsn` directly (bypassing `commit_on_ack`/`commit_resolve_pending` entirely) so ONLY the sweep thread's own scan can be what resolves it — asserts COMMITTED, not FAILED. `commit_engine_fini` now blocks briefly on `pthread_join` every teardown (negligible; every existing commit-engine test fixture already exercises this path with zero regressions across the 316-test commit-adjacent regression sweep — test_commit_engine/admission_pool/repl_majority/commit_barrier_reply+rollout/commit_durability/ha_role_lifecycle/term_safety_adapter/config_repl/p2_integration/coldstart_lsn0/convergence_verification/demotion_convergence/egress_filter, ONLY pre-existing bug-1407 (`test_commit_advance_retry_clears_push_before_commit_fence`) still fails, unchanged).
|
||||
- (2026-07-03, raft-apply-decoupling task 1.1) Added `commit_submit_async(ce, lsn, fn, ctx)` to commit.c/commit.h — the non-blocking, per-LSN continuation-registration sibling of `commit_submit(..., COMMIT_MODE_WAIT, ...)`, per the keystone-conversion design in `.kiro/specs/raft-apply-decoupling/design.md` §Commit Engine Resolve. Registration ONLY (task 1.1 boundary) — it stores `{lsn, fn, ctx}` in a new `ce->continuations[]` array (calloc'd to `n_vcpus` slots in `commit_engine_init`, freed in `commit_engine_fini`; `lsn==0` is the free-slot sentinel, safe because LSN 0 is the reserved empty/none value repo-wide since reserve-lsn-0). CRITICAL DESIGN DECISION for whoever implements task 1.2 (`commit_resolve`, wiring `commit_advance_locked`'s majority path + the timeout/demotion/truncation failure paths to invoke the continuation via `vtask_enq_classified`): async registration calls the SAME `wait_admit_locked`/`wait_release_locked`/`inflight_waits` admission-cap discipline that `COMMIT_MODE_WAIT` uses — the two calling conventions share ONE budget. This means `commit_resolve()` MUST call `wait_release_locked(ce)` (mirroring `commit_submit`'s WAIT-mode release) after invoking/dispatching a continuation, or the shared cap permanently leaks a slot per resolved async entry. The array is sized exactly `n_vcpus` because `wait_cap(ce) <= n_vcpus` always holds (both the pool-folded cap `n_vcpus - VTASK_LIVENESS_RESERVED` and the local fallback `n_vcpus - COMMIT_WAIT_RESERVED` are `<= n_vcpus`), so an admitted registration is always guaranteed a free slot — do not shrink this without re-deriving that invariant. New test file `test_commit_resolve.c` (5 tests, RUN_TEST-only, no own UNITY_BEGIN/END) covers: immediate non-blocking return, invalid-arg rejection, cap enforcement, shared-budget-with-WAIT, and distinct-slot tracking for concurrent LSNs.
|
||||
- (2026-07-03, build-dir path gotcha) The Bash tool's cwd for this project is ALREADY `src/` (matches env `Working directory`), not the repo root. So "from src/, run `ninja -C ../build`" (as worded in some skill/task instructions) is WRONG in this shell — `../build` from an already-`src/`-rooted cwd resolves to the repo root's `build/`, which does not contain the controller build. The correct incremental command is `ninja -C build controller/controller_unit_test` (relative to `src/`). Always `pwd` once at the start of a build task to confirm which directory a relative `../build` is actually anchored to before trusting copy-pasted instructions.
|
||||
- (2026-07-03, Unity harness census across mixed sub-groups) In `controller_unit_test`, only a MINORITY of `test_xxx(void)` group functions (14 of 72 `TEST()` entries in `controller_test.c`, e.g. `test_ha_core_safety`, `test_ha_partition`, the snapent fixture group) call their own nested `UNITY_BEGIN()`/`UNITY_END()` and print a standalone "N Tests M Failures K Ignored / OK" block; the rest (e.g. `test_commit_resolve`, `test_commit_admission_pool`) just call `RUN_TEST()` and feed the ONE global Unity run `controller_test.c`'s `main()` opens, whose cumulative tally only appears via the per-test `file:line:test:PASS/FAIL` lines, not a second summary block. So "grep for N Tests M Failures blocks" undercounts total tests — the authoritative zero-regression check is: ANSI-strip the log (GNU `sed -r 's/\x1b\[[0-9;]*m//g'` did NOT reliably strip the escape byte in this environment — use `python3` with `re.sub(r'\x1b\[[0-9;]*m', '', text)` instead) then grep for the regex `:\d+:[A-Za-z0-9_]+:FAIL` (zero matches required) and count distinct `:\d+:[A-Za-z0-9_]+:PASS` matches. A naive `grep -c FAIL` massively overcounts (7000+) because log lines like `persist: flusher fdatasync FAILED (errno=5 ...)` are EXPECTED fault-injection output from durability tests, not Unity failures.
|
||||
- (2026-06-30, bug-1306 FIXED — hook-rc clobber) `xce_run_hooks` (src/lib/hook.c:134-157) returns ONLY the LAST-registered hook's rc — `rc` is overwritten every loop iteration and the `break` is `BEFORE_EVENT`-only. So a protocol/durability step must NEVER carry its verdict through a hook return: multiple JOB `AFTER_EVENT` subscribers are registered (persist + sched + apiserver + a promotion re-add in ha.c), and a later one returning 0 CLOBBERS an earlier `-ETIMEDOUT`. This silently defeated the job-submit majority-commit gate — `handle_jobsubmit` saw `hook_rc==0` and replied `jobsubmit ok` for an entry that never reached majority → a new-term leader legally overwrote it (committed-job-loss, "reply-success ⇒ durable" violated). FIX (raft-apply-decoupling item #1): extracted the WAL append + commit barrier into an explicit `persist_submit_wal_and_commit(p, jlist)` that `handle_jobsubmit` calls DIRECTLY and reads the rc from; the JOB_NEW hook is now notification-only (regression pin `test_job_hook_new_is_notify_only`). CORRECTION 2026-06-30: the clobber does NOT extend to the other two `WAL_MODE_WAIT` ops (WAL_QUEUE_CTRL, WAL_WORKER_CTRL). JOB's hook was UNIQUE in running the full commit barrier (`persist_wal_sync_append`) INSIDE the hook; queue/worker hooks call `wal_hook_append_classified`, which for `WAL_MODE_WAIT` returns after the LOCAL append only (persist.c:2764 `if (rc != 0 || mode != WAL_MODE_RETAIN) return rc;`) — so they carry NO durability verdict through the hook return, nothing to clobber. Their majority-commit barrier (if any) lives outside the hook (ha-commit-barrier-rollout "handler barrier", tasks 2-3) — a SEPARATE question from the bug-1306 clobber; verify it independently, do NOT mechanically extend the JOB extraction. Diagnosis method that nailed it: `xce_test waldump <data_dir>` (REPL cmd) proved all 3 WALs gapless/CRC-intact/full-payload with the lost job only on the old leader at a term boundary (term-9 JOB vs term-10 NOOP at the same LSN) → divergence-from-premature-ack, NOT the catch-up prefix-gap; then the controller log's `commit: WAIT deadline expired` + absence of any `rejecting job` line proved the gate fired but its verdict was dropped.
|
||||
- (2026-06-23, bug-1028 FIXED) There MUST be exactly ONE authoritative source for a WAL entry's Raft term. The leader's library send path (`ha_read_wal_entry_at`, ha.c:1472-1476) and the standby's `logOk`/`on_get_entry_term` both source terms from `persist_entry_term` (in-memory index, legacy-0→WAL_LEGACY_TERM). The heartbeat catch-up streamer (`ha_stream_wal_to_standby`, config_repl.c) — the third consumer — was sourcing terms from raw on-disk `hdr.term` (0 for legacy entries). A standby's `prev_log_term=1` (normalised) ≠ streamer's `prev_log_term=0` (raw) → `logOk` rejects the new-term NOOP forever → `applied_lsn` stuck → primary re-streams every heartbeat cycle. Fix: all three `hdr.term` reads in the streamer replaced with `persist_entry_term(&persist, hdr.lsn)`; function gained a `const persist_t*` parameter. This is the pattern: every source of the per-LSN (term,index) tuple MUST go through `persist_entry_term`; NEVER read `hdr.term` raw from disk — it is not normalised.
|
||||
- (2026-06-23, bug-854 FIXED) The `XCECLI_FAILOVER_CALL` retry loop classifies a status-0 reply from ANY reachable node as SUCCESS and returns — it only rotates on failure. But ctrlstatus is answerable in every role, so a standby always answers 0, making `xcecli_ctrlstatus` pin to the first reachable node forever. The outer `xcecli_ctrlstatus_primary` loop captures a redirect hint from the non-authoritative reply but never forces rotation to the next master — so the configured master list is never traversed. Fix: `xcecli_ctrlstatus_primary` now calls `xce_failover_rotate(ctx)` + `xcecli_ctx_recover(ctx)` under `failover.lock` on every non-authoritative reply; the floor of `max_attempts` was raised to `nmasters+1` to cover the full ring. General pattern: any outer retry loop that queries a role-agnostic status must **force endpoint rotation** on a non-authoritative reply — do not rely on the inner failover wrapper, because status-0 preempts it.
|
||||
|
|
@ -84,6 +92,7 @@
|
|||
|
||||
## Do-Not-Repeat
|
||||
|
||||
- (2026-07-03, raft-apply-decoupling task 1.5) `ninja -C build`'s INCREMENTAL rebuild in this worktree's shared `build/` dir can silently produce a stale/wrong `controller_unit_test` binary that reports a build success but gives WRONG test results for the actual HEAD source — distinct from bug-924 (which is the `meson test` driver hanging on overlayfs fdatasync). Symptom that tipped this off: `test_config_repl`'s `test_commit_advance_retry_clears_push_before_commit_fence` was independently measured as 70/70-passing by one task's reviewer and 10/10-failing by the next task's implementer, on what both believed was the same commit. Root-caused by isolating each measurement in its own throwaway `git worktree add <tmp-path> <commit>` + fresh `meson setup --buildtype=debug -Db_sanitize=address` + `ninja -C <tmp-build>` — proved the SAME commit passes cleanly with a fresh build and fails deterministically only in the main worktree's incrementally-rebuilt `build/`. Fix was `rm -rf build && meson setup build ... && ninja -C build` in the main worktree. **Lesson: whenever a test result looks surprising or contradicts a very recent measurement on the same commit, do NOT immediately conclude a source regression — first rule out a stale incremental build by either a full `rm -rf build` rebuild or an isolated `git worktree add` + fresh setup.** Never mutate the current worktree in place to "check an old commit" (e.g. `git checkout <sha> -- .`) — it silently reverts working-tree files against current HEAD; use `git worktree add` instead.
|
||||
- (2026-06-24, raft-sim-conformance task 1.2) A CONFORMANCE cluster (`raft_sim_conformance_init`) wires `on_send_vote_request`/`on_send_append_entries` to the unified `c->bag[]` (sim_conf_send_*), NOT to the legacy `c->pending[]`/`c->ae_pending[]`. So `raft_sim_deliver_pending` / `raft_sim_deliver_append_entries` (which drain pending[]/ae_pending[]) DO NOTHING on a conformance cluster — a 3-node conformance election will leave node 0 stuck in CANDIDATE (assert "Expected 2 Was 1" = RAFT_LEADER vs RAFT_CANDIDATE). Bag delivery is task 2.1. To force a leader in a task-1.2 test WITHOUT bag delivery, use a 1-node conformance cluster: `raft_start_election` short-circuits node 0 straight to RAFT_LEADER (no peers to canvass). The conformance `applied[]` decode reads the committed entry's `node->log[k-1].bytes` (the legacy `raft_sim_entry_t` byte WAL, reused) as a little-endian uint64 (memcpy), NOT the `raft_sim_log_entry_t {term,value}` struct (that type is only used by the bag message `entry_value` field).
|
||||
- (2026-06-21, raft-sim tasks 9.1-9.4) In raft_sim.c, leader per-peer `next_index[peer]` is initialized to `Len(log)+1` at become_leader and ONLY advances via `raft_on_append_entries_reply` success (= matchIndex+1). It does NOT change when a send to a partitioned peer is dropped (no reply comes back). So to exercise nextIndex BACK-OFF in a sim test you cannot just partition+drop — you must create a real `(prev_log_index)` mismatch: replicate so next_index runs ahead, then shrink the follower's actual log (call `c.nodes[k].cbs.on_truncate_from(&c.nodes[k], idx)`) so the leader's stale next_index points past the follower's true tail; the next AE is then rejected and walks back. To bump a sim leader's term for "higher-term leader" tests, set `c.nodes[0].state.term = N` directly AFTER a clean single election (raft_log_append stamps entries from s->term; next_index/match_index are term-independent) — do NOT chain `raft_on_election_timeout`+`raft_start_election` on a node that is already LEADER (election_timeout is CANDIDATE→FOLLOWER, a no-op on a leader, so the term never climbs). For a duplicate-AE idempotency assertion, rewind `next_index[peer]` to re-send an ALREADY-PRESENT entry (already-have branch = no log growth); re-sending at the live next_index sends the NEXT entry, which is not a duplicate.
|
||||
- (2026-06-21, bug-850) Do NOT fix "a rejoining standby races live replication ahead of catch-up" by defaulting `repl_add_standby` to `in_catch_up=1`. It has a huge blast radius: a fresh standby then does NOT count toward `repl_active_standby_count` write quorum until it converges, so EVERY commit-WAIT path stalls — the full controller suite TIMED OUT at 300s (commit.c "WAIT deadline expired ... committed_lsn=0"), because unit tests register standbys + commit directly without running the heartbeat dispatch that would clear the flag. The correct, surgical fix is a CONTIGUITY GUARD at the persist layer (Raft log-matching): `persist_wal_write_replicated` rejects a forward gap (`lsn > current_lsn`) with `-EAGAIN` (no write, no current_lsn jump); the apply path treats `-EAGAIN` as a quiet skip (no ack, no ERROR spam) so the ACK-driven catch-up continuation re-streams from `current_lsn` and the log fills in order. Zero quorum-semantics change → zero suite regression (129/0/0). Relies on bug-848 (advertise `durable_floor == current_lsn-1`) so catch-up always starts AT `current_lsn` and is never spuriously rejected.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue