docs(scheduler-snapshot-job-allocation): correct snapshot.c scope-decision documentation

Task 4.4: the "Scope decisions" header comment previously stated a
job's allocs/accounts are "not captured" -- update it to describe the
now-implemented dup_job_alloc()/make_job_account() restoration path,
and add a note documenting job->rusages as a known, still-uncaptured
gap requiring a future capnp schema change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SSuXfK6nD1Nue5EjjdhjNZ
This commit is contained in:
Rongsong Shen 2026-08-28 04:55:46 +00:00
parent 9988e7141d
commit 93d791c688
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
2 changed files with 37 additions and 13 deletions

View file

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

View file

@ -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()