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
This commit is contained in:
Rongsong Shen 2026-08-23 20:14:18 +00:00
parent d4ada4474b
commit 16d90319db
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
2 changed files with 257 additions and 1 deletions

View file

@ -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.