From a9cfee5fef4716e5e90dd9fb7a4540b3f298225f Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 23 Aug 2026 21:39:01 +0000 Subject: [PATCH] test(scheduler-dispatch-commit): fix last stale pinned assertion, full regression clean (task 5.4, final) Fixes test_commit_barrier_reply.c's test_jobdispatch_receipt_ack_no_wal_append (owned by ha-commit-barrier-rollout), the last remaining assertion this feature's earlier tasks left stale: - WAL-count delta corrected from "must be zero" to lsn_before + 1 -- the batch's successful dispatch now appends one WAL_JOB_DISPATCH fact (task 3.1); mark_job_fail() was traced and confirmed to append nothing via any path. - The batch's terminal-failure job was replaced with a genuinely retryable one (WORKER_NULL site, unregistered host, new job id) rather than degrading the notify-count assertion to zero, which would also have silently killed the test's own reply-before-notify ordering pin (nothing left to order against). This keeps the test meaningfully exercising notify_dispatch()'s wiring through handle_jobdispatch(). Full regression, independently re-verified by review: - Scheduler suite at documented baseline (51/52, 1 pre-existing unrelated failure). Investigated and logged bug-1632, a new sibling of the already-known bug-1598 stack-lifetime pattern in a different test file -- confirmed pre-existing, untouched by this feature, and non-deterministic. - Controller suite clean: 1133 tests, only the 2 pre-existing bug-1561 failures. - Sequential HA soak: PASS (10 rounds, 36 jobs, zero loss, all 3 nodes led). - Chaotic HA soak: PASS (10 rounds, committed_divergence=0 committed_job_loss=0). This completes the scheduler-dispatch-commit spec: bug-1624 (resource-in-use round-boundary loss) and bug-1625 (scheduler discards dispatch-failure notify) are both fixed and regression-tested. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Cxa1PmqAB5PyXfJaNvj8TN --- .../specs/scheduler-dispatch-commit/tasks.md | 6 +- .../testsuite/test_commit_barrier_reply.c | 143 +++++++++++++----- 2 files changed, 112 insertions(+), 37 deletions(-) diff --git a/.kiro/specs/scheduler-dispatch-commit/tasks.md b/.kiro/specs/scheduler-dispatch-commit/tasks.md index f92c148a..ed2ab253 100644 --- a/.kiro/specs/scheduler-dispatch-commit/tasks.md +++ b/.kiro/specs/scheduler-dispatch-commit/tasks.md @@ -89,7 +89,7 @@ - _Requirements: 4.2, 4.5_ - _Depends: 3.2, 4.1_ -- [ ] 5.4 Full regression: existing suites and HA soak +- [x] 5.4 Full regression: existing suites and HA soak - Run the existing scheduler and controller test suites end to end, updating only the assertions that directly depended on the old confirmation-gated timing; run a full sequential and chaotic HA soak - Observable completion: both test suites pass and the soak shows no new job loss, state divergence, or liveness stall attributable to this feature - _Requirements: 5.2, 5.3, 5.4_ @@ -119,3 +119,7 @@ - Task 5.2: **bug-1631 (logged, NOT fixed, deliberately deferred)** — `dispatch_a_job_()`'s four retryable-failure branches (job.c, tasks 3.1/3.3) never revert `job.state`/`job.allocs` when a retryable failure occurs while the job's controller-side state is CURRENTLY RUN (they were only ever designed for a job whose every dispatch attempt starts from PEND, which is all that's reachable today). A kiro-debug investigation (HIGH confidence), independently re-verified even more exhaustively by this task's own reviewer (full trace of every `pendJobs`-reinsertion and every controller-side `job.state`-mutation call site), confirmed this scenario is genuinely UNREACHABLE via any real caller in the current, fully-implemented system: the scheduler's picker permanently removes a job from its PEND candidate pool the instant it dispatches it (task 2.1), and nothing already-implemented ever re-adds a successfully-dispatched (RUN) job back to that pool. The only mechanism design.md itself anticipates for that is `scheduler-preemption`, confirmed still unimplemented (`requirements-generated`, no approvals). **Revisit bug-1631 and add the state/allocs reversal to `dispatch_a_job_()`'s four retryable branches only once `scheduler-preemption` (or an equivalent mechanism) actually creates a real RUN→retryable-redispatch path** — not before, since there's no reachable caller to regression-test against today. `test_rpc_failure_bound_resets_after_intervening_success` (`test_dispatch_fail_retry.c`) proves Requirement 4.3's actual, literal ask (the counter genuinely resets on an intervening success) without asserting the unreachable state/allocs behavior. - Task 5.2: design.md's "vacuously by construction" reasoning (`Controller Dispatch Commit` and `Controller Dispatch-Failure Classification` sections — "on any of the six classified failure paths there is nothing to release") silently assumes every `dispatch_a_job_()` invocation starts from a job that's never had anything committed (i.e., a first-ever dispatch attempt) — true today (see bug-1631 above) but not stated as a scoped assumption in the doc text itself. Worth correcting design.md's wording if it's ever revisited for the `scheduler-preemption` follow-up. - Task 5.3: `test_dispatch_fail_terminal.c` (task 3.2) and `test_jobdispatch_no_double_notify.c` (task 4.1) each covered PART of this task but not the full combination in one run — 3.2's test bypasses `handle_jobdispatch()` entirely (via `dispatch_a_job_test()`), so it can't exercise the double-notify race at all; 4.1's test only proves an *aggregate* batch notify count, not a *direct* per-job zero-notify proof. `test_handle_jobdispatch_terminal_no_notify_no_leak` (extending 4.1's file, the one with the right entry point) closes this by combining EXIT resolution + a direct single-job zero-notify assertion + the canary-reference no-leak proof, all through the real `handle_jobdispatch()` entry point in one test. Pattern worth remembering: a batch-aggregate test and a bypassed-entry-point test can each look like they cover a scenario while neither actually does, in combination, what a later task's literal wording asks for ("all pass IN THE SAME TEST RUN"). +- Task 5.4 (final task — spec complete): fixed `test_commit_barrier_reply.c`'s `test_jobdispatch_receipt_ack_no_wal_append` (owned by `ha-commit-barrier-rollout`), the last remaining stale assertion from this feature's own changes. WAL-count delta corrected from "must be zero" to `lsn_before + 1` (the batch's successful dispatch now appends one `WAL_JOB_DISPATCH` fact; `mark_job_fail()` was traced and confirmed to append nothing via any path). Notify-count: rather than degrading to "must be zero" (which would also silently kill the test's own reply-before-notify ordering assertion, since there'd be nothing left to order against), the batch's terminal-failure job was replaced with a genuinely retryable one (`WORKER_NULL` site, unregistered host, new job id) — preserves the test's ability to meaningfully exercise `notify_dispatch()`'s wiring through `handle_jobdispatch()`. Independently reviewed and approved, including scrutiny of this specific choice. +- Task 5.4: discovered and logged **bug-1632** (scheduler suite) — a new sibling instance of the already-known bug-1598 pattern (unjoined `sim_picker_thread_fn` background thread reading a stack-local fixture after frame reuse), this time in `test_sim_verify.c`'s own separate `run_sim_and_capture_stdout()` helper (bug-1598's original fix only covered 3 stack-locals in `test_sim_event_loop.c`, not this one). Confirmed via `git log`/`git diff` as untouched by this feature and confirmed non-deterministic (multiple clean re-runs). Pre-existing, unrelated, NOT fixed — a candidate for whoever next touches `test_sim_verify.c`'s fixture pattern. +- Task 5.4: bug-1429's hang did NOT reproduce during this task's full-suite runs (multiple clean 1133-test passes) — consistent with its own buglog history describing it as non-deterministic/timing-dependent, not evidence it's resolved. Still treat any future `meson test` TIMEOUT on `test_commit_barrier_reply`/`test_commit_barrier_rollout` as this known, pre-existing, unrooted issue rather than a new regression, per all of tasks 3.1-5.3's own prior confirmations. +- **Feature complete**: all tasks (1.1-1.3, 2.1-2.2, 3.1-3.3, 4.1, 5.1-5.4) implemented, independently reviewed and approved, and committed. Final regression: scheduler suite at documented baseline (51/52, 1 pre-existing unrelated failure), controller suite clean (1133 tests, only 2 pre-existing bug-1561 failures), sequential HA soak PASS (10 rounds, zero job loss, all 3 nodes led), chaotic HA soak PASS (10 rounds, `committed_divergence=0 committed_job_loss=0`) — independently re-verified by task 5.4's reviewer via its own rebuild plus a 5-round spot-check of both soak modes. bug-1624 and bug-1625 (this spec's originating bugs) are fixed by tasks 3.1/2.1 and 2.2 respectively; `.wolf/buglog.json` should be updated to reflect FIXED status as a follow-up. diff --git a/src/controller/testsuite/test_commit_barrier_reply.c b/src/controller/testsuite/test_commit_barrier_reply.c index a78f0498..b01a7a07 100644 --- a/src/controller/testsuite/test_commit_barrier_reply.c +++ b/src/controller/testsuite/test_commit_barrier_reply.c @@ -1868,14 +1868,30 @@ static xce_job_dispatch_t g_dispatchmsg; static xce_job_alloc_t *g_dispatch_allocs[2]; static xce_request_t g_dispatch_req; -#define CBRP_DISP_JOBID 60u +#define CBRP_DISP_JOBID 60u +#define CBRP_DISP_JOBID_RETRY 61u -/* Full dispatch fixture: queue (get_queue_exec), pending job with a real - * submit (dispatch_a_job_ reads submit->queue and submit->start_info), - * worker with a live no-iomgr endpoint (the startjob request must be - * enqueueable), and a 2-job dispatch message exercising BOTH outcomes: - * allocs[0] — dispatchable to wkr-disp → notify_dispatch(ok, code 0) - * allocs[1] — nhosts == 0 → notify_dispatch(fail, -EINVAL) +/* Full dispatch fixture: queue (get_queue_exec), two pending jobs each with + * a real submit (dispatch_a_job_ reads submit->queue and submit-> + * start_info), worker with a live no-iomgr endpoint (the startjob request + * must be enqueueable), and a 2-job dispatch message exercising BOTH + * outcomes reachable through dispatch_a_job_()'s post-task-3.1/3.3 + * resolution (scheduler-dispatch-commit task 5.4 — see this file's + * "Implementation Notes" history for CBRP_DISP_JOBID's own scenario, which + * this replaces): + * allocs[0] — dispatchable to wkr-disp → succeeds: job.state + * becomes RUN, one WAL_JOB_DISPATCH fact is appended, no + * notify_dispatch() call (success is never notified). + * allocs[1] — host "wkr-disp-missing" is not registered in + * g_hc.workermgr (XCE_DISPATCH_FAIL_SITE_WORKER_NULL, + * mirroring test_dispatch_fail_retry.c's own + * test_worker_notfound_resolves_retryable fixture) → a + * genuinely RETRYABLE dispatch failure: no WAL append + * (nothing was ever committed), exactly one + * notify_dispatch(jobDispatchFail) call. Kept on a SEPARATE + * job id (CBRP_DISP_JOBID_RETRY) so this scenario doesn't + * depend on ordering effects from allocs[0]'s own state + * transition on the same job. * The allocs are heap-built via xcealloc helpers because the handler * takes ownership (MOVE_PTR + free_JobAlloc_ptr per job). */ static void cbrp_arrange_jobdispatch(void) @@ -1896,6 +1912,11 @@ static void cbrp_arrange_jobdispatch(void) g_disp_submit.user = (char *)"tester"; g_disp_submit.start_info = &g_disp_startinfo; cbrp_make_job(CBRP_DISP_JOBID, &g_disp_submit); + /* scheduler-dispatch-commit task 5.4: a second job backing allocs[1]'s + * retryable-failure scenario below — cbrp_make_job's own comment + * confirms submit is deep-copied by makeJob/makeJobsub, so reusing + * g_disp_submit/g_disp_startinfo for both jobs is safe. */ + cbrp_make_job(CBRP_DISP_JOBID_RETRY, &g_disp_submit); attrs = calloc(1, sizeof(*attrs)); TEST_ASSERT_NOT_NULL(attrs); @@ -1924,7 +1945,19 @@ static void cbrp_arrange_jobdispatch(void) make_allocs(1, make_alloc((char *)"slots", NULL, 0, 1.0f, 0))))); TEST_ASSERT_NOT_NULL(g_dispatch_allocs[0]); - g_dispatch_allocs[1] = make_job_alloc(CBRP_DISP_JOBID, 0, NULL); + /* scheduler-dispatch-commit task 5.4 (was: make_job_alloc(CBRP_DISP_ + * JOBID, 0, NULL), a terminal nhosts==0 failure — see the updated + * fixture-header comment above for why this changed): a genuinely + * RETRYABLE dispatch failure (unregistered worker host), so this test + * still drives exactly one real notify_dispatch() call through + * handle_jobdispatch() end-to-end, preserving the reply-before- + * processing ordering check below. */ + g_dispatch_allocs[1] = make_job_alloc( + CBRP_DISP_JOBID_RETRY, 1, + make_host_allocs(1, + make_host_alloc((char *)"wkr-disp-missing", 1, + make_allocs(1, + make_alloc((char *)"slots", NULL, 0, 1.0f, 0))))); TEST_ASSERT_NOT_NULL(g_dispatch_allocs[1]); memset(&g_dispatchmsg, 0, sizeof(g_dispatchmsg)); @@ -1940,13 +1973,36 @@ static void cbrp_arrange_jobdispatch(void) /* VERDICT PIN (Req 2.3): handle_jobdispatch (job.c:468-500, re-verified * 2026-06-12) replies status=0 IMMEDIATELY — an ACK-of-receipt per the - * umbrella Reply-Ordering Contract (see section header) — and a full - * dispatch cycle (one successful + one failed allocation) appends NO WAL - * entries: allocation results return via notify_dispatch and job-state - * changes arrive later as worker facts. ACKs are withheld for the whole - * test, so a regression that adds a barrier wait trips the latency bound - * and any WAL append from dispatch processing trips the unchanged-LSN - * assert (an unclassified type would abort in wal_classify first). */ + * umbrella Reply-Ordering Contract (see section header). ACKs are withheld + * for the whole test, so a regression that adds a barrier wait trips the + * latency bound. + * + * UPDATED (scheduler-dispatch-commit task 5.4, Req 5.3): the WAL/notify + * expectations below were re-pinned to reflect the dispatch-commit + * feature's new timing (Req 1.1/2.1) — they are no longer part of THIS + * test's own pin (which is only about the receipt ACK being immediate and + * ordered before per-job processing), just its supporting scenario: + * - dispatch processing is no longer guaranteed to append NO WAL entries + * — a job whose dispatch to the worker actually succeeds now commits + * its running-state transition synchronously (task 3.1), appending + * exactly one WAL_JOB_DISPATCH fact per successful dispatch. This + * test's batch has exactly one such successful dispatch (allocs[0]), + * so lsn_after must be lsn_before + 1, not lsn_before + 0. A + * genuinely retryable failure (allocs[1]) still appends nothing + * (nothing was ever committed for it — vacuously true by + * construction, per job.c's own task 3.3 comments), and the + * wal_classify XCE_TESTING guard still backstops any unclassified + * WAL type. + * - success is never notified (unchanged, predates this feature) and a + * terminal failure now resolves via mark_job_fail()'s own hook chain, + * not notify_dispatch() (task 3.2/4.1) — so only a genuinely + * RETRYABLE failure still produces a notify_dispatch(jobDispatchFail) + * call. This test's fixture was updated (see cbrp_arrange_jobdispatch) + * to give allocs[1] a retryable-classified failure specifically so + * this pin keeps exercising one real notify_dispatch() call end-to-end + * through handle_jobdispatch(), preserving the reply-before-processing + * ordering check below (which needs at least one notify call to prove + * anything). */ static void test_jobdispatch_receipt_ack_no_wal_append(void) { cbrp_reply_obs_t obs = { 0 }; @@ -1980,31 +2036,46 @@ static void test_jobdispatch_receipt_ack_no_wal_append(void) TEST_ASSERT_EQUAL_INT_MESSAGE(0, obs.last_status, "the receipt ACK reports acceptance of the message (status 0)"); - /* THE classification pin: dispatch processing appends no WAL entries, - * so there is nothing the receipt ACK could have falsely confirmed */ - TEST_ASSERT_EQUAL_UINT64_MESSAGE(lsn_before, lsn_after, - "a dispatch cycle must append NO WAL entries (Req 2.3 — any " - "future append from dispatch processing must move behind the " - "barrier or be re-classified; the wal_classify XCE_TESTING " - "guard backstops unclassified types)"); + /* scheduler-dispatch-commit task 5.4 (Req 5.3): re-pinned WAL-count + * expectation — allocs[0]'s dispatch to the live worker now succeeds + * and commits its running-state transition synchronously (task 3.1), + * appending exactly one WAL_JOB_DISPATCH fact; allocs[1]'s retryable + * failure appends nothing (see the VERDICT PIN comment above). Any + * OTHER/unexpected WAL append still trips this assert, and an + * unclassified type would still abort in wal_classify first. */ + TEST_ASSERT_EQUAL_UINT64_MESSAGE(lsn_before + 1, lsn_after, + "a dispatch cycle with one successful dispatch must append " + "exactly one WAL_JOB_DISPATCH fact (Req 2.4) and nothing else " + "(Req 2.3 — any further append from dispatch processing must " + "move behind the barrier or be re-classified; the wal_classify " + "XCE_TESTING guard backstops unclassified types)"); - /* notify_dispatch outcomes unaffected: one ok + one fail, in order */ - TEST_ASSERT_EQUAL_INT_MESSAGE(2, g_notify_count, - "both allocations must produce their per-job outcome notification"); + /* scheduler-dispatch-commit task 5.4 (Req 5.3): re-pinned notify-count + * expectation — success (allocs[0]) is never notified, and a terminal + * failure resolves via mark_job_fail()'s own hook chain rather than + * notify_dispatch() (tasks 3.2/4.1); only allocs[1]'s genuinely + * retryable failure produces a notify_dispatch(jobDispatchFail) call, + * so the batch total is exactly one, not two. */ + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_notify_count, + "only allocs[1]'s retryable dispatch failure notifies — success " + "(allocs[0]) and a terminal failure are never reported via " + "notify_dispatch()"); TEST_ASSERT_EQUAL_INT(NotifyData_data_jobDispatchFail, g_notify_events[0]); - TEST_ASSERT_EQUAL_INT(NotifyData_data_jobDispatchFail, - g_notify_events[1]); - TEST_ASSERT_EQUAL_UINT64(CBRP_DISP_JOBID, g_notify_jobids[0]); - TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_notify_codes[0], - "allocs[0] dispatch to the live worker must succeed (code 0)"); - TEST_ASSERT_EQUAL_UINT64(CBRP_DISP_JOBID, g_notify_jobids[1]); - TEST_ASSERT_EQUAL_INT_MESSAGE(-EINVAL, g_notify_codes[1], - "allocs[1] (nhosts == 0) must fail with -EINVAL"); + TEST_ASSERT_EQUAL_UINT64(CBRP_DISP_JOBID_RETRY, g_notify_jobids[0]); + TEST_ASSERT_TRUE_MESSAGE(g_notify_codes[0] < 0, + "allocs[1]'s dispatch to an unregistered worker must fail with a " + "negative code (exact errno value is librtti key_find()'s own " + "internal, opaque choice — not asserted, mirroring " + "test_dispatch_fail_retry.c's established precedent)"); - /* Ordering: the receipt ACK was already enqueued when the FIRST - * dispatch outcome fired — reply precedes processing (ACK-of-receipt, - * not ACK-of-outcome) */ + /* Ordering (this test's actual pin, unchanged by task 5.4): the + * receipt ACK was already enqueued when allocs[1]'s dispatch outcome + * fired — reply precedes processing (ACK-of-receipt, not + * ACK-of-outcome). This is why the fixture keeps exactly one + * notify_dispatch() call reachable in this batch (allocs[1]) — with + * zero calls there would be nothing left to prove this ordering + * against. */ TEST_ASSERT_TRUE_MESSAGE(g_notify_reply_already_enqueued[0], "the receipt ACK must be enqueued before any dispatch outcome " "notification (reply-before-processing is the verdict)");