myxce/.kiro/specs/resreq-maxtasks-wiring/research.md

# Research & Design Decisions

## Summary
- **Feature**: `resreq-maxtasks-wiring`
- **Discovery Scope**: Extension (existing system) — light discovery
- **Key Findings**:
  - `parse_resreq_primary()`/`parse_resreq()` take no job-context parameter at all, and 6 existing test call sites invoke them directly with the bare production signature — an "add a parameter" fix would break every one of them.
  - There are 5 real (non-test) call sites, not 3: fresh ingest (`core_build_job()`, `make_sched_job()`), snapshot restore (`snapshot_load.c`), and two previously-uncounted job-update handlers (`sched_update_existing_job()`, `core_events.c`'s `JOB_UPDATE` handler) that also re-parse a job's resreq.
  - A single job's resreq parse tree can contain multiple primaries when `COMPOUND`/`ALTER` syntax is used; only the job's own top-level standalone primary (`tag == XCE_RESREQ_PRIMARY`) should receive the job's task count — nested primaries must stay untouched to satisfy Requirement 3.3's non-regression guarantee.
  - `minTasks`/`maxTasks` are not independently persisted in the WAL/snapshot wire format — they are always re-derived at parse time from `job_submit_data_t.mintasks/maxtasks`, which is already correctly round-tripped. Fixing the derivation fixes both fresh-ingest and snapshot-restored jobs with no new wire schema.
  - `xcecd_virtual`'s multi-host relay code (`job_start_remote_task()`, `create_mpi_cmdline()`, `handle_runtasks()`) is completely unguarded by `VIRTUAL_CLUSTER` — it is byte-identical to the real worker's code, and multiple named virtual-worker instances can already run concurrently. No new worker-side test infrastructure is needed.
  - Today, the only way to produce a real `nhosts > 1` dispatch end-to-end is `xce_test`'s `jobrun` debug bypass, which hand-constructs the allocation and skips the scheduler's own decision entirely. Once this spec's fix lands, a plain `jobsub -n <count>` against a real scheduler with multiple real workers should produce a genuine multi-host split on its own — proving that is the actual point of this spec's E2E test, not exercising the pre-existing worker-side code (which was never broken).
  - Of the 5 parser call sites, only 2 are reachable in production today: `core_build_job()` and the snapshot-restore path. `make_sched_job()` (and its own caller chain, `sched_update_jobs()`/`sched_update()`) is dead code — confirmed by exhaustive grep and by pre-existing comments already in the codebase, and matching the identical, already-fixed sibling case bug-1740 established for `sched_apply_job_rusage()`. `sched_update_existing_job()` is dead for the same reason. `core_events.c`'s `JOB_UPDATE` handler is live but functionally inert for scheduling (bug-1773) and, separately, never receives a genuinely-changed resreq today since no live feature mutates a job's resreq post-submission.
  - "Snapshot restore" (`snapshot_load.c`) is live only through the `--sim FILE` offline simulation/replay driver (`sim.c`'s `sim_locate_and_load_snapshot()`), not through any live-cluster crash-recovery or post-failover path. A live scheduler restart rejoins via `on_scheduler_join()` → the same `core_build_job()` path fresh ingest uses, never touching `snapshot_load.c`. This means the HA soak provides zero coverage of that call site; a dedicated comparative test (Requirement 5.6) closes the gap. bug-1772's own original phrasing ("post-restart or post-failover restore") was corrected to reflect this.
  - Separately, "write-ahead-log restore" (Requirement 1.3's original wording) turned out not to be a third code path at all: a controller-restart's WAL-replayed job state reaches the scheduler via `on_scheduler_join()` → the identical `core_build_job()` path fresh ingest uses (confirmed by tracing `job.c`'s `restore_job()`/`restore_job_array()` → `sched.c`'s `on_scheduler_join()` → `xcesched_push_jobs()` → `handler.c`'s `sched_on_jobs()` → the same driver-translated ingest). Requirement 1.3 was reworded to refer only to the simulation/replay driver's snapshot loading, since that's the only genuinely distinct second ingest path that exists.

## Research Log

### `parse_resreq_primary()` call graph and fix-shape constraints
- **Context**: Requirements demand the job's real task count reach `xce_primary_resreq_t.minTasks/maxTasks`; needed the exact current code shape to pick a fix mechanism.
- **Sources Consulted**: Direct reads of `src/lib/resreq.c`, `src/include/resreq.h`, `src/scheduler/src/core_events.c`, `src/scheduler/src/schedobj.c`, `src/scheduler/src/snapshot_load.c`, and every test file calling either function.
- **Findings**:
  - `parse_resreq_primary(xce_primary_resreq_t*, char*, var, char**)` and `parse_resreq(xce_resreq_t*, char*, var, char**)` (`resreq.c:391-392`, `:448`) take no job object.
  - 5 real call sites: `core_build_job()` (`core_events.c:247`), `make_sched_job()` (`schedobj.c:182`), snapshot restore (`snapshot_load.c:438`), `sched_update_existing_job()` (`schedobj.c:638`), and the `JOB_UPDATE` handler (`core_events.c:1221`).
  - At every ingest call site, the job's real `mintasks`/`maxtasks` is available in scope at or immediately after the parse call, either via `j->submit->mintasks/maxtasks` (or `wj->submit->`) or, at the two update sites, via the already-resolved `job->mintasks/maxtasks` (the update handlers never refresh task count from `updated->submit`, so re-applying the job's existing value is the conservative, scope-preserving choice).
  - 6 test call sites (`test_resreq.c` x3, `test_core_events.c:1520`, `test_scheduler_cache_dispatch_compound.c:312/315`, `test_snapshot_load_allocator_regression.c:206`) call the bare 4-arg signature directly with no job context — ruling out any signature change.
  - `xce_primary_resreq_t` (`resreq.h:238-252`) has no field or comment documenting the relationship between its own `minTasks/maxTasks` and the job-level pair — this duplication (one job, potentially several primaries in a `COMPOUND`/`ALTER` tree, each historically hardcoded to 1/1) is confirmed by `scheduler.c:192-196`'s comment that `parse_resreq_primary()` runs "uniformly for EVERY primary in a resreq's parse tree... not only top-level ones."
- **Implications**: The fix must be an overwrite performed by each caller immediately after the parse call returns (not a parser-signature change), gated on the resreq's top-level tag being `XCE_RESREQ_PRIMARY` so `COMPOUND`/`ALTER` nested primaries are left exactly as today.

### Test infrastructure for a real multi-host execution proof
- **Context**: Requirement 4/5 need an automated test proving a real multi-host job launches and runs, but the controller/worker multi-host code has zero existing automated coverage.
- **Sources Consulted**: `.claude/skills/xce-test-ha/run-ha-soak.sh`, `.claude/skills/xce-deploy-ha/SKILL.md`, `.claude/skills/xce-test-resources/`, `src/worker/meson.build`, `src/worker/src/controller_connector.c`, `src/controller/testsuite/clean_failover_preservation.sh`, `src/controller/testsuite/test.c`'s `cmd_jobrun`.
- **Findings**:
  - `xcecd_virtual` is the same `xcecd_srcs` as the real worker, built only with `-DVIRTUAL_CLUSTER`; that macro gates identity/signal/timing shortcuts, never the multi-host relay code path. Multiple named virtual-worker instances can run concurrently (unlike real `xcecd`, which is single-instance per host).
  - `run-ha-soak.sh`'s `submit_worker_job()`/`verify_worker_rejoin_resync()` already proves the idiom for "submit real job via real `jobsub`, poll for real `RUN` then `DONE` against a real worker" — single-host only, but directly reusable as a pattern.
  - `xce_test`'s REPL (including `jobrun`) is already driven non-interactively via piped stdin in three existing scripts — no new automation mechanism needed.
  - No existing automated (meson/Unity) test spawns real, separate controller+worker processes; the one script that spawns real controller processes (`clean_failover_preservation.sh`) never starts a worker and isn't wired into meson.
- **Implications**: Build a new, small standalone script (same shape as `run-resource-verify.sh`/`clean_failover_preservation.sh`, single controller node — no HA needed for this proof), deploying two named `xcecd_virtual` instances and driving a real `jobsub`. Critically, drive it via plain `jobsub -n <count>`, not `jobrun`'s manual-allocation bypass — proving the *scheduler itself* decides to split across hosts is the actual deliverable of this spec; the worker-side launch code is pre-existing and unmodified.

### Persistence field-completeness (per `persistence.md`'s review requirement)
- **Context**: `xce_primary_resreq_t.minTasks/maxTasks` transitions from a compile-time constant to a real, job-derived value — steering requires an explicit per-field save/restore audit for any such change.
- **Findings**: `minTasks`/`maxTasks` are never independently serialized to the WAL or snapshot wire format. `snapshot_wire.c` persists `job_submit_data_t.mintasks/maxtasks` (the job-level source of truth) directly; `snapshot_load.c` restores it and then calls the same `parse_resreq()` chain fresh, which is exactly the derivation this spec fixes.
- **Implications**: No new wire field, no new snapshot schema, no round-trip gap introduced. Fixing the shared derivation function's callers fixes fresh-ingest and snapshot-restored jobs identically and simultaneously — this satisfies the persistence steering's field-completeness bar by construction, not by omission.

## Design Decisions

### Decision: Overwrite-after-parse via a shared helper, not a parser-signature change
- **Context**: `xce_primary_resreq_t.minTasks/maxTasks` must reflect the job's real task count.
- **Alternatives Considered**:
  1. Add job-context parameters to `parse_resreq_primary()`/`parse_resreq()` — rejected: breaks 6 existing test call sites that call the bare signature directly, and doesn't cleanly express "only the top-level standalone primary, never nested ones."
  2. Duplicate an inline overwrite at each of the 5 call sites — rejected: repeats the `COMPOUND`/`ALTER` tag-check five times (violates Simplification), harder to keep consistent.
  3. **Selected**: a single new helper, `apply_job_task_count()`. Originally scoped to all 5 real call sites; a later design review found 2 of the 5 (the job-update handlers) don't affect dispatch at all (`job->schedobj` is never rebuilt on update — see the dedicated Decision below and bug-1773), and a follow-up review found a 3rd (`make_sched_job()`) is dead code entirely (unreachable via `sched_update()`, which has zero live callers — see the dedicated Decision below and bug-1774's correction), narrowing this to the 2 live ingest call sites whose result actually reaches dispatch: `core_build_job()` and the snapshot-restore path.
- **Rationale**: Matches the codebase's own existing idiom (an un-abstracted field-copy from `submit->` onto the target struct right after it's built, already done 5 times for `job->mintasks/maxtasks` itself) while centralizing the one piece of real logic (the tag-gate and the defensive clamp) in one testable place.
- **Trade-offs**: Five call sites still each need one new line invoking the helper — irreducible given the parser's job-context-free signature, but each line is trivial and independently reviewable.
- **Follow-up**: `test_parjob_e2e_multislot.c` (scheduler-allocation feature)'s `PATCH_MAXTASKS_FOR_BUG_1770` workaround becomes obsolete once this fix lands and should be removed in favor of driving that test through real ingest — closing the loop in the same test that originally characterized the bug.

### Decision: E2E proof drives real `jobsub`, not the `jobrun` debug bypass
- **Context**: Need an automated test proving real multi-host execution; `jobrun` already provides a way to force a multi-host dispatch today.
- **Alternatives Considered**:
  1. Use `jobrun`'s manual `-m host:ntasks` allocation construction — proves the controller/worker wire path but bypasses the scheduler's own decision, which is not what this spec changes.
  2. **Selected**: plain `jobsub -n <mintasks,maxtasks>` against two real workers sized so neither alone can satisfy the request.
- **Rationale**: This spec's entire value is that the scheduler *itself* now correctly decides to span multiple hosts. A test that hand-constructs the allocation would pass identically whether or not this spec's fix exists, proving nothing about it.
- **Trade-offs**: None material — `jobsub`'s real dispatch path already includes everything `jobrun` would need to prove, once the fix lands.

### Decision: Scheduler-side min/max clamp is defensive-only, not a new rejection path
- **Context**: Requirement 1.4 requires the Scheduler not let an invalid task-count pair reach allocator dispatch; upstream (apiserver) already guarantees `mintasks <= maxtasks` twice before any job reaches the C layer.
- **Selected Approach**: `apply_job_task_count()` clamps `maxtasks` up to `mintasks` if violated (mirroring `convert.go`'s exact clamp direction), rather than rejecting the job.
- **Rationale**: Matches this project's "graceful degradation over complete failure" principle and does not introduce a new failure mode for a codepath that should be unreachable today; still gives Requirement 1.4 a real, testable invariant rather than one vacuously true by construction.

### Decision: Job-update call sites are out of implementation scope
- **Context**: An independent design review found that `job->schedobj->u.primary->resreq` — the object the scheduler actually dispatches on — is set once at ingest and never rebuilt by either job-update handler, which only replace `job->resreq` itself.
- **Alternatives Considered**:
  1. Apply the helper at both update call sites anyway, for `job->resreq`'s own internal consistency — rejected: it would be dead code with zero effect on dispatch, and could mislead a future reader into thinking it matters.
  2. Fix the underlying gap (rebuild `job->schedobj` on update) as part of this spec — rejected: this is a substantially larger, unrelated fix (an update to ANY field, not just task count, never takes effect for scheduling today) that belongs in its own spec.
  3. **Selected**: drop the two update-site changes entirely; reword Requirement 1.4 to describe the guarantee that already holds structurally; add a regression test proving it; log the underlying gap separately (bug-1773).
- **Rationale**: Matches the Simplification design principle — no production code should exist that has no observable effect. The regression test still gives Requirement 1.4 real, ongoing coverage without shipping dead code.
- **Follow-up**: bug-1773 (job-update never taking effect for scheduling, any field) is a real, independently valuable fix for a future spec.

### Decision: `make_sched_job()` is dead code — dropped from implementation scope entirely, not merely deferred
- **Context**: An independent review initially found `make_primary_schedobj()` (backing `make_sched_job()`) never computes `resreq_fp`, identical to bug-1772's snapshot-restore gap, and logged it as bug-1774 with Requirement 5.4 scoped to exclude that path. A SECOND, follow-up independent review then established that `make_sched_job()`'s entire call chain is unreachable in production: its only caller, `sched_update_jobs()`, is only called from inside `sched_update()`'s switch, and `sched_update()` (`scheduler.c`) has zero live callers anywhere in this codebase — confirmed by exhaustive grep (only the declaration, definition, and doc-comments describing it as dead/superseded exist) and corroborated by the identical, already-fixed sibling case in bug-1740 (`sched_apply_job_rusage()`, reachable only via the same dead path, fixed by moving the logic to the live `core_events.c` path instead).
- **Alternatives Considered**:
  1. Fix `make_sched_job()`'s `resreq_fp` gap now, since this spec already touches this exact class of code — rejected even before the dead-code finding, to stay consistent with the user's own prior scoping decision for bug-1772's sibling instance; rejected even more strongly once dead-code status was confirmed, since fixing unreachable code has zero production value.
  2. Wire `apply_job_task_count()` into `make_sched_job()` anyway, for code-path completeness — rejected: it would be dead code applying a fix to dead code, the same "no production code with no observable effect" concern that excluded the job-update call sites.
  3. **Selected**: drop `make_sched_job()`/`schedobj.c` from this spec's implementation scope entirely (not merely exclude it from one test's guarantee); correct bug-1774's original "confirmed live" claim (retracted, tagged dead-code/severity-low); move Requirement 5.4's actual exception to bug-1772 (the snapshot-restore gap, which IS live) instead.
- **Rationale**: Matches the Simplification design principle even more directly than the job-update decision above — there is no reason to wire, test, or even discuss a fix for code with zero live callers, beyond noting its existence for whoever eventually revives or removes it.
- **Follow-up**: bug-1774 remains open as a low-priority, dead-code-only finding; bug-1772 (now Requirement 5.4's real exception) should ideally be picked up alongside bug-1767's broader recommended remediation (share one real schedobj-construction implementation across the live call sites, rather than maintaining independent, drift-prone copies — including the dead one, if it's ever revived).

### Decision: Requirement 1.3 reworded, new Requirement 5.6 added, to accurately scope "snapshot restore"
- **Context**: A third independent review, applying the same liveness rigor now applied to `make_sched_job()`, found `snapshot_load.c`'s restore path is live only via the `--sim` offline simulation/replay driver — not live-cluster crash recovery or post-failover restart, which instead rejoins through the identical `core_build_job()` path fresh ingest uses. This meant Requirement 5.2's HA soak was implicitly (and wrongly) credited with covering this call site, and Requirement 1.3's "write-ahead-log restore" phrasing implied a third code path that doesn't actually exist.
- **Alternatives Considered**:
  1. Leave the wording as-is, since the implementation plan (wire `apply_job_task_count()` into `snapshot_load.c`) doesn't change — rejected: a reader would reasonably conclude the HA soak validates this path, which it does not, understating an actual test-coverage gap.
  2. **Selected**: reword Requirement 1.3 to name the simulation/replay driver specifically (dropping "write-ahead-log restore," which turned out to describe the same live path as fresh ingest, not a distinct one); add Requirement 5.6 requiring a direct comparative proof (snapshot-loaded task count equals what fresh ingest would derive for the same input); clarify Requirement 5.2's AC text that the HA soak doesn't exercise this path; correct bug-1772's own trigger description in the buglog.
- **Rationale**: The fix and its value (correct task count for `--sim`/tuning fidelity, per this project's driver-equivalence invariant) are unchanged — only the test-coverage claim needed correcting. An honest AC set with a real, explicit line of coverage is better than an implicit, incorrect one.
- **Follow-up**: none — `test_maxtasks_wiring_ingest.c`'s existing white-box test for the snapshot-restore call site already provides a natural home for the new comparative assertion; no new test file or infrastructure is needed.

### Decision: white-box MPI-launch test targets `job_start_remote_task()`'s gate, not `create_mpi_cmdline()` alone
- **Context**: A fourth independent review found the originally-planned test target, `create_mpi_cmdline()`, is a pure command-string builder one layer below the actual decision point. `job_start_remote_task()` (`controller_connector.c`, the real entry point `handle_startjob()` calls for `nhosts > 1`) has its own independent launch-type gate deciding whether to defer to MPI's own remote-launch mechanism or fan out via XCE's per-host dispatch — the actual behavior Requirement 4.4 depends on, with zero existing test coverage of any kind.
- **Selected Approach**: retarget `test_worker_launch_type_dispatch.c` primarily at `job_start_remote_task()`'s gate (correct no-op for MPI launch types, correct fan-out for the default type), secondarily at `job_start_local_task()`'s branch into `create_mpi_cmdline()`.
- **Rationale**: A regression in `job_start_remote_task()`'s gate would cause a real multi-host MPI job to be double-launched or wrongly launched — testing only the command-string builder one layer down would not catch that.
- **Follow-up**: none — same file, same white-box `#include` technique already precedented by `test_controller_connector.c`.

## Risks & Mitigations
- Note (non-blocking, documentation-only): `apply_job_task_count()`'s clamp never separately validates `mintasks == 0`. Confirmed unreachable from either real live call site (`convert.go`'s and `xce_test`'s `jobsub`'s own independent `mintasks == 0 -> 1` clamps both predate this feature), and even a hand-crafted `--sim` snapshot with a genuinely-zero `mintasks`/`maxTasks` degrades gracefully via `scheduler-allocation`'s own pre-existing `maxTasks <= 1` bail in `parjob_allocator.c` (a job pends as unsatisfiable, no crash). Not a defect; worth a one-line mention in Error Handling for a future reader's benefit, not a required fix.
- ~~Risk: a sixth, undiscovered call site to `parse_resreq()`/`parse_resreq_primary()` exists that this design missed~~ — RESOLVED: independently re-verified twice by separate design-review passes (exhaustive grep each time), no additional caller found beyond the original 5. No longer an open risk, though the implementation task should still re-grep once more immediately before wiring as cheap insurance.
- ~~Risk: driver/simulation divergence in `core_events.c`~~ — RESOLVED during design review: independently confirmed `driver.c` and `sim_loop.c` both reach `core_build_job()` through the same shared `xce_core_push_events()`, no parallel sim-specific job constructor exists. No longer an open risk.
- Risk (discovered during design review, now mitigated): the job-update call sites don't affect scheduling at all, because `job->schedobj` is never rebuilt on update — logged as bug-1773, removed from this spec's implementation scope, Requirement 1.4 reworded and given a structural regression test instead.
- Risk (discovered during design review, now mitigated): initially thought `make_sched_job()`'s ingest path had the same `resreq_fp` gap as bug-1772 (logged as bug-1774) and needed excluding from Requirement 5.4's test — a follow-up review found `make_sched_job()` is dead code entirely (unreachable in production), so it's dropped from implementation scope outright; Requirement 5.4's real exception is bug-1772 (snapshot-restore), corrected accordingly.
- Risk: the new E2E script's two-virtual-worker deployment introduces soak-infrastructure flakiness (per this project's documented zombie-process/port-conflict soak gotchas) — Mitigation: follow the same pre-flight process/port check convention already established for `xce-test-ha`.
- Risk: fixing task-count wiring exposes bug-1768's compound-nested resource-checking gap or bug-1771's fcfs NULL-queue crash under new, previously-unreachable conditions — Mitigation: both remain explicitly out of this spec's boundary; the new tests target standalone primaries and array jobs only, matching Requirement 3.3's scope.

## References
- `.wolf/buglog.json` bug-1770, bug-1768, bug-1769, bug-1771, bug-1772, bug-1773, bug-1774 — prior findings this spec builds on or explicitly defers. bug-1773 and bug-1774 were discovered during this spec's own design review; bug-1772 and bug-1774 both received a trigger/liveness correction during the same review (bug-1774's original "confirmed live" claim retracted; bug-1772's "post-restart or post-failover" phrasing corrected to "the `--sim` simulation/replay driver").
- Minor, non-blocking cleanup noted but out of this spec's file boundary: `core.c:354` has a stale comment referring to `make_sched_job()` as "still-live" — harmless (prose, not code) but inaccurate after this review's dead-code finding; worth fixing whenever that file is next touched.
- `.kiro/specs/scheduler-allocation/` — the dependency spec whose allocator logic this fix makes reachable.
- `.kiro/steering/persistence.md` — "Snapshot Save/Load Field-Completeness" section, cross-checked above.