fix(scheduler): retire reconciled jobs from picker candidates (bug-1759/1779)
sim_reconcile_unaccounted_jobs() (the mechanism that replays historical DISPATCH decisions in --sim mode) reconstructed a job's allocation but never called the picker's on_dispatch() hook, unlike the live allocator path which always does. The job therefore stayed a live candidate in the picker's own pendJobs bookkeeping, and sim_picker_thread_fn() (which unconditionally PENDs whatever it is still offered) kept re-offering and re-PENDing it every subsequent round -- live-observed as an already- dispatched job receiving a bogus PEND decision (bug-1759: 22/50 jobs in one batch; bug-1779: 5/10 array-job elements). This directly contradicts scheduler-sim-decision-parity's own design.md, which already documented "the picker never re-decides an already-resolved job" as the correct assumption -- the implementation had drifted from it. Fixed by calling on_dispatch() immediately after reconstructing the job's allocation, mirroring sched_allocator()'s own live-dispatch ordering. test_sim_verify_no_false_mismatch_across_round_separated_dispatch's own fixture had baked in the stale-PEND symptom as "expected" data; updated to match the corrected behavior (dispatch-only round 1, empty filler rounds). Both design docs updated in place with correction notes.
This commit is contained in:
parent
ef3c4670a6
commit
0d3f925c4d
5 changed files with 213 additions and 40 deletions
|
|
@ -218,6 +218,7 @@ void sim_reconcile_unaccounted_jobs(xce_sched_sim_driver_t* sim,
|
|||
|
||||
**Implementation Notes**
|
||||
- Integration with `--sim-verify` (CONFIRMED during design validation, resolving the prior "read the comparison driver at implementation time" placeholder): `sim_loop.c`'s verify branch (current code: the `while ((cbuf_get(&(sim->core.decisions), &d) == 0) ...)` loop) walks `decoded_session->decisions` *positionally* via an index `di`, comparing each live-drained decision `d` against `decoded_session->decisions[di]` and advancing `di` once per live decision. A recorded decision belonging to a job this round's reconciliation step consumes (i.e., a job whose allocation/decision visibility is being restored from the recording rather than freshly re-derived by the live picker) will **never** produce a corresponding live-drained `d` — the picker never re-decides an already-resolved job. Comparing positionally without accounting for this would misalign every subsequent entry and produce false mismatches (Requirement 5.2 violation), independent of any bug in the comparison logic itself.
|
||||
- **Correction (2026-09-07, bug-1759/bug-1779):** "the picker never re-decides an already-resolved job" was stated here as an assumption but was not actually mechanically enforced — the reconciliation step reconstructed the job's allocation/accounts but never called `picker_iface->on_dispatch()`, so the job remained a live candidate in the picker's own `pendJobs` and WAS re-offered (and re-PENDed, via `sim_picker_thread_fn()`'s unconditional-PEND behavior) on every subsequent round until an unrelated RUN-confirmation event happened to remove it. Live-observed (bug-1759, bug-1759's 50-job batch; bug-1779, a 10-element array job) as a job showing a PEND decision in the same or later rounds despite already having been dispatched. Fixed: the reconciliation step (`sim_loop.c`'s FOUND branch) now calls `picker_iface->on_dispatch()` immediately after reconstructing `job->allocs`/`job->accounts`, mirroring `sched_allocator()`'s own live-dispatch ordering (`scheduler.c`) exactly. This does not change this section's positional --sim-verify reasoning (still correct, now actually true rather than merely assumed) and does not touch scheduling/allocation *policy* (this spec's own Non-Goals) — only picker candidate-bookkeeping consistency for a job whose dispatch was already decided (by the original live recording, not re-decided here).
|
||||
- **Resolution**: the reconciliation step removes each recorded decision it consumes from `decoded_session->decisions` *before* the verify drain loop runs for that round — it already has the exact jobid-matched entry in hand from its own lookup, so this is a natural side effect of the same pass, not a second scan. This keeps the remaining (unconsumed) recorded entries positionally aligned with the live-drained stream exactly as they are today, when no reconciliation was needed for a job. No change to `sim_decision_eq()`/`sim_verify_job_alloc_eq()`/`sim_verify_job_preason_eq()` (the comparison predicates in `sim_verify.c`) is required — only `sim_loop.c`'s bookkeeping of which array entries reach that comparison.
|
||||
- **Memory ownership (must not be overlooked)**: `decoded_session->decisions`/`n_decisions` (`replay_session_t`, `typemap.h`) are a plain heap-owned array — `free_ReplaySession_ptr()` frees it via `free_ReplayDecision_list(d->n_decisions, d->decisions)`, which walks and frees exactly the first `n_decisions` entries, then frees the array itself. Consequently, simply decrementing `n_decisions` to "drop" a consumed entry (without separately freeing that entry) is **not** safe compaction — it silently leaks that entry's payload every round a reconciliation happens, since the round-end free walk will never reach it again. The reconciliation step must instead: (1) extract the data it needs (see ownership sub-point immediately below — this is NOT a plain read for a DISPATCH entry), (2) call the existing `free_ReplayDecision(entry)` + `free(entry)` (the same two calls `free_ReplayDecision_list()` already makes per-element) on the one consumed entry, then (3) compact the array by shifting the remaining entries down over the removed slot (`memmove`), and decrement `n_decisions` by one. Shift-down, not swap-with-last: swap-with-last is the usual O(1) removal trick, but it would relocate the last entry into the removed slot and change its position relative to the other remaining entries — corrupting the very relative ordering the positional `di` comparison in the point above depends on. `decoded_session` is decoded fresh once per round regardless, so this compaction is confined to the current round with no cross-round effect.
|
||||
- **Ownership hazard in step (1) above — a DISPATCH entry's `alloc` must be moved, not aliased, before step (2) frees the entry**: the established live-path precedent (`scheduler.c:1440`, `sched_commit_decision()`) is `job->allocs = decision->u.alloc;` — a pointer *takeover*, not a deep copy; `job->accounts` (`make_job_account(job->allocs)`) is then built directly on top of that same, now job-owned, memory. If reconciliation reconstructs `job->allocs` the same way from a recorded DISPATCH entry — `job->allocs = entry->body.dispatch->alloc;` (`sched_dispatch_decision_t.alloc`, an `xce_job_alloc_t*`, `typemap.h`) — that pointer is now aliased by BOTH `job->allocs` and the entry about to be freed in step (2): `free_ReplayDecision()` → `free_DispatchDecision()` → `free_JobAlloc_ptr(&d->alloc)` would free that exact `xce_job_alloc_t`, leaving `job->allocs` dangling immediately (and a double-free whenever the job's own allocation is later released through its normal lifecycle). **Required fix**: immediately after the takeover assignment, null out the source field — `entry->body.dispatch->alloc = NULL;` — before calling `free_ReplayDecision(entry)`; every `_ptr`-style free helper in this codebase (including `free_JobAlloc_ptr`) already no-ops on `NULL`, so this makes the subsequent free of the rest of the entry safe without a special case. This is DISPATCH-specific and, by construction, the *only* case this component needs to handle: the reconciliation trigger is a RUN-state job, and a job can only reach RUN-state via a recorded DISPATCH decision — a recorded PEND (or PREEMPT) entry for the same job in the same round is structurally impossible to match here, since PEND/PREEMPT are exactly the decisions that do *not* put a job into RUN-state. (Independently confirmed no `job->preason`/equivalent field exists to alias in the first place — `job->accounts`/`job->allocs` are DISPATCH-only concepts, consistent with this.) No PEND/PREEMPT ownership case needs to be designed or implemented.
|
||||
|
|
|
|||
|
|
@ -19,6 +19,8 @@
|
|||
- Any change to `--sim-verify`'s comparison semantics beyond not introducing a new false mismatch.
|
||||
- Reopening or modifying `scheduler-sim-decision-parity`'s own requirements, design, or tasks, or any normal-mode (`driver.c`) behavior.
|
||||
|
||||
**Correction (2026-09-07, bug-1759/bug-1779):** the "live picker" Non-Goal above was never meant to license `sim_reconcile_unaccounted_jobs()` leaving a reconciled job as a live picker candidate — it means "do not give the live picker real resource-matching allocation logic" (still out of scope, still `scheduler-allocation`'s territory). Whether a reconciled job stays in the picker's own `pendJobs` bookkeeping is a `scheduler-sim-decision-parity` concern (see that spec's design.md, "Integration with `--sim-verify`", which already documented "the picker never re-decides an already-resolved job" as the correct assumption). The implementation had drifted from that documented assumption: `sim_reconcile_unaccounted_jobs()` never called `picker_iface->on_dispatch()`, so a reconciled job stayed a live candidate and was live-observed being re-offered and re-PENDed every subsequent round (bug-1759: 22/50 already-dispatched jobs in one batch; bug-1779: 5/10 array elements). Fixed by adding that missing `on_dispatch()` call, mirroring `sched_allocator()`'s own live-dispatch ordering (`scheduler.c`) exactly — a correction to already-agreed-upon behavior, not a new capability, so it did not need to reopen this spec's own requirements/tasks. `test_sim_verify_no_false_mismatch_across_round_separated_dispatch` (`test_sim_verify.c`) was updated to match: rounds 2-3 of its fixture are now empty `ReplaySession`s (no more picker-echoed PEND for an already-reconciled job) rather than baking in the pre-fix stale-PEND behavior as "expected."
|
||||
|
||||
## Boundary Commitments
|
||||
|
||||
### This Spec Owns
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue