From 16b890b1af6265d315e95226db3b1fc4bd134060 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 7 Jul 2026 03:38:05 +0000 Subject: [PATCH] test(raft-apply-decoupling): update stale apply-before-commit assertions (task 7.3) Full 79-group unit/integration suite re-run found exactly 2 stale tests in test_commit_barrier_reply.c asserting task 3.2's documented interim contract ("no observable effect until task 6 completes it"), which the already-committed ordered apply pump (tasks 6.1/6.2) closed by construction. Updated both to assert the real, deterministic apply-after-commit settled state instead of the no-longer-true no-effect-yet state, matching the idiom already used elsewhere in the same file. No production code changed. Full suite: 0 failures across 79 groups. HA soak (--sequential, 5 rounds): zero committed-job loss, zero divergence, all 3 nodes served as primary. Both independently re-verified by adversarial review. --- .kiro/specs/raft-apply-decoupling/tasks.md | 3 +- .../testsuite/test_commit_barrier_reply.c | 137 +++++++++--------- 2 files changed, 74 insertions(+), 66 deletions(-) diff --git a/.kiro/specs/raft-apply-decoupling/tasks.md b/.kiro/specs/raft-apply-decoupling/tasks.md index 1e2c6fc9..6906834f 100644 --- a/.kiro/specs/raft-apply-decoupling/tasks.md +++ b/.kiro/specs/raft-apply-decoupling/tasks.md @@ -172,7 +172,7 @@ - _Requirements: 10.2, 10.4_ - _Depends: 6.3, 6.4_ -- [ ] 7.3 Re-run the full existing automated test suite and update only the tests tied to the old ordering +- [x] 7.3 Re-run the full existing automated test suite and update only the tests tied to the old ordering - Run the existing unit, integration, and HA soak suites - Update only the specific tests that asserted the old apply-before-commit ordering to assert the new ordering instead - Observable: the full suite passes with zero committed-job loss and zero state divergence across the existing failover soak @@ -196,6 +196,7 @@ - **1.2**: implemented `commit_resolve()` + wired it into all 6 commit.c-internal paths that can advance `committed_lsn`/`writable_lsn` (the COMMITTED/majority-ack outcome). Chose `VTASK_LIVE_BLOCK` for dispatch (never admission-gated, unlike `VTASK_REQ_BLOCK` which can fast-reject — unacceptable for "exactly-once, never unresolved"). **Reviewer caught a real task-graph gap, not a defect in 1.2 itself**: converting `COMMIT_MODE_WAIT` away from a blocking wait also removes the *only* mechanism that ever detected a commit timing out (the caller's own `pthread_cond_timedwait` deadline) — nothing yet arms an equivalent timeout for async-registered continuations, and nothing yet wires demotion/log-truncation detection to call `commit_resolve(FAILED, ...)`. Without this, a pending async command during quorum loss would hang forever instead of failing — worse than today's behavior. Split out as new tasks **1.3** (timeout arming) and **1.4** (demotion/truncation wiring), inserted before the renumbered **1.5** (was 1.3, concurrency test — now exercises the real detection paths from 1.3/1.4 instead of calling `commit_resolve()` directly). Task 3.2 now also depends on 1.3/1.4, since its own "failure/timeout outcome replies" observable is meaningless without them. - **1.3**: added a dedicated per-commit-engine sweep thread (not the shared `delaymgr` — would violate boundary + delaymgr's 200ms poll granularity vs. `DEFAULT_SYNC_TIMEOUT_MS`=500ms would add timing jitter). Race rule: majority-satisfied always wins over an already-expired deadline when both are true under the same lock hold. **Round 1 REJECTED**: `commit_sweep_once()` computed the next-deadline snapshot under `ce->lock`, unlocked to dispatch, then slept on the *stale* snapshot — a `commit_submit_async()` registration/broadcast landing during that unlocked dispatch window was lost (no thread parked on the condvar to receive it), so the sweep thread could sleep on an absent/late deadline indefinitely (worst case: unbounded delay in a low-traffic quorum-loss scenario, violating the "identical failure timing" no-regression bar). **Fixed** by holding `ce->lock` continuously across scan→wait decisions (releasing only for the unlocked dispatch step) and always re-scanning under the lock immediately before ever calling `cond_wait`/`cond_timedwait` — the standard recheck-then-wait idiom. Round 2 reviewer independently reproduced the round-1 bug shape to confirm the fix actually closes it, not just narrows it. **Precedent for 1.4**: any future code that needs to interrupt the sweep thread's wait (e.g., a demotion/truncation handler resolving a pending LSN out-of-band) must go through the same lock+broadcast discipline `commit_submit_async()` uses, or it risks the identical lost-wakeup class of bug. - **1.4**: added `commit_resolve_pending_above(ce, lsn_floor, rc)` (snapshot-then-`commit_resolve()`-per-entry, no parallel bookkeeping). Truncation: a single new `on_truncate`/`truncate_cb_ctx` callback slot on `persist_t` (same inversion-of-control shape as the existing `on_durable_advance` hook) fires from `persist_truncate_wal_above` after releasing `p->lock`, only when a truncation actually removed entries — this ONE wrapper covers all three real truncation call sites (`raft_cb_become_leader`'s post-election truncate, the library's `on_truncate_from` conflict-reconcile, `ha_demotion_convergence_fn`'s post-demotion truncate), verified by the reviewer reading all three. Demotion: `raft_cb_become_follower` calls `commit_resolve_pending_above(&c->commit, 0, -XCE_ERR_STANDBY)` (lsn_floor=0 → fails everything — a demoted node can never authoritatively confirm anything pending; verified against the CP posture). **Known residual, non-blocking**: a fourth internal truncation path (`persist.c` inside `persist_wal_write_replicated`'s tip-conflict-repair branch, STANDBY replicated-apply only) calls `truncate_wal_above_locked()` directly, bypassing the new wrapper — safe today only because demotion resolves ALL pending continuations before role flips to STANDBY (so nothing can still be registered by the time that path runs), but this is an *implicit* ordering invariant, not an asserted one. Flag for whoever next touches the STANDBY replicated-apply path: add a defensive comment/assertion there, or route it through the wrapper too, before relying on it further. Also: bug-1407 was believed closed at this point (70/70 across 3 runs) — **see 1.5's note below, this was a false reading, not a real fix**. +- **7.3**: closes out task-group 7's regression gate. Full re-run confirmed exactly 2 of 33 tests in `test_commit_barrier_reply.c` failing — `test_jobsubmit_returns_before_commit_and_registers_pending_apply` and `test_jobsubmit_array_mixed_outcome_no_phantom_job_for_failed_elements` — both asserting task 3.2's own explicitly-documented interim contract ("no independently observable behavior until task 6 completes it") that tasks 6.1/6.2 (already committed, ordered apply pump) closed by construction. Updated both to assert the new deterministic settled state (poll for reply, then assert success/job-exists), mirroring the idiom already used by the file's other, already-updated passing tests — no production code touched. One judgment call flagged and independently confirmed by reviewer: in the mixed-outcome array test, a committed sibling now applies (job visible) independently of a differently-failing sibling, while the batch's single client-visible reply still reflects the failure — this is the documented, intended interim behavior (collapsing array submission into one atomic WAL record is explicitly out-of-scope, owned by the separate `job-array-single-wal-record` spec), not a regression or scope violation. Verified (implementer + independent reviewer, both fresh runs): `test_commit_barrier_reply` 33/33 pass (reviewer ran 3x to rule out flakiness); full 79-group/926-test suite 0 failures including both bare-binary runs (reviewer ran the full unfiltered `controller_unit_test` binary directly and it passed clean, even through the two tests flagged elsewhere in this file's notes as environment-flaky, bug-1406/1407 — no regression signal, strengthens confidence); `/xce-test-ha --sequential 5` HA soak PASS twice independently (implementer's own run + reviewer's fresh re-run in a separate scratch dir), zero job loss, zero divergence, all 3 nodes led both times. - **7.2**: added `--factstream` mode to `run-ha-soak.sh` (kill-mid-fact-stream against a live 3-node cluster, real `jstart`/`jfinish`/`jfail` wire traffic via `xce_test`'s `worker` REPL context, extended in this task since libxce already had the calls but the REPL didn't expose them) and a phantom-fact checker that reads `waldump`'s decoded `state_raw` bits (extended in this task to decode every worker-fact WAL type, not just `WAL_JOB_SUBMIT`) rather than inferring "did START apply" from the pretty-printed job state string — `job_state_str()`'s RUN-checked-before-EXIT priority ordering hides co-set EXIT/PERR bits on a fail-without-finish job, a genuine phantom-check false-positive caught and fixed here (pre-existing, minor, display-only quirk in `job_state_str` itself is out of boundary, not touched). **Round 2 found a real production bug this task's own scenario was needed to surface**: bug-1416, a boot-time TOCTOU in `persist_verify_apply_correctness()` — a returning node's `persist_recover()` replay+verify pass can race the live apply pump and the standby live-replication apply path (both write the same `last_applied_lsn`/job-table state `persist_verify_apply_correctness` re-reads live), producing a false `APPLY_DIVERGENT` that permanently, incorrectly fences a healthy node (task 5.3's sticky fence never self-clears on a false positive) — reproduced 10/10 across ASAN/release builds, 8-20 job counts, only on the RESTART/catch-up path (a clean 3-node cold boot with no kill never hit it). Fixed with a new `_Atomic bool persist_t.recovery_complete` gate (`persist.h`/`persist.c`): `persist_apply_pump_kick()` no-ops while closed; `ha_apply_single_entry`/`ha_apply_term_entry_via_library` (`ha.c`) skip only the in-memory `apply_wal_entry()` call while closed — the durability write, replication ACK, and `applied_lsn` advance are never gated (mirrors the bug-1043 durability/materialization-decoupling precedent, confirmed in `.wolf/cerebrum.md` since bug-1043 predates the current buglog retention window). `persist_recover()` closes the gate at entry, reopens it plus issues one deliberate catch-up kick right before returning, unconditionally of verdict — a genuine divergence still fences exactly as before, this flag only prevents the *comparison itself* from racing. **A second, harder-to-find gap surfaced only via live verification, not code review**: a node stuck retrying its own losing/timing-out election as a candidate can receive and durably apply live-replicated entries through the *ungated* (`persist_init` default=`true`) path for tens of seconds before `cluster_init_phase2()` ever calls `persist_recover()` for the first time — reproducing the identical false-DIVERGENT symptom through an entry point `persist_recover()`'s own bracket can't see. Closed with `persist_mark_recovery_pending(&c.persist)` in `myapp_run()` (`myapp.c`), called immediately after `persist_init()`/`persist_start()` succeed and before `ha_start_election_async()`/`cluster_run()` can process any network traffic. `persist_init()`'s default stays `true` so none of this spec's ~20 direct-`persist_t`-construction unit fixtures are affected — `persist_mark_recovery_pending` is called only from `myapp.c`. **Round-2 review independently verified, not just re-reported**: ran `test_apply_correctness_verification` itself (11/11, including all 3 real-divergence fence tests, confirming detection wasn't accidentally suppressed), confirmed the ACK/durability paths are genuinely ungated by reading the `ha.c` call sites directly, confirmed the `myapp.c` fix is load-bearing (not noise) by reverting only that hunk and reproducing the original failure live (2 of 3 nodes fenced), confirmed bug-1043 precedent against `.wolf/cerebrum.md` (not in `.wolf/buglog.json` — predates current retention). Verified: live 3-node `--factstream` soak (2 rounds, kill+restart both) PASS, zero `APPLY_DIVERGENT` across 5 `persist_recover()` calls; broader `controller_unit_test` sweep (23 groups) zero new failures (only the 2 pre-existing task-6.1/6.2-documented `test_commit_barrier_reply.c` failures reserved for task 7.3 remain). **Do-not-repeat lesson**: when gating a live path behind a "recovery in progress" flag, the flag's own bracket around the recovery function itself is insufficient if OTHER code paths (election retry, live replication) can mutate the same state before that function is ever first called — the gate must close at the earliest point in the real process lifecycle traffic could possibly begin, not just around the specific function whose output it protects. - **6.4**: closes out task-group 6. Approved first round, zero production changes — group ends 2-for-4 on real bugs found (6.1, 6.2), 2-for-4 clean confirmations (6.3, 6.4). Extended `test_apply_pump_integration.c` (task 6.3) with 2 tests covering the MIXED facts+commands scenario 6.3 explicitly deferred here. **Test 3**: genuinely interleaved COMMAND/FACT/COMMAND/FACT/COMMAND sequence through a real `repl_remove_standby` outage, polls for zero commit/apply/reply throughout, restores quorum, and asserts STRICT original-log-order catch-up via a real append-only sequence spy (not a set-membership check) — an adversarial probe (corrupting the expected slot-0 jobid) was independently reproduced by the reviewer to confirm the assertion genuinely discriminates reordering, not just typos. **Test 4 — the highest-value test in this task**: wires a REAL 4-vcpu `vtaskmgr_t` (mirroring task 6.2's bug-1415 regression harness) to drive 4 commands + 2 facts through ONE covering ACK, independently traced by the reviewer to confirm this genuinely creates 5 vtasks (4 continuation dispatches + 1 apply-pump walk) competing for 4 real worker threads — true N-way concurrent racing of task 6.2's self-healing fix, not just one isolated forced pair like 6.2's own test. The "exactly once" check is per-jobid (not an aggregate total), so a skewed double-fire-on-one/hang-on-another distribution would be caught even though the aggregate count would look fine. Adversarial probe (corrupting expected reply count) independently reproduced. **Two honestly-scoped, explicitly non-blocking caveats, not gaps requiring fixes**: (1) task 6.2's own reviewer flagged a narrow, nanosecond-scale TOCTOU residual (unlocked self-heal check vs. locked list-link in `persist_register_pending_apply()`) as a non-blocking fast-follow — Test 4 does NOT deliberately stress this specific window (no injected synchronization primitive forces it, unlike 6.2's own barrier-based test); 50 clean runs across two independent builds are consistent with "didn't hit it by chance," not proof it's closed — stated explicitly here so it isn't assumed covered by this task's "several concurrent races" framing. (2) Test 4 doesn't directly re-assert per-command reply-after-apply ordering the way Test 1 (task 6.3) does via `reply_already_enqueued` — this property holds BY CONSTRUCTION (both the walk's take-call and the self-heal's `lsn <= last_applied_lsn` check only ever fire after `last_applied_lsn` already reflects that entry, verified in the actual code) and is independently covered by task 6.2's own dedicated test, so this is non-blocking, not a real gap. **Task-group 6 final summary**: the ordered apply pump is now live, activated (6.1), correctly hands off command replies (6.2), proven correct for both isolated (6.3) and mixed/bursty (6.4) command+fact scenarios under both normal and quorum-loss conditions — with 2 real, independently-verified, production-severity bugs found and fixed along the way (6.1's job-state corruption from stale receipt-time computation; 6.2's client-hang race from unordered async dispatch), both closed with dedicated regression coverage, the first ever in this spec to wire a real multi-worker `vtaskmgr_t`. **Optional fast-follow noted, not blocking**: a dedicated stress test that deliberately widens the `persist_register_pending_apply()` check-to-lock window (e.g. via a test-only injection point) to reliably exercise 6.2's disclosed residual TOCTOU gap, since neither 6.2's nor 6.4's tests reliably reach it today. - **6.3**: pure integration-test close-out, approved first round, zero production changes — breaks task-group 6's 2-for-2 real-bug streak (6.1, 6.2 each found genuine production-severity bugs; 6.3 confirmed the pump's existing behavior is correct rather than finding a new one). New `test_apply_pump_integration.c` (2 tests) exercises the FULL live pump (real commit engine, real apply dispatch, real hook subscribers — `pool=NULL` like every other fixture, does not overclaim bug-1415/6.2's dispatch-race coverage, which is that test's own job) for both a job submission and a fact. **Key design decision, independently verified true by the reviewer, not just asserted**: `apply_committed_entry()` fires ALL job-state facts via the generic `HOOK_DETAIL_JOB_UPDATE` only (task 2.2's documented deviation, confirmed UNCHANGED by 6.1's `_pad` fix — that fix changed which state-computation core runs, not which hook detail fires) — so `sched_job_start_fun`/`_exit_fun`/`_fail_fun`/apiserver.c's equivalents are structurally unreachable for any live-applied fact, meaning ONE reaction spy wired to `HOOK_DETAIL_JOB_NEW`+`HOOK_DETAIL_JOB_UPDATE` legitimately covers all three named reactions (worker slot release, scheduler dispatch trigger, client/apiserver notify) since they're all downstream of the identical single firing. **Strong independent confirmation this is load-bearing, not incidental**: the type-specific handlers are unconditionally `va_arg`-typed for `job_pidinfo_t*`/`xce_jobfinish_t*`/`job_failinfo_t*` — types `apply_committed_entry()` never passes (always `job_t*`) — so if a type-specific hook ever DID fire from the live pump, it would be an immediate type-confusion crash, not just a coverage gap. Repo-wide re-audit (not just job.c/persist.c) confirms exactly 2 live call sites ever fire these hook details: `apply_committed_entry()` (guarded) and `job_ctrl`'s `mark_job_update` (legitimate SIGCONT/SIGSTOP) — no orphaned third site. Both tests prove REAL ordering, not coincidental timing: reply-after-visibility is sampled synchronously inside the reaction-spy callback (not two separately-timed checks); Req 4.3's "no separate watermark re-check needed" is proven by construction (the walk only ever invokes `apply_committed_entry()` for LSNs already `<= committed_lsn`, so `committed_lsn_at_fire >= entry_lsn` holds structurally, not by scheduling luck) rather than merely asserted in prose. Delayed-commit and genuine quorum-loss windows are both polled repeatedly across real time (5×20ms), not checked once. RED phase independently reproduced by both implementer and reviewer via re-injecting the pre-4.1 eager-mutation bug class. **Task-group 6 running tally**: 3 tasks in, 2 real production-severity bugs found and fixed (6.1 data corruption, 6.2 client-hang race), 1 clean confirmation (6.3) — the pump's core mechanics and hook-routing design are now thoroughly adversarially tested from multiple independent angles. Remaining: 6.4 (quorum-loss catch-up ordering, mixing facts AND commands — 6.3's fact-only quorum-loss scenario is explicitly non-duplicative defense-in-depth per the reviewer, not a preemption of 6.4's charter). diff --git a/src/controller/testsuite/test_commit_barrier_reply.c b/src/controller/testsuite/test_commit_barrier_reply.c index c2ff2926..754fe31a 100644 --- a/src/controller/testsuite/test_commit_barrier_reply.c +++ b/src/controller/testsuite/test_commit_barrier_reply.c @@ -922,21 +922,30 @@ static void test_jobsubmit_withheld_acks_no_phantom_job(void) printf("test_jobsubmit_withheld_acks_no_phantom_job: PASS\n"); } -/* raft-apply-decoupling task 3.2: supersedes the pre-3.2 - * test_jobsubmit_success_reply_and_job_visible_after_commit (which drove - * handle_jobsubmit through cbrp_drive_acked, a helper that asserts the - * handler blocks until its reply is observable — no longer true here by - * this task's own explicit contract). This test asserts ONLY what task 3.2 - * itself makes observable on the committed-outcome path: the calling - * thread returns before the commit resolves, and the continuation hands - * the entry off to persist's pending-apply registry keyed by the correct - * lsn — NOT a reply, NOT a constructed job (task 6 owns both; neither - * exists yet). */ +/* raft-apply-decoupling task 3.2 (original intent, superseded by task 6.1/ + * 6.2 -- see raft-apply-decoupling task 7.3, Req 11.1): supersedes the + * pre-3.2 test_jobsubmit_success_reply_and_job_visible_after_commit (which + * drove handle_jobsubmit through cbrp_drive_acked, a helper that asserts + * the handler blocks until its reply is observable — no longer true here). + * This test still asserts task 3.2's own contract (the calling thread + * returns before the commit resolves, and the WAL entry is appended before + * that return); it no longer asserts task 3.2's INTERIM "no reply / no job + * yet" gap on the committed-outcome path, because the ordered apply pump + * built by task 6.1/6.2 now closes that gap: the pump consumes the + * pending-apply hand-off, applies the entry via apply_committed_entry(), + * and fires the client's success reply — all strictly after the entry + * commits, never before or during the calling thread's earlier return. + * (Catching persist_has_pending_apply()'s transient true window from a + * second thread is inherently racy against the pump's own consumption of + * it — proven flaky empirically across repeated runs of this test — so + * this test asserts the deterministic settled state instead, the same + * poll-for-reply idiom test_jobsubmit_success_reply_after_apply_carries_ + * jobid already uses for the identical ordering proof.) */ static void test_jobsubmit_returns_before_commit_and_registers_pending_apply(void) { - uint64_t lsn_before, watermark, t0, join_elapsed_ms, t1; + uint64_t lsn_before, watermark, t0, join_elapsed_ms; cbrp_reply_obs_t obs = { 0 }; - bool has_pending, job_exists; + bool job_exists; cbrp_setup(2000); /* generous timeout: must never be needed */ cbrp_arrange_jobsubmit(9101, 1); @@ -961,27 +970,13 @@ static void test_jobsubmit_returns_before_commit_and_registers_pending_apply(voi watermark = cbrp_poll_append(&g_hc.persist, lsn_before, 2000); - /* Deliver the covering ACK, then poll for the hand-off artifact - * (raft-apply-decoupling task 3.2's producer-side registration) — - * NOT for a reply or a constructed job, neither of which this task - * produces on the committed-outcome path. */ + /* Deliver the covering ACK, then poll for the settled, end-to-end + * committed-outcome effect: the ordered apply pump applies the entry + * and fires the client's success reply (raft-apply-decoupling task + * 6.1/6.2). */ repl_on_ack(&g_hc.persist.repl, &g_hep_a, watermark); + cbrp_poll_reply(&g_req_ep, 2000, &obs); - has_pending = false; - t1 = cbrp_mono_ms(); - while (cbrp_mono_ms() - t1 < 2000) { - if (persist_has_pending_apply(&g_hc.persist, watermark)) { - has_pending = true; - break; - } - cbrp_sleep_ms(5); - } - - /* Give the (non-existent) rest of the pipeline a beat, then confirm - * nothing leaked through — the task's own explicit "no independently - * observable behavior" contract for the committed-outcome path. */ - cbrp_sleep_ms(200); - cbrp_scan_replies(&g_req_ep, &obs); job_exists = cbrp_job_exists(9101); cbrp_teardown(); @@ -992,16 +987,16 @@ static void test_jobsubmit_returns_before_commit_and_registers_pending_apply(voi "delivered before this join"); TEST_ASSERT_TRUE_MESSAGE(watermark > 0, "job submission must still append its WAL entry"); - TEST_ASSERT_TRUE_MESSAGE(has_pending, - "a committed entry must be handed off to the ordered apply pump " - "via persist_register_pending_apply, keyed by its lsn " - "(raft-apply-decoupling task 3.2)"); - TEST_ASSERT_EQUAL_INT_MESSAGE(0, obs.n_replies, - "no reply may be sent on the committed-outcome path until task 6's " - "apply pump exists (raft-apply-decoupling task 3.2's own contract)"); - TEST_ASSERT_FALSE_MESSAGE(job_exists, - "no job may be constructed on the committed-outcome path until " - "task 6's apply pump exists (raft-apply-decoupling task 3.2)"); + TEST_ASSERT_TRUE_MESSAGE(obs.n_replies > 0, + "a committed entry must reach the client with a reply once the " + "ordered apply pump consumes its pending-apply hand-off " + "(raft-apply-decoupling task 6.1/6.2, Req 2.2)"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, obs.last_status, + "a committed, applied job submission must reply success"); + TEST_ASSERT_TRUE_MESSAGE(job_exists, + "the job must be visible in the in-memory job table once the " + "ordered apply pump applies its committed entry " + "(raft-apply-decoupling task 6.1/6.2, Req 2.2)"); printf("test_jobsubmit_returns_before_commit_and_registers_pending_apply:" " PASS\n"); @@ -1315,15 +1310,34 @@ static void test_jobsubmit_repeated_truncation_forced_failures_no_phantom_jobs(v * exists yet"): element 1 commits (covering ACK) while elements 2 and 3 * are left to time out FAILED. Confirms the failed elements produce zero * phantom jobs and that the committed element's independent resolution - * (hand-off to persist_register_pending_apply) is not blocked or skipped - * by its failing siblings -- task 3.2's per-element independence, proven - * by a real test rather than by code inspection alone. */ + * is not blocked or skipped by its failing siblings -- task 3.2's + * per-element independence, proven by a real test rather than by code + * inspection alone. + * + * raft-apply-decoupling task 7.3 (Req 11.1) update: this test originally + * asserted (task 3.2, before task 6.1/6.2 existed) that the committed + * element had NO job yet, only a pending-apply hand-off -- catching that + * hand-off's transient window from a second thread is inherently racy + * against the ordered apply pump's own consumption of it (proven flaky + * empirically across repeated runs of this test: sometimes the window + * was caught, sometimes it was already consumed by the time this test + * polled). Now that the pump exists, the deterministic, settled effect is + * that element 1's mutation applies (job1 becomes visible in c->jobmgr) + * independently of its failing siblings, exactly like any other committed + * entry -- the per-element independence this test exists to prove now + * extends from "resolution" to "apply", per the design's own stated + * intent that a command's apply is per-entry and never gated on sibling + * array elements (collapsing that behavior is job-array-single-wal- + * record's job, not this feature's). The client-visible batch reply is + * unaffected: it is still the single failure reply from the timed-out + * siblings, because jobsubmit_reply_success_once() is gated on the same + * actx->replied flag as jobsubmit_reply_failure_once() (job.c), and the + * failure reply always wins that race here. */ static void test_jobsubmit_array_mixed_outcome_no_phantom_job_for_failed_elements(void) { const uint64_t base_jobid = 9140; - uint64_t lsn_before, watermark, lsn1, t1; + uint64_t lsn_before, watermark, lsn1; int create_rc, join_rc; - bool has_pending1; cbrp_reply_obs_t obs = { 0 }; bool job1_exists, job2_exists, job3_exists; @@ -1353,16 +1367,10 @@ static void test_jobsubmit_array_mixed_outcome_no_phantom_job_for_failed_element * independently of element 1's success. */ repl_on_ack(&g_hc.persist.repl, &g_hep_a, lsn1); - has_pending1 = false; - t1 = cbrp_mono_ms(); - while (cbrp_mono_ms() - t1 < 2000) { - if (persist_has_pending_apply(&g_hc.persist, lsn1)) { - has_pending1 = true; - break; - } - cbrp_sleep_ms(5); - } - + /* Poll for the batch's single settled reply (the failing siblings' + * timeout), then read the deterministic end state of every element's + * job existence -- not a transient hand-off window (see doc comment + * above). */ cbrp_poll_reply(&g_req_ep, 300 + 1500, &obs); job1_exists = cbrp_job_exists(base_jobid); @@ -1375,21 +1383,20 @@ static void test_jobsubmit_array_mixed_outcome_no_phantom_job_for_failed_element TEST_ASSERT_EQUAL_INT_MESSAGE(0, join_rc, "pthread_join must succeed"); TEST_ASSERT_EQUAL_UINT64_MESSAGE(lsn_before + 2, watermark, "all three array elements must append before this poll returns"); - TEST_ASSERT_TRUE_MESSAGE(has_pending1, - "a sibling array element that commits must still be handed off to " - "the pending-apply registry even though other siblings in the " - "same batch are failing (raft-apply-decoupling task 3.2's " - "per-element independent resolution)"); TEST_ASSERT_EQUAL_INT_MESSAGE(1, obs.n_replies, "a partially-failed array batch must produce exactly one failure " "reply for the whole request (jobsubmit_reply_failure_once)"); TEST_ASSERT_EQUAL_INT_MESSAGE(-ETIMEDOUT, obs.last_status, "the batch's single reply must carry the failing elements' " "-ETIMEDOUT outcome"); - TEST_ASSERT_FALSE_MESSAGE(job1_exists, - "no element may have a constructed job yet -- task 6's apply pump " - "does not exist, so even the committed element has no job; this " - "test proves independent per-element RESOLUTION, not apply"); + TEST_ASSERT_TRUE_MESSAGE(job1_exists, + "the committed array element's mutation must apply independently " + "of its failing siblings, exactly like any other committed entry, " + "now that the ordered apply pump exists (raft-apply-decoupling " + "task 6.1/6.2) -- the client's batch reply still reports failure " + "because a failing sibling wins jobsubmit_async_ctx_t's " + "single-reply race, but that reply does not gate this element's " + "independent apply"); TEST_ASSERT_FALSE_MESSAGE(job2_exists, "a failed array element must never have a phantom job"); TEST_ASSERT_FALSE_MESSAGE(job3_exists,