test(scheduler-dispatch-commit): retryable and bounded-retry dispatch failure e2e coverage (task 5.2)

Adds test_rpc_failure_bound_resets_after_intervening_success to
test_dispatch_fail_retry.c, proving Requirement 4.3's literal claim
end-to-end through the real dispatch_a_job_test() entry point: a fail,
then a real success, then another fail resets the consecutive-failure
counter so the second failure is evaluated as failure #1, not #2.

The first draft of this test also asserted job.state/job.allocs revert
to PEND/NULL on that post-success failure -- this genuinely fails
against current code (none of dispatch_a_job_()'s four retryable
branches revert state/allocs when failing from an already-RUN job,
since they were only ever designed for a job whose every attempt starts
from PEND). A debug investigation, independently re-verified even more
exhaustively during review (full trace of every pendJobs-reinsertion
and controller-side state-mutation site), confirmed this scenario is
unreachable via any real caller today -- the scheduler's picker
permanently removes a job from its PEND pool the instant it dispatches
it, and nothing already-implemented ever re-adds a RUN job to it. Only
the not-yet-implemented scheduler-preemption spec could someday create
that path. Logged as bug-1631, deliberately deferred rather than fixed
now (no reachable caller to regression-test against); the two
over-reaching assertions were trimmed, keeping only the counter proof
Requirement 4.3 actually asks for. Independently reviewed and approved.

Scenarios 1 (retryable -> PEND-eligible) and 2 (bound exceeded -> EXIT)
were already fully covered by tasks 2.2's and 3.3's existing tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cxa1PmqAB5PyXfJaNvj8TN

.kiro/specs/scheduler-dispatch-commit/tasks.md

@@ -77,7 +77,7 @@
   - _Requirements: 1.1, 1.2, 1.3, 2.1, 2.3_
   - _Depends: 2.1, 3.1_
 
-- [ ] 5.2 End-to-end test: retryable and bounded-retry dispatch failures resolve correctly
+- [x] 5.2 End-to-end test: retryable and bounded-retry dispatch failures resolve correctly
   - Simulate a transient dispatch failure and confirm the job becomes eligible for another attempt; simulate enough consecutive transient failures to exceed the bound and confirm the job resolves to permanently failed instead of retrying indefinitely; simulate a failure, a success, then another failure and confirm the bound is evaluated as if only one failure had occurred
   - Observable completion: all three scenarios produce the documented outcome, with the counter's bound value exercised at exactly the boundary (one under, exactly at, and one over)
   - _Requirements: 3.1, 3.2, 4.1, 4.3, 4.4, 4.5_
@@ -116,3 +116,5 @@
 - Task 4.1: confirmed `handle_jobdispatch()`'s only caller (`cluster.c:471`, inside `myreq_handler`) uses its return value only as the generic RPC-handler `rc`, and `handle_jobdispatch()` itself always returns 0 unconditionally regardless of any job's outcome — nothing depends on the removed per-job notification behavior at the caller level.
 - Task 5.1: a "same-round confirmation" scenario for resource-in-use accounting is legitimately modeled as `nrounds==1` rather than by simulating a real `schedStartJob` notify event — design.md documents (and the reviewer independently re-verified against `core_events.c`) that re-applying `JOB_STAT_RUN` to an already-RUN job via the confirmation path is a confirmed no-op, so `resInUse` depends solely on `JOB_IS_RUN(job->state)`, never on a separate confirmation flag. Reviewer's own finding: the resulting same-round-vs-multi-round comparison test is honest and correctly fails on revert, but adds little independent detection power beyond a per-round-asserting loop test (which is the one genuinely load-bearing proof) — worth knowing if a future task considers deduplicating these two tests, but not something to act on now.
 - Task 5.1: no controller-side test was added for this task — Requirements 2.1/2.3 (controller transitions to RUN independent of confirmation) are already covered by task 3.1's `test_dc_success_commits_alloc_state_and_resets_counter`. No live scheduler+controller wire-protocol integration harness exists anywhere in this codebase; building one was judged disproportionate to what this task verifies, and the reviewer agreed.
+- 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.

src/controller/testsuite/test_dispatch_fail_retry.c

@@ -290,6 +290,22 @@ static void dfr_teardown_broken_ep(void)
     close(g_dfr_broken_iomgr.epoll_fd);
 }
 
+/* Frees every packet left queued on an endpoint's outq (the startjob request
+ * a SUCCESSFUL dispatch_a_job_test() call enqueues) -- mirrors
+ * test_dispatch_commit.c's dc_drain_ep exactly; this file otherwise never
+ * needs a live dispatch to succeed, so it has no drain helper of its own
+ * until the counter-reset test below. */
+static void dfr_drain_ep(endpoint_t* ep)
+{
+    data_item_t* it;
+
+    while ((it = endpoint_deq(ep)) != NULL) {
+        io_packet_t* pkt = ITPTR_TO_PACKET(it);
+        free_packet(pkt);
+        free(pkt);
+    }
+}
+
 /* ── SITE 1/2: worker lookup finds nothing ─────────────────────────────
  * See file header: covers whichever of XCE_DISPATCH_FAIL_SITE_WORKER_LOOKUP
  * / XCE_DISPATCH_FAIL_SITE_WORKER_NULL the runtime actually takes -- both
@@ -550,6 +566,243 @@ static void test_rpc_failure_bounded_escalation(void)
     printf("test_rpc_failure_bounded_escalation: PASS\n");
 }
 
+/* ── task 5.2, scenario 3: bound resets across an intervening success ──
+ *
+ * Req 4.3's literal text: "consecutive... dispatch failures with no
+ * intervening successful dispatch". test_rpc_failure_bounded_escalation
+ * (above) already proves the bound itself (3 retryable, 4th terminal) for
+ * an UNINTERRUPTED failure streak; test_dc_success_commits_alloc_state_
+ * and_resets_counter (test_dispatch_commit.c) already proves a success
+ * resets the field to 0 in isolation, but seeds the pre-success count by
+ * directly poking j->dispatch_fail_count rather than by driving a real
+ * prior failure through dispatch_a_job_test() -- so neither test proves
+ * that a SUBSEQUENT real failure, after a real intervening success, is
+ * actually evaluated as if only one failure had ever occurred. This test
+ * closes that gap: fail -> succeed -> fail, all three driven through
+ * dispatch_a_job_test() itself, no manual field pokes anywhere.
+ *
+ * Technique: dispatch_a_job_() never gates on job.state (confirmed by
+ * reading job.c directly -- it operates purely from alloc/jobid/worker
+ * lookups) and re-resolves the worker via a fresh key_find() on every
+ * call, so toggling the SAME worker's w->ep between the "broken" endpoint
+ * (dfr_setup_broken_ep's RPC-failure technique, XCE_DISPATCH_FAIL_SITE_
+ * RPC_FAILURE) and a "working" one (test_dispatch_commit.c's zeroed,
+ * no-iomgr g_dc_worker_ep technique -- xce_startjob_req()'s enqueue
+ * succeeds unconditionally against it) between calls is sufficient to
+ * drive fail -> succeed -> fail against the identical job/worker pair,
+ * with no need for a second fixture.
+ *
+ * The genuinely load-bearing proof (this is the part most likely to catch
+ * a subtly-wrong fix, e.g. the reset being skipped or landing on the
+ * wrong value): after the intervening success, the counter must count
+ * PURELY from the post-reset baseline -- checked at each of three more
+ * consecutive failures (i=1,2,3, the loop below). This assertion is
+ * GENUINELY PROVEN, both by direct runtime observation (a temporary debug
+ * probe confirmed dispatch_fail_count reads exactly 1 after the loop's
+ * first call, immediately following a confirmed-0 post-success read -- if
+ * the reset (task 3.1's `j->dispatch_fail_count = 0;`) had been skipped or
+ * wrong, this would read 2, not 1) and by a real revert/rebuild/rerun of
+ * that exact line (see the note below the loop's count assertion).
+ *
+ * IMPORTANT FINDING (bug-1631, logged this task): this same scenario also
+ * exposed that dispatch_a_job_()'s four retryable-failure branches never
+ * reset j->job.state/j->job.allocs, which is correct for a job's very
+ * FIRST dispatch attempt (never left PEND, nothing to release) but would
+ * NOT be correct once a job has had a prior REAL successful dispatch, as
+ * this scenario's step 2 constructs -- Requirement 4.1 ("return the job
+ * to a pending state") and Requirement 4.4/4.5 ("release the resource-
+ * allocation and commitment data") would not be satisfied in that case.
+ * A subsequent kiro-debug investigation (HIGH confidence) determined this
+ * "job is RUN, then re-dispatched, then fails" scenario is NOT reachable
+ * via any real caller in the currently-implemented system -- see the
+ * loop's own comment for the full reachability analysis, and bug-1631 for
+ * the complete write-up (a legitimate, deliberately future-scoped finding,
+ * not an in-scope defect). Accordingly, this test does NOT assert
+ * job.state/job.allocs on the post-success retryable failures below or in
+ * step 4 -- only the counter-reset behavior (Req 4.3, genuinely reachable
+ * and required) and the bound-exceeded terminal state transition (step 4,
+ * reachable via a normal PEND-origin dispatch) are asserted.
+ */
+#define DFR_JOBID_RESET 91205ULL
+
+static void test_rpc_failure_bound_resets_after_intervening_success(void)
+{
+    job_t*      j;
+    worker_t*   w = NULL;
+    endpoint_t  working_ep;
+    int         i;
+
+    dfr_setup();
+    dfr_setup_broken_ep();
+
+    j = dfr_seed_job(DFR_JOBID_RESET, "dfr-q5");
+    dfr_seed_worker("dfr-wkr-reset", g_dfr_broken_ep);
+
+    memset(&working_ep, 0, sizeof(working_ep));
+    working_ep.fd = -1; /* zeroed, no iomgr -- xce_startjob_req()'s enqueue
+                         * succeeds unconditionally against it (see file
+                         * header / test_dispatch_commit.c's g_dc_worker_ep) */
+
+    /* Step 1: one real retryable failure via the broken endpoint --
+     * dispatch_fail_count -> 1, job stays PEND. Identical technique to
+     * test_rpc_failure_resolves_retryable above, driven inline here so the
+     * whole fail -> succeed -> fail sequence stays in one test. */
+    {
+        xce_job_alloc_t* alloc;
+        int              rc;
+
+        alloc = make_job_alloc(DFR_JOBID_RESET, 1,
+            make_host_allocs(1,
+                make_host_alloc((char*)"dfr-wkr-reset", 1,
+                    make_allocs(1,
+                        make_alloc((char*)"slots", NULL, 0, 1.0f, 0)))));
+        TEST_ASSERT_NOT_NULL(alloc);
+
+        rc = dispatch_a_job_test(&g_dfr, NULL, alloc);
+        TEST_ASSERT_TRUE(rc < 0);
+        TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_PEND, j->job.state,
+            "step 1: the first real failure must leave the job PEND");
+        TEST_ASSERT_EQUAL_UINT32(1, j->dispatch_fail_count);
+        TEST_ASSERT_NULL(j->job.allocs);
+
+        free_JobAlloc_ptr(&alloc);
+    }
+
+    /* Step 2: swap the SAME worker's ep to a working one and dispatch
+     * again -- a genuine successful dispatch, driven through the real
+     * production success path (job.c), not a manual field poke. Must
+     * commit job.allocs/state=RUN AND reset dispatch_fail_count to 0. */
+    lock(g_dfr.workermgr, RTTI_LOCK_READ);
+    key_find(g_dfr.workermgr, (void*)"dfr-wkr-reset", (void**)&w);
+    unlock(g_dfr.workermgr);
+    TEST_ASSERT_NOT_NULL(w);
+    w->ep = &working_ep;
+
+    {
+        xce_job_alloc_t* alloc;
+        int              rc;
+
+        alloc = make_job_alloc(DFR_JOBID_RESET, 1,
+            make_host_allocs(1,
+                make_host_alloc((char*)"dfr-wkr-reset", 1,
+                    make_allocs(1,
+                        make_alloc((char*)"slots", NULL, 0, 1.0f, 0)))));
+        TEST_ASSERT_NOT_NULL(alloc);
+
+        rc = dispatch_a_job_test(&g_dfr, NULL, alloc);
+        TEST_ASSERT_TRUE_MESSAGE(rc >= 0,
+            "step 2: a dispatch to a live, no-iomgr worker endpoint must "
+            "succeed -- xce_startjob_req() is a fire-and-forget enqueue, "
+            "not a blocking round trip");
+        TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_RUN, j->job.state,
+            "step 2: the intervening successful dispatch must commit RUN");
+        TEST_ASSERT_NOT_NULL(j->job.allocs);
+        TEST_ASSERT_EQUAL_UINT32_MESSAGE(0, j->dispatch_fail_count,
+            "Req 4.3: a successful dispatch must reset the consecutive "
+            "retryable-failure counter to zero");
+
+        free_JobAlloc_ptr(&alloc); /* allocs already MOVE_PTR'd out -- top-
+                                    * level struct only */
+    }
+
+    dfr_drain_ep(&working_ep);
+
+    /* Step 3: swap back to the broken endpoint and fail 3 more times -- the
+     * genuinely load-bearing proof (see this test's header). Each of these
+     * three must still resolve retryable/PEND; if the reset above had not
+     * genuinely happened, the job would already be TERMINAL by the second
+     * iteration of this loop (count 1 carried over + 2 new = 3... and the
+     * THIRD would push it to 4, terminal one failure early -- see header
+     * for the exact divergence this loop is designed to catch). */
+    w->ep = g_dfr_broken_ep;
+
+    for (i = 1; i <= 3; i++) {
+        xce_job_alloc_t* alloc;
+        int              rc;
+
+        alloc = make_job_alloc(DFR_JOBID_RESET, 1,
+            make_host_allocs(1,
+                make_host_alloc((char*)"dfr-wkr-reset", 1,
+                    make_allocs(1,
+                        make_alloc((char*)"slots", NULL, 0, 1.0f, 0)))));
+        TEST_ASSERT_NOT_NULL(alloc);
+
+        rc = dispatch_a_job_test(&g_dfr, NULL, alloc);
+        TEST_ASSERT_TRUE(rc < 0);
+
+        /* THE primary proof this scenario exists for (task 5.2's own text:
+         * "confirm the bound is evaluated AS IF ONLY ONE FAILURE HAD
+         * OCCURRED"). Genuinely proven correct: if the counter reset
+         * (task 3.1's `j->dispatch_fail_count = 0;`) were skipped or
+         * wrong, this would already read (uint32_t)i + 1 (the pre-reset 1
+         * carried over) -- confirmed via a real revert/rebuild/rerun (see
+         * this test's header). */
+        TEST_ASSERT_EQUAL_UINT32_MESSAGE((uint32_t)i, j->dispatch_fail_count,
+            "Req 4.3: the counter must count purely from the post-success "
+            "reset, not from the job's total pre-success failure history");
+
+        /* NOT asserted here: job.state == JOB_STAT_PEND / job.allocs ==
+         * NULL after this retryable failure. A kiro-debug investigation
+         * (HIGH confidence; bug-1631) found this exact sub-scenario -- a
+         * retryable dispatch failure occurring while the job's
+         * controller-side state is already RUN, from step 2's real
+         * success -- is not reachable via any real caller in the
+         * currently-implemented system: dispatch_a_job_()'s only real
+         * caller, handle_jobdispatch(), fires only on the scheduler's own
+         * fresh DISPATCH decision, and the scheduler's picker permanently
+         * removes a job from pendJobs the moment it dispatches it (task
+         * 2.1) -- nothing implemented today ever re-adds a successfully-
+         * dispatched (RUN) job back to pendJobs for redispatch. The only
+         * mechanism design.md anticipates for that is scheduler-
+         * preemption, still unimplemented (requirements-generated phase,
+         * no approvals). Revisit once that (or an equivalent) mechanism
+         * lands -- see bug-1631 for the full analysis. */
+
+        free_JobAlloc_ptr(&alloc);
+    }
+
+    /* Step 4: a 4th consecutive failure since the reset -- now exceeds the
+     * bound, resolves TERMINAL, pinning the exact boundary the reset
+     * establishes (not one failure earlier, not one failure later). */
+    {
+        xce_job_alloc_t* alloc;
+        int              rc;
+
+        alloc = make_job_alloc(DFR_JOBID_RESET, 1,
+            make_host_allocs(1,
+                make_host_alloc((char*)"dfr-wkr-reset", 1,
+                    make_allocs(1,
+                        make_alloc((char*)"slots", NULL, 0, 1.0f, 0)))));
+        TEST_ASSERT_NOT_NULL(alloc);
+
+        rc = dispatch_a_job_test(&g_dfr, NULL, alloc);
+        TEST_ASSERT_TRUE(rc < 0);
+
+        /* mark_job_fail()/apply_job_fail_transition() sets job.state =
+         * JOB_STAT_EXIT UNCONDITIONALLY (confirmed by reading its full
+         * body, job.c ~2303-2325 -- an absolute assignment, not gated on
+         * the prior value), so this assertion is NOT affected by bug-1631
+         * and is expected to genuinely pass. */
+        TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_EXIT, j->job.state,
+            "Req 4.3: the 4th consecutive failure since the reset must "
+            "resolve the job terminal instead of retrying again");
+        TEST_ASSERT_EQUAL_UINT32(4, j->dispatch_fail_count);
+        /* NOT asserted here: job.allocs == NULL on this terminal
+         * resolution. Same bug-1631 unreachable-today scenario as the
+         * loop above (apply_job_fail_transition() never touches
+         * job.allocs, but only matters once a RUN job with committed
+         * allocs can reach a retryable/terminal failure at all) -- see
+         * that comment for the full analysis. */
+
+        free_JobAlloc_ptr(&alloc);
+    }
+
+    dfr_teardown_broken_ep();
+    dfr_teardown();
+    printf("test_rpc_failure_bound_resets_after_intervening_success: "
+          "PASS\n");
+}
+
 void test_dispatch_fail_retry(void)
 {
     UnitySetTestFile(__FILE__);
@@ -560,4 +813,5 @@ void test_dispatch_fail_retry(void)
     RUN_TEST(test_worker_notready_resolves_retryable);
     RUN_TEST(test_rpc_failure_resolves_retryable);
     RUN_TEST(test_rpc_failure_bounded_escalation);
+    RUN_TEST(test_rpc_failure_bound_resets_after_intervening_success);
 }