diff --git a/.kiro/specs/scheduler-dispatch-commit/tasks.md b/.kiro/specs/scheduler-dispatch-commit/tasks.md index 8e3f122c..d0752f83 100644 --- a/.kiro/specs/scheduler-dispatch-commit/tasks.md +++ b/.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. diff --git a/src/controller/testsuite/test_dispatch_fail_retry.c b/src/controller/testsuite/test_dispatch_fail_retry.c index cdf91972..37ceec17 100644 --- a/src/controller/testsuite/test_dispatch_fail_retry.c +++ b/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); }