diff --git a/.claude/skills/kiro-spec-design/rules/design-review-gate.md b/.claude/skills/kiro-spec-design/rules/design-review-gate.md index 06bc5feb..77758b9a 100644 --- a/.claude/skills/kiro-spec-design/rules/design-review-gate.md +++ b/.claude/skills/kiro-spec-design/rules/design-review-gate.md @@ -33,6 +33,13 @@ Before writing `design.md`, review the draft design and repair local issues unti - Avoid speculative abstraction: remove components, adapters, or interfaces that exist only for hypothetical future scope. - If a section is too vague for tasks to reference directly, rewrite it before finalizing the design. +## Persistence / Save-Restore Completeness Review (when applicable) + +- If the design adds or modifies any save/restore mechanism (a durable log entry, a snapshot/checkpoint format, a cache, or any other persist-then-later-reconstruct path), the design must enumerate every field of the entity being persisted and state explicitly, per field, whether it is captured on write and restored on every read/apply path — an implicit "the rest just works" is not sufficient. +- A state that reports as "recovered correctly" (right status, right row count, right cursor position) while silently missing a field it depends on is a design gap, not an acceptable simplification — a passing high-level check (row counts, cursor position) does not prove per-field completeness. +- An omitted field is acceptable ONLY if the design explicitly documents it as a deliberate scope decision with a stated reason — a silent omission discovered later is treated as a defect, not a scope choice made after the fact. +- If this project has a persistence steering doc (e.g. `persistence.md`) or an invariant-audit skill (e.g. `xce-audit-invariants`) covering this exact concern, cross-check the design against it and cite the relevant section/dimension rather than re-deriving the principle from scratch. + ## Mechanical Checks Before applying judgment, verify these mechanically: diff --git a/.claude/skills/kiro-validate-design/rules/design-review.md b/.claude/skills/kiro-validate-design/rules/design-review.md index 3209d8df..c842ad12 100644 --- a/.claude/skills/kiro-validate-design/rules/design-review.md +++ b/.claude/skills/kiro-validate-design/rules/design-review.md @@ -41,6 +41,12 @@ Conduct interactive quality review of technical design documents to ensure they - Clear API boundaries and data structures - Input validation and error handling coverage +### 5. Persistence / Save-Restore Field-Completeness (when applicable) +- Applies only when the design adds or modifies a save/restore mechanism (durable log entry, snapshot/checkpoint, cache, or any persist-then-reconstruct path); skip entirely otherwise. +- Every field of the persisted entity is either captured on write and restored on every read/apply path, or explicitly documented as a deliberate, reasoned omission — not silently dropped. +- A design that only checks aggregate signals (row counts, cursor position, "state looks right") without addressing per-field completeness should be flagged here, not waved through as a detail for implementation. +- Cross-check against this project's own persistence steering doc / invariant-audit skill if one exists, and cite the specific section rather than re-deriving the principle. + ## Review Process ### Step 1: Analyze diff --git a/.claude/skills/xce-audit-invariants/SKILL.md b/.claude/skills/xce-audit-invariants/SKILL.md index 05289679..b41b4f61 100644 --- a/.claude/skills/xce-audit-invariants/SKILL.md +++ b/.claude/skills/xce-audit-invariants/SKILL.md @@ -171,6 +171,26 @@ bug-1613/bug-1681 and confirm bug-1611's partial-fix status): does anything raise the `jbuf` cbuf capacity above 1, or bypass the rendezvous with a second concurrent producer/consumer? Re-derive Req 5 AC 7 in `.kiro/specs/scheduler-core/requirements.md` before touching this. +4. **Field-complete snapshot save/load parity** (new 2026-08-27, sibling of + controller dimension 7 / bug-1678 — same invariant, separate persistence + mechanism, does NOT share code or fixes with the WAL side) — for every + entity `xce_sched_core_save_snapshot()`/`xce_sched_core_load_snapshot()` + round-trips (job/queue/worker/user/project), is every field the live + entity carries actually (a) written into the wire type in + `snapshot_wire.c`'s `build_wire_*()` functions, not just held in the + in-memory struct, and (b) read back and adopted in + `snapshot_load.c`'s `xce_sched_core_load_snapshot()`? `snapshot.c`'s own + "Scope decisions" header documents several KNOWN, deliberate gaps (e.g. + `job->priority`, queue `project_name`/`job_pick_plugin`) — those are + fine, they're explicit; the audit is for UNDOCUMENTED gaps. Confirmed + instance: `job->allocs`/`job->accounts` were silently not captured + (`snapshot_wire.c:167`'s own comment flags it, but nothing closed the + gap until `scheduler-snapshot-job-allocation`) — a snapshot-restored + RUNNING job's resource allocation was permanently lost, matching + bug-1678's controller-side symptom shape exactly (state recovers + correctly, allocation/assignment silently doesn't). Cross-check + `persistence.md`'s "Snapshot Save/Load Field-Completeness (Scheduler)" + note for the shared underlying principle. Each subagent must report file:line evidence for every claimed violation — no speculative findings. Use the Agent tool directly (general-purpose or diff --git a/.kiro/specs/scheduler-snapshot-job-allocation/brief.md b/.kiro/specs/scheduler-snapshot-job-allocation/brief.md new file mode 100644 index 00000000..c718e7ff --- /dev/null +++ b/.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 `). 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. diff --git a/.kiro/steering/persistence.md b/.kiro/steering/persistence.md index 8508a7dd..815d4f22 100644 --- a/.kiro/steering/persistence.md +++ b/.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". diff --git a/CLAUDE.md b/CLAUDE.md index 571be0e1..0061f9c1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,6 +25,7 @@ For details on any topic below, read the corresponding steering file in `.kiro/s | TDD rules, C + Go test patterns, coverage targets | `testing.md` | | Error handling: Go ReturnError, C errnox, zap logging | `error-handling.md` | | RBAC, JWT, auth backends, input validation, secrets | `security.md` | +| WAL durability, recovery, save/restore field-completeness | `persistence.md` | ## Key Rules (from steering — summarized for quick reference) @@ -34,6 +35,7 @@ For details on any topic below, read the corresponding steering file in `.kiro/s - **Middleware chain** (non-negotiable): WhitelistURL → WhitelistIP → RateLimit → JWT → CORS → RBAC - **Error pattern**: `ReturnError(code, msg, nil); return` — early return on every error - **Build**: `/xce-build [component]` — debug + ASAN + tests. See `build-deployment.md` for full commands +- **Save/restore field-completeness**: any persistence mechanism (WAL fact, scheduler snapshot, checkpoint) must capture every field of the entity it claims to persist — a state that "recovers correctly" while silently missing a field it depends on (e.g. `job.state` correct but `job.allocs` lost) is a bug, not an acceptable simplification, unless the omission is explicitly documented as a scope decision (see `persistence.md`'s "Snapshot Save/Load Field-Completeness" + `APPLY_CLEAN`/bug-1678 sections). When adding or extending save/restore logic, verify the field round-trips through BOTH the write side and every read/apply seam, and run `/xce-audit-invariants` (controller dimension 7 / scheduler dimension 4) rather than assuming a passing test suite already proves it — `APPLY_CLEAN`-style verdicts check row counts and cursor position, not per-field completeness ## Key Files - `src/frontend/Makefile` — primary build/test orchestration