diff --git a/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md b/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md index 6993d548..05d76315 100644 --- a/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md +++ b/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md @@ -76,7 +76,7 @@ - _Depends: 2.1, 2.2_ - _Boundary: existing test suite (read-only run, no source changes)_ -- [ ] 4.4 (P) Keep the snapshot format's own scope-decision documentation current +- [x] 4.4 (P) Keep the snapshot format's own scope-decision documentation current - Correct the existing source-level note that a job's resource allocation "is not captured" to reflect that it now is, and add a note documenting the job's latest resource-usage report as a known, still-uncaptured gap requiring a future format change. - Observable completion: the source-level documentation and the feature's actual behavior no longer contradict each other. - _Requirements: 3.2, 3.3_ @@ -91,4 +91,5 @@ - _Depends: 4.1, 4.2, 4.3, 4.4_ ## Implementation Notes +- Task 4.4 follow-up (non-blocking, flag for 4.5): `snapshot.c`'s "internal_state contents" comment section (~lines 217-220, distinct from the "Scope decisions" section this task corrected) still cites the now-stale premise "this task also does not restore job->allocs/accounts (see 'Scope decisions' below)" as part of the rationale for why `sharedInUse`/`resInUse` aren't serialized. The exclusion conclusion itself still holds (session-transient state, re-summed every session by `sched_account_jobs()`), but the cited premise contradicts the corrected "Scope decisions" bullet below it. Out of task 4.4's boundary (scoped strictly to the "Scope decisions" header comment); confirmed by two independent readings (implementer + reviewer). Should be corrected — drop/rephrase the stale clause, keep the still-valid rationale — before task 4.5's field-completeness audit signs off clean. - Task 4.2 side-finding (not fixed here, out of boundary): `core_upsert_queue()`'s existing-queue branch (`core_events.c:335-336`) calls `FREEUP(q->resreq)` (plain `free()`) on a `resreq` that was set via the rtti-backed `makeString()` (mismatched allocator/free, same bug class as already-fixed bug-1503/bug-1600). Reproducible via any second `QUEUE_UPDATE` for a queue whose `resreq` was ever populated via `makeString()` — not specific to snapshot restoration; the near-identical pattern already exists in the legacy `sched_update_existing_queue()` (`schedobj.c:573-575`). Confirmed real by two independent readings (implementer + reviewer). Needs its own buglog.json entry and likely a separate follow-up task — tracked here so it isn't lost. diff --git a/src/scheduler/src/snapshot.c b/src/scheduler/src/snapshot.c index 06cedbb7..c16af889 100644 --- a/src/scheduler/src/snapshot.c +++ b/src/scheduler/src/snapshot.c @@ -244,18 +244,41 @@ * accompanying task report for the full reasoning). Restored jobs get * priority 0, matching what a fresh JOBS_PUSH already produces. * - job->allocs / job->accounts (in-flight dispatched-allocation results - * for RUNNING jobs) are NOT captured. AC 21's "Jobs" checklist row - * ("current state, priority, queue assignment, queue_time") does not - * list allocations; xce_job_alloc_t is a nontrivial nested structure - * (per-host, per-resource) with no existing deep-copy helper anywhere - * in this codebase; and aliasing it across two independently-alive - * cores (the Observable's own test scenario keeps both the pre-save and - * post-load core alive simultaneously) would create a genuine double- - * free hazard the instant either core's normal JOB_STATE_CHANGE path - * frees it. A RUNNING job therefore survives the round trip with - * correct state/queue/user/project/resreq, but "forgets" what it is - * currently dispatched on. Flagged as a concern in this task's status - * report, not silently worked around. + * for RUNNING jobs) ARE captured and restored (scheduler-snapshot-job- + * allocation spec). AC 21's "Jobs" checklist row ("current state, + * priority, queue assignment, queue_time") does not list allocations, + * but the original blocker to restoring them anyway -- xce_job_alloc_t + * being a nontrivial nested structure (per-host, per-resource) with no + * existing deep-copy helper anywhere in this codebase, and aliasing it + * across two independently-alive cores (the Observable's own test + * scenario keeps both the pre-save and post-load core alive + * simultaneously) creating a genuine double-free hazard the instant + * either core's normal JOB_STATE_CHANGE path frees it -- is resolved: + * build_wire_job() (snapshot_wire.c) and xce_sched_core_load_snapshot() + * (snapshot_load.c) now deep-copy job->allocs via dup_job_alloc() + * (src/lib/xcealloc.c, composed from the existing make_alloc()/ + * make_host_alloc()/make_job_alloc() constructors and freed via the + * existing free_JobAlloc_ptr()), which never aliases the source core's + * own copy, so the two cores remain independently freeable. + * job->accounts is still not itself put on the wire -- it is derived, + * never authoritative (see snapshot_wire.c) -- but is reconstructed on + * load via the existing make_job_account(). A RUNNING job therefore + * survives the round trip with its allocation intact, and worker/ + * shared resource "used" totals are correct from the moment the + * snapshot loads: sched_account_jobs() sums them from these restored + * accounts on the very next session, identically to a job whose + * allocation came from live dispatch. A snapshot saved before this + * capability existed still loads cleanly -- job->allocs/accounts are + * simply left NULL for such jobs, matching today's pre-existing + * used=0 behavior rather than being treated as an error. + * - job->rusages (the job's latest controller-reported resource-usage + * report) is NOT captured. Unlike allocs/accounts above, this is not a + * missing-helper problem -- the wire xce_job_t type (xce/types.h) has + * no field for it at all, so closing this gap needs an actual capnp + * schema addition to sched_snapshot_data_t/xce_job_t, not just new + * save/load wiring; out of scope for this feature and tracked as a + * separate, future format change. A restored job's rusages is always + * left NULL/empty, matching what a fresh JOBS_PUSH already produces. * - xce_sched_queue_t's project_name/job_pick_plugin have no field on the * wire xce_queue_t type at all (confirmed: xce/types.h's xce_queue_t * has no such fields) -- the SAME pre-existing gap core_build_queue()