diff --git a/.kiro/specs/scheduler-allocation/brief.md b/.kiro/specs/scheduler-allocation/brief.md index 08b01d5d..2d81d8b4 100644 --- a/.kiro/specs/scheduler-allocation/brief.md +++ b/.kiro/specs/scheduler-allocation/brief.md @@ -28,7 +28,7 @@ The first gap (hardcoded picker) is addressed in the sibling **scheduler-core** ### Iterator-Driven Allocation Architecture -A lazy `xce_alloc_plan_iter_t` (candidate `(nhosts, slots_per_host)` plans, most-preferred first) composed with a lazy `xce_host_iter_t` (candidate hosts for a given plan, filtered by the existing select-expression evaluator), a `parjob_allocator` implementation that walks the plan iterator to do real combinatorial multi-slot allocation, and a session-scoped unsatisfiable-resreq fingerprint cache so a job whose resreq no plan can satisfy doesn't re-pay full allocation cost for every later job with the same fingerprint in the same scheduling session. +A lazy `xce_alloc_plan_iter_t` (candidate plans, each a partition of the job's task count into per-host slot buckets -- e.g. `[2,1]` for 3 tasks across 2 hosts, not a uniform `(nhosts, slots_per_host)` pair, which cannot express uneven splits -- most-preferred first) composed with a lazy `xce_host_iter_t` (candidate hosts for a given plan, filtered by the existing select-expression evaluator), a `parjob_allocator` implementation that walks the plan iterator to do real combinatorial multi-slot allocation, and a session-scoped unsatisfiable-resreq fingerprint cache so a job whose resreq no plan can satisfy doesn't re-pay full allocation cost for every later job with the same fingerprint in the same scheduling session. ### Three-Layer Iterator Composition @@ -44,14 +44,21 @@ The iterator composition operates at the **PRIMARY leaf**. COMPOUND and ALTER ar ``` xce_alloc_plan_iter_t -- init(resreq) -> next() -> plan -> next() -> ... -> NULL -> fini() - │ plan + │ plan (a partition into per-host buckets, e.g. [2,1]) + ▼ +for each bucket in plan.bucket_slots[] -- NOT a 2-level pipeline: a plan + │ with N>1 buckets needs a DISTINCT + │ (fresh host_iter per bucket -- host per bucket, each found via + │ see "Corrected below") its own full host_iter scan ▼ xce_host_iter_t -- init(plan, workers) -> next() -> worker -> ... -> NULL -> fini() │ worker ▼ -try_alloc(worker, plan) -- check slots, shared res, commit or advance +try_alloc(worker, bucket_slots[i]) -- check slots, shared res, commit or advance ``` +Three levels of iteration, not two: plan -> bucket/partition -> worker. A single-bucket plan (seqjob's trivial case) collapses the middle level to one iteration, which is why it's easy to under-specify this as a 2-level pipeline -- see "PRIMARY leaf allocation pseudocode" below for why the middle level must be a real loop with its own fresh `host_iter` per bucket, not folded into the plan/host levels. + ### Key Properties - **Lazy**: `plan_iter->next()` generates one plan at a time. `host_iter->next()` returns one host at a time. No full materialization. @@ -94,19 +101,58 @@ Approach B chosen over A because the iterator abstraction pays off immediately ( ### PRIMARY leaf allocation pseudocode +**Corrected (this review) -- superseded from an earlier 2-level version of this pseudocode**, which tried one worker per plan and returned on the first success. That's only valid for a single-bucket plan (seqjob's trivial case); a plan with `nbuckets > 1` needs a distinct host per bucket, so the algorithm needs a third, explicit loop level. It also needs a *fresh* `host_iter` per bucket rather than one shared across the whole plan: `next()` is a forward-only cursor, so sharing it would permanently drop a host an earlier, larger bucket tried and rejected as too small -- even when that same host exactly satisfies a later, smaller bucket. Iterators are stack-allocated by the caller and initialized in place (matching this codebase's existing `sched_ctx_init(&ctx, ...)` convention), not returned by a heap-allocating factory. Full rationale (rollback scope, host exclusivity semantics, why descending bucket order matters for correctness not just speed): `requirements.md`'s "Composition Algorithm" section. + ```c int allocate_primary(sched, primary_resreq, ctx) { - plan_iter = plan_iter_init(primary_resreq, sched); + xce_alloc_plan_iter_t plan_iter_storage; + xce_alloc_plan_iter_t* plan_iter = &plan_iter_storage; + if (plan_iter->init(plan_iter, primary_resreq, sched) != 0) { + return ALLOC_FAIL; + } + while ((plan = plan_iter->next(plan_iter)) != NULL) { - host_iter = host_iter_init(plan, sched->workers); - while ((worker = host_iter->next(host_iter)) != NULL) { - if (try_alloc_on_host(worker, plan, ctx) > 0) { + // plan is a partition of maxTasks into per-host buckets, e.g. [2,1] + // for 3 tasks across 2 hosts -- NOT a uniform (nhosts, slots_per_host). + xce_sched_worker_t* claimed[primary_resreq->maxTasks]; // sized by maxTasks, the true upper bound on bucket count + int nclaimed = 0; + int plan_ok = 1; + + // Three levels of iteration: plan (above) -> bucket/partition (this + // for) -> worker (host_iter, fresh per bucket, below). + for (int b = 0; b < plan->nbuckets; b++) { + xce_host_iter_t host_iter_storage; + xce_host_iter_t* host_iter = &host_iter_storage; + xce_sched_worker_t* w; + int bucket_ok = 0; + + if (host_iter->init(host_iter, plan, sched->workers) == 0) { + while ((w = host_iter->next(host_iter)) != NULL) { + if (is_claimed(claimed, nclaimed, w)) { + continue; + } + if (try_alloc_on_host(w, plan->bucket_slots[b], ctx) > 0) { + claimed[nclaimed++] = w; + bucket_ok = 1; + break; + } + } host_iter->fini(host_iter); - plan_iter->fini(plan_iter); - return ALLOC_SUCCESS; + } + + if (!bucket_ok) { + plan_ok = 0; + break; // this bucket has no satisfying host -- abandon the plan } } - host_iter->fini(host_iter); + + if (plan_ok) { + commit_plan_allocation(claimed, nclaimed, ctx); + plan_iter->fini(plan_iter); + return ALLOC_SUCCESS; + } + + rollback_plan_allocation(claimed, nclaimed, ctx); // undo partial claims } plan_iter->fini(plan_iter); return ALLOC_FAIL; diff --git a/.kiro/specs/scheduler-allocation/design.md b/.kiro/specs/scheduler-allocation/design.md new file mode 100644 index 00000000..5a15d744 --- /dev/null +++ b/.kiro/specs/scheduler-allocation/design.md @@ -0,0 +1,467 @@ +# Design Document + +## Overview + +**Purpose**: This feature turns the scheduler's allocation layer from a partially-stubbed, one-shot mechanism into a real, retry-capable, combinatorial allocator, and adds a session-scoped cache so batches of identically-unsatisfiable jobs don't each re-pay full allocation cost. + +**Users**: Cluster operators submitting parallel (`maxTasks > 1`) jobs, who today get silent, permanent allocation failure regardless of actual cluster capacity; and scheduler maintainers who today cannot add a new allocation strategy without reimplementing host collection/filtering from scratch. + +**Impact**: `parjob_allocator.c`'s `my_alloc()` goes from an unconditional `return 0` stub to a real combinatorial allocator. `seqjob_allocator.c`'s `my_alloc()` is rewrapped onto the same iterator interfaces as a trivial case, with no change to its externally observable host-selection behavior. `scheduler.c` gains a session-scoped unsatisfiable-resreq cache wrapping the existing PRIMARY-resreq allocator dispatch. + +### Goals +- `parjob_allocator` allocates real multi-host, multi-slot jobs via lazy combinatorial plan generation (Requirement 4). +- A reusable lazy plan-iterator (Requirement 2) and host-iterator (Requirement 3) interface, shared by every current and future allocator plugin. +- A session-scoped unsatisfiable-resreq fingerprint cache (Requirement 7) that skips full allocation for a job whose resreq this session already proved unsatisfiable. +- `seqjob_allocator`'s externally observable behavior is provably unchanged after rewrap (Requirement 9.1). + +### Non-Goals +- Pluggable job picker, `builtin_fcfs`, per-queue picker override — **scheduler-core** (already implementation-complete). +- Preemption, victim selection, `XCE_ALLOC_PREEMPT`'s actual resource-borrowing behavior — **scheduler-preemption** (this spec only declares the `xce_alloc_mode_t` extension point it needs). +- Core/driver separation — **scheduler-core** (already implementation-complete). +- `span`/`same`/`affinity`-aware plan generation, topology/GPU-aware allocation, resreq template registry, select-expression changes — deferred; the iterator interfaces are shaped to accept these later without a signature change. +- Controller-side commit-barrier wiring (`ha-consistency-invariants`) — this spec produces allocation decisions the barrier will eventually consume, does not wire it. +- Fixing `sched_make_compound_allocation()`'s pre-existing missing-rollback-on-partial-failure defect (bug-1766) or the recurring schedobj-reconstruction-duplication pattern in `snapshot_load.c` (bug-1767) — both discovered during this spec's own review, both out of boundary, both logged for independent follow-up. + +## Boundary Commitments + +### This Spec Owns +- The `xce_alloc_plan_iter_t` interface and every concrete plan-iterator implementation (`parjob_allocator`'s combinatorial one, `seqjob_allocator`'s trivial one). +- The `xce_host_iter_t` interface and its one shared implementation, reused by every allocator. +- `parjob_allocator.c`'s `my_alloc()`/`parjob_prepare_allocation()` bodies — real implementations, replacing the stubs. +- `seqjob_allocator.c`'s `my_alloc()`/`seqjob_prepare_allocation()` bodies — rewrapped onto the shared iterator pattern, behavior-preserving. +- The session-scoped unsatisfiable-resreq cache: `xce_resreq_fingerprint_t`, `xce_cache_key_t`, `cache_key_ops` (`kv_key_ops_t`), and the `xce_scheduler_t.unsatisfiable_cache` field's full lifecycle (populate, lookup, clear). +- The `xce_alloc_mode_t` enum and `xce_sched_ctx_t.alloc_mode` field — declared here as an extension point; only `XCE_ALLOC_NORMAL` behavior is implemented. +- `rusage_hash` on `xce_primary_resreq_t` and `resreq_fp` on `xce_primary_schedobj_t`, and every construction site that must populate them. + +### Out of Boundary +- `COMPOUND`/`ALTER` resreq dispatch (`sched_make_compound_allocation`, `sched_make_alter_allocation`) — calls into this spec's per-primary allocator, otherwise untouched (Requirement 9.2). +- The existing select-expression evaluator, shared-resource tracking (`sharedInst`/`sharedInUse`), and plugin framework registration mechanics — consumed, not modified. +- Preemption's actual victim-selection logic and the resource-borrowing semantics `XCE_ALLOC_PREEMPT` will eventually trigger — **scheduler-preemption**'s job. +- `sched_make_compound_allocation()`'s missing partial-failure rollback (bug-1766) and the `snapshot_load.c` schedobj-reconstruction duplication pattern (bug-1767) — both pre-existing, both logged, neither fixed here. +- `resreq_fp` population on the snapshot-restore path (`snapshot_load.c`) — confirmed cosmetic (affects only offline simulation sessions, `sim.c`, never `driver.c`); not a requirement of this spec (see `requirements.md` Requirement 7's informational note). + +### Allowed Dependencies +- **scheduler-core** (implementation-complete) — core event interface pushing worker/resource state; this feature's iterators consume it but do not depend on the driver/I/O layer. +- **scheduler-dispatch-commit** (implementation-complete, bug-1624 fixed) — `resInUse`/`sharedInUse` totals this feature's `try_alloc_on_host()` reads must already be correct; this was the spec's own sequencing blocker, now resolved. +- Existing `plugin.h`/`plugin.c` plugin framework, `resreq.h`'s select-expression evaluator (`select_eval_ctx_t`), `xvec.c` (`xvec_op(..., XVEC_SUB_FLOAT)` for rollback), `kv.c`/`kv.h` (`kv_key_ops_t`, `kvtab_type`, `key_find`/`key_add`) — all consumed as-is, no changes to their own contracts. +- `xce_job_alloc_t`/`make_job_alloc()`/`xce_host_alloc_t` (`xce/types.h`, `xcealloc.c`) — already multi-host-capable; this feature is the first caller to exercise `nhosts > 1`. + +### Revalidation Triggers +- Any signature change to `xce_alloc_plan_iter_t`, `xce_host_iter_t`, or `xce_alloc_plan_t` (`bucket_slots[]` shape) — every allocator plugin consuming them must be re-checked. +- Any change to `xce_sched_ctx_t`'s `nslots`/`nhosts`/`alloc_mode` fields — **scheduler-preemption** depends on `alloc_mode` existing exactly as declared here. +- Any change to `xce_cache_key_t`'s field set or `plugin_prepare_allocation()`'s fan-out semantics. +- `scheduler-preemption` reaching implementation — its victim-selection flow wraps this spec's `allocate_primary()`-shaped logic and consumes `xce_alloc_mode_t`; a mismatch here blocks that spec. + +## Architecture + +### Existing Architecture Analysis + +- `sched_make_primary_alloc_()` (`scheduler.c:167-177`) is the existing choke point every `PRIMARY`-tagged job passes through: it dispatches to `sched_make_seqjob_allocation()`/`sched_make_parjob_allocation()` purely on `primary->maxTasks == 1`. This dispatch itself is **unchanged** by this spec — only what happens inside each allocator's `job_alloc_fn` changes. +- `plugin_prepare_allocation()` (`plugin.c:591-661`) unconditionally fans out to **every** registered plugin's `prepare_allocation_hook` before each host-satisfaction check, resetting `ctx->alloc_req.host`/`.shared` first. `seqjob_prepare_allocation()` currently has no `maxTasks` guard, so it already fires (harmlessly, since it's a no-op today) on every job regardless of allocator — this becomes load-bearing once `parjob_prepare_allocation()` stops being a no-op (Requirement 4.6). +- `xce_sched_ctx_t` (`sched/types.h`) already carries `nhosts`/`nslots` scratch fields, currently hardcoded to `1, 1` by `seqjob_prepare_allocation()`; this spec is the first to vary them per allocation attempt. +- `core_begin_session_internal()` (`core_session.c:152-168`) already resets other shared, per-session state (`core_reset_shared_resval()`, `core_reset_workers()`) with **zero explicit locking**, relying on `xce_core_begin_session()`'s own documented ordering guarantee: all begin-session reset work completes strictly before `xce_core_signal_go()` unblocks the picker/allocator threads. This spec's cache clear reuses that exact placement and safety argument rather than introducing new synchronization. +- `xce_job_alloc_t`/`make_job_alloc()` (`xce/types.h`, `xcealloc.c`) already generalize past one host (`nhosts` + `xce_host_alloc_t**`), but only `seqjob_allocator` exercises them today, always with `nhosts=1` via the variadic `make_host_allocs(1, halloc)`. This spec is the first caller needing a runtime-sized host array, which the existing variadic helper cannot provide. + +**Project Invariant Cross-Check** (per `design-review-gate.md`): `src/scheduler/CLAUDE.md`'s core-isolation invariant (`core.c`/`core_events.c`/`core_session.c`/`core_clock.c` carry zero controller I/O, mechanically enforced by `test_core_isolation.c`) was checked against every new/modified piece of this design. All new code (`xce_alloc_plan_iter_t`/`xce_host_iter_t` implementations, the cache, both allocator plugins) is pure in-memory computation — no I/O, no blocking calls — so it does not violate the invariant regardless of which file it lands in. The one touch inside the isolated set (`core_begin_session_internal()`'s cache clear, `core_session.c`) is a plain kvtab clear, the same shape as the two existing calls already there. + +### Architecture Pattern & Boundary Map + +```mermaid +graph TB + Allocator[sched_make_primary_alloc_] --> CacheCheck{Cache lookup} + CacheCheck -->|hit| Pend[Pend job immediately] + CacheCheck -->|miss| Dispatch{maxTasks} + Dispatch -->|equals 1| Seqjob[seqjob_allocator my_alloc] + Dispatch -->|greater than 1| Parjob[parjob_allocator my_alloc] + Seqjob --> PlanIterTrivial[Trivial plan iterator one bucket] + Parjob --> PlanIterCombo[Combinatorial plan iterator] + PlanIterTrivial --> HostIter[Shared host_iter] + PlanIterCombo --> HostIter + HostIter --> TryAlloc[try_alloc_on_host] + TryAlloc -->|success| Commit[Commit job_alloc] + TryAlloc -->|all plans exhausted| CacheInsert[Insert fingerprint into cache] + CacheInsert --> Pend +``` + +**Architecture Integration**: +- **Selected pattern**: two-layer lazy iterator composition (plan → host) at the `PRIMARY` leaf, unchanged `COMPOUND`/`ALTER` composition above it — see `requirements.md`'s "Composition Algorithm" for the full pseudocode (bucket-loop, host-exclusivity, rollback) this design does not restate. +- **Domain/feature boundaries**: plan generation is allocator-owned (each `job_alloc_fn` builds its own plan-iterator instance); host iteration is shared, reused infrastructure; the cache wraps the dispatch choke point, not either allocator individually. +- **Existing patterns preserved**: `sched_ctx_init`/`sched_ctx_fini`'s stack-allocate-then-init-in-place convention (extended to the new iterators); `plugin_prepare_allocation()`'s fan-out (extended, not replaced); `core_begin_session_internal()`'s lock-free per-session reset pattern (extended to the cache). +- **New components rationale**: `xce_host_iter_t` is new because host collection was previously eager and allocator-specific (`sched_collect_workers()` + `xlist_find()`, inlined in `seqjob_allocator.c`); the cache is new because no unsatisfiable-resreq memoization existed at all. +- **Steering compliance**: no new controller I/O (`ha-async-io.md`'s thread-role contract); no new persistence format, and the one persistence-adjacent question (should the cache or `resreq_fp` be snapshot-captured) was resolved as "no" with a stated reason (`persistence.md`-style completeness review, applied inline in `requirements.md`'s Requirement 7 notes). + +### Technology Stack + +| Layer | Choice / Version | Role in Feature | Notes | +|-------|------------------|------------------|-------| +| Language | C (gnu17) | All new code | No C++, no new external libraries | +| Hashing/KV | `src/lib/kv.c` (`kvtab_type`, `kv_key_ops_t`) | Backs the unsatisfiable-resreq cache | Existing infra; `cache_key_ops` is a new, third `kv_key_ops_t` instance alongside `int_key_ops`/`string_key_ops` | +| Arithmetic primitives | `src/lib/xvec.c` (`xvec_op`, `XVEC_SUB_FLOAT`) | Rollback's resource-debit reversal | Existing primitive, first caller to use the subtract op for this purpose | +| Concurrency | `src/lib/mylock.c`-style implicit ordering (no new locks) | Cache safety | Reuses `xce_core_begin_session()`/`xce_core_signal_go()`'s existing happens-before guarantee — no new mutex introduced | + +## File Structure Plan + +### Directory Structure +``` +src/scheduler/ +├── include/sched/ +│ ├── alloc_iter.h # NEW — xce_alloc_plan_t, xce_alloc_plan_iter_t, +│ │ # xce_host_iter_t interface declarations (Req 2, 3) +│ ├── alloc_cache.h # NEW — xce_cache_key_t, xce_resreq_fingerprint_t, +│ │ # cache_key_ops declaration (Req 7) +│ └── types.h # MODIFIED — xce_sched_ctx_t gains alloc_mode +│ # (Req 9.5); xce_primary_schedobj_t gains +│ # resreq_fp (Req 7) +├── src/ +│ ├── host_iter.c # NEW — the one shared xce_host_iter_t +│ │ # implementation (Req 3), reused by both +│ │ # allocators +│ ├── alloc_cache.c # NEW — cache_key_hash_/cmp_/assign_/destroy_, +│ │ # cache lookup/insert/clear helpers (Req 7) +│ ├── scheduler.c # MODIFIED — sched_make_primary_alloc_() wraps +│ │ # allocator dispatch with cache lookup/insert; +│ │ # xce_ctx_uncommit_host() rollback helper (in +│ │ # resource.c, see below) called from here +│ ├── resource.c # MODIFIED — new sched_ctx_uncommit_host() +│ │ # helper wired to xvec_op(..., XVEC_SUB_FLOAT) +│ ├── core_session.c # MODIFIED — core_begin_session_internal() +│ │ # clears the cache (Req 7.2) +│ ├── core_events.c # MODIFIED — core_build_schedobj()/ +│ │ # core_build_primary_schedobj() compute +│ │ # resreq_fp on ingest (Req 7.1) +│ ├── schedobj.c # MODIFIED — sched_ctx_init() gains nothing new; +│ │ # referenced for the stack-allocate-then-init +│ │ # convention new iterators follow +│ └── plugins/ +│ ├── parjob_allocator.c # MODIFIED — real my_alloc() (combinatorial +│ │ # plan iterator + bucket loop over shared +│ │ # host_iter.c); parjob_prepare_allocation() +│ │ # fixed to use ctx->nslots (Req 4) +│ └── seqjob_allocator.c # MODIFIED — my_alloc() rewrapped onto the +│ # same bucket-loop pattern as a trivial +│ # 1-bucket case; seqjob_prepare_allocation() +│ # gains maxTasks==1 guard (Req 9.1, 4.6) +├── include/ +│ └── scheduler.h # MODIFIED — xce_scheduler_t gains +│ # unsatisfiable_cache field (Req 7.1) +└── testsuite/ + ├── test_alloc_plan_iter.c # NEW (Req 2) + ├── test_host_iter.c # NEW (Req 3) + ├── test_alloc_cache.c # NEW (Req 7) + ├── test_parjob_combinatorial.c # NEW (Req 4) + ├── test_seqjob_rewrap_equivalence.c # NEW (Req 9.1) + ├── test_parjob_e2e_multislot.c # NEW — integration (Req 4, brief.md scope) + └── meson.build # MODIFIED — register the 6 new executables + +src/include/ +└── resreq.h # MODIFIED — xce_primary_resreq_t gains + # rusage_hash (Req 7.1) +src/lib/ +├── resreq.c # MODIFIED — parse_resreq_primary() computes +│ # rusage_hash (Req 7.1) +└── xcealloc.c # MODIFIED — new merge helper building a + # runtime-sized xce_host_alloc_t** array + # directly (make_host_allocs()'s variadic + # signature cannot take a runtime count), + # used by parjob's commit step (Req 4.3) +``` + +### Modified Files +See inline annotations above — each modified file's change is scoped to exactly the requirement(s) noted; no file listed above is touched for any other reason. + +## System Flows + +### Combinatorial Allocation with Cache (sequence) + +```mermaid +sequenceDiagram + participant D as sched_make_primary_alloc_ + participant C as unsatisfiable_cache + participant P as parjob_allocator + participant PI as plan_iter + participant HI as host_iter + D->>C: lookup(resreq_fp) + C-->>D: miss + D->>P: my_alloc(ctx) + P->>PI: init + next (plan 1) + loop each bucket in plan + P->>HI: init fresh, next until match or exhausted + HI-->>P: worker or none + end + alt all buckets satisfied + P-->>D: success, commit + else a bucket exhausted + P->>P: rollback claimed hosts + P->>PI: next (plan 2) + Note over P,PI: repeat until a plan succeeds or plan_iter exhausted + end + P-->>D: ALLOC_FAIL (all plans exhausted) + D->>C: insert(resreq_fp) + D-->>D: pend job +``` + +Full per-bucket accumulation/rollback logic (host-exclusivity via `is_claimed()`, fresh `host_iter` per bucket, `ctx->uncomitted` rollback scope) is specified in `requirements.md`'s "Composition Algorithm" — not restated here; this diagram shows only the cross-file call sequence design.md needs to fix component ownership. + +## Requirements Traceability + +| Requirement | Summary | Components | Interfaces | Flows | +|-------------|---------|------------|------------|-------| +| 2.1–2.6 | Lazy plan iterator: interface, one-plan-per-call, ordering, exhaustion signal, cleanup, cancel-safety | `xce_alloc_plan_iter_t` (per-allocator impl) | `init`/`next`/`fini` | Combinatorial Allocation sequence | +| 3.1–3.6 | Lazy host iterator: interface, filtering, exhaustion, composition with plan iterator, cleanup, cancel-safety | `host_iter.c` (shared) | `init`/`next`/`fini` | Combinatorial Allocation sequence | +| 4.1–4.7 | Parjob combinatorial allocation: plan generation, per-bucket distinct-host accumulation, commit/merge, stub-return preserved on exhaustion, no unconditional stub, `ctx->nslots` resource-want plumbing, shared-resource check unchanged | `parjob_allocator`, `alloc_cache.c` (fingerprint only, not the check itself) | `job_alloc_fn`, `prepare_allocation_hook` | Combinatorial Allocation sequence | +| 7.1–7.5 | Unsatisfiable-resreq cache: key/fingerprint shape and O(1) lookup, begin-session clear placement, populate-on-exhaustion, lookup-before-allocation, pend-reason consistency | `alloc_cache.c`, `core_session.c`, `core_events.c` | `cache_key_ops` (`kv_key_ops_t`), `key_find`/`key_add` | Combinatorial Allocation sequence | +| 9.1–9.5 | No regression: seqjob behavior unchanged, COMPOUND/ALTER unchanged, existing unit tests pass, HA soak clean, `xce_alloc_mode_t` extension point | `seqjob_allocator`, `xce_sched_ctx_t.alloc_mode` | `job_alloc_fn` (seqjob), `xce_alloc_mode_t` enum | Combinatorial Allocation sequence (seqjob path = 1-bucket case) | + +## Components and Interfaces + +| Component | Domain/Layer | Intent | Req Coverage | Key Dependencies (P0/P1) | Contracts | +|-----------|--------------|--------|--------------|--------------------------|-----------| +| `xce_alloc_plan_iter_t` (interface) | Scheduler / Allocation | Lazy per-allocator plan generation | 2.1–2.6 | `xce_primary_resreq_t` (P0) | Service | +| `host_iter.c` | Scheduler / Allocation | Shared lazy host-candidate filtering | 3.1–3.6 | select-expression evaluator (P0), `resource.c` (P0) | Service | +| `parjob_allocator` | Scheduler / Plugin | Real combinatorial multi-host allocation | 4.1–4.7 | `xce_alloc_plan_iter_t` (P0), `host_iter.c` (P0), `resource.c` (P0) | Service | +| `seqjob_allocator` | Scheduler / Plugin | Trivial single-host allocation, behavior-preserving rewrap | 9.1, 4.6 | `xce_alloc_plan_iter_t` (P0), `host_iter.c` (P0) | Service | +| `alloc_cache.c` | Scheduler / Allocation | Session-scoped unsatisfiable-resreq memoization | 7.1–7.5 | `kv.c` (P0), `core_session.c` (P0) | Service, State | +| `xce_alloc_mode_t` extension point | Scheduler / Allocation | Forward-compatible resource-pool-mode surface | 9.5 | none (v1: `XCE_ALLOC_NORMAL` only) | State | + +### Scheduler / Allocation + +#### `xce_alloc_plan_iter_t` + +| Field | Detail | +|-------|--------| +| Intent | Lazy, cancel-safe generation of candidate `(bucket_slots[])` partitions, most-preferred first, one per allocator | +| Requirements | 2.1, 2.2, 2.3, 2.4, 2.5, 2.6 | + +**Responsibilities & Constraints** +- Each concrete implementation (parjob's combinatorial, seqjob's trivial) owns its own plan-shape and partition policy; the interface itself is policy-agnostic. +- v1's combinatorial default (ascending host-count, canonical balanced ceil/floor partition, descending bucket order) is fully specified in `requirements.md`'s "Data Structure Changes Summary" — a pure function of `maxTasks`, never inspecting live worker state. +- Stack-allocated by the caller, initialized in place (`iter.init(&iter, resreq, sched)`) — never a heap-allocating factory (matches `sched_ctx_init(&ctx, ...)`'s existing convention). + +**Dependencies** +- Inbound: `parjob_allocator`, `seqjob_allocator` (P0, sole consumers) +- Outbound: `xce_primary_resreq_t` (P0, read-only — `minTasks`/`maxTasks` used in v1; `span`/`same`/`affinity` read by future allocators without an interface change) + +**Contracts**: Service [x] / API [ ] / Event [ ] / Batch [ ] / State [ ] + +##### Service Interface +```c +struct xce_alloc_plan_iter_s { + int (*init)(xce_alloc_plan_iter_t* iter, xce_primary_resreq_t* resreq, xce_scheduler_t* sched); + xce_alloc_plan_t* (*next)(xce_alloc_plan_iter_t* iter); + void (*fini)(xce_alloc_plan_iter_t* iter); + void* private; +}; +``` +- Preconditions: `resreq != NULL`; `init()` called exactly once before any `next()`. +- Postconditions: `next()` returns candidate plans strictly in most-preferred-first order until exhaustion (stable `NULL` thereafter); `fini()` releases all iterator-owned resources. +- Invariants: the pointer `next()` returns is valid only until the following `next()` or `fini()` call; `next()` calls `pthread_testcancel()` (or equivalent) at least once per invocation (Req 2.6). + +#### `host_iter.c` + +| Field | Detail | +|-------|--------| +| Intent | The one shared `xce_host_iter_t` implementation, filtering candidate hosts by select expression + resource availability for a given plan | +| Requirements | 3.1, 3.2, 3.3, 3.4, 3.5, 3.6 | + +**Responsibilities & Constraints** +- One implementation, reused by every allocator — no allocator-specific host-filtering logic anywhere else. +- Scoped **per plan bucket**, not per plan: a multi-bucket plan requires a fresh `xce_host_iter_t` instance for each bucket (`init()` called once per bucket), never one instance shared across a plan's buckets — see `requirements.md`'s Requirement 3.3/3.4 for why a shared, forward-only cursor is an incorrect implementation (it silently drops a host an earlier, larger bucket rejected even when it exactly satisfies a later, smaller one). +- Host exclusivity **across** buckets of the same plan (`is_claimed()`) is the caller's responsibility, not this component's — this component only ever answers "does this select expression + availability filter match," with no memory of prior buckets. + +**Dependencies** +- Inbound: `parjob_allocator`, `seqjob_allocator` (P0) +- Outbound: existing select-expression evaluator (`resreq.h`, `select_eval_ctx_t`) (P0, unchanged); `resource.c`'s `sched_check_host_satisfy()` (P0, unchanged) + +**Contracts**: Service [x] / API [ ] / Event [ ] / Batch [ ] / State [ ] + +##### Service Interface +```c +struct xce_host_iter_s { + int (*init)(xce_host_iter_t* iter, xce_alloc_plan_t* plan, var workers); + xce_worker_t* (*next)(xce_host_iter_t* iter); + void (*fini)(xce_host_iter_t* iter); + void* private; +}; +``` +- Preconditions: `plan != NULL`; a fresh instance per bucket (see above). +- Postconditions: `next()` returns `NULL` once no further candidate exists **for this instance's bucket search**; a fresh instance for the next bucket is unaffected. +- Invariants: independent lifecycle from the owning `xce_alloc_plan_iter_t` — `fini()` on one never affects the other; `next()` calls `pthread_testcancel()` (or equivalent) at least once per invocation (Req 3.6). + +**Implementation Notes** +- Integration: constructed fresh inside the per-bucket loop of both `parjob_allocator`'s and `seqjob_allocator`'s `my_alloc()` — see `requirements.md`'s "Composition Algorithm" for the exact loop shape. +- Validation: `test_host_iter.c` must include the exact adversarial case that motivated the fresh-per-bucket requirement (a host too small for an earlier, larger bucket remaining eligible for a later, smaller one). +- Risks: none new — this is the shared implementation of behavior `seqjob_allocator` already has today (`sched_collect_workers()` + `xlist_find()`), generalized. + +#### `alloc_cache.c` + +| Field | Detail | +|-------|--------| +| Intent | Session-scoped memoization of resreqs this session already proved unsatisfiable, keyed by a composite fingerprint | +| Requirements | 7.1, 7.2, 7.3, 7.4, 7.5 | + +**Responsibilities & Constraints** +- Owns `xce_cache_key_t`, `xce_resreq_fingerprint_t`, and `cache_key_ops` (a third `kv_key_ops_t` instance alongside `int_key_ops`/`string_key_ops`). Full field-by-field hash/cmp/assign/destroy implementations are in `requirements.md`'s "New types (Req 7)" section — corrected during design-phase discovery (the `.cmp` return-value polarity was inverted in an earlier draft; fixed to the real three-way, 0-means-equal convention `kv.c`'s `int_key_ops.cmp` establishes). +- Lookup/insert happen exclusively on the allocator dispatch path (`sched_make_primary_alloc_()`, wrapping either allocator's call) — never inside `parjob_allocator.c`/`seqjob_allocator.c` themselves, so both allocators benefit identically without duplicated cache-handling code. +- Clear happens exactly once, inside `core_begin_session_internal()` (`core_session.c`), alongside the existing `core_reset_shared_resval()`/`core_reset_workers()` calls — no new lock, reusing `xce_core_begin_session()`'s existing happens-before guarantee (see Existing Architecture Analysis above). +- Not part of the snapshot/replay wire format — pure in-memory, unconditionally cleared at the next `begin_session` regardless of what a loaded snapshot contained. + +**Dependencies** +- Inbound: `scheduler.c`'s `sched_make_primary_alloc_()` (P0, sole caller of lookup/insert); `core_session.c`'s `core_begin_session_internal()` (P0, sole caller of clear) +- Outbound: `kv.c`'s `kvtab_type`/`key_find`/`key_add` (P0); `core_events.c`'s `core_build_schedobj()` (P0, populates `resreq_fp` on ingest); `resreq.c`'s `parse_resreq_primary()` (P0, populates `rusage_hash`) + +**Contracts**: Service [x] / API [ ] / Event [ ] / Batch [ ] / State [x] + +##### Service Interface +```c +/* Correction (kiro-validate-design pass): a bare hit/miss sentinel cannot + * satisfy Req 7 AC 5 ("same pend reason category" on a hit) -- the value + * stored per key is the original miss's reason CATEGORY (an int code, + * e.g. xce_job_preason_t.main_reason), not a placeholder. Not the full + * detailed reason object: that carries job/host-specific breakdown + * (xce_host_reason_t*/xce_res_reason_t* arrays) that wouldn't transfer + * meaningfully to a different job sharing the same fingerprint. */ +int cache_lookup(xce_scheduler_t* sched, xce_sched_obj_t* obj, int* out_reason_category); + /* 1 = hit (out_reason_category set), 0 = miss */ +int cache_insert(xce_scheduler_t* sched, xce_sched_obj_t* obj, int reason_category); + /* 0 = ok, <0 = error */ +void cache_clear(xce_scheduler_t* sched); /* called from core_begin_session_internal() */ +``` +- Preconditions: `obj->tag == XCE_RESREQ_PRIMARY`; `obj->u.primary->resreq_fp` already populated (job-ingest time); `cache_insert()`'s `reason_category` is the exhausted plan iterator's own final pend reason's `main_reason`. +- Postconditions: `cache_lookup()` returning a hit means the caller pends the job using `*out_reason_category` (Req 7.5) and never invokes the plan/host iterators or preemption for this attempt. +- Invariants: cache is cleared exactly once per scheduling session, strictly before the picker/allocator threads touch it for that session. + +##### State Management +- State model: one `kvtab_t` per `xce_scheduler_t`, keyed by `xce_cache_key_t` (composing `xce_resreq_fingerprint_t` + `{user, queue, project}` xstring pointers), value is the reason-category `int` boxed as `(void*)(intptr_t)category` (no heap allocation needed for a single int; `NULL` datakiller, matching `int_key_ops`'s own value-handling convention). +- Persistence & consistency: none — explicitly not snapshot-captured (see Boundary Commitments). +- Concurrency strategy: no explicit lock; safety derives from `xce_core_begin_session()`'s documented ordering guarantee plus lookup/insert being confined to the single-threaded allocator path. + +**Implementation Notes** +- Integration: `cache_key_cmp_()`'s three-way-comparator fix (this design pass) must land alongside the rest of `alloc_cache.c` — a functionally dead cache from day one would not fail any test that doesn't specifically assert a hit, so `test_alloc_cache.c` must include a same-key-twice hit assertion, not only a miss-then-insert-then-lookup round-trip that could pass even with the inverted polarity if written carelessly. +- Validation: `test_alloc_cache.c` must also assert the reason-category round-trip (insert with category X, hit later returns X) — the value-model fix from this validation pass. +- Risks: `.cmp` polarity bug already fixed (see `research.md`); the reason-category round-trip is the one new surface this fix adds, worth its own explicit test rather than assuming the hit/miss path alone proves it. + +### Scheduler / Plugin + +#### `parjob_allocator` + +| Field | Detail | +|-------|--------| +| Intent | Real combinatorial multi-host, multi-slot job allocation | +| Requirements | 4.1, 4.2, 4.3, 4.4, 4.5, 4.6, 4.7 | + +**Responsibilities & Constraints** +- `my_alloc()` walks its own combinatorial plan iterator; for each plan, loops buckets, each bucket getting a fresh `host_iter.c` instance (host-exclusivity via `is_claimed()`, not iterator sharing). +- The per-plan `claimed[]` accumulator (tracking which host was claimed for each bucket, for both the merge step and rollback) is sized by `primary_resreq->maxTasks` — the true, tight upper bound on bucket count (each bucket needs ≥1 slot, so `nbuckets` can never exceed it) — not a fixed compile-time constant an implementer would otherwise have to guess (`kiro-validate-design` finding, corrected in `requirements.md`'s pseudocode). +- On full-plan success: merges claimed hosts into one `xce_job_alloc_t` via the new `xcealloc.c` helper (runtime-sized array, not `make_host_allocs()`'s variadic signature). +- On per-bucket failure: rolls back via `resource.c`'s new `sched_ctx_uncommit_host()`, scoped to `ctx->uncomitted` only (never touches real `resInUse`). +- On full exhaustion: returns `0`, identical to today's stub return value, so `sched_make_primary_alloc_()`'s pend-reason handling is unaffected (Req 4.4). +- `parjob_prepare_allocation()` sets `ctx->alloc_req` via `calc_wanted_host_resvals(..., ctx->nhosts, ctx->nslots)`, reading the current bucket's slot count from `ctx->nslots` (set by the bucket loop before each host try) rather than a hardcoded constant. +- Shared-resource satisfaction is checked once per primary resreq, independent of plan/bucket shape — unchanged from today's mechanism (Req 4.7). + +**Dependencies** +- Inbound: `sched_make_primary_alloc_()` (P0) +- Outbound: `xce_alloc_plan_iter_t` (own combinatorial impl, P0), `host_iter.c` (P0), `resource.c` (`sched_check_host_satisfy`, `sched_ctx_uncommit_host`, P0), `xcealloc.c` (merge helper, P0) + +**Contracts**: Service [x] / API [ ] / Event [ ] / Batch [ ] / State [ ] + +##### Service Interface +```c +static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx); /* job_alloc_fn */ +static int parjob_prepare_allocation(void* arg, xce_sched_ctx_t* ctx); /* prepare_allocation_hook */ +``` +- Preconditions: `ctx->primary->maxTasks > 1` (routed here by `sched_make_primary_alloc_()`). +- Postconditions: return `>0` and `ctx->alloc` populated on success; return `0` with `ctx->reason` populated on exhaustion — same contract shape `seqjob_allocator`'s `my_alloc()` already has. +- Invariants: never claims the same host twice within one plan attempt; never leaves a phantom `ctx->uncomitted` debit after a failed plan attempt. + +**Implementation Notes** +- Integration: registration in the plugin framework (`register_parjob_allocator()`) is unchanged — only the two hook bodies change. +- Validation: `test_parjob_combinatorial.c` must cover the plan-order-matters worked example from `requirements.md` (descending bucket order succeeding where ascending would fail, identical host pool) and the fresh-per-bucket `host_iter` requirement. +- Risks: the `seqjob_prepare_allocation()` fan-out corruption this fix depends on (Req 4.6) — mitigated by landing both changes together, never one without the other (see `research.md` Risks). + +#### `seqjob_allocator` + +| Field | Detail | +|-------|--------| +| Intent | Trivial single-host allocation, rewrapped onto the shared iterator pattern with provably unchanged external behavior | +| Requirements | 9.1, 4.6 | + +**Responsibilities & Constraints** +- `my_alloc()`'s bucket loop degenerates to exactly one iteration (`nbuckets == 1`, `bucket_slots == [1]`), using the same shared `host_iter.c` and `is_claimed()`/rollback machinery `parjob_allocator` uses — the abstraction validated on the simple case before the complex one. +- `seqjob_prepare_allocation()` gains a `primary->maxTasks == 1` guard — a no-op for seqjob's own behavior (it never sees `maxTasks > 1` jobs), required only so it stops corrupting `parjob_allocator`'s fixed prepare hook via `plugin_prepare_allocation()`'s fan-out. + +**Dependencies** +- Inbound: `sched_make_primary_alloc_()` (P0) +- Outbound: `xce_alloc_plan_iter_t` (own trivial impl, P0), `host_iter.c` (P0) + +**Contracts**: Service [x] / API [ ] / Event [ ] / Batch [ ] / State [ ] + +##### Service Interface +Same shape as `parjob_allocator`'s (`my_alloc`/`prepare_allocation_hook`) — see above; not restated. +- Preconditions: `ctx->primary->maxTasks == 1`. +- Postconditions: identical host selection to today's `sched_collect_workers()` + `xlist_find()` implementation, given identical worker/resource state (Req 9.1) — this is the requirement the rewrap must prove, not just aim for. +- Invariants: unchanged. + +**Implementation Notes** +- Integration: registration (`register_seqjob_allocator()`) unchanged. +- Validation: `test_seqjob_rewrap_equivalence.c` must construct fixture states where the rewrap could plausibly diverge (multiple candidate hosts, at least one failing the select filter) and assert identical outcomes pre/post rewrap — a test that only exercises the single-obvious-host case would not actually prove equivalence. +- Risks: none beyond the shared `seqjob_prepare_allocation()` guard dependency already noted under `parjob_allocator`. + +### Scheduler / Allocation (extension point) + +#### `xce_alloc_mode_t` + +| Field | Detail | +|-------|--------| +| Intent | Forward-compatible resource-pool-mode field on `xce_sched_ctx_t`, declared now so `scheduler-preemption` (and later modes) don't need to modify this spec's struct later | +| Requirements | 9.5 | + +**Responsibilities & Constraints** +- v1 declares `XCE_ALLOC_NORMAL = 0` and `XCE_ALLOC_PREEMPT`; only `XCE_ALLOC_NORMAL` behavior is implemented — no branch anywhere in this spec's `try_alloc_on_host()`/availability-check call sites reads this field for any purpose beyond existing. +- Adding further enum values or the actual borrowing behavior `XCE_ALLOC_PREEMPT` implies is explicitly `scheduler-preemption`'s job, not this spec's. + +**Contracts**: Service [ ] / API [ ] / Event [ ] / Batch [ ] / State [x] + +##### State Management +- State model: a single `xce_alloc_mode_t alloc_mode` field on `xce_sched_ctx_t`, defaulting to `XCE_ALLOC_NORMAL` (zero-value, matches `sched_ctx_init()`'s existing `memset(ctx, 0, sizeof(*ctx))`). +- Persistence & consistency: none — `xce_sched_ctx_t` is per-allocation-attempt scratch state, never persisted (see `requirements.md`'s "Rollback scope" discussion of the same struct). +- Concurrency strategy: none needed — single-threaded, per-attempt scratch. + +## Data Models + +### Domain Model +- **`xce_alloc_plan_t`**: a partition of a job's `maxTasks` into per-host slot buckets (`nbuckets`, `bucket_slots[]`) — the domain concept this whole feature revolves around. Not persisted; lives only for the duration of one plan attempt. +- **`xce_resreq_fingerprint_t`** / **`xce_cache_key_t`**: value objects, immutable once computed, fitting in a single cache line — see `requirements.md`'s exact field layout, not restated here. + +### Logical Data Model +No new persistent entities. The only durable-adjacent addition is `rusage_hash` on `xce_primary_resreq_t` (in-memory, process-lifetime, set once at parse time) and `resreq_fp` on `xce_primary_schedobj_t` (same lifetime class) — both scratch/derived fields, never wire-serialized, confirmed in `requirements.md`'s Requirement 7 notes and this spec's own Boundary Commitments. + +### Data Contracts & Integration +None — this feature introduces no new wire format, no new capnp schema, and (per the explicit design decision in `requirements.md`) no snapshot/replay format changes for the cache. + +## Error Handling + +### Error Strategy +Allocation failure is not an error condition in this domain — it is the expected, common "no plan/host satisfies this resreq right now" outcome, communicated via return value (`0`) and `ctx->reason`, exactly as today. Genuine errors (OOM, iterator `init()` failure) propagate as negative return codes and are treated the same as "no candidate for this bucket" at the allocation-decision level — a distinction `requirements.md`'s Composition Algorithm draws explicitly (host-iterator init failure treated as an infrastructure error, not a resource-availability determination). + +### Error Categories and Responses +- **Resource unavailable** (expected, high-frequency): plan exhausted → pend job with reason; cache the fingerprint so subsequent identically-shaped jobs skip the cost (Req 7). +- **Iterator init failure** (rare, e.g. OOM): treated as "this bucket/plan has no candidates," not a distinct error path — avoids a second failure taxonomy the caller would need to handle. +- **Fan-out corruption** (latent, closed by this design): `seqjob_prepare_allocation()`'s missing `maxTasks` guard, closed by Req 4.6 — see Existing Architecture Analysis. + +### Monitoring +No new logging/metrics surface beyond what `ctx->reason`/`xce_pendreason_t` already provides — this feature populates the same pend-reason machinery every allocator already uses, adding no new observability contract. + +## Testing Strategy + +- **Unit — plan iterator** (`test_alloc_plan_iter.c`, Req 2): one-plan-per-`next()`-call; most-preferred-first ordering for `maxTasks` values with and without a clean partition (e.g. `N=7` → `[3,2,2]` for `k=3`); stable exhaustion signal; `fini()` cleanup; `pthread_testcancel()` present. +- **Unit — host iterator** (`test_host_iter.c`, Req 3): select-expression + availability filtering unchanged from the existing evaluator; **the fresh-per-bucket adversarial case** (host too small for an earlier bucket remains eligible for a later, smaller one) — this is the single most important test in this spec, directly proving the bug found during design discussion is closed. +- **Unit — cache** (`test_alloc_cache.c`, Req 7): hit on identical fingerprint+identity; miss on any differing field; **same-key-twice hit** (not just insert-then-miss-different-key, which the inverted `.cmp` bug would still have passed); **reason-category round-trip** (insert with category X, later hit returns X, per Req 7.5 — the `kiro-validate-design` fix to the cache's value model); clear-at-begin-session. +- **Unit — parjob combinatorial** (`test_parjob_combinatorial.c`, Req 4): multi-bucket plan success and partial-failure-then-rollback; descending-vs-ascending bucket order worked example; `ctx->nslots` correctly varies the resource want per bucket. +- **Unit — seqjob rewrap equivalence** (`test_seqjob_rewrap_equivalence.c`, Req 9.1): identical host selection pre/post rewrap across fixture states including a failing-select-filter host, not just the single-obvious-host trivial case. +- **Integration — end-to-end multi-slot allocation** (`test_parjob_e2e_multislot.c`, brief.md scope): a real multi-host parallel job dispatched through the full `sched_make_primary_alloc_()` → `parjob_allocator` → commit path, asserting the final `xce_job_alloc_t.nhosts > 1` and correct per-host slot counts. +- **Regression — existing scheduler suite** (Req 9.3): full `scheduler` meson suite must pass unmodified in assertions, adjusted only where a test directly exercised the eager-one-shot or stub-return behavior this feature replaces. +- **HA soak** (Req 9.4): sequential and chaotic `/xce-test-ha` regression, confirming zero new job loss/divergence/liveness-stall attributable to this feature. + +## Supporting References +- Full pseudocode (composition algorithm, cache key hash/cmp/assign/destroy, v1 plan-shape/partition defaults) lives in `requirements.md` — not duplicated here per this template's own deduplication guidance. `design.md`'s job is component/file ownership and contracts; `requirements.md` remains the algorithmic source of truth. +- `research.md` — discovery findings (the `.cmp` polarity fix, the core-isolation boundary check, the test-file convention) and the three most consequential design decisions with alternatives considered. diff --git a/.kiro/specs/scheduler-allocation/requirements.md b/.kiro/specs/scheduler-allocation/requirements.md index b60505ae..3fad431a 100644 --- a/.kiro/specs/scheduler-allocation/requirements.md +++ b/.kiro/specs/scheduler-allocation/requirements.md @@ -12,7 +12,7 @@ This spec closes the remaining two allocation gaps: This spec introduces an **iterator-driven allocation architecture** with these components: -- A lazy `xce_alloc_plan_iter_t` -- candidate `(nhosts, slots_per_host)` plans, most-preferred first +- A lazy `xce_alloc_plan_iter_t` -- candidate plans (each a partition of the job's task count into per-host slot buckets, e.g. `[2,1]` for 3 tasks across 2 hosts), most-preferred first - A lazy `xce_host_iter_t` -- candidate hosts for a given plan, filtered by the existing select-expression evaluator - A `parjob_allocator` that walks the plan iterator to do real combinatorial multi-slot allocation - A session-scoped unsatisfiable-resreq fingerprint cache so a job whose resreq no plan can satisfy doesn't re-pay full allocation cost for every later job with the same fingerprint in the same scheduling session @@ -49,8 +49,8 @@ The iterator composition (`xce_alloc_plan_iter_t` + `xce_host_iter_t`) operates │ xce_alloc_plan_iter_t │ Req 2 │ │ │ init(primary_resreq, sched) │ - │ next() -> plan │ generates (nhosts, slots_per_host) - │ fini() │ candidates, most-preferred first + │ next() -> plan │ generates per-host bucket + │ fini() │ partitions, most-preferred first └────────────┬───────────────┘ │ plan ▼ @@ -73,21 +73,94 @@ The iterator composition (`xce_alloc_plan_iter_t` + `xce_host_iter_t`) operates ### Composition Algorithm (All Three Types) +A plan is a **partition** of the primary's task count into per-host buckets (e.g. for `maxTasks=3`: `[3]`, `[2,1]`, `[1,1,1]`) -- not a single `(nhosts, slots_per_host)` pair, which cannot express uneven splits. `allocate_primary()` must therefore satisfy every bucket in a plan against a **distinct** host before the plan counts as satisfied; satisfying only the first bucket and stopping (as a single-pass `try_alloc_on_host` loop would) is not a valid implementation of Requirement 4 -- it degenerates into the single-host plan and never actually exercises the multi-host case. + +**Host exclusivity within a plan** is enforced by the caller, not `xce_host_iter_t` itself: `allocate_primary()` skips any host already present in `claimed` before calling `try_alloc_on_host` -- Requirement 3's `init(plan, workers)` signature is unchanged, host_iter stays a pure select+availability filter. This distinction matters: a host *offered and rejected* for one bucket (too small for that bucket's slot count) must remain eligible for a later, smaller bucket in the same plan -- only a host *actually claimed* by a bucket is permanently excluded for the rest of that plan attempt. + +**Correction (this review): a fresh `host_iter` per bucket, not one shared across the whole plan.** An earlier version of this pseudocode created one `host_iter` before the bucket loop and let every bucket's `while` pull from the same cursor. Since `next()` is a forward-only cursor with no rewind, that silently breaks the "offered-and-rejected stays eligible for a later bucket" guarantee just stated: a host bucket 0 tries and rejects (too small for it) is gone from the cursor forever, even if it exactly satisfies bucket 1. Concretely, for plan `[2,1]` with host A (1 free slot) and host B (2 free slots): bucket 0 ("need 2") pulls A first, rejects it, pulls B, succeeds -- cursor now exhausted; bucket 1 ("need 1") gets nothing back even though A was sitting there the whole time. The fix: `host_iter_init(iter, plan, sched->workers)` is called fresh at the top of **each** bucket iteration (same plan, same host_filter every time -- only `is_claimed()`'s growing exclusion set differs call to call), and `fini()`'d at the end of that same bucket's search. This is a three-level nested loop (plan → bucket/partition → worker, the third level always starting fresh), not a two-level one with an inner index. + +**Bucket order is not merely a fail-fast heuristic -- for v1's single-resource-dimension model, descending (largest bucket first) can succeed where ascending would fail, with the identical set of available hosts.** Same example: with buckets tried ascending `[1,2]` instead, a fresh scan for "need 1" could greedily claim host B (2 free slots -- trivially satisfies 1, but wastefully overqualified), leaving only A (1 free slot) for "need 2", which fails. Descending order avoids this by construction: the largest ask is checked against the full pool before a smaller ask has a chance to consume a host only the larger one could have used. This is why the v1 default (descending, "Data Structure Changes Summary" below) is a real correctness property here, not just an optimization -- and why generating the reverse order as a *second* candidate plan for the same partition isn't needed on top of it. Which specific host gets picked among *multiple* hosts that satisfy the same bucket (host_iter's own internal candidate order, `HOST_ORDER_ANY`/`HOST_ORDER_MOST_FREE` etc.) is a separate, softer question already deferred as future optimization -- it affects efficiency under adversarial cases, not whether a solvable case gets missed outright the way the shared-cursor bug did. + +**Rollback scope.** `ctx->uncomitted` (`sched_ctx_init`/`sched_ctx_fini`, `schedobj.c`) is per-job-allocation-attempt scratch state, torn down at the end of one job's `sched_make_primary_allocation()`/`sched_make_compound_allocation()` call -- it never touches the real, session-wide `resInUse` books, which are only charged later, separately, once a job actually dispatches (`job->accounts`/`sched_account_jobs()`). So `rollback_plan_allocation()` can never corrupt cluster-wide resource state. It is still required, though: without it, a bucket that succeeded before a later bucket exhausted the host iterator leaves its provisional debit in `ctx->uncomitted.hosts[idx]` for the rest of that job's allocation attempt -- a *later* plan considering the same host would see it as falsely less available, purely from a phantom debit an earlier, abandoned plan left behind, which could wrongly pend a job a later plan could actually have satisfied. The reversal itself has no existing precedent to reuse (`sched_make_compound_allocation()`'s own on-failure path, `scheduler.c:206-238`, does not actually clean up partial state on a mid-loop failure -- a separate, pre-existing gap outside this spec's scope, tracked separately, not modeled here). The underlying subtract primitive already exists (`xvec_op(v1, v2, XVEC_SUB_FLOAT)`, `xvec.c`); it just needs a small new "uncommit" helper wired to it, mirroring the existing `XVEC_ADD_FLOAT` commit call in `sched_try_alloc_on_host_()`. For this to work, each `claimed[]` entry must retain the exact `alloc_req.host` snapshot captured when that bucket succeeded (needed for the merge step too -- see "Final allocation result" below), since `ctx->alloc_req` is reset and repopulated for every subsequent host tried. + +**Allocation convention (this review): iterators are stack-allocated by the caller, then initialized in place** -- matching `sched_ctx_init(&ctx, ...)`'s existing convention in this codebase, NOT a heap-allocating factory. `init()`'s own declared signature (`int (*init)(xce_alloc_plan_iter_t* iter, ...)`, "Data Structure Changes Summary" below) takes the iterator's own address as its first argument for exactly this reason. An earlier version of this pseudocode wrote `plan_iter = plan_iter_init(primary_resreq, sched)` as if it were a 2-arg factory function returning a new iterator -- inconsistent with the 3-arg in-place-init method the struct actually declares. Corrected below. + ```c // === PRIMARY leaf: iterator composition === // Returns >0 on success, 0 on failure int allocate_primary(sched, primary_resreq, ctx) { - plan_iter = plan_iter_init(primary_resreq, sched); + xce_alloc_plan_iter_t plan_iter_storage; + xce_alloc_plan_iter_t* plan_iter = &plan_iter_storage; + if (plan_iter->init(plan_iter, primary_resreq, sched) != 0) { + return ALLOC_FAIL; // exact error-propagation contract: design.md + } + while ((plan = plan_iter->next(plan_iter)) != NULL) { - host_iter = host_iter_init(plan, sched->workers); - while ((worker = host_iter->next(host_iter)) != NULL) { - if (try_alloc_on_host(worker, plan, ctx) > 0) { + // Correction (kiro-validate-design pass): sized by maxTasks, not an + // undefined fixed constant -- nbuckets can never exceed maxTasks + // (each bucket needs >=1 slot), so this is the true, tight upper + // bound. A fixed compile-time guess risks either overflow for a + // large parallel job or wasted stack for a small one. + xce_sched_worker_t* claimed[primary_resreq->maxTasks]; + int nclaimed = 0; + int plan_ok = 1; + + // Three levels of iteration: plan (outer while, above) -> bucket/ + // partition (this for) -> worker (host_iter, fresh per bucket, below). + for (int b = 0; b < plan->nbuckets; b++) { + // Correction (this review): a FRESH host_iter per bucket, not + // one shared across the whole plan -- next() is a forward-only + // cursor, so sharing it would drop any host an earlier, larger + // bucket tried and rejected, even if it exactly satisfies THIS, + // smaller bucket. Same plan (same select-expression host_filter) + // every call; only is_claimed()'s growing exclusion set differs. + xce_host_iter_t host_iter_storage; + xce_host_iter_t* host_iter = &host_iter_storage; + xce_sched_worker_t* w; + int bucket_ok = 0; + + if (host_iter->init(host_iter, plan, sched->workers) == 0) { + while ((w = host_iter->next(host_iter)) != NULL) { + if (is_claimed(claimed, nclaimed, w)) { + continue; + } + if (try_alloc_on_host(w, plan->bucket_slots[b], ctx) > 0) { + // snapshot ctx->alloc_req.host here (it is reset on + // the next plugin_prepare_allocation() cycle) -- + // needed for both rollback below and the merge step + // in commit_plan_allocation(). + claimed[nclaimed++] = capture_claim(w, ctx); + bucket_ok = 1; + break; + } + } host_iter->fini(host_iter); - plan_iter->fini(plan_iter); - return ALLOC_SUCCESS; + } + // host_iter->init() failing is treated the same as "no + // candidates" for this bucket -- an infrastructure error, not a + // resource-availability determination; see design.md for the + // exact error-propagation contract. + + if (!bucket_ok) { + plan_ok = 0; + break; // this bucket has no satisfying host -- abandon the plan } } - host_iter->fini(host_iter); + + if (plan_ok) { + commit_plan_allocation(claimed, nclaimed, ctx); + plan_iter->fini(plan_iter); + return ALLOC_SUCCESS; + } + + // Partial success within this plan attempt must not leak into the + // next plan attempt for the SAME job (ctx->uncomitted persists across + // plan attempts until sched_ctx_fini() -- see "Rollback scope" above): + // for each claimed host, subtract back exactly the debit + // sched_try_alloc_on_host_() added via XVEC_ADD_FLOAT, using the + // snapshot captured above (new xvec_op(..., XVEC_SUB_FLOAT)-based + // helper -- no existing "uncommit" primitive to call into). + rollback_plan_allocation(claimed, nclaimed, ctx); } plan_iter->fini(plan_iter); return ALLOC_FAIL; @@ -95,7 +168,12 @@ int allocate_primary(sched, primary_resreq, ctx) { // === COMPOUND: AND of primaries === // ALL primaries must allocate. On any failure, roll back prior allocations -// and return failure. Same logic as today's sched_make_compound_allocation(). +// and return failure. NOTE: today's sched_make_compound_allocation() does +// NOT actually do this rollback (see bug-1766, found during this spec's +// review) -- rollback_compound_allocations() below is new/corrective +// behavior this spec should introduce, not a port of existing logic. Out of +// this spec's own boundary per Req 9 AC2 (COMPOUND/ALTER dispatch +// unchanged) unless the user elects to fold bug-1766's fix in here. int allocate_compound(sched, compound_resreq, ctx) { for (int i = 0; i < compound_resreq->primaries.count; i++) { primary = compound_resreq->primaries[i]; @@ -137,7 +215,7 @@ int allocate_job(sched, job, ctx) { } ``` -**COMPOUND rollback note:** If primaries 1 and 2 of a compound allocate but primary 3 fails, primaries 1 and 2 must be rolled back -- the compound is atomic. Today this is handled by `sched_make_compound_allocation()` tracking allocations in `ctx` and cleaning up on failure. This behavior is unchanged; the iterator interfaces don't alter the rollback contract. +**COMPOUND rollback note:** If primaries 1 and 2 of a compound allocate but primary 3 fails, primaries 1 and 2 *should* be rolled back -- the compound is meant to be atomic. **Correction (this review): today's `sched_make_compound_allocation()` does not actually do this** -- see bug-1766. It shares one `ctx` across the whole primaries loop, `break`s on the first failing primary without undoing earlier successes, and its unconditional post-loop `if (ctx.alloc != NULL) MOVE_PTR(obj->alloc, ctx.alloc)` can attach a partial, orphaned allocation to a compound job reported as failed. This spec's iterator rework does not touch that path (Req 9 AC2 keeps COMPOUND/ALTER dispatch unchanged) -- the pre-existing gap is tracked separately in bug-1766, not fixed here, unless scope is deliberately expanded to include it. ### Who Uses This Composition @@ -150,8 +228,8 @@ int allocate_job(sched, job, ctx) { ### Key Design Properties - **Lazy**: `plan_iter->next()` generates one plan at a time. `host_iter->next()` returns one host at a time. No full materialization. -- **Cancel-safe**: both `next()` calls have `pthread_cancel` cancellation points. -- **Independent lifecycles**: a `host_iter` is created from a plan, used, and destroyed; the `plan_iter` is advanced to the next plan only after the `host_iter` is done. `fini()` on one does not affect the other. +- **Cancel-safe**: both `next()` calls have `pthread_cancel` cancellation points (Req 2 AC 6 / Req 3 AC 6 -- a testable requirement, not just a design intent, since neither loop has a natural cancellation point of its own). +- **Independent lifecycles**: a `host_iter` is created from a plan, used, and destroyed -- and, for a multi-bucket plan, this happens **once per bucket**, not once per plan (a fresh instance each time, per Req 3 AC 3/4). The `plan_iter` is advanced to the next plan only after every bucket's `host_iter` for the current plan is done. `fini()` on one does not affect the other. - **COMPOUND/ALTER unchanged**: the composition rules above the PRIMARY leaf (AND for compound, OR for alter) are unchanged from today. The iterator interfaces only change what happens inside `allocate_primary()`. - **Seqjob rewrap**: `seqjob_allocator` is rewrapped onto the iterator interfaces as a trivial case -- its plan iterator always returns the single plan (1 host x 1 slot), and the inner loop is identical to today's first-match logic. This validates the abstraction before applying it to the complex parjob case. - **Pluggable plan generation, shared host iteration**: plan generation is allocator-specific — each `job_alloc_fn` brings its own strategy (trivial, combinatorial, topology-aware, bin-packing) and creates its own `xce_alloc_plan_iter_t` internally. Host iteration is shared infrastructure — `xce_host_iter_t` has one implementation reused by all allocators, parameterized by sort order (`HOST_ORDER_ANY`, `HOST_ORDER_MOST_FREE`, etc.). Swapping allocation strategy = swapping the plugin; no core loop change. @@ -176,7 +254,7 @@ int allocate_job(sched, job, ctx) { v1 implements simple combinatorial generation: `1host×N, 2hosts×ceil(N/2), ..., Nhosts×1`. Future allocators read `span`/`same`/`affinity` to constrain the plan space without touching the `init()` signature. -2. Each call to `next()` shall generate exactly one candidate `(nhosts, slots_per_host, host_filter)` plan without generating or materializing any other plan in the same call. +2. Each call to `next()` shall generate exactly one candidate plan -- a partition of `maxTasks` into per-host buckets (`xce_alloc_plan_t.bucket_slots[]`, see "Composition Algorithm" above; NOT a uniform `(nhosts, slots_per_host)` pair, which cannot express uneven splits) -- without generating or materializing any other plan in the same call. 3. The Scheduler shall order plans returned by successive `next()` calls from most-preferred to least-preferred, per the owning allocator's preference policy. @@ -184,9 +262,11 @@ int allocate_job(sched, job, ctx) { 5. Calling `fini()` shall release all resources held by the iterator, and the interface shall define no other required cleanup step. +6. `next()` shall call `pthread_testcancel()` (or otherwise provide an equivalent, real cancellation point) at least once per invocation, matching the existing convention `sched_job_picker()`/`sched_allocator()` already establish at the top of their own loops (`scheduler.c`). This is required because, unlike those threads' blocking `cbuf_get()` calls (a POSIX cancellation point via `pthread_cond_wait`), combinatorial plan generation is fast, non-blocking pure computation with no cancellation point of its own -- an implementation satisfying AC 1-5 without this would silently violate the "Cancel-safe" design property stated for this iterator without failing any other criterion. + ### Requirement 3: Lazy host candidate iterator -**Objective:** As a maintainer of the scheduler's allocator plugins, I want a reusable lazy host-candidate abstraction per allocation plan, so that host filtering reuses the existing select-expression evaluator instead of each allocator reimplementing worker collection and filtering. +**Objective:** As a maintainer of the scheduler's allocator plugins, I want a reusable lazy host-candidate abstraction for finding a host satisfying one plan bucket at a time, so that host filtering reuses the existing select-expression evaluator instead of each allocator reimplementing worker collection and filtering. (Correction, this review: NOT "one `xce_host_iter_t` per plan" -- a plan with multiple buckets requires a fresh instance per bucket, per Req 4 AC 2.) #### Acceptance Criteria @@ -194,37 +274,47 @@ int allocate_job(sched, job, ctx) { 2. Each call to `next()` shall return the next candidate host satisfying the given plan's host filter (select expression plus resource availability), evaluated against the existing select-expression evaluator without changes to that evaluator. -3. `next()` shall return NULL once no further candidate host exists for the current plan, and the caller shall not need to call `next()` again after receiving NULL for that plan. +3. `next()` shall return NULL once no further candidate host exists for the current bucket's search, and the caller shall not need to call `next()` again on that same `host_iter` instance after receiving NULL. This does NOT mean the caller is done with the plan: a plan with more buckets remaining requires a fresh `host_iter` instance (a new `init()` call) for the next bucket, per Req 4 AC 2 -- exhaustion is scoped to one `host_iter` instance, not to the plan as a whole. -4. The host iterator shall compose with the plan iterator: a new `xce_host_iter_t` shall be constructible from any plan produced by Requirement 2's `xce_alloc_plan_iter_t.next()`. +4. The host iterator shall compose with the plan iterator: a new `xce_host_iter_t` shall be constructible from any plan produced by Requirement 2's `xce_alloc_plan_iter_t.next()` -- and, for a multi-bucket plan, constructible **repeatedly** from the same plan, once per bucket (Req 4 AC 2), each a fully independent instance. 5. Calling `fini()` shall release all resources held by the host iterator, independent of the plan iterator's lifecycle. +6. `next()` shall call `pthread_testcancel()` (or otherwise provide an equivalent, real cancellation point) at least once per invocation, for the same reason as Requirement 2 AC 6: scanning/filtering candidate hosts is non-blocking pure computation with no cancellation point of its own, so this must be explicit rather than assumed. + ### Requirement 4: Parallel job combinatorial allocation **Objective:** As a cluster operator submitting a job that needs multiple task slots, I want `parjob_allocator` to actually allocate across hosts, so that jobs with `maxTasks > 1` are no longer silently unallocatable. #### Acceptance Criteria -1. When a job's `xce_primary_resreq_t.maxTasks > 1` reaches `parjob_allocator`'s `job_alloc_fn` (`my_alloc()` in `parjob_allocator.c`), the Scheduler shall generate candidate plans covering at minimum the sequence 1 host x N slots, 2 hosts x ceil(N/2) slots, ..., N hosts x 1 slot, using Requirement 2's plan iterator. +1. When a job's `xce_primary_resreq_t.maxTasks > 1` reaches `parjob_allocator`'s `job_alloc_fn` (`my_alloc()` in `parjob_allocator.c`), the Scheduler shall generate candidate plans covering at minimum the host-count sequence `1..N` hosts, where each plan's bucket sizes are a partition of `maxTasks` into that many buckets (e.g. for `N=3`: `[3]`, `[2,1]`, `[1,1,1]` -- balanced ceil/floor split, not a uniform `slots_per_host` that cannot express uneven splits), using Requirement 2's plan iterator. -2. For each candidate plan, the Scheduler shall use Requirement 3's host iterator to collect hosts with enough available slots to satisfy that plan, and shall attempt allocation before advancing to the next plan. +2. For each candidate plan, the Scheduler shall use Requirement 3's host iterator to find, for every bucket in the plan, a distinct host with enough available slots to satisfy that bucket -- accumulating claimed hosts one bucket at a time -- and shall attempt allocation before advancing to the next plan. A plan is satisfied only when every one of its buckets has a claimed host; if any bucket exhausts the host iterator without a match, the plan fails as a whole and any buckets already matched within that same plan attempt are released before advancing to the next plan (see Composition Algorithm's `rollback_plan_allocation`). -3. When a plan's host collection satisfies the plan's slot requirement, the Scheduler shall commit that allocation and shall not evaluate any remaining plan for that job. + **A new `xce_host_iter_t` instance shall be constructed for each bucket** (three levels of iteration: plan -> bucket/partition -> worker, the third always starting fresh) -- never one shared, continuously-advancing instance reused across every bucket of a plan. Reusing one instance is an incorrect implementation: `next()`'s forward-only cursor would permanently drop any host an earlier, larger bucket tried and rejected as too small, even when that same host exactly satisfies a later, smaller bucket -- silently failing plans a correct implementation would have satisfied. Only `is_claimed()` (Criterion 2's own "distinct host" exclusion, applied by the caller across each bucket's fresh scan) may exclude a host across buckets; rejection by an earlier bucket's size check must not. + +3. When every bucket of a plan is satisfied, the Scheduler shall commit that allocation as a single `xce_job_alloc_t` merging one `xce_host_alloc_t` per claimed bucket/host (`nhosts` == the plan's bucket count; see "Final allocation result" under parjob_allocator changes, below) and shall not evaluate any remaining plan for that job. 4. When no plan among those generated in Criterion 1 can be satisfied, `my_alloc()` shall return the same not-satisfied signal (`0`) that the current stub returns today, so pend-reason handling in `sched_make_primary_alloc_()` is unaffected. 5. `parjob_allocator`'s `job_alloc_fn` shall no longer unconditionally `return 0` regardless of `ctx` contents -- the current stub behavior -- for any job with `maxTasks > 1`. +6. Before each candidate host is checked against a plan's bucket, the Scheduler shall set `ctx->nslots` (and `ctx->nhosts`, for the shared/global-resource pass) to that bucket's slot count so that `parjob_prepare_allocation()` computes the per-host resource want via the existing `calc_wanted_host_resvals(..., ctx->nhosts, ctx->nslots)` call -- the same mechanism `seqjob_prepare_allocation()` already uses, currently hardcoded to `1, 1`. Concurrently, `seqjob_prepare_allocation()` shall gain a `primary->maxTasks == 1` guard (it currently has none, unlike `parjob_prepare_allocation()`, which already gates on `maxTasks`). This guard is required, not optional cleanup: `plugin_prepare_allocation()` (`plugin.c:591`) unconditionally calls every registered plugin's `prepare_allocation_hook`, and `xvec_idval_store()` (`xvec.c:247`) appends `(idx, val)` pairs with no dedup by resource idx -- so once `parjob_prepare_allocation()` stores real values, an unguarded `seqjob_prepare_allocation()` would *also* fire for the same `maxTasks > 1` job and append a second, wrong (`nslots=1`) entry for the same resource idx into `ctx->alloc_req`, corrupting the availability check that Criterion 2 depends on. This is a latent defect in the existing fan-out, invisible today only because `parjob_prepare_allocation()` is currently a no-op. + +7. Shared-resource satisfaction (`sched_check_shared_satisfy()`, `ctx->alloc_req.shared`) shall be computed and checked once per primary resreq -- independent of which plan or bucket-shape is currently being tried -- exactly as it already is today for `seqjob_allocator`. Plan/bucket iteration (Criteria 1-3) governs only per-host slot allocation; it does not change when or how often the job-level shared-resource check runs. + ### Requirement 7: Session-level unsatisfiable resreq cache **Objective:** As a maintainer of scheduling-loop performance, I want jobs with a resreq the current session already proved unsatisfiable to skip allocation immediately, so that a batch of similarly-shaped unsatisfiable jobs doesn't each re-pay full plan/host iteration cost. #### Acceptance Criteria -1. The Scheduler shall maintain a per-session unsatisfiable-resreq fingerprint cache on `xce_scheduler_t`, keyed by `xce_cache_key_t` — a value type composing `xce_resreq_fingerprint_t` (pre-computed `{minTasks, maxTasks, select_node_id, rusage_hash}`) and identity (`{user, queue, project}` as xstring pointers). The resreq fingerprint is stored on the scheduling object (`xce_primary_schedobj_t`) and computed once on job ingest. The full cache key is constructed on the stack at lookup time — a single `kvtab_lookup()` with `cache_key_hash()` and `cache_key_eq()`. Fully O(1): no list walks, no string traversals, no allocations. This captures everything the allocator and limit plugin consume. +1. The Scheduler shall maintain a per-session unsatisfiable-resreq fingerprint cache on `xce_scheduler_t`, keyed by `xce_cache_key_t` — a value type composing `xce_resreq_fingerprint_t` (pre-computed `{minTasks, maxTasks, select_node_id, rusage_hash}`) and identity (`{user, queue, project}` as xstring pointers). The resreq fingerprint is stored on the scheduling object (`xce_primary_schedobj_t`) and computed once on job ingest. The full cache key is constructed on the stack at lookup time and looked up via a single `key_find()` call against a `kvtab_t` bound to a new `cache_key_ops` (`kv_key_ops_t`) — this codebase's real hash-table API binds hash/cmp/assign/destroy to the table at creation, not per call (see "New types" below; no free-function `kvtab_lookup()` taking hash/eq params exists). Lookup itself is O(1): no list walks, no string traversals; `key_find()` does not allocate (only `key_add()`, on insert, allocates the heap-owned key copy). This captures everything the allocator and limit plugin consume. -2. The Scheduler shall clear the unsatisfiable-resreq cache at the start of every scheduling session (`begin_session`). +2. The Scheduler shall clear the unsatisfiable-resreq cache at the start of every scheduling session (`begin_session`), by placing the clear inside `core_begin_session_internal()` (`core_session.c`) alongside the existing, unlocked `core_reset_shared_resval()`/`core_reset_workers()` calls -- a core-level function both `driver.c` (normal mode) and `sim_loop.c`/`sim.c` (simulation mode) reach identically through `xce_core_begin_session()`, so no driver-specific clearing code is needed for either mode. No explicit lock is required: this matches the existing, already-safe pattern those two sibling resets use, which relies on `xce_core_begin_session()`'s own documented ordering guarantee -- the driver completes all begin-session reset work strictly before `xce_core_signal_go()` unblocks the picker thread (the one `cbuf_put()` that lets the picker/allocator touch anything for the new session), so the mutex/condvar inside that `cbuf_put()`/`cbuf_get()` pair provides the memory-barrier the reset needs, the same way it already does for worker `resvals`/`sharedInUse`. Neither driver touches the cache object anywhere else -- lookup (AC 4) and insert (AC 3) happen exclusively on the allocator thread, inside `allocate_primary()`. + + **Not part of the snapshot/replay wire format.** The cache is pure in-memory, session-scoped state -- `sched_snapshot_data_t` (`snapshot.c`/`typemap.h`) does not capture it, and none should be added. Since AC 2 unconditionally clears it at the next `begin_session` regardless of what a loaded snapshot contained, any captured cache state would be discarded before a single lookup against it could occur -- persisting it would be dead weight with no observable effect. 3. When a job's primary resreq's plan iterator (Requirement 2) is exhausted without a successful allocation, the Scheduler shall populate the cache with that resreq's fingerprint before pending the job. In v1, cache population is preemption-agnostic: it fires on allocation failure alone. When the scheduler-preemption spec ships, preemption fires between allocation failure and cache population — if preemption succeeds, no cache entry is created; if preemption fails or is inapplicable, the cache entry is populated as normal. The allocation layer does not call into preemption; the caller (scheduling loop) orchestrates the sequence. @@ -232,6 +322,8 @@ int allocate_job(sched, job, ctx) { 5. A cache hit shall attribute the same pend reason category as the original unsatisfiable determination, so operators are not misled about why a later job was skipped. +**Note on `resreq_fp` and snapshot restore (informational, not an AC):** `resreq_fp` is computed at job-ingest time (`core_build_schedobj()`/`core_events.c`) but not by `snapshot_load.c`'s independent local `schedobj` reconstruction (scheduler-snapshot-job-allocation spec) -- so a session restored via `xce_scheduler -i -s ` would run with a permanently-cold cache for this Requirement. Confirmed (`xce_sched_core_load_snapshot()`'s 12 call sites are all in `sim.c`; none in `driver.c`) that this can only ever affect offline simulation/investigation sessions, never live production scheduling throughput -- so this is *not* elevated to a formal Acceptance Criterion. Tracked in bug-1767 alongside the broader recurring-duplication pattern in that reconstruction path, should someone want to close it later. + ### Requirement 9: No regression to existing allocators or HA soak **Objective:** As a maintainer, I want the iterator-driven rework scoped so that existing single-slot allocation and COMPOUND/ALTER dispatch are unaffected, so that this framework change is safe to ship independently of any new allocator. @@ -246,6 +338,10 @@ int allocate_job(sched, job, ctx) { 4. **HA soak**: A full HA soak (sequential and chaotic `/xce-test-ha` regression) shall show no new job loss, divergence, or liveness stall attributable to this feature, confirming the scheduler-side change does not perturb controller-side commit/replication behavior it reads state from. +5. **Allocation-mode extension point**: The Scheduler shall add an `xce_alloc_mode_t` enum (`XCE_ALLOC_NORMAL = 0`, `XCE_ALLOC_PREEMPT`) and an `alloc_mode` field on `xce_sched_ctx_t`, defaulting to `XCE_ALLOC_NORMAL`. This spec's own plan/host iterators and `try_alloc_on_host()` shall not branch on any value other than `XCE_ALLOC_NORMAL` -- implementing what `XCE_ALLOC_PREEMPT` (or any future mode) actually does to resource-availability checks is `scheduler-preemption`'s job, not this spec's. The field exists here, in this spec's own scope, purely so `scheduler-preemption` and later resource-pool modes (e.g. reservation-aware backfill) can extend `xce_sched_ctx_t` and the availability-check call sites without a further interface change to types this spec already owns -- avoiding a sibling spec's first task being "go modify scheduler-allocation's struct." Adding further enum values beyond the two declared here is explicitly out of this spec's scope. + +**Note (informational, not an AC):** Requirement 7's `resreq_fp` field is only ever populated on the live job-ingest path in this spec's v1 -- `snapshot_load.c`'s independent restore path does not compute it (see Requirement 7's note above and bug-1767). Since that path is exclusively simulation/investigation tooling (`sim.c`, never `driver.c`), this does not warrant a dedicated `/xce-audit-invariants scheduler` pass as a formal AC here; ordinary unit-test coverage of Requirement 7's cache hit/miss behavior is sufficient for this spec's scope. + ## Data Structure Changes Summary ### `xce_alloc_plan_iter_t` (new, Req 2) @@ -270,12 +366,58 @@ struct xce_alloc_plan_iter_s { void* private; }; -// A plan describes how many hosts and slots to attempt +// A plan is a partition of maxTasks into per-host buckets, e.g. for +// maxTasks=3: [3], [2,1], [1,1,1] -- NOT a uniform (nhosts, slots_per_host) +// pair, which cannot express uneven splits. typedef struct { - int nhosts; // number of hosts needed - int slots_per_host; // slots required on each host + int nbuckets; // number of hosts this plan needs (== len(bucket_slots)) + int* bucket_slots; // partition of maxTasks; bucket_slots[i] = slots + // needed on the i-th distinct host claimed for this plan // host_filter: derived from primary_resreq->select, plus slot availability } xce_alloc_plan_t; + +// Generating which host-counts to try, in what order (plan-shape policy) and +// how to partition maxTasks for a given host-count (partition policy) are +// deliberately left as internal, swappable policy inside the plan iterator's +// reference implementation -- not fixed by this interface. v1 ships ONE +// concrete, fully-specified, deterministic default of each -- "swappable +// later" does not mean "unspecified now": +// +// Plan-shape order (v1 default): ascending host-count, k = 1, 2, ..., N +// (N = maxTasks). Most-consolidated (fewest hosts) tried first, most-spread +// (N hosts x 1 slot) tried last -- the brief's original "1hostxN, 2hostsx +// ceil(N/2), ..., Nhostsx1" sequence, generalized to real partitions below. +// +// Partition order (v1 default), for a given host-count k: the canonical +// BALANCED split -- (N mod k) buckets get ceil(N/k) slots, the remaining +// (k - N mod k) buckets get floor(N/k) slots. E.g. N=7, k=3 -> [3,2,2]. +// No other partition of N into k parts is generated (bounds plan count to +// exactly N, not the full integer-partition combinatorial space). +// +// Bucket order within one plan (v1 default): descending by slot count -- +// largest bucket first, so a plan fails fast on its hardest-to-place chunk +// rather than discovering that late. A simplest-deterministic tie-break, +// not itself claimed as an optimization. +// +// This is deliberately "blind": v1's plan-shape and partition choices are a +// pure function of maxTasks alone -- they never inspect sched->workers' +// actual free-slot counts, topology, or load when deciding WHAT to generate +// or in WHAT order (only try_alloc_on_host/Req 3's host_iter ever look at +// live worker state, to decide whether a generated plan succeeds). Accepted +// consequence: a job with a very large maxTasks (e.g. thousands) on a modest +// cluster generates up to N candidate plans/host-counts in v1, most of which +// exceed the live worker count and fail immediately once host_iter finds too +// few candidates -- cheap per attempt, but not free, and not capped by +// anything cluster-state-aware. Real optimization -- e.g. skipping a +// host-count already known infeasible from current worker counts, or +// ordering by observed cluster state -- is explicitly future work, not v1; +// ordering MAY consult the full +// primary_resreq already passed to init() (span/same/affinity) without an +// interface change, matching AC1's existing forward-compatibility note. +// Deferred to a later enhancement, not this spec: a formal pluggable-strategy +// API for swapping either policy independently. v1 hardcodes both inside the +// combinatorial plan iterator; only allocator swap-out (Req 2 AC1's existing +// "future allocators bring their own iterator" path) is required now. ``` ### `xce_host_iter_t` (new, Req 3) @@ -324,23 +466,90 @@ typedef struct { const char* project; // xstring pointer } xce_cache_key_t; -// Hash — single call site, no ad-hoc field assembly -static inline uint64_t cache_key_hash(const xce_cache_key_t* k) { - uint64_t h = FNV64_INIT; - hash_update(&h, &k->resreq, sizeof(k->resreq)); - hash_update_ptr(&h, k->user); - hash_update_ptr(&h, k->queue); - hash_update_ptr(&h, k->project); +// Correction (this review): this codebase's real kvtab API (kv.h/kv.c) does +// NOT take a hash/eq pair per lookup call -- a kvtab_t binds ONE fixed +// kv_key_ops_t (.hash/.assign/.cmp/.destroy) at table-CREATION time +// (kv_params_t), and every lookup/insert goes through the generic +// key_find()/key_add()/key_del() using whatever keyops that table was built +// with. int_key_ops/string_key_ops are the only existing kv_key_ops_t +// instances -- a composite struct key needs a THIRD one, new to this spec: +static uint32_t cache_key_hash_(void* k) { + const xce_cache_key_t* key = (const xce_cache_key_t*)k; + uint32_t h = 2166136261u; /* FNV-1a 32-bit -- no + * existing hash-update + * helper in this codebase; + * this is new, self- + * contained code. */ + const unsigned char* p = (const unsigned char*)&key->resreq; + for (size_t i = 0; i < sizeof(key->resreq); i++) { + h = (h ^ p[i]) * 16777619u; + } + h = (h ^ (uint32_t)(uintptr_t)key->user) * 16777619u; + h = (h ^ (uint32_t)(uintptr_t)key->queue) * 16777619u; + h = (h ^ (uint32_t)(uintptr_t)key->project) * 16777619u; return h; } -// Equality — memcmp on fixed-size resreq portion + pointer equality on identity -static inline bool cache_key_eq(const xce_cache_key_t* a, const xce_cache_key_t* b) { - return memcmp(&a->resreq, &b->resreq, sizeof(a->resreq)) == 0 - && a->user == b->user - && a->queue == b->queue - && a->project == b->project; +// cmp -- three-way comparator, 0 == equal (confirmed against kv.c's real +// int_key_ops.cmp -> cmp_int_key: returns -1/0/1, and kvtab_get() checks +// `cmp(...) == 0` for a match). Correction (design-phase discovery): an +// earlier version of this function returned a BOOLEAN (1 for match, via +// &&-chained == comparisons) -- exactly inverted from the real contract. +// As originally written, every lookup would have been a false miss, +// silently disabling the entire cache (Req 7) without failing any test +// that didn't specifically assert a hit. memcmp() on the resreq portion +// already returns a 3-way result; the identity pointers just need the +// same shape. +static int cache_key_cmp_(void* a, void* b) { + const xce_cache_key_t* ka = (const xce_cache_key_t*)a; + const xce_cache_key_t* kb = (const xce_cache_key_t*)b; + int rc = memcmp(&ka->resreq, &kb->resreq, sizeof(ka->resreq)); + if (rc != 0) { + return rc; + } + if (ka->user != kb->user) { + return (ka->user < kb->user) ? -1 : 1; + } + if (ka->queue != kb->queue) { + return (ka->queue < kb->queue) ? -1 : 1; + } + if (ka->project != kb->project) { + return (ka->project < kb->project) ? -1 : 1; + } + return 0; } + +// assign -- kvtab keys are heap-owned by the table (see string_key_ops' +// strdup-on-insert precedent); *dest must be a fresh heap copy of the +// stack-constructed key passed at the lookup site, not an alias of it. +static int cache_key_assign_(void** dest, void* src) { + xce_cache_key_t* copy = (xce_cache_key_t*)malloc(sizeof(xce_cache_key_t)); + if (copy == NULL) { + return -ENOMEM; + } + memcpy(copy, src, sizeof(xce_cache_key_t)); + *dest = copy; + return 0; +} + +// destroy -- frees only the key struct itself; user/queue/project are +// borrowed xstring pointers (not owned, not freed here). +static void cache_key_destroy_(void* k) { + free(k); +} + +static kv_key_ops_t cache_key_ops = { + .hash = cache_key_hash_, + .assign = cache_key_assign_, + .cmp = cache_key_cmp_, + .destroy = cache_key_destroy_, +}; + +// Table construction: standard kvtab_type RTTI pattern (see sched->core.plugins/ +// sched->core.resmgr for a live, already-working example of this exact +// construction call site in this codebase) with +// kv_params_t{ .nslots = N, .keyops = &cache_key_ops, .datakiller = NULL } +// (NULL datakiller: a cache "value" is a hit/miss sentinel, nothing to free). ``` ### `xce_primary_resreq_t` addition (Req 7) @@ -364,29 +573,33 @@ xce_resreq_fingerprint_t resreq_fp; // populated on job ingest, read by cache l ### Lookup site (pseudocode) ```c -// Before allocation: construct key on stack, single hash + lookup +// Before allocation: construct key on stack, single hash + lookup via the +// real kvtab API (key_find/key_add -- NOT a free-function kvtab_lookup() +// taking hash/eq params, which doesn't exist in this codebase). xce_cache_key_t key = { .resreq = obj->u.primary->resreq_fp, .user = obj->user, .queue = obj->queue, .project = obj->project, }; -if (kvtab_lookup(sched->unsatisfiable_cache, &key, cache_key_hash, cache_key_eq)) { +void* unused = NULL; +if (key_find(sched->unsatisfiable_cache, &key, &unused) > 0) { pend_job(obj, original_reason); return; } +// on exhaustion (Req 7 AC 3): key_add(sched->unsatisfiable_cache, &key, SENTINEL) ``` ### Key design decisions -- **struct-based**: hash and equality live next to the type. Single call site, no scattered field assembly. +- **struct-based**: hash/cmp/assign/destroy live together in one `kv_key_ops_t`, matching this codebase's existing `int_key_ops`/`string_key_ops` idiom rather than an ad-hoc per-call hash/eq pair. - **`memcmp` on resreq portion**: all fields are fixed-size integers/IDs — no pointers, no deep compare. - **pointer equality on identity**: xstring guarantees identical content → identical pointer. - **select via `node_id`**: CSE intern table deduplicates identical select expressions. O(1). - **rusages via pre-computed hash**: set once at parse time, stable for life of resreq. O(1). - **span/same/affinity deferred**: currently empty structs; add to `xce_resreq_fingerprint_t` when they gain fields. -### `xce_sched_ctx_t` addition (allocation mode) +### `xce_sched_ctx_t` addition (allocation mode, Req 9 AC 5) A mode field that controls which resource pool the allocator draws from. Defaults to `XCE_ALLOC_NORMAL`. Consumed by `try_alloc_on_host()` and any resource-availability check — applies to all resource types (slots, shared resources, limit instances, licenses, etc.), not just CPU slots. @@ -425,12 +638,17 @@ The default policy (`lowest_priority_first`) generates victim sets lazily: small ### `parjob_allocator` changes (Req 4) - `my_alloc()` is replaced from a stub (`return 0`) to a real implementation that creates a plan iterator, walks candidate plans, and commits on first success. -- The allocator's registration in the plugin framework is unchanged -- only the `job_alloc_fn` implementation changes. +- **Correction (this review): "only `job_alloc_fn` changes" is wrong** -- an earlier pass of this document said so before Req 4 AC 6 was added; `parjob_prepare_allocation()` (the plugin's separate `prepare_allocation_hook`) also changes, per the next bullet. What IS unchanged is the plugin's registration struct itself (`name`/`init_fn`/`fini_fn`/`job_elect_fn`/`data`, `register_parjob_allocator()`) -- only the bodies of its two hook implementations change. +- **`parjob_prepare_allocation()` (currently also a stub -- see Req 4 AC 6) must be fixed alongside `my_alloc()`**: it is the hook that populates `ctx->alloc_req.host`/`ctx->alloc_req.shared` (via `calc_wanted_host_resvals`/`calc_wanted_global_resvals`, the same calls `seqjob_prepare_allocation()` already makes) for whichever bucket is currently being tried. `xce_sched_ctx_t` already carries `nhosts`/`nslots` fields for exactly this; the per-bucket loop sets `ctx->nslots = bucket_slots[b]` (`ctx->nhosts = 1`) before each host try, and the fixed hook reads them instead of the hardcoded `1, 1` seqjob's version uses. Because `ctx->alloc_req` is a single scratch buffer reset and repopulated per candidate host (`plugin_prepare_allocation()`, `plugin.c:591`, `xvec_idval_fini`+`init`s it before each fan-out), this is safe to recompute per host-try -- see Req 4 AC 6 for the corollary `seqjob_prepare_allocation()` gate this depends on. +- **Do not bypass `plugin_prepare_allocation()`'s fan-out to precompute a bucket's resource want once and reuse it across every candidate host.** The rusage-derived portion (`calc_wanted_host_resvals`) is host-independent and in principle cacheable per bucket, but the same function's tail call to `sched_fill_limit_requests(sched, &ctx->alloc_req.host, hlimits)` reads `hlimits = zarray_get(&obj->hlimits, ctx->worker->idx)` -- genuinely host-specific per-host limit-tracking state. A full per-bucket bypass would silently stop enforcing per-host limits for jobs allocated through the new iterator path. Caching only the rusage-derived slice (still running the limit-fill per host) is a valid future optimization but is not required for this spec. + +**Final allocation result -- merged from each claimed bucket, no new type needed.** `xce_job_alloc_t` (`xce/types.h`) already generalizes past one host: `nhosts` + `xce_host_alloc_t** allocs`. Today only `seqjob_allocator` exercises it, always with `nhosts=1` via the variadic `make_host_allocs(1, halloc)`. For a successful multi-bucket plan, `commit_plan_allocation()` builds one `xce_host_alloc_t` per claimed host (each from that bucket's own snapshotted `alloc_req.host`/`uncomit`, captured at the moment `try_alloc_on_host` succeeded for it -- `alloc_req.host` is overwritten on the next bucket's prepare/check cycle, so it cannot be read lazily after the loop moves on), collects them into an `xce_host_alloc_t** hallocs` array of length `nbuckets` (built directly with `calloc`+loop, since `make_host_allocs()`'s variadic signature only fits a compile-time-fixed argument list, not a runtime-sized claimed set), and calls the existing `make_job_alloc(jobid, nbuckets, hallocs)` unchanged. Shared-resource allocation (`allocs_shared`/`nres_shared`) stays job-level and computed once for the whole primary resreq, exactly as it is today -- it does not vary per bucket and is not part of the merge. ### `seqjob_allocator` changes (Req 9 AC 1) - Internal rewrap: `my_alloc()` is restructured to use the plan and host iterator interfaces internally as a trivial case (1 host x 1 slot plan, then host iteration matching today's `sched_collect_workers()` + `xlist_find()` logic). - Externally observable behavior is unchanged -- same host selection given same state. +- **`seqjob_prepare_allocation()` gains a `primary->maxTasks == 1` guard** (Req 4 AC 6). This is a no-op for seqjob's own observable behavior -- it never handles `maxTasks > 1` jobs regardless -- but is required for `parjob_allocator` correctness once its own prepare hook stops being a stub; see Req 4 AC 6 for why the unguarded fan-out is unsafe. ## Architecture Diagram @@ -444,8 +662,8 @@ The default policy (`lowest_priority_first`) generates victim sets lazily: small │ │ │ │ ▼ │ │ ┌──────────────────────┐ │ -│ │ plan_iter->next() │ ←─ generates (nhosts, │ -│ │ returns plan or NULL │ slots_per_host) candidates │ +│ │ plan_iter->next() │ ←─ generates per-host bucket │ +│ │ returns plan or NULL │ partitions (e.g. [2,1]) │ │ └──────────┬───────────┘ most-preferred first │ │ │ │ │ plan │ NULL (exhausted) │ @@ -477,6 +695,8 @@ The default policy (`lowest_priority_first`) generates victim sets lazily: small └──────────────────────────────────────────────────────────┘ ``` +**Simplified -- see "Composition Algorithm" above for the authoritative version.** This diagram shows one worker being tried per plan, which is exactly right for `seqjob_allocator` (`nbuckets == 1`) but is NOT the full picture for `parjob_allocator`'s multi-bucket plans: a real plan with `nbuckets > 1` requires an inner loop over each bucket, accumulating a distinct claimed host per bucket before the plan counts as satisfied (host-exclusivity, per-bucket resource snapshotting, and rollback-on-partial-failure all live in that inner loop, none of which fits cleanly into this box diagram). Treat this diagram as the single-bucket special case; the pseudocode under "Composition Algorithm" is the one to implement against. + ### Unsatisfiable Resreq Cache (Req 7) ``` @@ -552,33 +772,38 @@ When `alloc_mode != XCE_ALLOC_NORMAL`, resource-availability checks count the fu ### Full Preemption Flow +**Cache placement, corrected:** the cache is keyed by `resreq_fp`, which lives on `xce_primary_schedobj_t` -- a PRIMARY-level concept, not a whole-job one (a COMPOUND/ALTER job has no single resreq shape of its own to fingerprint). So the cache check wraps `allocate_primary()` itself, not `allocate_job()`'s top-level dispatch. For a bare PRIMARY job this is the whole story; for a COMPOUND or ALTER job, each nested primary gets its own cache check for free, once for every call `allocate_compound()`/`allocate_alter()` already makes into `allocate_primary()` -- no job-level cache entry is needed or possible. + ```c -int allocate_job(sched, job) { - // 1. Cache check - if (cache_lookup(job)) return pend(job); +// Wraps allocate_primary() -- called for EVERY primary reached, whether the +// job's own resreq IS a primary, or this is one primary nested inside a +// COMPOUND (allocate_compound()'s loop) or an ALTER's compound (transitively). +int allocate_primary_with_preempt(sched, primary_resreq, ctx, job) { + // 1. Cache check (Req 7) -- keyed on THIS primary's resreq_fp, not job-level + if (cache_lookup(primary_resreq, job)) return PEND; // 2. Normal allocation - if (allocate_primary(sched, job->resreq, ctx) > 0) return dispatch(job); + if (allocate_primary(sched, primary_resreq, ctx) > 0) return ALLOC_SUCCESS; // 3. Preemption (if applicable) if (job->queue->preemptible_queues == NULL) { - cache_insert(job); - return pend(job); + cache_insert(primary_resreq, job); + return PEND; } victim_iter_init(&viter, job, sched); while ((victims = viter.next(&viter)) != NULL) { ctx.alloc_mode = XCE_ALLOC_PREEMPT; - if (allocate_primary(sched, job->resreq, &ctx) > 0) { + if (allocate_primary(sched, primary_resreq, &ctx) > 0) { viter.fini(&viter); - return commit_preempt(victims, job); + return commit_preempt(victims); } ctx.alloc_mode = XCE_ALLOC_NORMAL; } viter.fini(&viter); - cache_insert(job); - return pend(job); + cache_insert(primary_resreq, job); + return PEND; } ``` diff --git a/.kiro/specs/scheduler-allocation/research.md b/.kiro/specs/scheduler-allocation/research.md new file mode 100644 index 00000000..bd084463 --- /dev/null +++ b/.kiro/specs/scheduler-allocation/research.md @@ -0,0 +1,81 @@ +## Summary +- **Feature**: `scheduler-allocation` +- **Discovery Scope**: Extension (existing system) — light discovery per `design-discovery-light.md`. The bulk of architectural discovery already happened during an extended, adversarial requirements-review pass (documented inline in `requirements.md`, not repeated here); this log covers the additional, implementation-level verification done specifically to unblock design.md. +- **Key Findings**: + - `kv.c`'s real `kv_key_ops_t.cmp` contract is a three-way comparator (0 = equal), confirmed against `cmp_int_key`/`kvtab_get`'s `cmp(...) == 0` check — `requirements.md`'s illustrative `cache_key_cmp_()` had the inverted (boolean) polarity and has been corrected in place. + - `sel_key_assign_fn` (`xce_select.c`) is a live precedent for exactly the struct-copy-on-insert pattern `cache_key_ops.assign` needs — same shape, confirms the approach rather than inventing one. + - The scheduler's `sched_core_isolation_lib` (zero-controller-I/O, mechanically tested) does not constrain this feature: `xce_alloc_plan_iter_t`/`xce_host_iter_t`/the cache are pure computation with no I/O, and live in `scheduler.c`/new plugin files — outside that isolated set — so no invariant conflict. + +## Research Log + +### `kvtab_t`'s real key-ops contract (vs. requirements.md's illustrative pseudocode) +- **Context**: Requirements review flagged that `requirements.md`'s Req 7 cache pseudocode used an invented `kvtab_lookup(tab, key, hash_fn, eq_fn)` free function and an unverified `.cmp` return convention — both needed confirming against the real `src/lib/kv.c`/`kv.h` before design could treat them as implementable. +- **Sources Consulted**: `src/lib/kv.h` (`kv_key_ops_t`, `kv_params_t` declarations), `src/lib/kv.c` (`kvtab_get`, `kvtab_add`, `cmp_int_key`, `int_key_ops`, `string_key_ops`), `src/lib/xce_select.c` (`sel_key_assign_fn`, an existing struct-key precedent). +- **Findings**: + - A `kvtab_t` binds exactly one `kv_key_ops_t` (`.hash`/`.assign`/`.cmp`/`.destroy`) at table-creation time via `kv_params_t`; lookups/inserts go through generic `key_find()`/`key_add()`/`key_del()`, never a per-call hash/eq pair. + - `cmp_int_key()` (`int_key_ops.cmp`) returns `-1`/`0`/`1` (a `strcmp`-style three-way comparator), and `kvtab_get()` checks `t->keyops->cmp(e->key, key) == 0` for a match. `requirements.md`'s original `cache_key_cmp_()` returned a boolean (1 for match) — inverted. **Fixed in place** during this discovery pass (see requirements.md's own inline correction note). + - `sel_key_assign_fn()` (`xce_select.c:113-122`) is a real, already-shipped `.assign` implementation for a composite struct key (`sel_node_key_t`): `malloc` + `memcpy` + `*dst = copy`, returning `1` on success. `requirements.md`'s `cache_key_assign_()` returns `0` on success instead — functionally equivalent (`kvtab_add()` only checks `rc < 0` as failure) but a one-line style deviation from precedent, not worth changing. + - Only `int_key_ops`/`string_key_ops` exist as pre-built `kv_key_ops_t` instances; `cache_key_ops` (Req 7) is a legitimate third, following the same shape as `sel_node_key_t`'s. +- **Implications**: `xce_cache_key_t`'s `kv_key_ops_t` is fully specified and correct as of this pass; design.md can reference `requirements.md`'s corrected code directly rather than re-deriving it. + +### Scheduler core-isolation boundary vs. this feature's new code +- **Context**: `src/scheduler/CLAUDE.md` documents a mechanically-enforced invariant (`test_core_isolation.c`) that `core.c`/`core_events.c`/`core_session.c`/`core_clock.c` carry zero controller I/O. Needed to confirm this feature's new types/functions don't land inside that isolated set, or if they do, that they don't violate it. +- **Sources Consulted**: `src/scheduler/CLAUDE.md`, `src/scheduler/src/scheduler.c` (`sched_allocator`, `sched_job_picker`, `sched_make_*_allocation`), `src/scheduler/src/core_session.c` (`core_begin_session_internal`, `xce_core_begin_session`/`xce_core_signal_go`/`xce_core_end_session`). +- **Findings**: + - This feature's new code (`xce_alloc_plan_iter_t`, `xce_host_iter_t`, `parjob_allocator`/`seqjob_allocator` bodies, the unsatisfiable-resreq cache and its `kv_key_ops_t`) lives in `scheduler.c` and `src/plugins/*.c` — outside the four isolated-core files. + - The one exception is the cache's `begin_session` clear, which is placed inside `core_begin_session_internal()` (`core_session.c`) — already inside the isolated set. This is safe: the clear touches only the cache's own in-memory kvtab, no I/O, matching the existing `core_reset_shared_resval()`/`core_reset_workers()` calls already living in that same function with zero locking. + - `xce_core_begin_session()`'s own doc comment (`core_session.c:359-368`) already documents the happens-before property this feature's cache-clear placement depends on (driver completes all begin-session reset work strictly before `xce_core_signal_go()` unblocks the picker/allocator) — no new synchronization primitive needed. +- **Implications**: No core-isolation conflict. Design.md's Boundary Commitments section cites this directly rather than re-deriving it (per design-review-gate.md's "Project Invariant Cross-Check"). + +### Test file / build registration convention +- **Context**: File Structure Plan needs concrete new test file paths and their meson wiring. +- **Sources Consulted**: `src/scheduler/testsuite/meson.build` (existing `executable()`/`test()` block pattern, `scheduler_test_deps`). +- **Findings**: Every test unit is its own `executable()` (sharing `scheduler_test_deps`) plus a matching `test()` registration in the `scheduler` suite — one block per `test_.c` file, following `structure.md`'s stated convention (`test_.c` naming, `testsuite/` location). +- **Implications**: New test files (plan iterator, host iterator, cache, parjob combinatorial, seqjob-rewrap equivalence) each get their own `executable()`/`test()` block, not folded into an existing one. + +## Architecture Pattern Evaluation + +Already resolved in `requirements.md`'s own "Alternatives Considered" section (Approach A: eager pre-computed plan array, rejected for memory pressure and no reusable abstraction; Approach B: lazy iterator composition, chosen). Not re-litigated here — see `requirements.md` brief for the full comparison table and rationale. + +## Design Decisions + +### Decision: Where the unsatisfiable-resreq cache's `begin_session` clear lives +- **Context**: Req 7 AC 2 requires the cache cleared every scheduling session, correctly, without a race against the allocator thread reading/writing it. +- **Alternatives Considered**: + 1. A new, independent hook (e.g. its own plugin `begin_session_fn`) — adds a synchronization question that doesn't already have a proven answer. + 2. Wire the clear directly into `core_begin_session_internal()`, alongside the existing `core_reset_shared_resval()`/`core_reset_workers()` calls. +- **Selected Approach**: (2). +- **Rationale**: `core_begin_session_internal()` already resets other shared, per-session state with zero explicit locking, relying on `xce_core_begin_session()`'s documented ordering guarantee (reset work completes strictly before `xce_core_signal_go()` unblocks the picker/allocator). Reusing this exact placement gets the same safety property for free instead of inventing a new one. +- **Trade-offs**: Couples the cache's lifecycle to `core_session.c`, a file outside this feature's otherwise-isolated new code — acceptable, since it's a one-line addition next to an established, analogous pattern, not new architecture. +- **Follow-up**: None — verified against live source during requirements review, not merely assumed. + +### Decision: `xce_alloc_plan_t` as a bucket-list partition, not `(nhosts, slots_per_host)` +- **Context**: The original brief/requirements draft modeled a plan as a uniform `(nhosts, slots_per_host)` pair, which cannot express uneven splits (e.g. 3 tasks across 2 hosts as `[2,1]`). +- **Alternatives Considered**: + 1. Uniform pair (original) — simple, but structurally cannot represent the general case Req 4 requires. + 2. Bucket-list partition (`nbuckets` + `bucket_slots[]`) — generalizes correctly; v1 generates exactly one canonical (balanced ceil/floor) partition per host-count, bounding plan count to O(N). +- **Selected Approach**: (2). +- **Rationale**: Req 4's own worked examples (`N=3` → `[3]`, `[2,1]`, `[1,1,1]`) are not representable under (1). Balanced-partition-only generation keeps v1's plan count linear in `maxTasks` rather than the full integer-partition space. +- **Trade-offs**: Slightly more complex struct than a pair, but the complexity is inherent to the problem, not incidental. +- **Follow-up**: None. + +### Decision: `host_iter` is re-created fresh per plan bucket, never shared across a plan's buckets +- **Context**: A shared, forward-only `host_iter` across all buckets of a multi-bucket plan permanently drops a host an earlier (larger) bucket tried and rejected as too small, even when it exactly satisfies a later, smaller bucket — found live during design discussion by tracing a concrete worked example (`plan=[2,1]`, host A with 1 free slot, host B with 2). +- **Alternatives Considered**: + 1. One `host_iter` per plan, shared across buckets (original, incorrect) — silently fails solvable multi-host allocations. + 2. A fresh `host_iter` instance per bucket, `is_claimed()` as the only cross-bucket exclusion. +- **Selected Approach**: (2). +- **Rationale**: Only a host actually *claimed* by an earlier bucket should be excluded from later buckets; a host merely *offered and rejected* (too small) must remain eligible. Only a fresh per-bucket scan can express this correctly, since a forward-only cursor cannot "reconsider" a previously-rejected candidate. +- **Trade-offs**: One more `init()`/`fini()` pair per bucket than the (incorrect) shared-cursor version — a fixed, small constant-factor cost per bucket, not a complexity-class change. +- **Follow-up**: None — this is now Req 4 AC 2's own explicit language, not just an implementation detail. + +## Risks & Mitigations +- **`parjob_prepare_allocation()`/`seqjob_prepare_allocation()` fan-out corruption** — `plugin_prepare_allocation()` unconditionally calls every registered plugin's `prepare_allocation_hook`, and `xvec_idval_store()` has no dedup by resource idx. Fixing `parjob_prepare_allocation()` without also gating `seqjob_prepare_allocation()` on `maxTasks == 1` would append a second, wrong resource-want entry for every parjob allocation attempt. Mitigation: both changes land together (Req 4 AC 6), never one without the other. +- **`resreq_fp` cold on snapshot-restore sessions** — accepted, not mitigated: confirmed `xce_sched_core_load_snapshot()`'s 12 call sites are all in `sim.c` (simulation/investigation tooling), never `driver.c`, so this can only ever cost cycles in an offline debugging session, never production throughput. Tracked in bug-1767 for future cleanup, deliberately not a blocking requirement here. +- **`sched_make_compound_allocation()`'s missing rollback on partial COMPOUND failure** (bug-1766) — pre-existing, out of this spec's boundary (Req 9 AC 2 keeps COMPOUND/ALTER dispatch unchanged). Not fixed here; flagged so a future spec addressing it doesn't have to re-discover it. + +## References +- `requirements.md` (this spec) — the primary source of truth for this design; contains the full discovery/design discussion this research log summarizes rather than duplicates. +- `.wolf/buglog.json` bug-1766, bug-1767 — pre-existing/adjacent gaps found during review, deliberately kept out of scope. +- `src/scheduler/CLAUDE.md` — core-isolation invariant, driver/picker/allocator threading model. +- `.kiro/steering/resource-accounting.md` — confirms the `available = resvals - resInUse` admission-check formula this feature's `try_alloc_on_host()` relies on, unchanged by this spec. diff --git a/.kiro/specs/scheduler-allocation/spec.json b/.kiro/specs/scheduler-allocation/spec.json index d0b8993d..7cf11dc2 100644 --- a/.kiro/specs/scheduler-allocation/spec.json +++ b/.kiro/specs/scheduler-allocation/spec.json @@ -1,28 +1,29 @@ { "feature_name": "scheduler-allocation", "created_at": "2026-07-27T00:00:00Z", - "updated_at": "2026-07-27T00:00:00Z", + "updated_at": "2026-09-04T00:00:00Z", "language": "en", - "phase": "requirements-generated", + "phase": "tasks-generated", "parent_spec": "scheduler-framework", "description": "Lazy iterator-driven allocation architecture with parallel job support and resreq caching for the XCE scheduler", "approvals": { "requirements": { "generated": true, - "approved": false + "approved": true }, "design": { - "generated": false, - "approved": false + "generated": true, + "approved": true }, "tasks": { - "generated": false, - "approved": false + "generated": true, + "approved": true } }, - "ready_for_implementation": false, + "ready_for_implementation": true, "blocked_on": [ - "RESOLVED 2026-09-03: scheduler-core reached implementation-complete (2026-08-19). scheduler-dispatch-commit reached implementation-complete (2026-08-24), including bug-1624 (task 2.1/3.1, 2026-08-23), the resInUse/sharedInUse corruption this spec's try_alloc_on_host() sequencing note was waiting on. Neither dependency remains blocking; requirements.md still awaits approval before design can proceed." + "RESOLVED 2026-09-03: scheduler-core reached implementation-complete (2026-08-19). scheduler-dispatch-commit reached implementation-complete (2026-08-24), including bug-1624 (task 2.1/3.1, 2026-08-23), the resInUse/sharedInUse corruption this spec's try_alloc_on_host() sequencing note was waiting on.", + "RESOLVED 2026-09-04: requirements.md approved after extensive review -- 7 original findings plus several more surfaced during live design discussion (a self-contradiction in the parjob_allocator changes section, a plan/host-iterator init calling-convention mismatch vs. the declared struct interface, and a real correctness bug in the composition algorithm: host_iter was shared/forward-only across all buckets of a multi-bucket plan instead of freshly re-scanned per bucket, which would silently drop solvable multi-host allocations), all fixed in place; brief.md brought back into alignment with requirements.md's corrected architecture (it had drifted, still showing the pre-fix 2-level algorithm). Two adjacent, pre-existing gaps found along the way (sched_make_compound_allocation()'s missing rollback on partial failure; the recurring schedobj-reconstruction-duplication pattern in snapshot_load.c) logged as bug-1766/bug-1767 and deliberately kept out of this spec's boundary. No remaining blocker -- design phase clear to start." ], "sibling_specs": ["scheduler-core", "scheduler-preemption", "scheduler-dispatch-commit"], "sequencing_note": "Added 2026-08-23 (user directive): blocked on scheduler-dispatch-commit landing first -- this spec's try_alloc_on_host() reads the same resInUse/sharedInUse totals bug-1624 (fixed by scheduler-dispatch-commit) corrupts, and the planned XCE_ALLOC_PREEMPT borrowing mode interacts with what counts as committed/borrowable under that spec's redefined RUN-state timing." diff --git a/.kiro/specs/scheduler-allocation/tasks.md b/.kiro/specs/scheduler-allocation/tasks.md new file mode 100644 index 00000000..2bafeb70 --- /dev/null +++ b/.kiro/specs/scheduler-allocation/tasks.md @@ -0,0 +1,105 @@ +# Implementation Plan + +- [ ] 1. Foundation: allocation data types and extension points +- [ ] 1.1 Add the lazy plan-generation and host-candidate iterator contracts + - Define the plan representation as a partition of a job's task count into per-host slot buckets, most-preferred ordering to be produced by whichever component generates plans + - Define the plan-iterator and host-iterator contracts (initialize, produce next candidate, release resources) as reusable abstractions any allocator can adopt + - Observable: the contracts compile and are referenced by at least a stub/no-op implementation with no allocator wired in yet + - _Requirements: 2.1, 3.1_ +- [ ] 1.2 Add resreq fingerprinting for the unsatisfiable-resreq cache + - Compute a stable resource-usage fingerprint once when a resource request is parsed + - Store that precomputed fingerprint on the job's scheduling object once at ingest, so every later cache lookup for that job reuses it instead of recomputing it (the full cache key, fingerprint plus user/queue/project identity, is still assembled fresh at each lookup) + - Write a unit test proving the fingerprint is stable across repeated computation for an identical resource request and differs when any contributing field changes + - Observable: a passing test demonstrates identical requests produce identical fingerprints and any differing field changes the fingerprint + - _Requirements: 7.1_ +- [ ] 1.3 Add the forward-compatible allocation-mode field + - Add an allocation-mode field to the per-attempt allocation context, defaulting to normal-resources-only + - Observable: the field exists, defaults correctly, and no existing allocation code path branches on any value other than the default + - _Requirements: 9.5_ +- [ ] 1.4 Add the per-host resource-debit reversal primitive + - Add a helper that reverses exactly the provisional resource debit an earlier successful host-claim made, using the existing subtraction primitive + - Write a unit test that debits a host's provisional usage, reverses it, and confirms the host's provisional usage returns to its pre-debit value + - Observable: a passing test shows debit-then-reverse leaves provisional usage unchanged from baseline + - _Requirements: 4.2_ + +- [ ] 2. Core: shared allocation infrastructure +- [ ] 2.1 (P) Implement and test the lazy host-candidate iterator + - Implement the shared host-candidate iterator against the existing select-expression evaluator and resource-availability check, with no allocator-specific logic + - Ensure each call advances a real cancellation point so a long-running scan can be safely interrupted + - Write a unit test proving a host too small for one candidate search remains a valid candidate for a later, smaller search using a freshly-initialized iterator over the same underlying worker set + - Observable: a passing test suite covers select-expression filtering, availability filtering, exhaustion signaling, and the too-small-then-fits-smaller-search case + - _Requirements: 3.1, 3.2, 3.3, 3.4, 3.5, 3.6_ + - _Boundary: host_iter.c_ +- [ ] 2.2 (P) Implement and test the unsatisfiable-resreq cache + - Implement the cache's key type and its hash/comparison/copy/release operations, backed by the existing keyed-table infrastructure, storing a reason category (not a bare present/absent marker) as each entry's value + - Implement lookup, insert, and clear operations against the cache + - Write a unit test proving a lookup on an identical fingerprint-plus-identity is a hit that returns the same reason category given at insert time, a lookup on any differing field is a miss, and clear empties all entries + - Observable: a passing test suite includes a same-key-twice hit assertion (not only an insert-then-miss-on-a-different-key round-trip) and a reason-category round-trip assertion + - _Requirements: 7.1, 7.3, 7.4, 7.5_ + - _Boundary: alloc_cache.c_ + +- [ ] 3. Core: seqjob allocator rewrap +- [ ] 3.1 Rewrap seqjob's allocation onto the shared iterators + - Restructure seqjob's allocation to use the shared plan-iterator and host-iterator contracts as a trivial single-bucket case, replacing its own inline worker-collection loop + - Add a guard so seqjob's resource-request preparation only ever acts on single-task jobs, closing the fan-out gap this creates for the parallel allocator's later fix + - Observable: seqjob's allocation still runs end to end using the shared iterators, with no allocator-specific worker-collection logic left in seqjob's own code + - _Requirements: 9.1, 4.6_ +- [ ] 3.2 Prove seqjob's rewrap preserves today's host-selection behavior + - Write a test suite constructing fixture states with multiple candidate hosts, including at least one host failing the select filter, asserting identical host selection to today's pre-rewrap behavior given identical worker/resource state + - Observable: a passing equivalence test suite demonstrates no behavior change across fixture states beyond the single-obvious-host trivial case + - _Requirements: 9.1_ + - _Depends: 3.1_ + +- [ ] 4. Core: parjob combinatorial allocation +- [ ] 4.1 Implement parjob's lazy combinatorial plan generation + - Generate candidate plans as a pure function of a job's task count: ascending host-count order, one balanced partition per host-count, descending bucket order within each plan + - Ensure each call advances a real cancellation point and that plan generation never inspects live worker state (a pure function of the task count alone) + - Write a unit test proving one-plan-per-call generation, most-preferred-first ordering (including an uneven split, e.g. splitting 7 tasks across 3 hosts), and a stable exhaustion signal + - Observable: a passing test suite covers plan ordering, uneven-split partitioning, and exhaustion signaling + - _Requirements: 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 4.1_ +- [ ] 4.2 Implement parjob's per-bucket allocation loop + - Replace the parallel allocator's stub with a real implementation that, for each candidate plan, claims a distinct host for every bucket using a freshly-initialized host search per bucket, excluding only hosts already claimed elsewhere in the same plan attempt + - Fix the parallel allocator's resource-request preparation to compute each bucket's want from that bucket's own slot count rather than a fixed value + - On a bucket failure, reverse the provisional debits already made for that plan's earlier successful buckets before advancing to the next candidate plan + - Return the same not-satisfied signal as before when every candidate plan is exhausted, leaving pend-reason handling unaffected + - Write a unit test proving a plan whose buckets are tried in descending size order succeeds where the same host pool would fail in ascending order, and a test proving a mid-plan bucket failure correctly reverses earlier successful claims before the next plan is tried + - Observable: a passing test suite covers multi-bucket success, the descending-vs-ascending worked example, and partial-failure-then-reversal + - _Requirements: 4.2, 4.4, 4.5, 4.6, 4.7_ + - _Depends: 3.1_ +- [ ] 4.3 Implement parjob's multi-host allocation commit + - Merge every claimed bucket's host allocation into a single job allocation record spanning all claimed hosts, leaving job-level shared-resource allocation computed once and unaffected by how many hosts were used + - Observable: a successfully allocated multi-host job's allocation record correctly lists every claimed host with its own slot count + - _Requirements: 4.3_ + +- [ ] 5. Integration: dispatch and cache wiring +- [ ] 5.1 (P) Wire the unsatisfiable-resreq cache around the allocator dispatch + - Before running full allocation for a primary resource request (whether the job's own request is a primary, or one nested inside a compound or an alternative), look up its fingerprint in the cache; on a hit, pend the job immediately using the stored reason category without invoking either allocator + - When an allocator exhausts every candidate plan, populate the cache with the job's fingerprint and the resulting reason category before pending the job + - Observable: a job sharing an identical fingerprint and identity with a previously-exhausted job is pended immediately with the same reason category, without either allocator running + - _Requirements: 7.3, 7.4, 7.5_ + - _Boundary: scheduler.c dispatch_ + - _Depends: 2.2, 3.1, 4.2_ +- [ ] 5.2 (P) Clear the cache at the start of every scheduling session + - Clear the cache as part of the existing per-session reset step, alongside the other state that step already resets + - Observable: a job cached as unsatisfiable in one session is re-evaluated fresh (not immediately pended from the cache) in the next session + - _Requirements: 7.2_ + - _Boundary: core_session.c_ + - _Depends: 2.2_ +- [ ] 5.3 End-to-end multi-slot parallel job allocation proof + - Drive a real multi-host parallel job through the full dispatch path from submission to a committed multi-host allocation + - Observable: the resulting job allocation spans more than one host with correct per-host slot counts, confirmed through the same dispatch path production jobs use + - _Requirements: 4.1, 4.2, 4.3_ + - _Depends: 5.1, 5.2, 4.3_ + +- [ ] 6. Validation: regression and soak +- [ ] 6.1 Full existing scheduler regression pass + - Run the existing scheduler test suite, updating only the assertions that directly exercised the eager-one-shot or stub-return behavior this feature replaces + - Confirm the compound and alternative resource-request dispatch paths remain exercised, unmodified, by their existing tests + - Observable: the full existing scheduler suite passes with no unexpected assertion changes + - _Requirements: 9.2, 9.3_ + - _Depends: 5.3_ +- [ ] 6.2 HA soak regression + - Run a full sequential and chaotic HA regression soak + - Observable: the soak reports zero new job loss, divergence, or liveness stall attributable to this feature + - _Requirements: 9.4_ + - _Depends: 6.1_ diff --git a/.wolf/buglog.json b/.wolf/buglog.json index bee2d15c..1036a72d 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -20860,6 +20860,50 @@ "related_bugs": [], "occurrences": 1, "last_seen": "2026-09-03T14:51:54Z" + }, + { + "id": "bug-1766", + "timestamp": "2026-09-04T08:03:28Z", + "error_message": "OPEN, NOT A CRASH -- discovered while reviewing the scheduler-allocation spec requirements against live scheduler.c source: sched_make_compound_allocation() does not roll back already-succeeded primaries when a LATER primary in the same COMPOUND resreq fails allocation, and its own post-loop code can misattribute a partial allocation to a failed compound.", + "root_cause": "sched_make_compound_allocation() (src/scheduler/src/scheduler.c:206-238) loops over a COMPOUND resreq's primaries, calling sched_make_primary_alloc_(sched, &ctx) for each with ONE shared ctx across the whole loop. On a mid-loop failure (rc <= 0) it just `break`s -- it never undoes the primaries that already succeeded (no zeroing of ctx.alloc, no reversal of any resource state those primaries touched). Worse: seqjob/parjob's job_alloc_fn sets ctx->alloc directly on success (see seqjob_allocator.c my_alloc()), so if primary 1 of a 2-primary compound succeeds (leaving ctx.alloc non-NULL) and primary 2 then fails, the function's unconditional post-loop `if (ctx.alloc != NULL) { MOVE_PTR(obj->alloc, ctx.alloc); }` still fires regardless of the overall `rc <= 0` -- so a COMPOUND job that is reported as allocation-FAILED can still end up with obj->alloc set to primary 1's partial, orphaned allocation. requirements.md for scheduler-allocation (Req 9 AC2) had assumed, based on the sibling scheduler-framework brief's own text, that this path already 'tracks allocations in ctx and cleans up on failure' -- that claim does not hold against the actual code; requirements.md was corrected during this review. Not yet confirmed whether the orphaned ctx.alloc is ever read for a job later found to be PEND/failed (i.e. whether this is dormant/leak-only or produces an observably wrong result) -- not investigated further since it is out of the scheduler-allocation spec's own boundary (Req 9 AC2 explicitly freezes COMPOUND/ALTER dispatch as unchanged).", + "fix": "NOT FIXED -- out of scope for the scheduler-allocation spec (which explicitly leaves COMPOUND/ALTER dispatch unchanged, Req 9 AC2) and no other spec currently owns sched_make_compound_allocation(). Discovered only because scheduler-allocation's own NEW per-plan rollback (Req 4's rollback_plan_allocation(), for the parjob multi-host-bucket case) needed a reusable precedent to model itself on; this path was found not to provide one. Needs its own follow-up: likely either (a) track which primaries in the compound loop actually succeeded and explicitly free/clear their contribution to ctx.alloc before returning on a later failure, or (b) defer the MOVE_PTR(obj->alloc, ctx.alloc) until the loop confirms overall success, freeing ctx.alloc on the failure path instead. Whether this ever produces an observably wrong result (vs. a dormant orphaned pointer) is unconfirmed and should be checked first.", + "tags": [ + "scheduler", + "compound-resreq", + "rollback", + "partial-allocation", + "open-gap", + "scheduler-allocation-spec", + "requirements-review", + "not-fixed" + ], + "related_bugs": [], + "occurrences": 1, + "last_seen": "2026-09-04T08:03:28Z" + }, + { + "id": "bug-1767", + "timestamp": "2026-09-04T08:03:28Z", + "error_message": "OPEN, NOT A CRASH -- recurring pattern finding, not a single defect: snapshot_load.c's local, hand-duplicated reconstruction of xce_sched_obj_t/schedobj (kept independent of core_events.c's core_build_schedobj() because that helper is `static` and snapshot.c/snapshot_load.c are compiled as separate objects) has now caused THREE independent field-completeness gaps in the same narrow reconstruction path, each discovered separately.", + "root_cause": "snapshot.c's own header comment (Why not reuse core_events.c's core_build_job/core_build_queue/core_build_worker, snapshot.c:296-314) documents the deliberate choice to keep snapshot_load.c's job/queue/worker/schedobj reconstruction as a second, independent, field-for-field-identical implementation rather than exporting core_events.c's static builders -- justified there for core_build_job/queue/worker specifically (they also need an independent SAVE-direction implementation regardless, and funneling through xce_core_push_events() reintroduces a config/queue double-application bug). That reasoning does not equally apply to core_build_schedobj() alone: schedobj is never wire-captured (no build_wire_schedobj(), no SAVE-direction counterpart needed) and has no xce_core_push_events() involvement, so the only real objection to exporting it is 'larger, unrequested change to core_events.c's surface' -- a scope call, not an architectural wall. This duplication has now independently produced: (1) scheduler-snapshot-job-allocation spec's own motivating bug -- job->allocs/job->accounts never restored from snapshot (documented, deliberate scope decision at the time, later fixed); (2) the same spec's own further finding -- every snapshot-restored job (PENDING or RUNNING) got schedobj==NULL, an undocumented latent crash risk, fixed by adding a local field-for-field mirror of core_build_schedobj() to snapshot_load.c; (3) scheduler-allocation spec (this one) -- the new xce_resreq_fingerprint_t resreq_fp field added to xce_primary_schedobj_t (Req 7) is computed at core_build_schedobj()'s call site but has no reason to be computed by snapshot_load.c's independent copy, silently degrading Req 7's unsatisfiable-resreq cache to a permanent miss for any session started via snapshot+replay (the documented common real-investigation path) -- addressed in scheduler-allocation's own requirements.md (Req 7 AC 6) as a one-line addition, not by eliminating the duplication.", + "fix": "NOT FIXED -- each individual instance above was/will be fixed at its own site (additively, preserving the duplication), not by addressing the duplication itself. Recommended follow-up, out of scope for any single already-scoped spec: export core_build_schedobj() (with a signature change to take user/queue/project strings directly instead of a full xce_job_t*, since snapshot_load.c has the restored strings but not a real xce_job_t/submit struct at reconstruction time) and have snapshot_load.c call it instead of maintaining its own copy -- eliminating the ONE remaining independent-reconstruction risk in this path that isn't justified by a genuine SAVE-direction or xce_core_push_events() concern the way core_build_job/queue/worker's duplication is. Tag for persistence.md's 'Snapshot Save/Load Field-Completeness (Scheduler)' section and xce-audit-invariants scheduler dimension 4 -- this is exactly the class of gap that invariant was formalized to catch, and has now recurred a third time in the same function.", + "tags": [ + "scheduler", + "snapshot", + "schedobj", + "field-completeness", + "duplication", + "open-gap", + "persistence", + "xce-audit-invariants", + "not-fixed", + "recurring-pattern" + ], + "related_bugs": [ + "bug-1766" + ], + "occurrences": 3, + "last_seen": "2026-09-04T08:03:28Z" } ] }