docs: formalize save/restore field-completeness as a standing invariant

Prompted by rediscovering the same bug class in two places this
session: the controller's WAL already has this documented for one
mechanism (persistence.md's APPLY_CLEAN/bug-1678 section, xce-audit-
invariants controller dimension 7) but the scheduler's separate
snapshot save/load mechanism had the identical gap (job->allocs, this
session's scheduler-snapshot-job-allocation brief) with no equivalent
tracking. Closes that gap and makes the invariant self-reinforcing
going forward rather than something that has to be independently
rediscovered per persistence mechanism:

- xce-audit-invariants: new scheduler dimension 4 ("Field-complete
  snapshot save/load parity"), sibling of controller dimension 7,
  scoped to snapshot.c/snapshot_wire.c/snapshot_load.c.
- persistence.md: new "Snapshot Save/Load Field-Completeness
  (Scheduler)" section cross-referencing the WAL-side bug-1678 writeup
  and the new scheduler dimension.
- CLAUDE.md: new top-level Key Rules bullet stating the invariant
  plainly, plus adding persistence.md to the steering-file pointer
  table (it was missing).
- kiro-spec-design's design-review-gate.md (the mandatory pre-write
  gate) and kiro-validate-design's design-review.md (the optional deep
  review): new conditional review sections -- applies only when a
  design touches a save/restore mechanism, requires enumerating every
  persisted field's capture status rather than trusting an aggregate
  "state looks right" check. Catches this class of gap at design time,
  before implementation, rather than only via a later audit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg

.kiro/specs/scheduler-snapshot-job-allocation/brief.md

@@ -0,0 +1,53 @@
+# Brief: scheduler-snapshot-job-allocation
+
+## Problem
+Simulation-mode capacity/contention analysis is untrustworthy for any scenario seeded from a snapshot -- the common case, since real investigations start from a live snapshot + replay (`xce_scheduler -i -s <replay>`). Reported live: after loading a snapshot with 22 running jobs and replaying 79 sessions forward, `info summary` correctly counts 22 running jobs, but `info worker`'s "used" resource values only ever reflect 2 of them -- the rest permanently show zero contribution, making any capacity check against a snapshot-seeded scenario silently wrong.
+
+## Current State
+- `xce_sched_core_load_snapshot()` (`snapshot_load.c`) restores a job's `state` correctly (so a snapshot-loaded job shows as RUN) but never restores `job->allocs`/`job->accounts` -- confirmed: `build_wire_job()` (`snapshot_wire.c:167`) carries an explicit `/* job->allocs/accounts: not captured */` comment.
+- This was a deliberate, documented scope decision from `scheduler-core` task 8.1 (`snapshot.c`'s own "Scope decisions" header comment), not an oversight -- there was no deep-copy helper for `xce_job_alloc_t` anywhere in the codebase, and aliasing it (instead of deep-copying) across two independently-alive cores would create a genuine double-free hazard the moment either core's normal job-lifecycle path frees it. The header explicitly flags the consequence: a restored RUNNING job "forgets what it is currently dispatched on."
+- `scheduler-sim-eager-reconciliation` (this session, already shipped) added `sim_reconcile_unaccounted_jobs()`, which DOES reconstruct `job->allocs`/`job->accounts` -- but only by finding a matching DISPATCH decision within the replay event stream currently being processed. A job already running as of the snapshot had its dispatch decision happen before the snapshot was taken, so that event can never appear in the subsequent replay -- this mechanism structurally cannot recover it. It correctly handles the complementary case (dispatch decision discoverable later in replay); this spec closes the other half.
+- `sched_account_jobs()` recomputes every worker's `resInUse` fresh each round purely by walking jobs where `JOB_IS_RUN(state) && accounts != NULL` and charging their accounts -- confirmed via source read. Once `job->accounts` is correctly populated from any source, "used" accounting works automatically; no separate accounting-side change is needed.
+- Viability check (this session, subagent-verified) found the fix is smaller than first assumed: the wire type already has an `allocs` field -- `Job.allocs`/`SchedJob.allocs @7: JobAlloc` in the capnp schema map directly onto `xce_job_t.allocs` (same C struct, no translation layer), and `JobAlloc`/`HostAlloc`/`ResAlloc` already fully mirror `xce_job_alloc_t`/`xce_host_alloc_t`/`xce_alloc_t`. It already flows through the existing generic `encode_SnapshotEntry_ptr()`/`decode_SnapshotEntry()` calls with zero schema changes needed -- no version-gating required (that pattern is only needed for the opaque `internal_state` byte blob elsewhere in the same file, not for a real typed field).
+- No existing deep-copy helper for `xce_job_alloc_t`/`xce_host_alloc_t`/`xce_alloc_t` anywhere in the codebase (confirmed via grep for `dup_`/`copy_`/`clone_` conventions). `snapshot.c`/`snapshot_wire.c`/`snapshot_load.c` deliberately contain no capnp code (an existing, intentional architectural boundary) -- the deep-copy should be composed from the existing `make_alloc`/`make_host_alloc`/`make_job_alloc` constructors (`src/lib/xcealloc.c`), not the capnp-generated codec, and paired with the existing `free_JobAlloc_ptr` for cleanup.
+- Side-finding, already fixed and committed this session ahead of this spec (bug-1722): `make_job_account()`/`make_host_allocs()` in `xcealloc.c` had a live heap buffer overflow for any job spanning more than one host (`calloc(1, ...)` sized for one host pointer, loop wrote up to `nhosts-1`). Fixed independently before this spec starts, since this spec's own tests will exercise multi-host allocations through `make_job_account()` more than any existing code path does today.
+- **Direct precedent, same bug class, sibling persistence layer, already fixed (bug-1678, Gitea #43):** the controller's WAL had the identical gap for the exact same data -- after restarting all daemons, a previously-running job's `STATUS=RUN` recovered correctly on replay but `EXEC_HOST` came back permanently blank, because `WAL_JOB_DISPATCH` never persisted the job's allocation. Part of a larger campaign (bug-1687/1689/1690/1691/1692, all already committed before this session, plus bug-1697 start_time, committed this session) fixing WAL replay silently dropping various job fields on restart. Bug-1678's own writeup confirms `JobAlloc`/`xce_job_alloc_t` was already a full top-level capnp struct requiring **no schema change** -- the fix was pure save/load glue (`capnp_encode_joballoc()`/`decode_joballoc()` in `persist.c`, mirroring existing `capnp_encode_worker()`/`restore_job()` patterns), landed as `db142d41 fix(controller): persist dispatch-time resource allocation into WAL_JOB_DISPATCH`. This independently confirms the viability check's finding for the scheduler side (existing wire field, no schema change, pure glue) via a working, shipped implementation of the identical shape in a sibling persistence mechanism -- the controller's WAL and the scheduler's snapshot are separate, non-sharing persistence layers, so fixing one did not fix the other.
+
+## Desired Outcome
+A job that is already running as of a snapshot's save point survives a save/load round-trip with its full allocation intact: `job->allocs` populated, `job->accounts` reconstructed via the existing `make_job_account()`, and worker/shared resource "used" accounting correctly reflects it from the moment the snapshot loads -- no longer dependent on whether its dispatch decision happens to also appear later in a replay stream. A snapshot file saved before this change still loads cleanly (no crash, no misinterpretation) -- those jobs simply have nothing to restore, the same `used=0` behavior as today, a clean degrade rather than a regression.
+
+## Approach
+Extend snapshot save/load to populate/restore the wire type's already-existing `allocs` field -- no schema change:
+1. New deep-copy helper(s), composed from the existing `make_alloc`/`make_host_alloc`/`make_job_alloc` constructors, that build a fresh, independent `xce_job_alloc_t` from a source one -- paired with the existing `free_JobAlloc_ptr` for cleanup.
+2. `build_wire_job()` (`snapshot_wire.c`) populates the deep-copied allocation when `job->allocs != NULL`.
+3. `xce_sched_core_load_snapshot()` (`snapshot_load.c`) restores it via the same deep-copy (never aliasing the source snapshot's own copy) and calls `make_job_account()` to rebuild `job->accounts` -- mirroring the exact pattern `sim_reconcile_unaccounted_jobs()` already established for its own narrower case.
+4. Backward compatibility is a plain NULL/`nhosts>0` guard, not the `internal_version` byte-blob pattern -- unnecessary for a real typed field.
+
+## Scope
+- **In**: the deep-copy helper; `build_wire_job()` save-side wiring; `xce_sched_core_load_snapshot()` load-side wiring + `job->accounts` reconstruction; cross-core round-trip tests (multi-host, multi-resource, shared-resource allocations); an old-snapshot-file backward-compat test; a sim-mode end-to-end test proving `info worker`'s "used" value is correct immediately after loading a snapshot with running jobs (closing the originally reported gap).
+- **Out**: any change to `sim_reconcile_unaccounted_jobs()` itself (already correct for its own case, stays as-is); any capnp wire schema change (not needed); any change to `internal_state`/`internal_version` (unrelated field); the `job->priority` and queue `project_name`/`job_pick_plugin` gaps `snapshot.c`'s own "Scope decisions" also documents (separate, unrelated, pre-existing, not part of this fix).
+
+## Boundary Candidates
+- **Deep-copy helper** (`xcealloc.c`, or a new file if it grows past the codebase's ~800-line-per-file convention): the one piece of genuinely new logic.
+- **Save-side wiring** (`build_wire_job`/`snapshot_wire.c`): small, mechanical once the helper exists.
+- **Load-side wiring** (`xce_sched_core_load_snapshot`/`snapshot_load.c`): small, mirrors `sim_reconcile_unaccounted_jobs()`'s existing `job->accounts` reconstruction pattern.
+- **Test coverage**: cross-core round-trip (multi-host/multi-resource/shared), backward-compat (old snapshot file), and the end-to-end sim-mode "used" accounting proof.
+
+## Out of Boundary
+- `job->priority`, queue `project_name`/`job_pick_plugin` -- both already-documented, unrelated pre-existing snapshot gaps.
+- Any change to the live (non-snapshot) dispatch/accounting path.
+- `sim_reconcile_unaccounted_jobs()` itself.
+
+## Upstream / Downstream
+- **Upstream**: `scheduler-core`'s existing snapshot save/load (task 8.1) -- this is a direct extension of it, not independently founded. `scheduler-sim-eager-reconciliation` (this session) -- the complementary reconciliation mechanism this fix does not replace.
+- **Downstream**: any future `scheduler-sim-diagnostics` work inspecting resource accounting; the roadmap's existing backlog item for `xce_test`'s future `snapdump` tooling may want to report whether a snapshot carries job allocations, once this exists.
+
+## Existing Spec Touchpoints
+- **Extends**: `scheduler-core` (owns `snapshot.c`/`snapshot_wire.c`/`snapshot_load.c`) -- not reopened as a spec (matching this session's own precedent for `scheduler-sim-eager-reconciliation`), but this spec's design must respect its existing architecture/conventions: no capnp code in these 3 files, the field-copy-not-shared-helper convention `snapshot.c`'s own header already documents and justifies (avoiding `core_events.c`'s static, unexported `core_build_job`/`core_build_queue`/`core_build_worker`).
+- **Adjacent**: `scheduler-sim-eager-reconciliation` -- complementary mechanism, same symptom class the user originally reported, different root-cause layer. `scheduler-dispatch-commit` (owns `WAL_JOB_DISPATCH`, the controller-side fact bug-1678 extended to carry the allocation) -- no code dependency, but its bug-1678 fix is the direct working precedent for this spec's approach.
+
+## Constraints
+- No capnp code in `snapshot.c`/`snapshot_wire.c`/`snapshot_load.c` -- existing architectural boundary, `snapshot.c`'s own header explains why.
+- Must never alias the deep-copied allocation across two independently-alive cores -- the exact hazard the original scope decision flagged. Every restore must be a genuine deep copy, correctly paired with `free_JobAlloc_ptr` on cleanup.
+- `xce_sched_core_save_snapshot()`/`xce_sched_core_load_snapshot()` both already enforce `session_active==0` (-EBUSY guard) -- confirmed no concurrent job-list mutation race during the deep copy. This fix must preserve that guarantee, not weaken it.
+- This spec is the confirmed instance motivating a new standing invariant, formalized alongside this brief: `xce-audit-invariants`' scheduler dimension 4 ("Field-complete snapshot save/load parity") and `persistence.md`'s "Snapshot Save/Load Field-Completeness (Scheduler)" section. Tasks generated for this spec should include running `/xce-audit-invariants scheduler` (or at minimum manually re-checking dimension 4) as part of final validation, not just the feature's own unit tests -- proving THIS field round-trips doesn't by itself prove no OTHER field has the same gap.

.kiro/steering/persistence.md

@@ -67,6 +67,10 @@ A **separate** verdict from `recover_verdict_t` (above): `persist_apply_verdict(
 
 **`APPLY_CLEAN` is entity-count + cursor-position only — it does NOT prove per-field completeness (bug-1678).** `persist_apply_verdict()`'s two checks (WAL_JOB_SUBMIT-count-vs-table-growth, `last_applied_lsn` position) both passed while a job's `allocs` (the resource allocation the scheduler assigned) silently replayed as `NULL` after a full restart — `job.state` was correctly `RUN`, the row existed, the cursor was in the right place, and `APPLY_CLEAN` reported healthy the entire time. Root cause: `WAL_JOB_DISPATCH`'s payload (`wal_jobstate_t`) never carried the allocation at all, so no apply-side check could have caught the gap even in principle — the field was never in the log to compare against. **The real invariant `APPLY_CLEAN` should imply — but today only partially enforces — is: online-apply and replay-apply of the same committed log MUST produce byte-identical materialized state for every persisted field of every entity, not just matching row counts and an advancing cursor.** When adding a new WAL fact type or a new field to an existing one, verify explicitly that (a) the field is actually encoded into the WAL payload (not just the in-memory struct at dispatch time), and (b) both apply seams — `apply_committed_entry()` (primary boot replay) and `apply_wal_entry()` (standby catch-up / replicated-apply) — decode and adopt it. Tracked as unimplemented future work at the systemic level in `roadmap.md`'s raft-apply-decoupling item #4 / raft-sim-conformance follow-up (a generic per-field snapshot-diff regression harness); until that lands, treat this as a standing audit dimension (see `xce-audit-invariants` skill) rather than a one-time fix.
 
+## Snapshot Save/Load Field-Completeness (Scheduler)
+
+**The identical invariant applies to the scheduler's own, separate persistence mechanism** (`xce_sched_core_save_snapshot()`/`xce_sched_core_load_snapshot()`, `src/scheduler/src/snapshot{,_wire,_load}.c`) -- a completely different code path from this file's WAL-based durability (no shared code, no shared fixes), but the same failure shape: a snapshot-restored entity can report the *correct state* while silently missing a *field* that state depends on, with nothing in the load path able to detect it because the field was never written to the wire type in the first place. Confirmed instance: `job->allocs`/`job->accounts` (a running job's resource allocation) were never captured by `build_wire_job()` -- `snapshot_wire.c:167`'s own comment flags it -- so a snapshot-restored RUNNING job showed the right `state` but permanently zero resource usage, the scheduler-side mirror of this section's `job->allocs`/EXEC_HOST story above (bug-1678). Scoped as its own spec, `scheduler-snapshot-job-allocation`, rather than folded into WAL work, since the two persistence mechanisms don't share fixes. Unlike the WAL side, `snapshot.c`'s own "Scope decisions" header already documents SOME gaps explicitly (`job->priority`, queue `project_name`/`job_pick_plugin`) -- those are fine, the invariant is about UNDOCUMENTED gaps. Tracked as `xce-audit-invariants`' scheduler dimension 4 -- when adding a new field to a snapshot-round-tripped entity, verify explicitly that it's written in the matching `build_wire_*()` function AND read back in `xce_sched_core_load_snapshot()`, or add it to the documented-gaps list if omitting it is a deliberate choice.
+
 ## Fail-Loud, Never Self-Exit
 
 When a member cannot reach a committed-consistent state (recovery-truncated/behind, no healthy peer), it enters a **loud permanent fence**: log a loud operator-visible ERROR (truncation boundary vs cluster HWM), set the sticky `recovery_unrecoverable` flag, hold non-promotable, **stay alive, and keep retrying** — it self-heals when a peer returns. It MUST NEVER `exit`/`abort` as a designed response. The decision is the pure `persist_recover_decision` → `RECOVER_DECISION_{ELIGIBLE,FENCE,FENCE_UNRECOVERABLE}`; the action is `persist_recover_loud_fence_if_unrecoverable` (no terminal). Authoritative statement: `error-handling.md` + `src/controller/CLAUDE.md` "Process Lifecycle".