feat(worker-rejoin-resync): reconcile unrecognized job to pending (task 2.1)
Add record_job_reconcile_pending_fact(cluster_t*, job_t*) to job.c: eagerly applies apply_job_update_transition(job, JOB_STAT_PEND) (real, live mutation, not a deferred peek -- matching dispatch_a_job_'s established pattern, not record_job_finish_fact's), releases job->job.allocs on an applied (non-no-op) transition, then durably appends via the generic persist_submit_job_fact(WAL_JOB_UPDATE). Caller holds job's lock throughout and is responsible for calling persist_notify_job_updated_already_persisted() after unlocking -- this function deliberately does not call it itself. Closes bug-1631 at this new call site: a RUN job with a real allocation reconciled to PEND (Requirement 4.6, worker reports it unrecognized) would otherwise carry a stale allocation into its next dispatch attempt, exactly the gap bug-1631 documented as dormant until something re-admits an already-dispatched job to the scheduler's PEND pool -- this feature is that path. dispatch_a_job_ itself is unchanged (still correct for a job's first-ever dispatch, the only case reachable there). 3 tests: allocs release + eager-mutation-before-unlock proof, no-op on already-pending, and the Requirement 4.4 terminal-state-preservation proof (rejects overwriting JOB_STAT_DONE). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4M5MJM2zK6nU43da267Ku
This commit is contained in:
parent
eda2eb1bf0
commit
cfe66dde16
7 changed files with 433 additions and 2 deletions
|
|
@ -18411,7 +18411,7 @@
|
|||
"error_message": "New test test_rpc_failure_bound_resets_after_intervening_success (test_dispatch_fail_retry.c) fails: 'Req 4.1 (bug-1631, NOT YET FIXED): a retryable dispatch failure must return the job to PEND -- fails here only because this job previously had a REAL successful dispatch (step 2)'. Expected JOB_STAT_PEND, got JOB_STAT_RUN (and separately, job.allocs stays non-NULL instead of being released).",
|
||||
"file": "src/controller/job.c",
|
||||
"root_cause": "dispatch_a_job_()'s four retryable-failure resolution branches (XCE_DISPATCH_FAIL_SITE_WORKER_LOOKUP/_WORKER_NULL/_WORKER_NOT_READY/_RPC_FAILURE, job.c ~lines 1040-1435, all four added/restructured by scheduler-dispatch-commit task 3.3) never write j->job.state or j->job.allocs on the 'resolved_retryable' (within-bound) path -- only job.dispatch_fail_count is touched. This is CORRECT for a job's very FIRST dispatch attempt: the job was never anything but PEND and allocs was never committed, so 'no reversal needed' (design.md's own 'Controller Dispatch Commit' state-model text: 'on RPC failure, state is untouched -- still PEND, never entered RUN, so no reversal is needed, per the Research Log finding') is vacuously true. That reasoning silently assumes the job has NEVER had a prior successful dispatch. It breaks the moment dispatch_a_job_() is called again on a job that previously succeeded (went RUN, committed allocs) and now fails retryably again -- exactly the scenario Requirement 4.3's own text ('consecutive... dispatch failures with no intervening successful dispatch') and task 3.1's own observable-completion criterion ('a job that fails, succeeds, then fails again is evaluated against the bound as if it had only failed once') explicitly anticipate. In that scenario job.state incorrectly stays RUN and job.allocs incorrectly stays committed across the retry, violating Requirement 4.1 ('the Scheduler and Controller shall return the job to a pending state') and Requirement 4.4 ('the Scheduler and Controller shall each release the resource-allocation and commitment data'). The terminal (bound-exceeded) resolution path is separately confirmed affected the same way: mark_job_fail()/apply_job_fail_transition() (job.c ~2303-2325) unconditionally sets job.state = JOB_STAT_EXIT (so the STATE outcome is correct on the terminal path) but never touches job.allocs either, so Requirement 4.5 is also affected on that path. Every existing retryable-failure test (test_dispatch_fail_retry.c's other three tests, test_rpc_failure_bounded_escalation's own 4-consecutive-failure test) only ever drives a job that starts and stays PEND with allocs always NULL throughout, so none of them could have caught this -- only a real intervening success (constructed by the new fail->succeed->fail scenario, scheduler-dispatch-commit task 5.2) exposes it. This is a design-level gap, not just an implementation oversight: design.md's own Controller Dispatch-Failure Classification / Controller Dispatch Commit sections both assert-by-omission that job.state/job.allocs need no reversal on any of the six failure paths, without qualifying that assertion to 'only true for a job's first-ever dispatch attempt'.",
|
||||
"fix": "NOT YET FIXED. Discovered live while implementing scheduler-dispatch-commit task 5.2 (test-only task; the fix requires modifying src/controller/job.c, out of that task's test-only boundary -- reported as BLOCKED). Candidate fix: in each of the four retryable branches' 'resolved_retryable = true' case (still under the existing lock(j, RTTI_LOCK_WRITE)), add `j->job.state = JOB_STAT_PEND;` and release any committed j->job.allocs (free_JobAlloc_ptr(&j->job.allocs); j->job.allocs = NULL;) before unlock(j) -- mirroring, in reverse, what Controller Dispatch Commit's own success path (job.c ~1343-1372) does when transitioning PEND->RUN. Needs a design-review pass first: design.md's 'Controller Dispatch-Failure Classification' and 'Controller Dispatch Commit' sections both need their state-model text corrected to no longer claim the failure paths need no reversal unconditionally. UPDATE (2026-08-23, task 5.2 completion): a kiro-debug investigation (HIGH confidence, independently re-verified even more exhaustively by task 5.2's own reviewer via a full trace of every pendJobs-reinsertion and controller-side 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. Only the not-yet-implemented scheduler-preemption spec (confirmed still requirements-generated, no approvals) could someday create that path. Per that finding, the COMMITTED version of test_rpc_failure_bound_resets_after_intervening_success (src/controller/testsuite/test_dispatch_fail_retry.c, commit 16d90319) deliberately does NOT assert the job.state/job.allocs reversal described above -- it keeps only the Req 4.3 counter-reset assertions (which are correct and GREEN). There is no live RED test for this bug today; it remains an intentionally-deferred, documented gap to revisit once scheduler-preemption (or an equivalent mechanism) actually creates a reachable RUN-then-retryable-redispatch path -- not before.\n\nADDENDUM (2026-09-01, worker-rejoin-resync design re-validation): worker-rejoin-resync's design.md now explicitly names this bug as the reason Requirement 4.6 (reconciling an 'unrecognized' RUN job back to PEND, eligible for re-dispatch) must not be shipped without the candidate fix described above. design.md's `record_job_reconcile_pending_fact()` is specified to release `job->job.allocs` at the same point it sets the job PEND -- the exact fix this entry already proposed, now scoped to a concrete call site instead of a hypothetical future one. Still NOT YET FIXED IN CODE (worker-rejoin-resync is design-phase only as of this note) -- this addendum records that the 'unreachable, deliberately deferred' status ends the moment that spec is implemented, and the fix must land in the same change as Requirement 4.6, not as a follow-up.",
|
||||
"fix": "NOT YET FIXED. Discovered live while implementing scheduler-dispatch-commit task 5.2 (test-only task; the fix requires modifying src/controller/job.c, out of that task's test-only boundary -- reported as BLOCKED). Candidate fix: in each of the four retryable branches' 'resolved_retryable = true' case (still under the existing lock(j, RTTI_LOCK_WRITE)), add `j->job.state = JOB_STAT_PEND;` and release any committed j->job.allocs (free_JobAlloc_ptr(&j->job.allocs); j->job.allocs = NULL;) before unlock(j) -- mirroring, in reverse, what Controller Dispatch Commit's own success path (job.c ~1343-1372) does when transitioning PEND->RUN. Needs a design-review pass first: design.md's 'Controller Dispatch-Failure Classification' and 'Controller Dispatch Commit' sections both need their state-model text corrected to no longer claim the failure paths need no reversal unconditionally. UPDATE (2026-08-23, task 5.2 completion): a kiro-debug investigation (HIGH confidence, independently re-verified even more exhaustively by task 5.2's own reviewer via a full trace of every pendJobs-reinsertion and controller-side 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. Only the not-yet-implemented scheduler-preemption spec (confirmed still requirements-generated, no approvals) could someday create that path. Per that finding, the COMMITTED version of test_rpc_failure_bound_resets_after_intervening_success (src/controller/testsuite/test_dispatch_fail_retry.c, commit 16d90319) deliberately does NOT assert the job.state/job.allocs reversal described above -- it keeps only the Req 4.3 counter-reset assertions (which are correct and GREEN). There is no live RED test for this bug today; it remains an intentionally-deferred, documented gap to revisit once scheduler-preemption (or an equivalent mechanism) actually creates a reachable RUN-then-retryable-redispatch path -- not before.\n\nADDENDUM (2026-09-01, worker-rejoin-resync design re-validation): worker-rejoin-resync's design.md now explicitly names this bug as the reason Requirement 4.6 (reconciling an 'unrecognized' RUN job back to PEND, eligible for re-dispatch) must not be shipped without the candidate fix described above. design.md's `record_job_reconcile_pending_fact()` is specified to release `job->job.allocs` at the same point it sets the job PEND -- the exact fix this entry already proposed, now scoped to a concrete call site instead of a hypothetical future one. Still NOT YET FIXED IN CODE (worker-rejoin-resync is design-phase only as of this note) -- this addendum records that the 'unreachable, deliberately deferred' status ends the moment that spec is implemented, and the fix must land in the same change as Requirement 4.6, not as a follow-up.\n\nRESOLVED IN CODE (2026-09-01, worker-rejoin-resync task 2.1): record_job_reconcile_pending_fact() (src/controller/job.c, after mark_job_update) now performs exactly the fix this entry proposed -- free_JobAlloc_ptr(&job->job.allocs) immediately after a successful (JOB_TRANSITION_APPLIED) apply_job_update_transition(job, JOB_STAT_PEND), under the same caller-held write lock, before the durable WAL_JOB_UPDATE fact is appended. This is called when a worker reports a queried job as 'unrecognized' during rejoin reconciliation (Requirement 4.6) -- exactly the RUN-job-returned-to-PEND-and-re-admitted-to-the-dispatch-pool precondition this entry's own root_cause said was needed to make the gap reachable. Regression-tested directly: test_reconcile_pending_applies_releases_allocs_and_notifies (test_worker_resync.c) seeds a real JOB_STAT_RUN job with a real non-NULL allocation and asserts allocs == NULL immediately after the call. Independently reviewed and approved (kiro-review). dispatch_a_job_()'s own four retryable-failure branches (the original bug-1631 site) are still untouched by this fix -- they remain correct for a job's first-ever dispatch attempt, which is still the only reachable case THERE; this fix closes the gap at the new call site this feature introduces, not by changing dispatch_a_job_ itself.",
|
||||
"tags": [
|
||||
"controller",
|
||||
"dispatch-failure",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue