myxce/.kiro/specs/scheduler-dispatch-commit/research.md

# Research & Design Decisions

## Summary
- **Feature**: `scheduler-dispatch-commit`
- **Discovery Scope**: Extension (existing scheduler + controller subsystems)
- **Key Findings**:
  - `scheduler.c`'s `sched_update()`/`sched_job_state_change()` (the notify-driven state-change apply path) is **dead code in the live production driver** — `main.c`'s loop calls only `xce_sched_driver_run_normal_mode_round()`, which drains pending controller events via `xce_sched_driver_translate_pending_events()` → `xce_core_push_events()` → `core_events.c`'s `XCE_CORE_EVENT_JOB_STATE_CHANGE` case, a parallel, independently-maintained mirror of the same logic. Design must target the live path, not the vestigial one.
  - `builtin_fcfs_on_job_state_change()` is confirmed idempotent for a repeated RUN notification (comment: "a no-op, via `ordq_del`'s 'not found' return, if it wasn't a candidate already — e.g. two non-pending transitions in a row"). A later, now-redundant `schedStartJob` confirmation re-applying `JOB_STAT_RUN` to an already-RUN job is therefore safe with zero code change to that path.
  - `sched_commit_decision()`'s DISPATCH branch (`scheduler.c:1441-1447`) is the correct, still-live integration point for the scheduler-side transition — it is the sole call site of `sched_charge_account()`/`job->accounts` assignment already, and is invoked from the decision callback (`xce_sched_driver_on_decision()`), a separate (and still-live) mechanism from the notify-apply path above.
  - `dispatch_a_job_()` (`job.c:831-963`) currently sets `j->job.allocs` **unconditionally**, after attempting `xce_startjob_req()`, regardless of that call's return code — an existing, unrelated latent bug (stale allocation left behind on RPC failure) that Requirement 4's cleanup obligation subsumes rather than leaving as a separate patch.
  - The `limit` plugin (`plugins/limit.c`) does not gate any of its own usage accounting on `JOB_IS_RUN`/`job->state` (`grep` found zero matches) — it is not a second instance of bug-1624's class of bug, and is correctly out of this spec's boundary.
  - `commit_resolve_dispatch_t`/`commit_resolve_dispatch_entry` (`commit.c`) is the generic WAL-commit continuation dispatch primitive from `raft-apply-decoupling` — a naming coincidence ("dispatch" = vtask dispatch, not job dispatch), not a pre-built mechanism for this feature. The relevant precedent instead is `record_job_fail_fact()`'s "durable append only, no synchronous job mutation, apply-time recompute" pattern.

## Research Log

### Which driver code path actually applies a `JOB_STATE_CHANGE` in production
- **Context**: Requirement 1 requires the scheduler's dispatch-commit transition to interoperate correctly with the *existing* RUN-confirmation transition. Needed to find where that existing transition is actually applied today, not just where an equivalently-named function exists.
- **Sources Consulted**: `src/scheduler/src/main.c` (production loop), `src/scheduler/src/driver.c` (`xce_sched_driver_run_normal_mode_round`, `xce_sched_driver_translate_pending_events`), `src/scheduler/src/scheduler.c` (`sched_update`, `sched_job_state_change`), `src/scheduler/src/core_events.c` (`XCE_CORE_EVENT_JOB_STATE_CHANGE` case).
- **Findings**: `main.c`'s `while(1)` loop calls only `xce_sched_driver_run_normal_mode_round(&sched)`, whose body never calls `sched_update()`. `sched_update()` (and therefore `sched_job_state_change()`, only reachable through it) has no live caller in the current driver architecture. The actual apply path is `xce_sched_driver_translate_pending_events()` → `xce_core_push_events()` → `core_events.c`'s `XCE_CORE_EVENT_JOB_STATE_CHANGE` case, documented there as mirroring `sched_job_state_change()` "logic-identical, signature-relocated."
- **Implications**: The design does not modify or reuse `scheduler.c`'s `sched_job_state_change()`/`sched_update()`. All discussion of "the existing RUN-confirmation path" in the design refers to `core_events.c`'s case, which needs **no code change** — it already behaves correctly (idempotently) when applied to a job whose state is already RUN.

### Ordering of the controller-side state transition relative to the worker RPC
- **Context**: Requirement 2 AC1 (as drafted) says the transition happens "before attempting to notify the assigned worker." Tracing `dispatch_a_job_()`'s actual failure points revealed a simpler alternative.
- **Findings**: `dispatch_a_job_()` has five failure points that return before ever attempting `xce_startjob_req()` (jobid lookup, invalid alloc, worker lookup, worker-not-ready, queue-exec-info lookup) and one failure point at the RPC call itself. If the state transition (and `job.allocs` commitment) is gated on `xce_startjob_req()` returning success, none of these six failure modes ever produces a RUN transition that needs to be reversed — the job simply remains PEND, exactly where it already was. This eliminates an entire class of "revert RUN back to PEND" complexity on the controller side that would otherwise be needed to match Requirement 4.
- **Implications**: Design decision below (**Gate controller commit on RPC outcome**) supersedes the literal "before attempting to notify" phrasing in requirements.md AC1 — the *intent* (transition and allocation commit as one atomic, worker-latency-independent operation, not gated on separate confirmation) is preserved; only the specific ordering relative to the RPC changes, and only because it removes a whole reversal path rather than adding one. Flagged for requirements.md wording cleanup during task generation, not a scope change.

## Design Decisions

### Decision: Gate the controller's dispatch-time RUN transition and allocation commit on RPC success, not on decision receipt
- **Context**: Where exactly within `dispatch_a_job_()` to place the state transition, and whether dispatch-RPC failure needs a state-reversal path.
- **Alternatives Considered**:
  1. Transition to RUN immediately on receiving the decision, before any worker contact — matches the literal AC1 wording, but requires a full reversal path (RUN → PEND) for every one of `dispatch_a_job_()`'s six failure points.
  2. Transition to RUN (and commit `job.allocs`) only after `xce_startjob_req()` returns success — no reversal path needed for any failure point; the job simply never leaves PEND on failure.
- **Selected Approach**: Option 2.
- **Rationale**: Every one of `dispatch_a_job_()`'s failure points already returns before or at the RPC call. Gating on RPC success collapses "was RUN, must revert" into "never became RUN," which is strictly simpler and removes a whole category of possible bugs (a reversal that's forgotten on one of the five early-return paths).
- **Trade-offs**: A job dispatched to a worker that never replies at all (hangs, rather than erroring) would not reach RUN until the RPC layer's own timeout fires — acceptable, since that is already how `job.allocs` commitment behaves today, and no requirement asks for a stronger guarantee under a hung RPC.
- **Follow-up**: None — this closes the finding above rather than opening a new question.

### Decision: Reuse `mark_job_fail()`/WAL-fact-append pattern for durability of the new transition, rather than the deferred-apply-first pattern `record_job_start_fact()` uses
- **Context**: Requirement 2 AC4 requires the transition to survive a controller failover. `record_job_start_fact()` uses a "durable append only, no synchronous mutation, recompute at apply time" pattern specifically because a JOB_START fact can race against other pending facts for the same job (merge-at-apply-time, per `wal_types.h`). The dispatch-commit transition has no such racing-fact concern: it is the *first* transition after a job leaves PEND, triggered synchronously by the one function that already holds the authoritative decision.
- **Alternatives Considered**:
  1. Full deferred-apply (submit fact, mutate only when the WAL entry is later applied) — correct but makes the transition NOT synchronously observable, conflicting with Requirement 2 AC1/AC3's "before/from" ordering language.
  2. Synchronous in-memory mutation (matching today's `j->job.allocs` assignment) plus a separate, best-effort durable WAL fact append recording the same transition for recovery purposes.
- **Selected Approach**: Option 2.
- **Rationale**: Matches this function's own existing precedent for `job.allocs` (synchronous, not deferred-apply) while still giving the transition a durable record for post-failover recovery, without introducing a same-job merge-ordering hazard `record_job_start_fact()`'s pattern exists to solve elsewhere.
- **Trade-offs**: A crash in the narrow window after the in-memory mutation but before the WAL fact durably commits would leave the pre-crash primary's own view momentarily ahead of what a promoted standby recovers to. Bounded by the existing commit-durability guarantee (`ha-commit-durability`) once the fact is submitted; the narrow pre-submission window is the same class of gap `job.allocs`'s own non-durable commit already accepts today, not a new one this feature introduces.
- **Follow-up**: Design phase specifies the new WAL fact's contract (extends `wal_jobstate_t`'s existing `_pad`-discriminated shape, precedent: `WAL_JOB_START`/`WAL_JOB_FAIL`/`WAL_JOB_UPDATE`). Exact bit layout is implementation-task detail.

### Decision: Controller classifies once, authoritatively; `jobDispatchFail` becomes exclusively the retry signal, reusing the existing `schedFailJob` path for terminal resolution
- **Context**: Requirement 4 requires the same failure to resolve the same way (retry vs. terminal) regardless of which component acts on it. The controller observes a dispatch failure synchronously and directly (it makes the RPC); the scheduler only learns of it later, asynchronously.
- **Alternatives Considered**:
  1. Each side classifies independently against a shared error-code table — risks drift if the bounded-retry counter (Requirement 4 AC3) is tracked separately on each side.
  2. Controller classifies once (owns the freshest information and the durable retry counter) and sends the scheduler an explicit resolution field alongside the existing `jobDispatchFail` notify.
  3. Controller classifies once, and routes the two outcomes through two *already-distinct* existing channels instead of one overloaded one: terminal failures call the existing `mark_job_fail()` (`job.c:1844`), whose already-wired `HOOK_DETAIL_JOB_FAIL` → `sched.c` hook handler already sends `NotifyData_data_schedFailJob` — a notify the scheduler's `sched_handle_notify()` **already handles today**, unmodified, correctly freeing `job->accounts`/`job->allocs` on the transition away from RUN. Retryable failures keep using the existing (currently unhandled) `notify_dispatch()`/`NotifyData_data_jobDispatchFail`, whose meaning narrows to exclusively "return this job to pending."
- **Selected Approach**: Option 3.
- **Rationale**: Discovered while tracing `mark_job_fail()`'s hook chain (`job.c` → `HOOK_DETAIL_JOB_FAIL` → `sched.c:502` → `NotifyData_data_schedFailJob`) — the terminal path already has a fully-working, already-scheduler-handled notification mechanism with zero new code required. This reduces the scheduler-side new-code surface (bug-1625's actual fix) to a single new `case` in `sched_handle_notify()`'s switch that unconditionally applies `JOB_STAT_PEND` — no resolution field, no classification logic, no new payload shape needed on the scheduler side at all.
- **Trade-offs**: None identified — this is strictly less new surface than Option 2 for an equivalent guarantee.
- **Follow-up**: Design phase specifies exactly where the controller-side classification (including the bounded-retry check) is computed and where `dispatch_fail_count` is stored/reset.

## Addendum (2026-08-23, found during `/kiro-validate-design`)

- **`dispatch_a_job_()` and `job_ctrl()`'s existing call to `mark_job_fail()` never take `RTTI_LOCK_WRITE` on the job object, despite `apply_job_fail_transition()`'s own doc comment requiring it** (a genuine `pthread` rwlock, `my_rwlock_wlock()`, confirmed in `src/lib/jobj.c:708-725` — not a liveness/reference guard). Traced directly: `dispatch_a_job_()` (`job.c:831-963`) takes only `RTTI_LOCK_READ` on the containing maps and the worker, never on the job itself; `job_ctrl()` releases its own `RTTI_LOCK_READ` on the job (`job.c:1016`) well before calling `mark_job_fail()` (`job.c:1083`). Logged as bug-1626, pre-existing, independent of this feature. This directly affected the "no staleness window" locking claim originally drafted for design.md's Controller Dispatch Commit component — corrected there to explicitly acquire `RTTI_LOCK_WRITE` around this feature's own new/restructured mutations, closing the gap for those call sites specifically (`job_ctrl()`'s own separate call site is left as the still-open, out-of-boundary instance of bug-1626).
- **The scheduler-side `dispatch_fail_count` field originally planned for `xce_sched_job_t` was removed** — nothing in the design ever reads it (only the controller's copy drives classification, per the Decision above), so it was dead weight. Caught during the same validation pass.
- **`notify_dispatch()` (`job.c:1108-1125`) unconditionally emits `NotifyData_data_jobDispatchFail` regardless of whether `code` is 0 (success) or negative (failure) — confirmed by reading its body.** `handle_jobdispatch()`'s existing loop calls it for *every* `dispatch_a_job_()` return, success or failure alike. Combined with classification originally being planned for `handle_jobdispatch()` (the caller), this would have produced two conflicting notifies for a terminal failure — `jobDispatchFail` (apply PEND) racing against `schedFailJob` (apply EXIT) for the same event. Fixed by moving classification and all dispatch-outcome notification ownership fully inside `dispatch_a_job_()` (which also resolves the redundant-lookup/TOCTOU concern a caller-side classification would have had, since `handle_jobdispatch()` only retains `alloc->jobid` after `dispatch_a_job_()` returns) and removing `handle_jobdispatch()`'s own `notify_dispatch()` calls entirely. Found during round 2 of `/kiro-validate-design`.
- **`persist.c` has TWO separate functions that each need their own new `WAL_JOB_DISPATCH` case, not one** — `apply_wal_entry()` (`persist.c:3375+`, recovery/replay-from-disk path) and `apply_committed_entry()` (`persist.c:3570+`, live-apply-on-commit path, the one `raft-apply-decoupling` introduced `_pad`-discriminated routing for) each have their own independent fall-through group for simple absolute-value job-state facts (`persist.c:3452-3466` and `persist.c:3700-3706` respectively). An earlier draft of design.md pointed only at the first (recovery) group, which alone would satisfy replay-after-restart but not live application when the WAL entry actually commits during normal operation — the primary case Requirement 2.4 is actually about. Found by tracing both functions directly rather than assuming a single switch statement.
- **`dispatch_a_job_()` leaks one `reference(j)` on exactly its three retryable-class failure paths** (workermgr lookup failure, worker not found, worker not ready) — `reference(j)` is taken once at entry (`job.c:854`) but never released with a matching `dereference(j)` on these three returns, confirmed by direct comparison against the two terminal-class failure paths (`nc <= 0`, `get_queue_exec()` failure), both of which already correctly `dereference(j)`. Logged as bug-1627, pre-existing. Since this design already restructures exactly these three call sites (to add the retryable-path lock/counter/notify sequence), the missing `dereference(j)` is added there too — closed as a side effect of the restructuring already underway, not a separate patch. Found during round 3 of `/kiro-validate-design`.

## Risks & Mitigations
- **Risk**: A dispatched-then-terminally-failed job's resource-allocation cleanup on the scheduler side must run through the same code that already frees `job->accounts`/`job->allocs` on any transition away from RUN (`core_events.c`'s `XCE_CORE_EVENT_JOB_STATE_CHANGE` case) — if the new dispatch-failure handling bypasses this and frees ad hoc, it risks a double-free or a leak. Mitigation: route dispatch-failure resolution through the existing `XCE_SCHED_OP_JSTATE_CHANGE` → `XCE_CORE_EVENT_JOB_STATE_CHANGE` machinery (same enqueue path `schedStartJob`/`schedExitJob`/`schedFailJob` already use), not a new, parallel mutation path.
- **Risk**: The bounded-retry counter (Requirement 4 AC3) is new per-job state with no existing analog — if not reset on a fresh successful dispatch, a job that failed twice, succeeded, ran to completion, and was later resubmitted with the same in-memory job object (should not happen given job objects are per-jobid, but worth stating) could inherit a stale count. Mitigation: reset the counter in the same operation that commits a fresh dispatch decision (`sched_commit_decision()`'s DISPATCH branch), not only on success at the end of a retry sequence.
- **Risk**: `--sim`/simulation mode does not call `sched_commit_decision()` at all (confirmed in the sibling `scheduler-sim-eager-reconciliation` investigation) — no risk of driver divergence from this feature's scheduler-side change, but worth stating explicitly since `scheduler-driver-equivalence.md` is a hard project invariant. Mitigation: none needed; documented as a boundary note in requirements.md and design.md rather than a code change.

## References
- `.kiro/steering/scheduler-driver-equivalence.md` — state-equivalence invariant between normal-mode and simulation-mode drivers; confirms this feature's scheduler-side change has no simulation-mode counterpart to keep in sync (see Risks).
- `.kiro/specs/scheduler-sim-decision-parity/design.md` — house style precedent for C-domain adaptation of the design template (Components as C modules/functions, Service Interface blocks as real C signatures with pre/post/invariants).
- `src/include/wal_types.h` — `wal_jobstate_t`'s `_pad`-discriminated shape, the precedent this feature's new WAL fact type follows.