diff --git a/.kiro/specs/raft-apply-decoupling/tasks.md b/.kiro/specs/raft-apply-decoupling/tasks.md index 157dc59..a5e4776 100644 --- a/.kiro/specs/raft-apply-decoupling/tasks.md +++ b/.kiro/specs/raft-apply-decoupling/tasks.md @@ -1,6 +1,6 @@ # Implementation Plan -- [ ] 1. Commit engine event-driven resolve (keystone prerequisite) +- [x] 1. Commit engine event-driven resolve (keystone prerequisite) - [x] 1.1 (P) Add per-LSN continuation registration to the commit engine for the synchronous (WAIT) commit mode, replacing the blocking wait - Register a continuation (callback + context) against an appended LSN instead of parking the calling thread - Preserve the existing per-LSN bookkeeping and admission-cap discipline already used by the commit engine @@ -31,7 +31,7 @@ - _Boundary: Commit Engine Resolve integration points — this task is a narrow, explicit exception to the "commit.c only" boundary used by 1.1-1.3: it may touch the demotion/role-transition site and the log-truncation site (wherever they currently live) in addition to commit.c, but must not touch job.c or any command-handler code_ - _Depends: 1.3_ -- [ ] 1.5 Unit test the commit engine's resolve behavior under concurrent in-flight commits +- [x] 1.5 Unit test the commit engine's resolve behavior under concurrent in-flight commits - Cover exactly-once resolution across majority ack, timeout, demotion, and truncation, exercised through the real detection paths built in 1.3/1.4 (not by calling `commit_resolve()` directly, which 1.2's own tests already cover) - Cover that a burst of concurrently pending commits does not exhaust the bounded request-handling capacity - Observable: a test suite run demonstrates bounded worker usage and exactly-once resolution under a concurrent-commit burst @@ -192,4 +192,5 @@ - **1.1 review**: bare `controller_unit_test` (no `--run` filter) segfaults deterministically in this environment (SSL_CTX_free/iomgr_fini crash, `test_ha_vep_teardown.c`) — reproduced identically on the pre-task base commit, confirmed pre-existing and unrelated. Logged as bug-1406. Also `test_commit_advance_retry_clears_push_before_commit_fence` (`test_config_repl.c`) fails independently of this feature, also reproduced on base — logged as bug-1407, flagged because it's adjacent to `commit_advance_locked`, which task 1.2 modifies. **Always validate with a filtered `--run ` invocation or the tightly-coupled commit-engine cluster, not the bare unfiltered binary, until 1406/1407 are separately fixed.** - **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 (pre-existing `test_config_repl` flake, unrelated to this feature) is now closed — 70/70 passing across 3 independent runs on this task's diff; root mechanism not isolated (candidate: task 1.2's `commit_advance_locked` wiring), re-open and bisect if it resurfaces. +- **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**. +- **1.5**: pure test-only close-out of task-group 1 — 3 new genuine-multi-thread tests added across `test_commit_resolve.c`/`test_demotion_convergence.c`/`test_persist_truncate.c` (real pthreads, not sequential calls dressed as concurrent): an admission-cap burst with a concurrent monitor thread, a mixed majority-ack/timeout-sweep burst proving bounded dispatch-thread usage, and a genuine two-real-thread ack-vs-sweep-thread race (closing the "single-thread white-box only" gap both task-1.3 review rounds had flagged as a nice-to-have). No production code touched. **bug-1407 saga, definitively resolved**: it was NEVER a source bug. 1.4's "closed, 70/70" and 1.5's initial "reopened, 10/10 failing" were BOTH real, correct, *contradictory* measurements of the exact same source commit (9b4b805) — the difference was a stale/corrupted incremental-build artifact in the shared `build/` directory, which had been `ninja -C build`-rebuilt incrementally across all 5 tasks without ever a fresh `meson setup`. Proven by isolating each measurement in its own throwaway `git worktree add` + fresh `meson setup`/`ninja`: pre-feature commit and the exact failing commit both pass cleanly (0 failures) on a fresh build; only the main worktree's stale incremental `build/` produced the failure. Fixed with `rm -rf build && meson setup build --buildtype=debug -Db_sanitize=address && ninja -C build`; bug-1407 is now genuinely CLOSED with high confidence. **Lesson for every remaining task in this series**: distrust a surprising/contradictory `ninja -C build` incremental result before concluding a source regression — rebuild clean (or isolate via a throwaway worktree) first. Never use `git checkout -- .` to inspect an old commit in place — it silently reverts working-tree files against current HEAD; use `git worktree add ` instead. Both lessons are now in `.wolf/cerebrum.md` Do-Not-Repeat and `.wolf/buglog.json` bug-1407. diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 9a84bc1..dd9d045 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -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" } ] diff --git a/.wolf/cerebrum.md b/.wolf/cerebrum.md index f0616c3..41147c8 100644 --- a/.wolf/cerebrum.md +++ b/.wolf/cerebrum.md @@ -9,6 +9,14 @@ ## 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 `) 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 ` (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 ` + fresh `meson setup --buildtype=debug -Db_sanitize=address` + `ninja -C ` — 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 -- .`) — 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. diff --git a/src/controller/testsuite/test_commit_resolve.c b/src/controller/testsuite/test_commit_resolve.c index 89d2127..b21461b 100644 --- a/src/controller/testsuite/test_commit_resolve.c +++ b/src/controller/testsuite/test_commit_resolve.c @@ -67,6 +67,22 @@ * last-look-before-timeout tie-break. * * Requirements (task 1.3): 1.3, 1.4, 9.3, 9.4 + * + * Concurrency half (task 1.5, closes task-group 1): the tests above prove + * each detection path (majority ack, timeout, demotion, truncation) works + * one-at-a-time, sequentially. The cases below prove the SAME mechanisms + * hold up TOGETHER, under genuine multi-threaded concurrency: real pthreads + * racing commit_submit_async's admission gate, and real concurrent + * detection paths (a majority-ack-driving thread vs. the engine's own + * dedicated sweep thread) resolving a whole burst of pending continuations + * without any double-resolve, leak, or hang, and without the admission cap + * ever being exceeded. The demotion- and truncation-triggered burst cases + * live alongside their existing single/pair-continuation counterparts in + * test_demotion_convergence.c and test_persist_truncate.c respectively, + * reusing those files' real cluster_t / persist_t fixtures rather than + * duplicating them here. + * + * Requirements (task 1.5): 9.1, 9.2, 9.3, 9.4 */ #include #include @@ -795,6 +811,356 @@ static void test_commit_submit_async_registration_during_dispatch_not_lost(void) "PASS\n"); } +/* ════════════════════════════════════════════════════════════════════════ + * Task 1.5 — concurrent in-flight commits: exactly-once resolution and + * bounded admission under a GENUINE multi-threaded burst (real pthreads, + * not sequential calls dressed up as concurrent). + * ════════════════════════════════════════════════════════════════════════ */ + +/* ------------------------------------------------------------------ * + * (14) Admission-cap burst: a genuine multi-threaded burst of * + * concurrent registrations must never admit more than `cap` * + * simultaneously, even under real contention on ce->lock — not * + * just the two-sequential-call shape existing test (3) exercises. * + * ------------------------------------------------------------------ */ +#define BURST_ADMISSION_N 16 + +typedef struct { + uint64_t lsn; + int rc; + continuation_recorder_t* rec; +} admission_racer_arg_t; + +static void* admission_racer_thread(void* p) +{ + admission_racer_arg_t* a = (admission_racer_arg_t*)p; + + a->rc = commit_submit_async(&g_ce, a->lsn, recorder_fn, a->rec); + return NULL; +} + +/* Samples ce->inflight_waits at a tight interval while the racer burst is + * in flight, recording the highest and lowest values observed — proves the + * admission cap is never breached and inflight_waits never goes negative, + * under REAL concurrent contention, not just at the two checkpoints a + * sequential test would sample. */ +typedef struct { + _Atomic int stop; + int max_seen; + int min_seen; +} admission_monitor_arg_t; + +static void* admission_monitor_thread(void* p) +{ + admission_monitor_arg_t* m = (admission_monitor_arg_t*)p; + + while (!atomic_load(&m->stop)) { + pthread_mutex_lock(&g_ce.lock); + int v = g_ce.inflight_waits; + pthread_mutex_unlock(&g_ce.lock); + if (v > m->max_seen) { + m->max_seen = v; + } + if (v < m->min_seen) { + m->min_seen = v; + } + usleep(50); + } + return NULL; +} + +static void test_commit_submit_async_concurrent_burst_respects_admission_cap(void) +{ + continuation_recorder_t recs[BURST_ADMISSION_N] = {0}; + admission_racer_arg_t args[BURST_ADMISSION_N]; + pthread_t tids[BURST_ADMISSION_N]; + admission_monitor_arg_t mon = {0}; + pthread_t mon_tid; + const int cap = 4; /* max(1, n_vcpus(6) - COMMIT_WAIT_RESERVED(2)) */ + + /* n_vcpus=6 -> local admission cap = 4 (same formula the existing + * sequential test 3 exercises); this proves it holds under genuine + * multi-threaded contention instead of two calls in program order. */ + resolve_setup(/*n_peers=*/3, /*n_standbys=*/2, + /*appended_lsn=*/BURST_ADMISSION_N, /*n_vcpus=*/6); + + TEST_ASSERT_EQUAL_INT( + 0, pthread_create(&mon_tid, NULL, admission_monitor_thread, &mon)); + + for (int i = 0; i < BURST_ADMISSION_N; i++) { + args[i].lsn = (uint64_t)(i + 1); + args[i].rc = -9999; + args[i].rec = &recs[i]; + TEST_ASSERT_EQUAL_INT( + 0, pthread_create(&tids[i], NULL, admission_racer_thread, &args[i])); + } + for (int i = 0; i < BURST_ADMISSION_N; i++) { + pthread_join(tids[i], NULL); + } + atomic_store(&mon.stop, 1); + pthread_join(mon_tid, NULL); + + int accepted = 0, rejected = 0; + for (int i = 0; i < BURST_ADMISSION_N; i++) { + if (args[i].rc == 0) { + accepted++; + } else if (args[i].rc == -XCE_ERR_NOT_READY) { + rejected++; + } else { + TEST_FAIL_MESSAGE("unexpected rc from a concurrent registration"); + } + } + + TEST_ASSERT_EQUAL_INT_MESSAGE( + cap, accepted, + "exactly `cap` concurrent registrations may be admitted, no more, no " + "fewer, even under genuine multi-threaded contention (Req 9.1/9.2)"); + TEST_ASSERT_EQUAL_INT(BURST_ADMISSION_N - cap, rejected); + TEST_ASSERT_EQUAL_INT_MESSAGE(cap, g_ce.inflight_waits, + "inflight_waits must equal exactly the " + "accepted count"); + TEST_ASSERT_TRUE_MESSAGE( + mon.max_seen <= cap, + "inflight_waits must never exceed the admission cap, even sampled " + "mid-burst under real thread contention"); + TEST_ASSERT_TRUE_MESSAGE(mon.min_seen >= 0, + "inflight_waits must never go negative"); + + /* Registration alone, even racing under contention, must never invoke + * any continuation. */ + for (int i = 0; i < BURST_ADMISSION_N; i++) { + TEST_ASSERT_EQUAL_INT(0, atomic_load(&recs[i].calls)); + } + + resolve_teardown(); + printf("test_commit_submit_async_concurrent_burst_respects_admission_cap: " + "PASS\n"); +} + +/* ------------------------------------------------------------------ * + * (15) Concurrent burst resolves exactly once via a MIX of the two * + * real detection paths (majority ack + timeout sweep), racing * + * each other on ce->lock, with dispatch bounded to a small, fixed * + * vtaskmgr worker pool rather than growing with the burst size. * + * ------------------------------------------------------------------ */ + +/* Decouples the continuation registry's capacity (ce_n_vcpus, sized to fit + * the whole burst) from the vtaskmgr pool's actual OS worker-thread count + * (pool_n_vcpus, deliberately smaller than the burst) — resolve_pool_setup + * ties both to the same n_vcpus value, which cannot demonstrate that + * dispatch stays bounded to a small worker set instead of growing with the + * number of concurrently-resolving continuations (Req 9.1/9.2). */ +static void bounded_pool_setup(int n_peers, int n_standbys, uint64_t appended_lsn, + int ce_n_vcpus, int pool_n_vcpus) +{ + resolve_setup(n_peers, n_standbys, appended_lsn, ce_n_vcpus); + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_init(&g_mgr, pool_n_vcpus)); + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_start(&g_mgr)); + commit_set_pool(&g_ce, &g_mgr); +} + +#define BURST_MIX_N 8 + +typedef struct { + uint64_t lsn; + int rc; + continuation_recorder_t* rec; +} mix_register_arg_t; + +static void* mix_register_thread(void* p) +{ + mix_register_arg_t* a = (mix_register_arg_t*)p; + + a->rc = commit_submit_async(&g_ce, a->lsn, recorder_fn, a->rec); + return NULL; +} + +/* Count pairwise-distinct pthread_t values in threads[0..n) — proves + * dispatch is bounded to a small worker set rather than one thread per + * resolved continuation (pthread_t is opaque; must compare via + * pthread_equal, never ==). */ +static int count_distinct_threads(const pthread_t* threads, int n) +{ + int distinct = 0; + + for (int i = 0; i < n; i++) { + int seen_before = 0; + + for (int j = 0; j < i; j++) { + if (pthread_equal(threads[i], threads[j])) { + seen_before = 1; + break; + } + } + if (!seen_before) { + distinct++; + } + } + return distinct; +} + +static void test_commit_resolve_concurrent_burst_majority_and_timeout_exactly_once(void) +{ + continuation_recorder_t recs[BURST_MIX_N] = {0}; + mix_register_arg_t args[BURST_MIX_N]; + pthread_t tids[BURST_MIX_N]; + pthread_t dispatch_threads[BURST_MIX_N]; + + /* ce_n_vcpus == BURST_MIX_N: the continuation registry has exactly + * enough slots for the whole burst, none to spare. pool_n_vcpus=3: a + * small, fixed, REAL vtaskmgr worker pool that must service all 8 + * resolutions — if dispatch spawned a thread per resolution instead of + * reusing the bounded pool, more than 3 distinct threads would appear + * below. */ + bounded_pool_setup(/*n_peers=*/3, /*n_standbys=*/2, + /*appended_lsn=*/BURST_MIX_N, + /*ce_n_vcpus=*/BURST_MIX_N, /*pool_n_vcpus=*/3); + + /* Arrange: register the whole burst CONCURRENTLY from real threads. */ + for (int i = 0; i < BURST_MIX_N; i++) { + args[i].lsn = (uint64_t)(i + 1); + args[i].rc = -9999; + args[i].rec = &recs[i]; + TEST_ASSERT_EQUAL_INT( + 0, pthread_create(&tids[i], NULL, mix_register_thread, &args[i])); + } + for (int i = 0; i < BURST_MIX_N; i++) { + pthread_join(tids[i], NULL); + } + for (int i = 0; i < BURST_MIX_N; i++) { + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, args[i].rc, + "every concurrent registration must succeed -- the registry has " + "exactly enough slots for this burst"); + } + TEST_ASSERT_EQUAL_INT(BURST_MIX_N, g_ce.inflight_waits); + + /* Act: satisfy majority for the LOWER half (lsn 1..4) via the REAL + * ack-driven path (repl_on_ack + commit_on_ack, exactly what task 1.2 + * wired) -- committed_lsn advances contiguously to 4, resolving lsn + * 1..4 as COMMITTED in one bulk commit_resolve_pending() scan. The + * UPPER half (lsn 5..8) is never ACKed, so it is left entirely to the + * real timeout-sweep thread (task 1.3). Both detection paths now race + * each other for ce->lock concurrently: this ack-driving thread and the + * commit engine's own dedicated sweep thread (which independently scans + * the SAME registry and would classify lsn 1..4 as majority-satisfied + * too, if its own scan happens to run before this thread's + * commit_resolve_pending() call reaches them first -- commit_resolve's + * idempotent slot-clear-under-lock is what makes that real race safe). */ + repl_on_ack(&g_p.repl, &g_eps[0], BURST_MIX_N / 2); + commit_on_ack(&g_ce, 0, BURST_MIX_N / 2); + + for (int i = 0; i < BURST_MIX_N; i++) { + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, poll_calls(&recs[i].calls, 1, DEFAULT_SYNC_TIMEOUT_MS + 2000), + "every continuation in the burst must resolve, through one of " + "the two real concurrent detection paths"); + } + + for (int i = 0; i < BURST_MIX_N; i++) { + /* Exactly-once: no double delivery for any lsn in the burst. */ + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, atomic_load(&recs[i].calls), + "no continuation in the burst may be resolved more than once"); + dispatch_threads[i] = recs[i].last_thread; + + if (i < BURST_MIX_N / 2) { + TEST_ASSERT_EQUAL_INT_MESSAGE( + COMMIT_OUTCOME_COMMITTED, recs[i].last_outcome, + "the lower half must resolve COMMITTED via the real " + "majority-ack path"); + TEST_ASSERT_EQUAL_INT(0, recs[i].last_rc); + } else { + TEST_ASSERT_EQUAL_INT_MESSAGE( + COMMIT_OUTCOME_FAILED, recs[i].last_outcome, + "the upper half, never acked, must resolve FAILED via the " + "real timeout-sweep path"); + TEST_ASSERT_EQUAL_INT(-ETIMEDOUT, recs[i].last_rc); + } + } + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, g_ce.inflight_waits, + "every admission slot in the burst must be released exactly once"); + + /* Bounded worker usage (Req 9.1/9.2): all BURST_MIX_N resolutions must + * be dispatched across no more than the pool's 3 real worker threads -- + * proving the burst is serviced by a small, fixed pool rather than one + * thread spawned per resolved continuation. */ + int distinct = count_distinct_threads(dispatch_threads, BURST_MIX_N); + TEST_ASSERT_TRUE_MESSAGE( + distinct <= 3, + "continuation dispatch for the whole burst must stay within the " + "bounded vtaskmgr worker pool, not grow with the burst size"); + + resolve_pool_teardown(); + printf("test_commit_resolve_concurrent_burst_majority_and_timeout_exactly_" + "once: PASS\n"); +} + +/* ------------------------------------------------------------------ * + * (16, nice-to-have) Genuine two-thread race: a real second thread * + * delivers a real majority ACK right at the edge of the * + * registration's own (unmanipulated) DEFAULT_SYNC_TIMEOUT_MS * + * deadline, racing the commit engine's own dedicated sweep * + * thread for ce->lock -- unlike test (12) above, which proves the * + * same tie-break with single-thread white-box deadline * + * manipulation, this drives it with two REAL OS threads. * + * ------------------------------------------------------------------ */ +typedef struct { + uint64_t lsn; +} delayed_ack_arg_t; + +static void* delayed_ack_thread(void* p) +{ + delayed_ack_arg_t* a = (delayed_ack_arg_t*)p; + + /* Sleep to just before lsn's own real deadline (150ms of margin absorbs + * scheduler jitter under ASAN). commit_advance_locked() broadcasts + * ce->cond on every real advance, so this ACK wakes the sweep thread + * EARLY -- well before its own timed-out wait would naturally fire -- + * and the sweep thread's re-scan finds wait_done_locked() satisfied + * first (majority wins, per commit_sweep_scan_locked's own check + * order). */ + usleep((DEFAULT_SYNC_TIMEOUT_MS - 150) * 1000); + repl_on_ack(&g_p.repl, &g_eps[0], a->lsn); + commit_on_ack(&g_ce, 0, a->lsn); + return NULL; +} + +static void test_commit_resolve_genuine_two_thread_race_majority_wins_over_timeout(void) +{ + continuation_recorder_t rec = {0}; + delayed_ack_arg_t arg = {.lsn = 1}; + pthread_t tid; + + resolve_pool_setup(/*n_peers=*/3, /*n_standbys=*/2, /*appended_lsn=*/1, + /*n_vcpus=*/4); + TEST_ASSERT_EQUAL_INT(0, commit_submit_async(&g_ce, 1, recorder_fn, &rec)); + + TEST_ASSERT_EQUAL_INT(0, pthread_create(&tid, NULL, delayed_ack_thread, &arg)); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, poll_calls(&rec.calls, 1, DEFAULT_SYNC_TIMEOUT_MS + 2000), + "the raced lsn must still resolve exactly once"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + COMMIT_OUTCOME_COMMITTED, rec.last_outcome, + "a genuine majority ACK landing near the deadline boundary, on a " + "real second thread racing the real sweep thread, must still win " + "over the timeout -- exactly-once, majority-wins under REAL " + "two-thread concurrency (not the white-box simulation in test 12)"); + TEST_ASSERT_EQUAL_INT(0, rec.last_rc); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, atomic_load(&rec.calls), + "exactly one resolution, regardless of which real thread won"); + TEST_ASSERT_EQUAL_INT(0, g_ce.inflight_waits); + + pthread_join(tid, NULL); + resolve_pool_teardown(); + printf("test_commit_resolve_genuine_two_thread_race_majority_wins_over_" + "timeout: PASS\n"); +} + void test_commit_resolve(void) { RUN_TEST(test_commit_submit_async_returns_immediately_before_commit); @@ -812,4 +1178,7 @@ void test_commit_resolve(void) RUN_TEST(test_commit_submit_async_resolves_failed_on_timeout); RUN_TEST(test_commit_submit_async_majority_wins_race_at_expired_deadline); RUN_TEST(test_commit_submit_async_registration_during_dispatch_not_lost); + RUN_TEST(test_commit_submit_async_concurrent_burst_respects_admission_cap); + RUN_TEST(test_commit_resolve_concurrent_burst_majority_and_timeout_exactly_once); + RUN_TEST(test_commit_resolve_genuine_two_thread_race_majority_wins_over_timeout); } diff --git a/src/controller/testsuite/test_demotion_convergence.c b/src/controller/testsuite/test_demotion_convergence.c index 4d73b1d..f5e4756 100644 --- a/src/controller/testsuite/test_demotion_convergence.c +++ b/src/controller/testsuite/test_demotion_convergence.c @@ -458,6 +458,88 @@ static void test_demotion_from_primary_resolves_pending_continuations_failed(voi "checked\n"); } +/* raft-apply-decoupling task 1.5 (Req 9.1-9.4): a CONCURRENT burst of + * pending continuations, registered from real pthreads racing each other + * for ce->lock, must ALL resolve exactly once (FAILED/-XCE_ERR_STANDBY) + * through the SAME real demotion detection path the test above exercises + * for a fixed pair -- proving commit_resolve_pending_above's + * snapshot-then-resolve loop holds against a registry populated under + * genuine multi-threaded contention, not just two sequential registrations. */ +#define DC_BURST_N 10 + +typedef struct { + uint64_t lsn; + int rc; + dc_continuation_recorder_t* rec; +} dc_burst_arg_t; + +static void* dc_burst_register_thread(void* p) +{ + dc_burst_arg_t* a = (dc_burst_arg_t*)p; + + a->rc = commit_submit_async(&g_dc.commit, a->lsn, dc_recorder_fn, a->rec); + return NULL; +} + +static void test_demotion_concurrent_burst_resolves_all_pending_exactly_once(void) +{ + dc_continuation_recorder_t recs[DC_BURST_N] = {0}; + dc_burst_arg_t args[DC_BURST_N]; + pthread_t tids[DC_BURST_N]; + + dc_setup("burst", /*n_peers=*/3, /*n_standbys=*/2, + /*appended_lsn=*/DC_BURST_N + 4, /*n_vcpus=*/16); + atomic_store(&g_dc.persist.committed_lsn, 4); + + /* Register a burst of DC_BURST_N continuations CONCURRENTLY from real + * threads, all for LSNs above the committed floor (4) -- majority is + * unreachable in this fixture (standbys never ACK), so every one stays + * genuinely pending, not already resolved, by the time demotion fires. */ + for (int i = 0; i < DC_BURST_N; i++) { + args[i].lsn = (uint64_t)(5 + i); + args[i].rc = -9999; + args[i].rec = &recs[i]; + TEST_ASSERT_EQUAL_INT( + 0, pthread_create(&tids[i], NULL, dc_burst_register_thread, &args[i])); + } + for (int i = 0; i < DC_BURST_N; i++) { + pthread_join(tids[i], NULL); + } + for (int i = 0; i < DC_BURST_N; i++) { + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, args[i].rc, + "every concurrent registration ahead of the demotion must succeed"); + } + TEST_ASSERT_EQUAL_INT(DC_BURST_N, g_dc.commit.inflight_waits); + + /* Act: a single real demotion edge. The burst above is already fully + * registered by the time this fires -- this proves the BULK resolve + * path handles a registry populated by real concurrent registrants + * correctly, not just the pre-existing two-continuation case above. */ + raft_cb_become_follower_with_cluster_for_test(&g_dc, /*term=*/9); + + for (int i = 0; i < DC_BURST_N; i++) { + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, atomic_load(&recs[i].calls), + "every continuation in the burst must resolve exactly once on " + "demotion"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + (int)COMMIT_OUTCOME_FAILED, (int)recs[i].last_outcome, + "demotion must resolve every pending continuation as FAILED"); + TEST_ASSERT_EQUAL_INT_MESSAGE(-XCE_ERR_STANDBY, recs[i].last_rc, + "demotion must use the existing -XCE_ERR_STANDBY vocabulary"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE((uint64_t)(5 + i), recs[i].last_lsn, + "the recorder must observe the lsn it was registered for"); + } + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, g_dc.commit.inflight_waits, + "every admission slot in the burst must be released exactly once"); + + dc_teardown(); + printf("test_demotion_concurrent_burst_resolves_all_pending_exactly_once: " + "checked\n"); +} + /* Regression: demotion from STANDBY must NOT truncate, NOT restart, NOT disable * hooks beyond today's behavior — byte-equivalent to the pre-spec path. A * STANDBY has no registered hooks and no convergence work. */ @@ -897,6 +979,7 @@ void test_demotion_convergence(void) RUN_TEST(test_demotion_from_primary_disables_hooks); RUN_TEST(test_demotion_from_primary_resolves_pending_continuations_failed); + RUN_TEST(test_demotion_concurrent_burst_resolves_all_pending_exactly_once); RUN_TEST(test_demotion_schedules_convergence_truncate_and_restart); RUN_TEST(test_self_restart_failure_invokes_fatal_hook); RUN_TEST(test_demotion_from_standby_no_truncation_no_restart); diff --git a/src/controller/testsuite/test_persist_truncate.c b/src/controller/testsuite/test_persist_truncate.c index 7f04894..ab08b7d 100644 --- a/src/controller/testsuite/test_persist_truncate.c +++ b/src/controller/testsuite/test_persist_truncate.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -375,6 +376,118 @@ static void test_truncate_noop_does_not_resolve_pending_continuations(void) printf("test_truncate_noop_does_not_resolve_pending_continuations: PASS\n"); } +/* ── Concurrent burst (raft-apply-decoupling task 1.5, Req 9.1-9.4) ──────── + * Many continuations, registered from real threads racing each other for + * ce->lock and split across the truncation cutoff, must ALL resolve exactly + * once through the SAME real persist_truncate_wal_above detection path + * exercised above: entries above the new tail FAILED/-ENOENT, entries at/ + * below left untouched -- proving the bulk resolve holds under genuine + * concurrent registration, not just the fixed handful the tests above use. */ +#define TRUNC_BURST_ABOVE 6 +#define TRUNC_BURST_BELOW 3 +#define TRUNC_BURST_TOTAL (TRUNC_BURST_ABOVE + TRUNC_BURST_BELOW) + +typedef struct { + commit_engine_t* ce; + uint64_t lsn; + int rc; + trunc_continuation_recorder_t* rec; +} trunc_burst_arg_t; + +static void* trunc_burst_register_thread(void* p) +{ + trunc_burst_arg_t* a = (trunc_burst_arg_t*)p; + + a->rc = commit_submit_async(a->ce, a->lsn, trunc_recorder_fn, a->rec); + return NULL; +} + +static void test_truncate_concurrent_burst_resolves_above_cutoff_exactly_once(void) +{ + char tmpdir[] = "/tmp/xce-trunc-burst-XXXXXX"; + TEST_ASSERT_NOT_NULL(mkdtemp(tmpdir)); + + persist_t *p = make_fresh_persist(tmpdir); + TEST_ASSERT_NOT_NULL(p); + + commit_engine_t ce; + TEST_ASSERT_EQUAL_INT(0, + commit_engine_init(&ce, p, /*n_peers=*/3, /*n_vcpus=*/TRUNC_BURST_TOTAL + 4)); + + int dummy = 0; + for (int i = 0; i < TRUNC_BURST_TOTAL + 2; i++) + persist_wal_sync_append(p, WAL_JOB_SUBMIT, &dummy, sizeof(dummy), NULL); + + /* Cutoff placed so lsn 1..TRUNC_BURST_BELOW sit at/below the new tail + * and lsn (TRUNC_BURST_BELOW+1)..TRUNC_BURST_TOTAL sit above it. */ + const uint64_t cutoff = TRUNC_BURST_BELOW; + + trunc_continuation_recorder_t recs[TRUNC_BURST_TOTAL] = {0}; + trunc_burst_arg_t args[TRUNC_BURST_TOTAL]; + pthread_t tids[TRUNC_BURST_TOTAL]; + + /* Register the whole burst -- below-cutoff and above-cutoff LSNs alike + * -- CONCURRENTLY from real threads. */ + for (int i = 0; i < TRUNC_BURST_TOTAL; i++) { + args[i].ce = &ce; + args[i].lsn = (uint64_t)(i + 1); + args[i].rc = -9999; + args[i].rec = &recs[i]; + TEST_ASSERT_EQUAL_INT( + 0, pthread_create(&tids[i], NULL, trunc_burst_register_thread, &args[i])); + } + for (int i = 0; i < TRUNC_BURST_TOTAL; i++) { + pthread_join(tids[i], NULL); + } + for (int i = 0; i < TRUNC_BURST_TOTAL; i++) { + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, args[i].rc, + "every concurrent registration ahead of the truncation must succeed"); + } + TEST_ASSERT_EQUAL_INT(TRUNC_BURST_TOTAL, ce.inflight_waits); + + /* Act: a single real truncation above cutoff. The burst above is + * already fully registered by the time this fires -- this proves the + * BULK resolve path handles a registry populated by real concurrent + * registrants correctly, not just the fixed three-continuation case + * above. */ + int rc = persist_truncate_wal_above(p, cutoff); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_UINT64(cutoff + 1, p->current_lsn); + + for (int i = 0; i < TRUNC_BURST_TOTAL; i++) { + uint64_t lsn = (uint64_t)(i + 1); + + if (lsn > cutoff) { + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, atomic_load(&recs[i].calls), + "every continuation above the new tail must resolve exactly " + "once on truncation, even when the whole burst was " + "registered concurrently"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + (int)COMMIT_OUTCOME_FAILED, (int)recs[i].last_outcome, + "truncation must resolve as FAILED"); + TEST_ASSERT_EQUAL_INT_MESSAGE(-ENOENT, recs[i].last_rc, + "truncation must use the existing -ENOENT vocabulary"); + TEST_ASSERT_EQUAL_UINT64(lsn, recs[i].last_lsn); + } else { + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, atomic_load(&recs[i].calls), + "a continuation at/below the new tail must NOT be resolved " + "by truncation, even under concurrent registration"); + } + } + TEST_ASSERT_EQUAL_INT_MESSAGE( + TRUNC_BURST_BELOW, ce.inflight_waits, + "only the above-cutoff admission slots are released by truncation; " + "the below-cutoff ones remain pending"); + + commit_engine_fini(&ce); + teardown_persist(p); + printf("test_truncate_concurrent_burst_resolves_above_cutoff_exactly_once: " + "PASS\n"); +} + /* ── Test group entry point ────────────────────────────────────────────────── */ void test_persist_truncate(void) @@ -387,5 +500,6 @@ void test_persist_truncate(void) RUN_TEST(test_truncate_noop_at_current_lsn); RUN_TEST(test_truncate_above_resolves_pending_continuations_as_failed); RUN_TEST(test_truncate_noop_does_not_resolve_pending_continuations); + RUN_TEST(test_truncate_concurrent_burst_resolves_above_cutoff_exactly_once); UNITY_END(); }