diff --git a/.kiro/specs/raft-apply-decoupling/design.md b/.kiro/specs/raft-apply-decoupling/design.md new file mode 100644 index 00000000..ad630aa5 --- /dev/null +++ b/.kiro/specs/raft-apply-decoupling/design.md @@ -0,0 +1,447 @@ +# Design Document: raft-apply-decoupling + +## Overview + +**Purpose**: This feature gives the XCE controller one canonical, ordered path from a client/worker request to committed, applied cluster state — append → majority-commit → apply → notify — replacing today's mix of an already-partially-fixed command path (bug-1306) and a still-fully-optimistic fact path (worker-reported job start/finish/fail/rusage, applied and reacted to on receipt, ahead of commit). + +**Users**: Cluster operators (who need failover to never lose or fabricate job state) and maintainers of the controller's submit/commit/replay code (who need one apply seam instead of two divergent ones). + +**Impact**: Changes the commit engine's `COMMIT_MODE_WAIT` path from a blocking wait to an event-driven resolve; introduces a single `apply_committed_entry()` seam invoked by both live commit-advance and restart replay; moves worker-fact state mutation and reactions from receipt-time to post-commit-time; adds a post-replay CLEAN/DIVERGENT verification. + +### Goals +- One explicit raft step (append + majority commit) per state-changing operation, invoked directly by its handler — not incidentally from a hook. +- One apply seam, used identically by live commit-advance and restart replay, applied strictly in commit order. +- Commands and facts converge on that one apply seam; only their ingress and commit-wait policy differ. +- A quorum-loss window degrades safely by construction (retained-but-unapplied), with no special-case "local apply" code path. +- A restart proves its own correctness (CLEAN/DIVERGENT) instead of only checking WAL structural integrity. +- No regression to any HA correctness or throughput property already proven by prior specs. + +### Non-Goals +- Rewriting the raft library's protocol core (`raft_advance_commit`, `on_commit_advanced`, etc.) — consumed as-is. +- Collapsing a job array into a single WAL record (`job-array-single-wal-record`). +- Moving snapshot send / config-push / peer-connect off the iomgr/delaymgr threads (`raft-drive-async`, already done separately). +- Building `worker-rejoin-resync` — this design depends on it as a future backstop, consistent with every sibling spec that already accepted this gap. +- Entity-bearing snapshot content or read-consistency/leader-lease work. + +## Boundary Commitments + +### This Spec Owns +- The explicit raft-commit step for command entries and its call sites (already partially landed for job submit via bug-1306; this spec completes the ordering and generalizes the mechanism). +- The commit engine's `COMMIT_MODE_WAIT` continuation/resolve mechanism (the keystone conversion away from blocking wait). +- The single `apply_committed_entry()` seam and its dispatch table, and the "apply pump" that drives it off commit-advance in strict LSN order. +- The fact-ingestion split (durable-append-only at receipt vs. mutation+reaction at apply). +- The post-replay CLEAN/DIVERGENT verification and its non-promotable fence. +- Conformance/regression assertions for apply-ordering, idempotency, and no-reaction-before-commit. + +### Out of Boundary +- The raft library's callback contracts (`on_persist_entry`, `on_get_entry_term`, `on_send_append_entries`, `on_truncate_from`, `on_commit_advanced`, `raft_advance_commit`) — consumed unchanged. +- Transport/timer-thread blocking (snapshot send, config-push wait, peer connect) — `raft-drive-async`'s domain; this design only *consumes* its `vtask_enq_classified` / slow-lane primitives. +- Worker join/rejoin protocol changes — `worker-join-refactor` / `worker-rejoin-resync`. +- Entity-bearing snapshot format/content — `ha-entity-snapshots` / `config-upgrade-snapshot`. This design's replay-side changes must remain compatible with `ha-entity-snapshots`' restore-before-advance `load_snapshot` path, which already shares `restore_job`/`restore_queue_state` with WAL replay (see Architecture). +- Removing `job_submit_committed()` (egress.c:47) — becomes a harmless tautology once this design lands, but its removal is a documented follow-up, not in scope (research.md, Design Decisions). + +### Allowed Dependencies +- `ha-commit-engine` / `ha-commit-barrier-rollout`: majority-commit and idempotency primitives, consumed as-is. +- `ha-term-safety` (complete): `on_commit_advanced` / `raft_advance_commit` wiring on the ACK path (advisory `raft_commit_index`, Option A) — this design's apply pump is a **new** consumer of the existing durable-majority commit advance; it does not change how `committed_lsn` itself advances. +- `raft-drive-async` (complete): `vtask_enq_classified` + slow-lane executor + `VTASK_LIVENESS`/`VTASK_LIVE_BLOCK`/`VTASK_REQ_BLOCK` admission classes — reused for dispatching resolved-commit continuations off the ACK-processing thread. +- `raft-sim-conformance`: existing deterministic-simulator harness, extended (not replaced) with new assertions. + +### Revalidation Triggers +- Any change to the commit engine's per-LSN structure (`commit_engine_t`, `ce->...`) or its locking discipline (`p->lock` vs `ce->lock` ordering, persist.c:3347-3350). +- Any change to `WAL_MODE_WAIT`/`WAL_MODE_RETAIN` classification (`wal_classify`, persist.c:3218) — the apply pump's per-type behavior depends on this classification being exhaustive. +- Any new WAL entry type — must be added to the `apply_committed_entry()` dispatch table or it silently fails to apply. +- `worker-rejoin-resync` landing — must re-verify the fact-apply idempotency guards against its replay/resync ordering (shared `mark_job_*` guards). +- `job-array-single-wal-record` landing — builds its array-expand step directly on `apply_committed_entry()`; a signature change here is a breaking change there. + +## Architecture + +### Existing Architecture Analysis + +Current state, verified against the live codebase (not the 9-day-old brief — see `research.md`): + +| Aspect | Current state | Gap vs. this design | +|---|---|---| +| Job-submit raft step | Already extracted from the hook fan-out into `persist_submit_wal_and_commit()`, called directly by `handle_jobsubmit` (job.c:626, bug-1306) | Requirement 1 done for submit. No other command type (config change) has this yet. | +| Job-submit apply ordering | `create_job()` (job.c:583) runs **before** the commit barrier; on commit failure the created job is **not** rolled back | Requirement 2 violated today — a failed/timed-out submit leaves a phantom job in `c->jobmgr` | +| Commit-wait drive | `repl_wait_majority_ack` → `commit_submit(..., COMMIT_MODE_WAIT, ...)` → blocking `pthread_cond_timedwait` (commit.c:409), pooled worker, 500ms default | Requirement 9's keystone: still fully blocking | +| Online vs. replay apply (submit) | Replay uses `restore_job()` (idempotent, LSN-tagged); online uses raw `create_job()` directly | Requirement 3: half-unified — replay already has the reusable primitive, online doesn't use it | +| Online vs. replay apply (facts) | Online uses guarded `mark_job_start/finish/fail/update()` (monotonic-rank checks); replay uses raw `restore_job_state()` (**no** guard) | Requirement 3/7: genuinely divergent — replay can regress state that online would reject | +| Fact mutation timing | `mark_job_*` mutates `job->job.state` synchronously at receipt, **before** the (already-async) WAL append | Requirement 5/6: fully unaddressed — no apply-after-commit exists for facts at all | +| Hook fan-out | `AFTER_EVENT` for job submit is already notification-only (dead WAIT branch, persist.c:3466); facts' `AFTER_EVENT` still triggers the WAL append itself | Requirement 4: partially true for submit, not for facts | +| Restart verification | CRC-chain + entry-CRC only (`replay_wal`, persist.c:2667-2723); no materialized-state-vs-log-tip check | Requirement 8: fully unaddressed | +| Async task submission | `vtask_enq_classified` + slow-lane executor already exist (`raft-drive-async`, complete) | Requirement 9: reusable as-is, no new primitive needed | + +### Architecture Pattern & Boundary Map + +```mermaid +graph TB + Client[Client jobsubmit] + Worker[Worker fact report] + Handler[Command handler] + FactRecv[Fact receipt handler] + RaftStep[Explicit raft step] + CommitEngine[Commit engine resolve] + ApplyPump[Apply pump LSN ordered] + ApplySeam[apply_committed_entry] + Replay[Restart replay] + Hooks[After hooks notify only] + Sched[Scheduler dispatch] + Reply[Client reply] + + Client --> Handler + Handler --> RaftStep + RaftStep --> CommitEngine + Worker --> FactRecv + FactRecv -->|durable append only| RaftStep + CommitEngine -->|resolve on majority| ApplyPump + Replay -->|same seam| ApplySeam + ApplyPump --> ApplySeam + ApplySeam --> Hooks + Hooks --> Sched + Hooks --> Reply + CommitEngine -.->|resolve failure or timeout| Reply +``` + +**Architecture Integration**: +- **Selected pattern**: Commit-then-apply pipeline with a single apply seam, mirroring the pattern already used for the raft library's own log/commit split (`raft-log-replication` owns log, `raft-commit-safety` owns commit-index, controller owns apply) — this design is the controller finishing that same separation one layer up, for its own state machine. +- **Domain/feature boundaries**: the raft library and commit engine remain unaware of "jobs"; `apply_committed_entry()` is the only place WAL-entry semantics meet job/queue/worker state. +- **Existing patterns preserved**: the commit engine's per-LSN registry, `wal_classify`'s WAIT/RETAIN table, the hook system's BEFORE/AFTER shape, `restore_job`/`mark_job_*`'s existing idempotency logic — all reused, not replaced. +- **New components rationale**: a continuation-registration API on the commit engine (replaces blocking wait); an LSN-ordered apply pump (didn't exist — nothing today reacts to RETAIN commit); a post-replay verification step. +- **Steering compliance**: preserves persist-before-ACK durability and the synchronous-commit-as-policy invariant (`ha-async-io.md`); the raft step continues to run off the iomgr thread; a divergent-verdict fence reuses the existing "fail loud, stay alive" mechanism (`persist_recover_loud_fence_if_unrecoverable`), never introducing a new terminal path. + +### Implementation Sequencing + +This design has a hard build order, not just a hard conceptual one: + +1. **Commit Engine Resolve** (the keystone) must land first — every other component either registers a continuation with it (Raft Step, Fact Ingestion's future apply trigger) or depends on the apply pump it drives. +2. **Apply Seam** (`apply_committed_entry`) depends on (1)'s apply pump existing to call it live, but its dispatch logic and the `restore_job`/guarded-`mark_job_*` extraction can be built and unit-tested in parallel with (1). +3. **Raft Step (Command)** ordering fix and **Fact Ingestion**'s receipt/apply split both depend on (1) and (2) being in place; they touch disjoint call sites (`handle_jobsubmit` vs. `on_job_start/finish/fail/update`) and can proceed in parallel with each other once (1)-(2) land. +4. **Restart Replay & Verification** depends on (2) (`apply_committed_entry` must exist before replay can be pointed at it) but is otherwise independent of (3). +5. **Conformance & Regression** depends on all of the above being functionally complete — it verifies the assembled behavior, not any single component in isolation. + +### Technology Stack + +| Layer | Choice / Version | Role in Feature | Notes | +|-------|------------------|------------------|-------| +| Controller core | C (gnu17), existing | All changes land in `src/controller/{job.c,persist.c,commit.c,repl.c,ha.c}` | No new language/runtime | +| Async dispatch | `vtaskmgr.c` `vtask_enq_classified` (raft-drive-async) | Dispatches resolved-commit continuations off the ACK-processing path | Reused, not new | +| Serialization | Cap'n Proto (existing `controller.capnp`) | WAL entry payload encode/decode, unchanged | No schema change | +| Test harness | `raft_sim.c` (raft-sim-conformance), `xcelib_test`/`controller_unit_test`, `/xce-test-ha` | Apply-ordering assertions, idempotency assertions, live soak | Extends existing harnesses | + +## File Structure Plan + +### Modified Files +- `src/controller/commit.c` — Replace the `COMMIT_MODE_WAIT` blocking-wait branch with a continuation-registration + `commit_resolve()` mechanism; `commit_advance_locked()` walks newly-decided LSNs and calls `commit_resolve()` for each instead of `cond_broadcast`-ing a single waiter set. `COMMIT_MODE_RETAIN` registration is unchanged (already non-blocking). +- `src/controller/repl.c` — `repl_wait_majority_ack()` becomes `repl_submit_majority_commit()`: registers a continuation instead of blocking; retains its current callers' error-code contract (same negative-rc vocabulary) for the continuation's resolved outcome. +- `src/controller/persist.c` — Add `apply_committed_entry()` as the single dispatch seam (extracted from today's `apply_wal_entry()` switch body, persist.c:2452-2562, plus the fact-transition guard fix). `apply_wal_entry()` (replay) and the new apply pump both call it. Add the post-replay CLEAN/DIVERGENT verification at the end of `persist_recover()`/`replay_wal()`. `persist_job_hook_fun`'s dead `WAL_MODE_WAIT` branch is removed (no longer needed once `repl_submit_majority_commit` fully replaces the old call convention). `wal_hook_append_classified()` (fact receipt path) stops mutating job state — it becomes durable-append-only. +- `src/controller/job.c` — `handle_jobsubmit()`: defer `create_job()`/job-object construction to fire from `apply_committed_entry()` after commit succeeds, not before. `mark_job_start/finish/fail/update()`: split into a guard+mutation core (callable from apply) and a receipt-time validation-only entry point; their `AFTER_EVENT` hook-firing relocates to fire from the apply step. `restore_job_state()` is retired in favor of routing replay through the same guarded core. +- `src/controller/ha.c` — Wire the apply pump: on `on_commit_advanced` (already-existing callback, ha.c:1776) and on the existing durable-majority `committed_lsn` advance, walk `[last_applied_lsn+1 .. committed_lsn]` in order and invoke `apply_committed_entry()` for each, dispatched via `vtask_enq_classified`. +- `src/controller/egress.c` — No functional change; `job_submit_committed()` (egress.c:47) is left in place as a now-tautological defensive predicate (see research.md). + +### New Test Files +- `src/controller/testsuite/test_apply_seam.c` — unit tests: apply-after-commit ordering, no-mutation-on-failed-commit, idempotent re-apply, strict LSN ordering. +- `src/controller/testsuite/test_commit_resolve.c` — unit tests for the continuation/resolve conversion: exactly-once resolution, resolve on timeout/demotion/truncation, admission behavior under concurrent in-flight commits. +- Extend `src/lib/raft/raft_sim.c` (or its conformance test driver) with apply-ordering and idempotent-re-apply assertions (Requirement 10.1-10.3). +- Extend the `/xce-test-ha` soak scenario set with a kill-mid-fact-stream case (Requirement 10.4). + +## System Flows + +### Command submission (target ordering) + +```mermaid +sequenceDiagram + participant C as Client + participant H as handle_jobsubmit + participant R as Raft step + participant CE as Commit engine + participant AP as Apply pump + participant AS as apply_committed_entry + participant N as After hooks notify + + C->>H: job submit request + H->>H: BEFORE_EVENT validate + H->>R: encode and append WAL_JOB_SUBMIT + R->>CE: register continuation for LSN + CE-->>CE: majority ack or timeout or demotion + CE->>AP: resolve LSN outcome + alt committed + AP->>AS: apply_committed_entry creates job + AS->>N: AFTER_EVENT notify scheduler apiserver + AP->>H: reply success + else failed or timed out + AP->>H: reply failure, no apply + end +``` + +Key decision not obvious from the diagram: the job object itself does not exist in `c->jobmgr` until the committed branch runs — there is no rollback needed because there is nothing to roll back (Requirement 2.3 by construction, not by compensating action). + +### Worker fact ingestion (target ordering) + +```mermaid +sequenceDiagram + participant W as Worker + participant F as Fact receipt handler + participant WAL as Durable append RETAIN + participant CE as Commit engine + participant AP as Apply pump + participant AS as apply_committed_entry + participant N as Reactions slot free dispatch notify + + W->>F: job start or finish report + F->>F: BEFORE_EVENT validate only + F->>WAL: append entry, return immediately + WAL->>CE: register RETAIN LSN + Note over CE: majority may be delayed by quorum loss; entry stays pending, never applied + CE->>AP: resolve on eventual majority commit, in original order + AP->>AS: apply_committed_entry mutates job state guarded + AS->>N: fire reactions post commit +``` + +Key decision: quorum-loss safety (Requirement 6) requires no special-case code — the apply pump only ever walks committed LSNs, so an unreachable majority simply means the pump has nothing new to walk. Retained facts commit and apply in original order automatically once majority is restored, because the commit engine's `committed_lsn` itself only ever advances as a contiguous prefix. + +### Restart replay and verification + +```mermaid +flowchart LR + Start[persist_recover] --> CRC[CRC chain check] + CRC -->|corrupt| Truncated[RECOVER_TRUNCATED] + CRC -->|clean| Replay[replay_wal applies each entry via apply_committed_entry] + Replay --> Verify[Compare materialized state vs committed log tip] + Verify -->|match| Clean[CLEAN verdict] + Verify -->|mismatch| Divergent[DIVERGENT verdict] + Divergent --> Fence[Non promotable fence, stays alive, retries] +``` + +## Requirements Traceability + +| Requirement | Summary | Components | Flows | +|---|---|---|---| +| 1.1-1.4 | Extract raft step from hook fan-out | Raft Step (job.c, persist.c) | Command submission | +| 2.1-2.3 | Apply-after-commit ordering for commands | Apply Pump, apply_committed_entry | Command submission | +| 3.1-3.4 | Single shared apply seam, online = replay | apply_committed_entry, Restart Replay | Command submission, Restart replay | +| 4.1-4.3 | Hooks demoted to notification-only | Notification (hook.c usage) | Command submission | +| 5.1-5.3 | Facts apply-after-commit | Fact Ingestion, apply_committed_entry | Fact ingestion | +| 6.1-6.3 | Quorum-loss retained-fact behavior | Apply Pump (commit-gated by construction) | Fact ingestion | +| 7.1-7.3 | Idempotent, ordered re-application | apply_committed_entry (guarded transitions) | Command submission, Fact ingestion, Restart replay | +| 8.1-8.4 | Restart apply-correctness verification | Restart Replay & Verification | Restart replay | +| 9.1-9.4 | Bounded resource behavior under concurrent commits | Commit Engine (continuation/resolve) | Command submission | +| 10.1-10.4 | Regression/conformance coverage | Conformance (raft_sim.c), soak suite | (cross-cutting) | +| 11.1-11.4 | Preserve existing behavior and performance | All components (verification gate) | (cross-cutting) | + +## Components and Interfaces + +| Component | Domain/Layer | Intent | Req Coverage | Key Dependencies (P0/P1) | Contracts | +|---|---|---|---|---|---| +| Commit Engine Resolve | commit.c/repl.c | Convert `COMMIT_MODE_WAIT` from blocking wait to continuation/resolve | 1.3, 1.4, 9.1-9.4 | vtaskmgr `vtask_enq_classified` (P0) | Service | +| Apply Seam | persist.c | Single dispatch point mutating state from a committed entry | 2, 3, 5.2, 7 | restore_job/mark_job_* guards (P0) | Service | +| Raft Step (Command) | job.c/persist.c | Explicit append+commit invoked by command handlers | 1.1, 1.2, 2.1 | Commit Engine Resolve (P0) | Service | +| Fact Ingestion | job.c/persist.c | Durable-append-only at receipt; defers mutation/reaction | 5, 6 | Apply Seam (P0) | Service | +| Restart Replay & Verification | persist.c | Replay via Apply Seam; post-replay CLEAN/DIVERGENT check | 3.1-3.2, 8 | Apply Seam (P0), existing loud-fence mechanism (P0) | Service | +| Notification (hooks) | hook.c usage in job.c/persist.c | AFTER_EVENT fires only post-apply, pure notify | 4 | Apply Seam (P0) | Event | +| Conformance & Regression | raft_sim.c, testsuite, `/xce-test-ha` | Assert ordering/idempotency invariants | 10, 11 | Apply Seam, Commit Engine Resolve (P1) | Batch | + +### Commit Engine + +#### Commit Engine Resolve + +| Field | Detail | +|-------|--------| +| Intent | Replace the blocking majority-commit wait with continuation registration and event-driven resolution | +| Requirements | 1.3, 1.4, 9.1, 9.2, 9.3, 9.4 | + +**Responsibilities & Constraints** +- Owns the per-LSN commit registry's WAIT-mode bookkeeping (already exists; behavior at resolution changes). +- Must resolve every registered LSN's outcome exactly once, across `{majority-ack, timeout, demotion, truncation}` (Req 9.4). +- Must not block the thread that calls `commit_advance_locked()` (the replication-ACK path) — dispatch continuations via `vtask_enq_classified`, never inline. +- Preserves the existing lock-ordering rule: never hold `p->lock` across `ce->lock` (persist.c:3347-3350) — the continuation dispatch happens after `ce->lock` is released. + +**Dependencies** +- Outbound: `vtaskmgr.c` `vtask_enq_classified` — dispatch a resolved continuation (Criticality P0) +- Inbound: `repl_submit_majority_commit()` (repl.c, replaces `repl_wait_majority_ack`) — registers a continuation (Criticality P0) + +**Contracts**: Service [x] + +##### Service Interface +```c +typedef enum { COMMIT_OUTCOME_COMMITTED, COMMIT_OUTCOME_FAILED } commit_outcome_t; + +typedef void (*commit_continuation_fn)(void* ctx, uint64_t lsn, + commit_outcome_t outcome, int rc); + +/* Registers a continuation for lsn under COMMIT_MODE_WAIT. Returns 0 on + * successful registration, negative on admission rejection (mirrors today's + * -XCE_ERR_NOT_READY). Never blocks. */ +int commit_submit_async(commit_engine_t* ce, uint64_t lsn, + commit_continuation_fn fn, void* ctx); + +/* Called from commit_advance_locked() (majority path) and from the existing + * timeout/demotion/truncation handling (failure path). Dispatches the + * registered continuation, if any, via vtask_enq_classified. Idempotent: + * a second resolve() for an already-resolved lsn is a no-op. */ +void commit_resolve(commit_engine_t* ce, uint64_t lsn, + commit_outcome_t outcome, int rc); +``` +- Preconditions: `lsn` was previously appended locally. +- Postconditions: exactly one of `{COMMITTED, FAILED}` is delivered to the registered continuation, exactly once. +- Invariants: resolution order across distinct LSNs is not required to be FIFO by this component alone — LSN application order is enforced by the Apply Seam's pump, not by resolve dispatch order. + +### Apply Seam + +#### apply_committed_entry + +| Field | Detail | +|-------|--------| +| Intent | The single function that mutates in-memory state for one committed WAL entry, used by both live apply and replay | +| Requirements | 2.2, 2.3, 3.1, 3.2, 3.3, 3.4, 5.2, 7.1, 7.2, 7.3 | + +**Responsibilities & Constraints** +- Dispatches on WAL entry type: `WAL_JOB_SUBMIT` → job construction via the existing `restore_job()` primitive (already idempotent, already LSN-tagged); `WAL_JOB_{START,FINISH,FAIL,UPDATE,...}` → the guarded transition core factored out of `mark_job_start/finish/fail/update()`; `WAL_QUEUE_CTRL` → `restore_queue_state()`; `WAL_WORKER_CTRL` → `worker_from_capnp()`. +- Fires the type's `AFTER_EVENT` hook after the mutation completes, never before. +- Must be safe to invoke more than once for the same entry (replay re-running an already-applied LSN, or a resync re-delivering an already-applied fact) — this safety comes from the guards already present in `restore_job` (`-EEXIST`-as-success) and the (relocated) `mark_job_*` monotonic-rank checks, not from new dedup logic. +- Must never be invoked out of LSN order by either caller (live pump or replay loop). + +**Dependencies** +- Outbound: `restore_job`, `restore_queue_state`, `worker_from_capnp`, the guarded `mark_job_*` core (all job.c) (Criticality P0) +- Inbound: Apply Pump (live path), `replay_wal` (restart path) (Criticality P0) + +**Contracts**: Service [x] / Batch [x] + +##### Service Interface +```c +/* Applies one committed WAL entry to in-memory state. Called with entries + * strictly in ascending lsn order by both the live apply pump and replay. + * Returns 0 on success (including a no-op idempotent re-apply), negative on + * a genuine apply failure (which is a fenceable condition, not a retry). */ +int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn, + const void* payload, uint32_t len); +``` +- Preconditions: `lsn` is committed (majority-durable); every entry below `lsn` has already been applied. +- Postconditions: in-memory state reflects the entry; the type's `AFTER_EVENT` hook has fired exactly once for this application. +- Invariants: applying the same `(wal_type, lsn, payload)` twice produces the same observable state as applying it once. + +##### Batch / Job Contract +- Trigger: commit-advance (live) walking `[last_applied_lsn+1 .. committed_lsn]`, or replay iterating the recovered log. +- Input / validation: WAL entry header + payload, already CRC-verified by the point apply is reached. +- Output / destination: in-memory job/queue/worker state; `AFTER_EVENT` hook firing. +- Idempotency & recovery: guaranteed by the underlying `restore_job`/guarded-transition primitives; `last_applied_lsn` is the resumable cursor for restart. + +### Raft Step (Command) + +#### Explicit command raft step + +| Field | Detail | +|-------|--------| +| Intent | Append + majority-commit a command entry, invoked directly by the command handler, never from a hook | +| Requirements | 1.1, 1.2, 2.1 | + +**Responsibilities & Constraints** +- For job submit: already implemented as `persist_submit_wal_and_commit()` (job.c:626) — this design changes its caller-side contract from "returns synchronously, caller replies inline" to "registers a continuation via `repl_submit_majority_commit()`, caller's reply logic becomes the continuation." +- The command handler (`handle_jobsubmit`) no longer calls `create_job()` before this step; object construction moves to the continuation's success branch, via `apply_committed_entry()`. +- Must run off the iomgr thread (unchanged from today). + +**Dependencies** +- Outbound: Commit Engine Resolve (`commit_submit_async`) (Criticality P0) +- Outbound: Apply Seam, on the continuation's success branch (Criticality P0) + +**Contracts**: Service [x] + +**Implementation Notes** +- Integration: the existing `persist_submit_wal_and_commit()` encode/append loop is retained; only its commit-wait call changes from blocking to continuation-registering. +- Validation: a config-change command handler follows the identical pattern once introduced (out of scope to build here, but the contract is designed to generalize — see Requirements Traceability 1.1 covering "job submit, configuration change"). +- Risks: converting the one existing `COMMIT_MODE_WAIT` call site is narrow in surface area but touches a durability-critical path — full existing HA soak suite (Req 11.2) must pass before/after. + +### Fact Ingestion + +#### Fact receipt split + +| Field | Detail | +|-------|--------| +| Intent | Split today's receipt-time mutate-then-append into durable-append-only at receipt, mutate+react at apply | +| Requirements | 5.1, 5.2, 5.3, 6.1, 6.2, 6.3 | + +**Responsibilities & Constraints** +- `on_job_start`/`on_job_finish`/`on_job_fail`/`on_job_update` (job.c) validate (existing `BEFORE_EVENT`, unchanged) and then only encode + durably append via `wal_hook_append_classified` (`WAL_MODE_RETAIN`) — no state mutation, no `AFTER_EVENT` firing at this point. +- The guarded mutation core (extracted from today's `mark_job_*`) and the `AFTER_EVENT` hook firing both relocate to run from `apply_committed_entry()`. +- No local-apply allowlist under quorum loss (Req 6.2) — enforced structurally: the apply pump only ever processes committed LSNs, so there is no code path that can apply an uncommitted fact. + +**Dependencies** +- Outbound: durable append (`wal_hook_append_classified`, existing, unchanged) (Criticality P0) +- Outbound: Apply Seam, for the deferred mutation (Criticality P0) + +**Contracts**: Event [x] / Batch [x] + +##### Event Contract +- Published events: `AFTER_EVENT` for `HOOK_DETAIL_JOB_START`/`_EXIT`/etc., now fired from the apply step, not the receipt handler. +- Subscribed events: none new. +- Ordering / delivery guarantees: strict commit order via the shared apply pump; at-least-once redelivery for facts lost before append remains `worker-rejoin-resync`'s responsibility (out of boundary). + +**Implementation Notes** +- Integration: `mark_job_start/finish/fail/update`'s guard predicates (`job_state_is_backward`, `job_state_is_terminal`) become the guard logic inside `apply_committed_entry`'s fact-transition case — the same predicates, relocated, not rewritten. +- Validation: audit every `HOOK_DETAIL_JOB_*` subscriber for an assumption that `BEFORE_EVENT` and the state mutation happen together at receipt; only the mutation moves, `BEFORE_EVENT` validation timing is unchanged. +- Risks: this is the widest behavioral change in the design — worker-visible latency for slot-free/dispatch grows by one commit round-trip (accepted cost, documented in `facts-in-raft.md`, Req 11.4 bounds it to "no per-fact synchronous barrier," not "no latency change"). + +### Restart Replay & Verification + +#### Post-replay apply-correctness verification + +| Field | Detail | +|-------|--------| +| Intent | Prove materialized state matches the committed log tip after replay, beyond CRC structural checks | +| Requirements | 3.1, 3.2, 8.1, 8.2, 8.3, 8.4 | + +**Responsibilities & Constraints** +- Runs after `replay_wal()`'s existing CRC-chain verification succeeds (RECOVER_CLEAN), applying every entry via `apply_committed_entry()` (replacing today's direct `restore_job`/`restore_job_state` calls in `apply_wal_entry`). +- Computes a lightweight expectation from the committed log tip (e.g., last-applied LSN reaches the recovered commit watermark, plus a count/hash of materialized entities) and compares it to the actual post-replay state. +- On mismatch: reuses `persist_recover_loud_fence_if_unrecoverable()`'s existing mechanism (persist.c:2990) and the `recovery_unrecoverable` sticky flag / `ha->in_catch_up` promotion gate — a DIVERGENT verdict is a new *input* to that existing fence function, not a new fence mechanism. + +**Dependencies** +- Outbound: Apply Seam (Criticality P0) +- Outbound: existing loud-fence mechanism (`persist_recover_loud_fence_if_unrecoverable`, `recovery_unrecoverable`, `ha->in_catch_up`) (Criticality P0) + +**Contracts**: Batch [x] / State [x] + +##### State Management +- State model: `last_applied_lsn` (new, small addition to `persist_t`) tracks replay/apply progress; `recovery_unrecoverable` (existing) is the sticky fence flag. +- Persistence & consistency: `last_applied_lsn` is derived at replay time (not itself persisted) — restart always recomputes it from the WAL, so it cannot itself drift from disk state. +- Concurrency strategy: single-threaded during replay (unchanged); the live apply pump takes over `last_applied_lsn` advancement once replay completes. + +## Data Models + +No new persistent schema. `WAL_JOB_SUBMIT`/`WAL_JOB_START`/`WAL_JOB_FINISH`/`WAL_JOB_FAIL`/`WAL_JOB_UPDATE`/`WAL_QUEUE_CTRL`/`WAL_WORKER_CTRL` entry formats, `wal_classify`'s WAIT/RETAIN table, and `job_t`'s state bits (`JOB_STAT_PEND/RUN/DONE/EXIT`) are all unchanged. + +**In-memory additions**: +- `persist_t.last_applied_lsn` (`_Atomic uint64_t`) — the apply pump's resumable cursor, analogous to the existing `committed_lsn` / `durable_lsn` watermarks. +- `commit_engine_t`'s per-LSN WAIT entry gains a `commit_continuation_fn` + `void* ctx` field (replacing the `pthread_cond_t` wait registration for that mode; `COMMIT_MODE_RETAIN` entries are unaffected). + +## Error Handling + +### Error Strategy +- **Failed/timed-out command commit**: `commit_resolve(..., COMMIT_OUTCOME_FAILED, rc)` fires the registered continuation with the original error-code vocabulary (`-ETIMEDOUT`, `-XCE_ERR_NOT_READY`, etc., unchanged from today); the continuation replies failure to the client and never calls `apply_committed_entry()` — no rollback needed because no mutation occurred (Req 2.3). +- **Apply-time failure** (e.g., a decode error on an already-committed, already-CRC-valid entry): a genuine programming-invariant violation, not an operational condition — treated per the controller's existing "never self-terminate" rule: log loud, do not apply, and let the DIVERGENT-verdict fence (restart path) or an equivalent live-path fence catch it; never `assert()`/abort on this path in production. +- **DIVERGENT restart verdict**: non-promotable fence via the existing `recovery_unrecoverable` mechanism — stays alive, keeps retrying catch-up, never exits (steering `ha-async-io.md` / controller `CLAUDE.md` process-lifecycle invariant). +- **Quorum loss during a retained fact's commit**: no error surfaces to the worker (facts have no requester awaiting confirmation, unchanged); the fact remains durably appended and unapplied until majority is restored (Req 6). + +### Monitoring +- Log a WARN when a retained fact's backlog age (durably appended, not yet committed) exceeds a threshold, consistent with `facts-in-raft.md`'s D1 refinement (metric on retained-fact backlog age). +- Log the CLEAN/DIVERGENT verdict at INFO/ERROR respectively on every restart, alongside the existing CRC verdict. + +## Testing Strategy + +- **Unit Tests**: + - Apply-after-commit ordering: a failed commit produces zero observable state mutation (Req 2.3). + - Idempotent re-apply: applying the same committed entry twice (simulated replay-of-already-applied) is a no-op (Req 7.1, 7.2). + - Commit-resolve exactly-once: registering a continuation and forcing each of `{majority-ack, timeout, demotion, truncation}` resolves it exactly once (Req 9.4). + - Fact monotonicity under replay: a duplicate/out-of-order FINISH-then-START replay sequence does not regress state (Req 7.2 — directly targets the `restore_job_state` gap found in `research.md`). +- **Integration Tests**: + - End-to-end job submit: commit success → job visible and notified; commit failure → job never visible, client sees failure (Req 2, 11.3). + - End-to-end fact flow: worker reports job start → no slot-free/dispatch/notify until the entry commits (Req 5.3). + - Quorum-loss-then-restore: facts submitted during a majority-loss window apply in original order once quorum returns, with no local-apply escape hatch exercised (Req 6). + - Restart CLEAN verdict on a normal shutdown/restart; forced DIVERGENT verdict (inject a materialization mismatch) confirms the fence engages and the process stays alive (Req 8). +- **Regression / Conformance**: + - Extend `raft_sim.c`'s conformance harness with "no reaction before commit" and "idempotent re-apply" assertions layered on its existing apply-after-`on_commit_advanced` loop (Req 10.1-10.3). + - Extend `/xce-test-ha` with a kill-the-primary-mid-fact-stream scenario asserting no phantom slot-frees and no notification for an uncommitted fact (Req 10.4). + - Full existing unit/integration/soak suite re-run as a regression gate (Req 11.1, 11.2), with the known, intentional exception of tests that asserted the old apply-before-commit ordering (updated, not skipped). + +### Performance & Scalability +- Target: fact-commit throughput under sustained high-volume worker churn must not regress relative to pre-feature behavior (Req 11.4) — the group-commit amortization for `WAL_MODE_RETAIN` entries is unchanged by this design; only the post-commit reaction timing shifts. +- The keystone conversion removes one pooled-worker-thread-per-in-flight-commit cost entirely (today's `COMMIT_WAIT_RESERVED`-gated blocking wait), which should *improve* concurrent-submit headroom, not merely preserve it — validate via Req 9's bounded-resource tests under a concurrent-submit burst. diff --git a/.kiro/specs/raft-apply-decoupling/requirements.md b/.kiro/specs/raft-apply-decoupling/requirements.md new file mode 100644 index 00000000..cec28354 --- /dev/null +++ b/.kiro/specs/raft-apply-decoupling/requirements.md @@ -0,0 +1,159 @@ +# Requirements Document + +## Introduction + +The XCE controller's raft protocol step is currently a side effect of the notification hook fan-out rather than an explicit, ordered part of the request path. `persist_job_hook_fun` runs as an `AFTER_EVENT` hook and, inside that callback, performs the WAL append and blocks on the synchronous majority-commit barrier. Meanwhile `handle_jobsubmit` mutates the in-memory job table (`create_job()`) **before** that commit step runs, so a commit that fails or times out does not cleanly revert the optimistic mutation. Because apply happens before commit, downstream consumers (scheduler push, client reply) must defensively re-gate on a commit watermark to avoid acting on not-yet-committed state. This mis-layering is the structural root of the bug-795/828/851/1060 divergence family. + +This feature restructures the controller onto canonical Raft ordering with one explicit apply seam: pre-hooks (validation only) → an explicit raft step (append → replicate → majority commit) → a single `apply_committed_entry()` seam that mutates in-memory state **after** commit → after-hooks that are pure notification delivery observing already-committed, already-applied state. The same `apply_committed_entry()` seam is reused by restart replay, and a post-replay verification proves materialized state matches the committed log. Per `ha-consistency-invariants` criterion 2.9 (the Facts-Apply invariant), this feature also reconciles worker-reported facts (`WAL_MODE_RETAIN`: job start/finish/fail/rusage) onto the same commit-then-apply seam — today they are applied and reacted to on receipt, ahead of commit. + +Everything in this feature is downstream of one conversion: the commit-wait drive is currently blocking (`repl_wait_majority_ack` parks a pooled worker thread per in-flight commit via `pthread_cond_timedwait`). This feature includes converting that drive from blocking-wait to event-driven resolve as its keystone prerequisite, because the apply step, the deferred client reply, and the fact-reaction timing all sit on top of it. The separate transport/timer-thread blocking violations (snapshot send, config-push wait, peer connect on iomgr/delaymgr) are a distinct defect class owned by `raft-drive-async`, not this feature. + +The authoritative subject for the acceptance criteria below is **the XCE controller** — specifically its command/fact submission, commit, and apply path. + +## Boundary Context + +- **In scope**: + - Extracting the raft protocol step (WAL append + majority-commit barrier) out of the `AFTER_EVENT` hook fan-out into an explicit step invoked directly by command handlers. + - Canonical ordering for commands (job submit, configuration change): pre-hook validation → commit → apply → after-hook notification, with no in-memory state mutation before commit. + - A single shared `apply_committed_entry()` seam used by both live commit-advance and restart replay, applying committed entries strictly in commit order. + - Demoting the hook fan-out to notification-only: after-hooks observe already-committed, already-applied state and no longer carry WAL/commit responsibility, and downstream consumers no longer need to defensively re-check a commit watermark before acting on a notification. + - Reconciling worker-reported facts (`WAL_MODE_RETAIN`) onto the same commit-then-apply seam (criterion 2.9): durable append stays on receipt, but the state mutation and reactions (worker slot release, scheduler dispatch trigger, client/apiserver notify) move to after commit. + - The quorum-loss policy for retained facts: block-but-durable (append durably, do not apply or react until commit; no local-apply allowlist) — decision D1. + - Idempotent, state-monotonic, strictly commit-ordered re-application of entries (replay, retry, worker resync). + - Post-restart verification that materialized state matches the committed log tip, surfacing a CLEAN/DIVERGENT verdict. + - The keystone conversion of the commit-wait drive from blocking (`repl_wait_majority_ack` / `pthread_cond_timedwait`) to an event-driven resolve, and the resulting bound on request-handling capacity under concurrent in-flight commits. + - Regression and conformance coverage for the above ordering, apply-idempotency, and no-reaction-before-commit invariants. + - No regression of existing HA behavior, correctness guarantees, or fact-commit throughput as a result of this reordering. +- **Out of scope**: + - Collapsing a job-array submission into a single atomic WAL record — owned by `job-array-single-wal-record`, which builds on top of the apply seam this feature delivers. + - Changes to the raft library's protocol core (`raft_advance_commit`, `on_commit_advanced`, `on_persist_entry`, `on_get_entry_term`, `on_send_append_entries`, `on_truncate_from`) — owned by `raft-log-replication` / `raft-commit-safety` and consumed as-is. + - Entity-bearing snapshot content and the config-upgrade snapshot format — owned by `ha-entity-snapshots` / `config-upgrade-snapshot`. + - Read-consistency and leader-lease optimizations. + - Moving snapshot send, configuration-push waiting, or peer connect off the iomgr/delaymgr threads — owned by `raft-drive-async` (P2 transport/timer drive hardening); this feature owns only the commit-wait drive conversion (P1). + - Changes to the worker join/rejoin protocol itself — owned by `worker-join-refactor` / `worker-rejoin-resync`. + - Asynchronous DNS resolution for peer connections. +- **Adjacent expectations**: + - This feature relies on the majority-commit and idempotency primitives already delivered by `ha-commit-engine` and `ha-commit-barrier-rollout`; it does not re-implement them. + - This feature touches the same submit/commit path that `ha-term-safety` is concurrently wiring into the raft library adapter; the two must be sequenced/coordinated so neither reintroduces a term-blind or apply-before-commit regression. + - Full recovery of a fact that never reached the controller, or that was lost with the controller across a failover window, remains the responsibility of `worker-rejoin-resync` (roadmap backlog, not yet built) — this feature relies on it as the designated backstop but does not own or implement it. Until `worker-rejoin-resync` lands, such facts remain lost, consistent with the umbrella-locked decision already accepted by `ha-consistency-invariants` and `ha-commit-barrier-rollout`. + - `job-array-single-wal-record` depends on the `apply_committed_entry()` seam this feature delivers and must land after it. + - Ordering and apply-correctness assertions introduced here are consumed by `raft-sim-conformance`'s regression harness. + - This feature must preserve the existing "controllers never self-terminate" invariant and the existing persist-before-ACK durability and synchronous majority-commit-barrier policy; it changes ordering and apply timing, not the durability contract. + +## Requirements + +### Requirement 1: Extraction of the Raft Commit Step from the Hook Fan-Out + +**Objective:** As a maintainer, I want the WAL append and majority-commit barrier for a command to run as an explicit step invoked directly by the command handler rather than as a side effect of an `AFTER_EVENT` hook callback, so that the raft protocol step is a first-class, ordered part of the request path instead of incidental hook behavior. + +#### Acceptance Criteria + +1. When a command handler (job submit, configuration change) processes a request, the controller shall perform the WAL append and majority-commit barrier as an explicit step invoked by the handler, not from within a `BEFORE_EVENT` or `AFTER_EVENT` hook callback. +2. The controller shall not register the majority-commit barrier as a hook subscriber for any command entry type. +3. The controller shall perform the explicit raft commit step without blocking the iomgr event-loop thread, preserving the existing async-I/O thread-role contract. +4. The controller shall preserve persist-before-ACK durability and the synchronous majority-commit barrier as policy; extracting the raft step from the hook fan-out shall not weaken or remove either. + +### Requirement 2: Canonical Command Ordering — Apply-After-Commit + +**Objective:** As a cluster operator, I want a job-submit or configuration command's in-memory effect to occur only after its entry is durably committed to a majority, so that a commit that fails or times out never leaves the cluster's observable state showing an operation that did not durably happen. + +#### Acceptance Criteria + +1. When a command entry is accepted for processing, the controller shall not apply its state mutation until the entry has committed to a majority. +2. When a command entry commits to a majority, the controller shall apply its state mutation before producing any success reply or notification for that entry. +3. If a command entry's majority commit fails or times out, then the controller shall report failure to the requester and shall not apply the entry's state mutation, so the controller's observable state is unchanged by the failed command. + +### Requirement 3: Single Shared Apply Seam — Online and Replay Parity + +**Objective:** As a maintainer, I want one shared `apply_committed_entry()` seam used both when a commit advances live and when the controller replays its log after restart, so that a node's materialized state is always the deterministic result of its committed log regardless of how it got there. + +#### Acceptance Criteria + +1. When the controller applies a committed entry during live operation, the controller shall invoke the same `apply_committed_entry()` seam that restart replay invokes for that entry type. +2. When restart replay processes a committed entry, the controller shall apply it through `apply_committed_entry()` rather than through a separate replay-only mutation path. +3. The controller shall apply committed entries strictly in commit order, never applying an entry out of order relative to an entry with a lower commit position. +4. The controller shall produce identical materialized state from a committed log regardless of whether that log is applied via live commit-advance or via restart replay. + +### Requirement 4: Hook Fan-Out Demotion to Notification-Only + +**Objective:** As a maintainer, I want after-hooks to be pure notification delivery observing already-committed, already-applied state, so that scheduler push, client reply, and apiserver updates no longer need to carry or defensively re-check commit responsibility. + +#### Acceptance Criteria + +1. When an `AFTER_EVENT` hook fires for a command or fact entry, the controller shall treat that hook exclusively as a notification of already-committed, already-applied state (client reply, scheduler dispatch trigger, SSE/apiserver update). +2. The controller shall not perform WAL append, commit-barrier waiting, or state-machine mutation from within any `AFTER_EVENT` or `BEFORE_EVENT` hook callback. +3. The controller shall not require a downstream consumer (scheduler, apiserver) to re-check a commit watermark before acting on a notification delivered by an after-hook, since that notification only fires after the entry is committed and applied. + +### Requirement 5: Facts Apply-After-Commit + +**Objective:** As a cluster operator, I want a worker-reported fact's state mutation and its consequential reactions (worker slot release, scheduler dispatch, notification) to occur only after the fact is committed to a majority, so that a leader that loses an uncommitted fact never leaves behind a phantom effect such as a freed slot, a triggered dispatch, or a sent notification. + +#### Acceptance Criteria + +1. When the controller receives a reported fact from a worker, the controller shall durably append the fact to its log without performing the fact's state mutation or reactions at that time. +2. When a reported fact's entry commits to a majority, the controller shall apply its state mutation and trigger its reactions through the same `apply_committed_entry()` seam used for commands. +3. The controller shall not release a worker slot, trigger scheduler dispatch, or send a client/apiserver notification for a fact whose entry has not committed to a majority. + +### Requirement 6: Quorum-Loss Behavior for Retained Facts + +**Objective:** As a cluster operator, I want reported facts to be safely retained — neither dropped nor prematurely applied — when the controller cannot reach a majority, so that the cluster degrades safely and self-heals without operator intervention once quorum is restored. + +#### Acceptance Criteria + +1. While the controller cannot reach a majority of cluster members, the controller shall retain durably-appended, uncommitted facts without applying their state mutations or triggering their reactions. +2. The controller shall not apply a fact's state mutation or reactions through any path other than majority commit, regardless of quorum state; no local-only apply allowlist shall exist. +3. When a majority becomes reachable again after a quorum-loss period, the controller shall commit and apply the retained facts in their original log order. + +### Requirement 7: Idempotent, Ordered Re-Application + +**Objective:** As a maintainer, I want re-applying an already-applied entry — via replay, retry, or worker resync — to be a safe no-op, so that duplicate or out-of-order delivery never regresses or corrupts job state. + +#### Acceptance Criteria + +1. When the controller applies an entry whose effect has already been applied, by prior commit, replay, or resync, the controller shall treat the re-application as a no-op with respect to observable job state. +2. If a duplicate or out-of-order fact is applied for a job, then the controller shall compare it against the job's current lifecycle state and shall not regress that job to an earlier lifecycle stage. +3. The controller shall apply entries in strict commit order and shall never apply an entry ahead of another entry that committed before it. + +### Requirement 8: Restart Apply-Correctness Verification + +**Objective:** As a cluster operator, I want the controller to verify after restart that its materialized state matches its committed log, so that a hidden apply defect is surfaced immediately rather than silently causing future divergence. + +#### Acceptance Criteria + +1. When the controller completes WAL replay after a restart, the controller shall verify whether the materialized state matches the expectation derived from the committed log tip, in addition to the existing CRC chain verification. +2. When the post-replay verification matches, the controller shall report a CLEAN verdict. +3. If the post-replay verification does not match, then the controller shall report a DIVERGENT verdict as a loud, operator-visible condition. +4. If the controller reports a DIVERGENT verdict, then the controller shall enter a non-promotable fenced state and shall continue running and retrying catch-up rather than exiting or aborting, consistent with the "controllers never self-terminate" invariant. + +### Requirement 9: Bounded Resource Behavior Under Concurrent In-Flight Commits + +**Objective:** As a cluster operator, I want the controller to remain responsive to new requests even while many operations are simultaneously awaiting commit, so that a burst of concurrent commands or facts cannot make the controller unresponsive. + +#### Acceptance Criteria + +1. While multiple entries are simultaneously awaiting majority commit, the controller shall continue to accept and begin processing new incoming requests. +2. The controller shall not exhaust its bounded request-handling capacity as a direct result of the number of entries currently awaiting commit. +3. When an entry's commit outcome (success, failure, or timeout) becomes known, the controller shall resolve exactly one reply or notification for that entry's originator. +4. If the controller loses leadership or a pending entry's log tail is truncated while the entry is awaiting commit, then the controller shall resolve that entry's outcome as a failure exactly once, rather than leaving it unresolved. + +### Requirement 10: Apply-Ordering Regression and Conformance Verification + +**Objective:** As a maintainer, I want automated regression coverage asserting the apply-after-commit ordering, so that a future change cannot silently reintroduce apply-before-commit or lost-notification defects. + +#### Acceptance Criteria + +1. The project's conformance test suite shall assert that no entry's state mutation or reactions occur before that entry commits. +2. The project's conformance test suite shall assert that a failed or timed-out commit produces no residual state mutation. +3. The project's conformance test suite shall assert that re-applying an already-committed entry, whether by simulated replay or retry, is idempotent. +4. The project's regression suite shall include a live-cluster scenario that kills the primary mid-fact-stream and asserts no phantom slot-frees, no committed-state divergence, and no notification for a fact that did not commit. + +### Requirement 11: Preservation of Existing Behavior and Performance + +**Objective:** As a cluster operator, I want this refactor to preserve every existing HA behavior, correctness guarantee, and performance characteristic that this feature does not specifically intend to change, so that fixing the apply-ordering defect does not introduce a new regression elsewhere. + +#### Acceptance Criteria + +1. The controller's existing automated unit and integration test suites shall continue to pass after this feature lands, except for tests that specifically assert the pre-existing apply-before-commit ordering, which shall be updated to assert the new apply-after-commit ordering instead. +2. The controller's existing HA failover regression and soak suites shall continue to report zero committed-job loss and zero committed-state divergence across a primary failover, matching the acceptance bar already established by prior HA specs. +3. When a client submits a job, cancels a job, or queries job/queue/worker status, or a worker reports a job's start, finish, failure, or resource usage, the controller shall produce the same externally observable end result as before this feature; this feature shall change only the internal timing of when that result becomes visible, not the result itself. +4. While a sustained high-volume worker-fact stream is in progress, the controller shall not regress fact-commit throughput relative to pre-feature behavior, and shall not introduce a per-fact synchronous commit barrier. diff --git a/.kiro/specs/raft-apply-decoupling/research.md b/.kiro/specs/raft-apply-decoupling/research.md new file mode 100644 index 00000000..614c70bf --- /dev/null +++ b/.kiro/specs/raft-apply-decoupling/research.md @@ -0,0 +1,103 @@ +# Research & Design Decisions + +## Summary +- **Feature**: `raft-apply-decoupling` +- **Discovery Scope**: Complex Integration (extension of a live, fast-moving HA/Raft subsystem — light discovery upgraded to comprehensive verification because the source brief (`brief.md`, 2026-06-24/25) is 9 days old relative to the code and several sibling specs land in the exact same seam) +- **Key Findings**: + - `bug-1306` (commit `15be5d9`, 2026-06-30) already extracted the WAL-append + majority-commit barrier out of `persist_job_hook_fun` for job submit, ahead of this spec's formal implementation — Requirement 1 is partially pre-landed for the WAIT path. + - The in-memory job-submit mutation (`create_job()`) still runs **before** the commit barrier (`persist_submit_wal_and_commit()`), and a failed/timed-out commit does **not** roll it back — the exact optimistic-divergence window Requirement 2 targets is still live today, just narrowed by the bug-1306 fix (which only fixed the reply-ack race, not the apply-ordering). + - `ha-term-safety` is fully done (20/20 tasks); `on_commit_advanced` / `raft_advance_commit` are wired and live on the replication-ACK path (advisory-only, Option A) — no conflict with this spec's keystone. + - `raft-drive-async` already built `vtask_enq_classified` + a slow-lane executor + admission classes (`VTASK_LIVENESS` / `VTASK_LIVE_BLOCK` / `VTASK_REQ_BLOCK`) — this closes the "no general non-IO task-submit path" gap the brief flagged as missing, so the keystone conversion can reuse it instead of building a new primitive. + - Online-apply and replay-apply are **not** unified today: job submit online uses raw `create_job()`; replay uses `restore_job()` (decode + idempotent `-EEXIST` guard + LSN tag). Fact state transitions online go through guarded `mark_job_start/finish/fail/update()` (monotonic-rank checks); replay applies facts via `restore_job_state()`, a raw `j->job.state = state` overwrite with **no** guard at all. This is concrete, evidenced proof of the divergence Requirement 3/7 exist to close. + - `WAL_MODE_RETAIN` facts already register into the commit engine (`commit_submit(ce, lsn, COMMIT_MODE_RETAIN, 0)`) for durability bookkeeping, but nothing today reacts to a RETAIN entry's commit — the state mutation already happened synchronously at receipt, before the WAL append. The "apply pump" driven off commit-advance (Requirement 5/6) does not exist in any form yet. + - `raft-sim-conformance`'s harness structurally enforces apply-only-after-commit already (it calls `raft_advance_commit`, then applies via `on_commit_advanced`, then checks State Machine Safety) — Requirement 10's conformance assertions mostly compose with existing harness structure rather than requiring new harness machinery. + +## Research Log + +### Current submit/hook path (job.c, persist.c) +- **Context**: Brief claimed `persist_job_hook_fun` does WAL-append + commit-wait inside an `AFTER_EVENT` hook (persist.c:2532/2198, 2026-06-24 citation). +- **Sources Consulted**: `src/controller/job.c:560-662` (`handle_jobsubmit`), `src/controller/persist.c:3392-3478` (`persist_submit_wal_and_commit`, `persist_job_hook_fun`), git log (`15be5d9`). +- **Findings**: + - `handle_jobsubmit` (job.c:560): `BEFORE_EVENT` hook (validation) → `job_submit()` → `create_job()` (in-memory insert into `c->jobmgr`, job.c:583) → **explicit** `persist_submit_wal_and_commit()` (job.c:626, persist.c:3406) reading the commit verdict directly → on failure, reply error and **return** (job.c:627-637) — the already-created job object is **not** removed from `c->jobmgr` → on success, `AFTER_EVENT` hook fires as notification-only (job.c:643, persist.c:3466-3478 confirms the WAIT branch inside `persist_job_hook_fun` is now a no-op) → reply success. + - `persist_job_hook_fun`'s `WAL_MODE_WAIT` branch is dead code by design (returns 0 unconditionally, persist.c:3466-3478) — kept as a documented no-op, not removed, so hook registration order doesn't matter for job submit anymore. +- **Implications**: Requirement 1 (extract raft step from hook) is **done** for the job-submit WAIT path. Requirement 2 (apply-after-commit ordering) is **not** done — `create_job()` still runs before the barrier, and there is no rollback on commit failure. Design must move the job **object construction** itself (not just a secondary mutation) into the post-commit apply step. + +### Commit engine and ha-term-safety completion (commit.c, ha.c) +- **Context**: The keystone prerequisite (`repl_wait_majority_ack` blocking-to-event-driven conversion) is claimed as this spec's first-landed item; needed to confirm nothing else already did it and that `ha-term-safety`'s concurrent work on the same seam doesn't conflict. +- **Sources Consulted**: `src/controller/repl.c:533-558`, `src/controller/commit.c:195-409`, `src/controller/ha.c:1680,1776,1829-1831,3764`, `.kiro/specs/ha-term-safety/tasks.md`. +- **Findings**: + - `repl_wait_majority_ack` (repl.c:557) unconditionally delegates to `commit_submit(ce, lsn, COMMIT_MODE_WAIT, timeout_ms)` (commit.c:358), which blocks via `pthread_cond_timedwait` (commit.c:409) under the `COMMIT_WAIT_RESERVED` admission cap. Unchanged — still a pooled-worker blocking wait, 500ms default timeout. + - `ha-term-safety`: all 20 tasks done. `on_commit_advanced` (ha.c:1776) is wired and live, firing from `raft_advance_commit` (raft.c call site ha.c:3764) on the replication-ACK path; it feeds `commit_set_raft_commit_index()` as an **advisory** cap only (Option A — the durable-majority barrier remains authoritative; the library commit index does not gate anything today). +- **Implications**: No conflict — `ha-term-safety` supplies the library-side commit-index plumbing this spec's apply pump can key off of (`on_commit_advanced`), but the blocking-wait-to-resolve conversion itself is untouched and squarely this spec's keystone to build. + +### Reusable async primitives from raft-drive-async (vtaskmgr.c) +- **Context**: The brief identified a gap — no general (non-IO) task-submit path exists — as an API prerequisite for the keystone's async continuations. +- **Sources Consulted**: `.kiro/specs/raft-drive-async/tasks.md`, `src/controller/vtaskmgr.c:467,497,528`. +- **Findings**: Task 1.2 (slow-lane executor, non-blocking submit) and Task 1.6 (`vtask_class_t` + `vtask_enq_classified`, admission-gated by class: `VTASK_LIVENESS` / `VTASK_LIVE_BLOCK` / `VTASK_REQ_BLOCK`) are both done and live (default-on since 2026-07-02, bug-1376). `vtask_enq_classified(mgr, task, class)` is a general, non-packet-bound submit path. +- **Implications** (Build vs. Adopt): Adopt `vtask_enq_classified` with a `VTASK_REQ_BLOCK`-equivalent class for the resolved-commit continuation (reply dispatch / apply trigger) instead of building a new submission primitive. This directly resolves the brief's "API gap" concern at zero new-code cost. + +### Restart replay and the apply-seam divergence (persist.c, job.c) +- **Context**: Requirement 3 requires one shared `apply_committed_entry()` seam for online and replay; needed to establish exactly how much divergence exists today. +- **Sources Consulted**: `src/controller/persist.c:2452-2562` (`apply_wal_entry`), `src/controller/persist.c:2667-2723` (`replay_wal` CRC verification), `src/controller/job.c:44-177` (`create_job`, `restore_job`, `restore_job_state`), `src/controller/job.c:664-774` (`mark_job_start`, `mark_job_finish`). +- **Findings**: + - `apply_wal_entry` dispatches `WAL_JOB_SUBMIT` → `restore_job()` (persist.c:2505) and `WAL_JOB_{START,FINISH,FAIL,...}` → `restore_job_state()` (persist.c:2520). + - `restore_job()` (job.c:96) decodes the Cap'n Proto payload, checks for an existing job by jobid (idempotent `-EEXIST` guard, job.c:117-130), and calls the **same** `create_job()` (job.c:44, thin wrapper over `makeJob`) that the online submit path uses. So submit-side apply is **half**-unified: replay already funnels through a shared primitive; online does not (it calls `create_job()` directly, skipping the idempotency guard and LSN tagging that `restore_job()` provides). + - `restore_job_state()` (job.c:164) is a **raw** `j->job.state = (int)state` overwrite — no `job_state_is_backward`/`job_state_is_terminal` check at all. The online path's `mark_job_start`/`mark_job_finish`/`mark_job_update` (job.c:664-850) **do** carry those monotonic-rank guards. This is a genuine, currently-live idempotency gap: a duplicate or out-of-order fact entry replayed today (segment-boundary restart replay, or a future retained-fact retry) can regress job state, because replay's mutation path bypasses the same guard the online path enforces. +- **Implications**: `apply_committed_entry()` should standardize on `restore_job()` for submits (already idempotent, already reusable) and on a **guarded** state-transition function for facts — meaning `mark_job_start/finish/fail/update`'s guard logic must become reachable from both the post-commit online path and replay, and `restore_job_state()` must stop being a second, weaker mutation path. This is a Build-vs-Adopt case: adopt the existing guards, relocate their call site. + +### raft-sim-conformance harness readiness +- **Context**: Requirement 10 needs regression coverage for apply-ordering; checked whether the existing deterministic simulator can carry these assertions without new harness machinery. +- **Sources Consulted**: `.kiro/specs/raft-sim-conformance/{spec.json,requirements.md,design.md}`. +- **Findings**: The harness already asserts Election Safety, Log Matching, Leader Completeness, and State Machine Safety, and its commit/apply loop is structured as `raft_advance_commit` → `on_commit_advanced` → apply → State-Machine-Safety check — i.e., it is already apply-after-commit by construction. +- **Implications**: New assertions (no-reaction-before-commit, idempotent re-apply) are additive checks layered on the existing harness loop, not a new harness. The live-cluster kill-mid-fact-stream scenario (Requirement 10.4) is new and belongs to the `/xce-test-ha`-style soak tooling, not the simulator. + +## Architecture Pattern Evaluation + +| Option | Description | Strengths | Risks / Limitations | Notes | +|--------|-------------|-----------|----------------------|-------| +| Single `apply_committed_entry()` dispatch function, keyed on WAL type | One function, switch on `wal_type`, called from both the post-commit online path and `apply_wal_entry()` | Matches steering's "library-owned, thin-adapter" pattern already used for raft callbacks; smallest change that satisfies Req 3 | Must not become a god-function — keep per-type logic in the existing `restore_*`/`mark_job_*` helpers, `apply_committed_entry()` only dispatches | Selected | +| Separate apply seams per entry type (no unifying dispatcher) | Each WAL type keeps its own online-vs-replay pair, just guard-aligned | Less code to touch per type | Does not satisfy Req 3's "same seam" requirement literally; re-invites future divergence | Rejected | +| Event-sourced full replay-log-as-truth (rebuild all reactions from log on every apply) | Maximal purity | Over-engineered — Design Risk A in `facts-in-raft.md` already concluded reactions must NOT be replayed, only state; resync (existing) is the re-derivation mechanism | Rejected (already settled in brief.md) | + +## Design Decisions + +### Decision: Keystone conversion reuses the existing per-LSN commit registry and `vtask_enq_classified`, not a new registry or submit path +- **Context**: `repl_wait_majority_ack` → `commit_submit(..., COMMIT_MODE_WAIT, ...)` blocks a pooled worker on `pthread_cond_timedwait`. Converting to event-driven `resolve()` needs (a) a way to register a continuation instead of parking a thread, and (b) a way to run that continuation without new infrastructure. +- **Alternatives Considered**: + 1. Build a new pending-request registry alongside the commit engine's existing per-LSN structure. + 2. Reuse the commit engine's existing per-LSN structure (`ce`), replacing `cond_broadcast` with a `resolve(lsn, outcome)` call that invokes a registered continuation. +- **Selected Approach**: (2). `commit_submit` keeps its per-LSN bookkeeping; `COMMIT_MODE_WAIT` callers register a continuation (function pointer + context) instead of blocking; `commit_advance_locked`'s existing `cond_broadcast` call site becomes the single place that walks newly-committed/newly-failed LSNs and invokes `resolve()`, which dispatches the continuation via `vtask_enq_classified` (raft-drive-async) rather than a bespoke thread hop. +- **Rationale**: No new concurrent data structure to get wrong; reuses the already-verified admission-cap discipline (`COMMIT_WAIT_RESERVED` / `VTASK_*` classes) instead of inventing a second one. +- **Trade-offs**: The commit engine's internal contract changes from "callers block on `ce->cond`" to "callers register a callback" — every existing `COMMIT_MODE_WAIT` call site (job submit today; future command types) must be converted together, not incrementally, or the engine would need to support both calling conventions simultaneously. +- **Follow-up**: Verify during implementation that `commit_advance_locked`'s lock-ordering discipline (documented at persist.c:3347-3350: never hold `p->lock` across `ce->lock`) still holds once resolution dispatches into `vtask_enq_classified` from inside the commit-advance path. + +### Decision: `apply_committed_entry()` standardizes on `restore_job()` / a guard-carrying state-transition function, not new mutation primitives +- **Context**: See Research Log finding above — replay already has an idempotent submit primitive (`restore_job`); online submit doesn't use it. Online facts have a guarded transition primitive (`mark_job_*`); replay doesn't use it. +- **Alternatives Considered**: + 1. Write new, from-scratch apply functions for both submit and facts. + 2. Adopt `restore_job()` as-is for submit; extract the guard logic embedded in `mark_job_start/finish/fail/update` into a form callable from both the post-commit path and `apply_wal_entry`, retiring `restore_job_state()`'s unguarded overwrite. +- **Selected Approach**: (2). +- **Rationale**: Both primitives already carry the exact idempotency semantics Requirement 7 asks for; the gap is call-site wiring, not missing logic. Writing new functions would duplicate logic that already exists and is already unit-tested. +- **Trade-offs**: `mark_job_start/finish/fail/update` today fire their own `BEFORE_EVENT`/`AFTER_EVENT` hooks inline (job.c:680-681, 699-700) as part of the online receipt path — that hook-firing must move to occur only when these functions are invoked from the post-commit apply step, not from the (now durable-append-only) receipt handler, or the ordering guarantee (Req 5) would be violated. This is a real refactor, not a pure reuse. +- **Follow-up**: Confirm no other caller of `restore_job_state()` (grep found only `apply_wal_entry`) depends on its current unguarded-overwrite behavior before retiring it. + +### Decision: `egress.c`'s `job_submit_committed()` watermark gate becomes redundant for job submit once Req 2 lands, but is not removed by this spec +- **Context**: Requirement 4.3 says downstream consumers should not need to re-check a commit watermark once notifications only fire post-commit. `job_submit_committed()` (egress.c:47) exists precisely because today a job is visible in `c->jobmgr` before it's committed. +- **Selected Approach**: Design leaves `job_submit_committed()` in place as a defense-in-depth predicate (it becomes a tautology once Req 2 lands — a job is never in `jobmgr` before commit — so it's harmless to keep), and records its removal as a documented candidate cleanup, not an in-scope deletion. +- **Rationale**: Removing it requires confirming every caller of the scheduler-export path relies solely on this gate and not on some other assumption — that audit is a distraction from the core ordering fix and belongs in a follow-up cleanup pass, matching the Simplification lens's guidance not to over-scope "just in case." +- **Follow-up**: Revalidation trigger — if a future spec removes `job_submit_committed()`, it must re-verify no path still relies on it as a correctness gate rather than a no-op. + +## Risks & Mitigations +- **Risk**: Converting every `COMMIT_MODE_WAIT` call site to the continuation-based API in one change is a wide, mechanically risky diff (touches the commit engine's core contract). — **Mitigation**: today there is exactly one `COMMIT_MODE_WAIT` call site (`repl_wait_majority_ack`, used only by job submit); the conversion is narrow in practice even though the contract change is conceptually broad. +- **Risk**: Relocating `mark_job_*`'s hook-firing out of the receipt-time handler could silently change BEFORE_EVENT hook timing/semantics for subscribers that assume BEFORE fires at receipt. — **Mitigation**: audit all `HOOK_DETAIL_JOB_START`/`_EXIT`/etc. subscribers before relocating; BEFORE_EVENT for facts should fire at receipt (validation), only the AFTER_EVENT + state mutation defers to apply. +- **Risk**: `worker-rejoin-resync` (hard dependency for full fact-reaction correctness per `facts-in-raft.md` Design Risk A) does not exist yet. — **Mitigation**: accepted, documented gap, consistent with prior specs (`ha-consistency-invariants`, `ha-commit-barrier-rollout`) that already ship with this same accepted gap. +- **Risk**: The keystone conversion changes lock-ordering-sensitive code in the commit engine that several other specs depend on (`ha-commit-durability`, `ha-term-safety`). — **Mitigation**: preserve the documented lock-ordering rule (never hold `p->lock` across `ce->lock`) as an explicit invariant in design and tests; run the full existing HA soak suite (Req 11) before/after. + +## References +- `.kiro/specs/raft-apply-decoupling/brief.md` — original discovery brief (2026-06-24/25) +- `.kiro/specs/raft-apply-decoupling/facts-in-raft.md` — facts-through-raft candidate plan, D1-D3 decisions, keystone rationale +- `.kiro/specs/ha-consistency-invariants/requirements.md` criterion 2.9 — Facts-Apply invariant, ownership assignment +- `.kiro/specs/ha-term-safety/tasks.md` — confirms full completion, no conflict +- `.kiro/specs/raft-drive-async/tasks.md` — confirms `vtask_enq_classified` / slow-lane availability +- `.kiro/specs/raft-sim-conformance/{requirements.md,design.md}` — existing conformance harness structure +- Commit `15be5d9` — bug-1306 fix, partial pre-landing of Requirement 1 diff --git a/.kiro/specs/raft-apply-decoupling/spec.json b/.kiro/specs/raft-apply-decoupling/spec.json new file mode 100644 index 00000000..226d3fb0 --- /dev/null +++ b/.kiro/specs/raft-apply-decoupling/spec.json @@ -0,0 +1,22 @@ +{ + "feature_name": "raft-apply-decoupling", + "created_at": "2026-07-03T03:32:11Z", + "updated_at": "2026-07-03T04:35:00Z", + "language": "en", + "phase": "tasks-generated", + "approvals": { + "requirements": { + "generated": true, + "approved": true + }, + "design": { + "generated": true, + "approved": true + }, + "tasks": { + "generated": true, + "approved": true + } + }, + "ready_for_implementation": true +} diff --git a/.kiro/specs/raft-apply-decoupling/tasks.md b/.kiro/specs/raft-apply-decoupling/tasks.md new file mode 100644 index 00000000..8eb6950f --- /dev/null +++ b/.kiro/specs/raft-apply-decoupling/tasks.md @@ -0,0 +1,170 @@ +# Implementation Plan + +- [ ] 1. Commit engine event-driven resolve (keystone prerequisite) +- [ ] 1.1 (P) Add per-LSN continuation registration to the commit engine for the synchronous (WAIT) commit mode, replacing the blocking wait + - Register a continuation (callback + context) against an appended LSN instead of parking the calling thread + - Preserve the existing per-LSN bookkeeping and admission-cap discipline already used by the commit engine + - Observable: a unit test can register a continuation for an LSN and the calling thread returns immediately, before the LSN commits + - _Requirements: 1.3, 1.4, 9.1, 9.2_ + - _Boundary: Commit Engine Resolve — commit.c per-LSN registry only_ + +- [ ] 1.2 Wire majority-commit and failure-path resolution to invoke the registered continuation exactly once + - On majority ack, invoke the continuation with a committed outcome + - On timeout, demotion, or log-tail truncation of a pending LSN, invoke the continuation with a failed outcome and the existing error-code vocabulary + - Dispatch every continuation invocation off the calling thread using the existing non-blocking task-submission primitive, never inline on the replication-ACK or iomgr path + - Observable: forcing each of {majority ack, timeout, demotion, truncation} in a test resolves the registered continuation exactly once, with no case left unresolved + - _Requirements: 1.3, 9.3, 9.4_ + +- [ ] 1.3 Unit test the commit engine's resolve behavior under concurrent in-flight commits + - Cover exactly-once resolution across majority ack, timeout, demotion, and truncation + - Cover that a burst of concurrently pending commits does not exhaust the bounded request-handling capacity + - Observable: a test suite run demonstrates bounded worker usage and exactly-once resolution under a concurrent-commit burst + - _Requirements: 9.1, 9.2, 9.3, 9.4_ + +- [ ] 2. Shared apply seam for committed entries +- [ ] 2.1 (P) Extract the guarded state-transition logic used by fact handling into a form usable outside the receipt-time call site + - Keep the existing monotonic-rank checks (reject backward transitions, no-op duplicate terminal transitions) intact and unchanged in behavior + - Decouple the transition logic from firing its own hook inline, so hook-firing can be relocated by a later task without touching this logic again + - Observable: the extracted transition logic can be invoked directly by a test without going through the worker-report receipt path, and produces the same guard behavior as before extraction + - _Requirements: 3.3, 7.1, 7.2_ + - _Boundary: Apply Seam — persist.c / job.c apply-dispatch and replay call sites only, no dependency on the commit-engine change in task 1_ + +- [ ] 2.2 Build the single dispatch function that applies one committed entry to in-memory state, covering every classified entry type + - Route job submission through the existing idempotent, jobid-addressable construction primitive instead of a raw uncoordinated insert + - Route job state transitions through the extracted guarded logic from 2.1 + - Route queue control and worker control entries through their existing restore primitives + - Fire the entry type's post-mutation notification hook from inside this function, after the mutation completes + - Observable: calling the dispatch function directly with a sample entry of each classified type produces the correct in-memory mutation and fires exactly one post-mutation notification + - _Requirements: 2.2, 3.1, 3.2, 4.1, 4.2, 5.2_ + - _Depends: 2.1_ + +- [ ] 2.3 Point restart replay's per-entry application at the new dispatch function + - Replace replay's direct calls to the individual restore primitives with calls to the dispatch function from 2.2 + - Observable: restart replay of a log containing every classified entry type produces identical materialized state to before this change, verified by an existing or updated replay test + - _Requirements: 3.1, 3.2_ + - _Depends: 2.2_ + +- [ ] 2.4 Unit test idempotent and ordered re-application of the dispatch function + - Cover applying the same committed entry twice is a no-op with respect to observable state + - Cover that a duplicate or out-of-order fact entry does not regress a job's lifecycle state + - Observable: a test applies a FINISH entry followed by a stale, previously-unapplied START entry for the same job and confirms the job remains in its terminal state + - _Requirements: 3.4, 7.1, 7.2, 7.3_ + - _Depends: 2.2_ + +- [ ] 3. Command apply-after-commit ordering +- [ ] 3.1 (P) Stop constructing the in-memory job object before the commit barrier + - Change job submission to carry forward only the data needed to build the WAL entry payload, without inserting anything into the in-memory job table until the (still-synchronous, at this point) commit call returns success + - Remove the now-unreachable code path that previously ran the durability step from inside the notification hook for job submission + - Observable: a job submission whose commit fails or times out leaves no job object in the in-memory job table; a successful submission still replies synchronously exactly as before this task + - _Requirements: 1.2, 2.1, 2.3_ + - _Boundary: Raft Step (Command) — job.c handle_jobsubmit and its call chain only, no dependency on task 1_ + +- [ ] 3.2 Convert job submission to register its commit outcome through the new continuation mechanism instead of blocking + - Replace the synchronous commit call from 3.1 with a registration against the task 1 continuation API + - On a failed or timed-out outcome, reply failure directly from the continuation — no apply is needed on this path + - On a committed outcome, hand the entry off to the ordered apply pump (task 6) rather than applying or replying from this continuation directly, so this entry's apply and reply happen in strict commit order relative to every other entry + - Observable: the calling thread returns before the commit resolves; a forced failure/timeout outcome still replies failure immediately with no job ever appearing (the committed-outcome path has no independently observable behavior until task 6 completes it) + - _Requirements: 1.1, 1.4, 2.1, 2.3_ + - _Depends: 1.1, 1.2_ + +- [ ] 3.3 Unit test the command failure path leaves no observable trace + - Force repeated commit failures and timeouts for job submission and confirm no job object is ever created + - Observable: the test suite demonstrates zero phantom job objects across repeated forced commit failures + - _Requirements: 2.1, 2.3_ + - _Depends: 3.2_ + +- [ ] 4. Fact apply-after-commit split +- [ ] 4.1 (P) Change worker-fact receipt handling to validate and durably append only + - Remove the state mutation and the post-mutation notification hook firing from the receipt-time handlers for job start, finish, fail, and update reports + - Keep the existing pre-mutation validation hook firing at receipt time, unchanged + - Observable: after a worker reports a job event, the job's in-memory state is unchanged immediately after the receipt handler returns + - _Requirements: 5.1_ + - _Depends: 2.1_ + - _Boundary: Fact Ingestion — job.c on_job_start/on_job_finish/on_job_fail/on_job_update receipt handlers only, disjoint from handle_jobsubmit_ + +- [ ] 4.2 Unit test durable retention of facts submitted while majority is unreachable + - Submit facts while majority is unreachable and confirm they are durably appended and that job state remains unchanged for the duration of the outage + - Confirm no local-only apply code path exists by inspection of the receipt handler from 4.1 (no runtime check needed — the guarantee is structural) + - Observable: the test demonstrates zero state mutation during a simulated quorum-loss window + - _Requirements: 6.1, 6.2_ + - _Depends: 4.1_ + +- [ ] 5. Restart apply-correctness verification +- [ ] 5.1 (P) Compute a post-replay expectation from the committed log tip and compare it against materialized state + - Derive a lightweight expectation (e.g., last-applied position and a count or checksum of materialized entities) from the recovered log + - Compare the expectation against the actual state produced by replay through the shared apply dispatch function + - Observable: a clean replay produces a CLEAN verdict alongside the existing structural (CRC) verdict + - _Requirements: 8.1, 8.2_ + - _Depends: 2.3_ + - _Boundary: Restart Replay & Verification — persist.c replay/recovery path only_ + +- [ ] 5.2 Route a verification mismatch through the existing non-promotable fence mechanism + - Reuse the existing loud-fence function and sticky fence flag rather than introducing a new terminal or fencing path + - Observable: an injected materialization mismatch produces a DIVERGENT verdict, a loud operator-visible log entry, and a non-promotable fenced state + - _Requirements: 8.3, 8.4_ + - _Depends: 5.1_ + +- [ ] 5.3 Unit test the DIVERGENT verdict path stays alive and keeps retrying + - Force a DIVERGENT verdict and confirm the process does not exit or abort + - Confirm the fence clears once a subsequent recovery reaches a consistent state + - Observable: the test process remains running and retrying after a forced DIVERGENT verdict, never terminating + - _Requirements: 8.4_ + - _Depends: 5.2_ + +- [ ] 6. Ordered apply pump integration +- [ ] 6.1 Build the strict-order walk that applies every newly committed entry + - Walk entries from the last-applied position up to the current committed position, in ascending order, with no skipping or reordering + - Invoke the shared apply dispatch function for every entry in that range, regardless of its commit mode + - Observable: a burst of interleaved command and fact entries committing out of submission order still applies in commit order + - _Requirements: 3.3, 7.3_ + - _Depends: 1.2, 2.2_ + +- [ ] 6.2 Fire a command's registered continuation immediately after its entry applies + - After the walk in 6.1 applies an entry that has a registered commit continuation (from task 3.2), invoke that continuation now, after the mutation, not before + - Leave fact entries with no further action beyond what the apply dispatch function itself performs + - Observable: a successful job submission's client reply carries the job id only after the job is visible in the in-memory job table; the reply always occurs strictly after the mutation + - _Requirements: 1.1, 2.1, 2.2, 2.3_ + - _Depends: 6.1, 3.2_ + +- [ ] 6.3 Integration test command and fact apply/notify timing through the full pump + - Cover a successful job submission becoming visible and notified only after commit, exercised through the full pump (superseding the earlier synchronous-path check from task 3.1) + - Cover that worker slot release, scheduler dispatch triggering, and client/apiserver notification for a fact do not occur until that fact's entry is committed + - Observable: the test suite shows zero fact-reaction side effects during a delayed-commit window, and a submitted job's reply always follows its visibility in the job table + - Confirm a consumer acting on the post-apply notification never needs to separately re-check a commit watermark, since the notification's occurrence already implies the entry is committed and applied + - _Requirements: 2.2, 4.3, 5.2, 5.3_ + - _Depends: 6.2, 4.1_ + +- [ ] 6.4 Integration test quorum-loss-then-restore catch-up ordering through the full pump + - Submit a mix of facts and commands while majority is unreachable, then restore majority and confirm every retained entry commits and applies in its original log order with no local-apply escape hatch exercised + - Observable: the test demonstrates full, correctly-ordered catch-up after quorum returns, with zero out-of-order application + - _Requirements: 6.3, 7.3, 9.4_ + - _Depends: 6.1, 4.2_ + +- [ ] 7. Conformance, regression, and existing-behavior preservation +- [ ] 7.1 Extend the deterministic conformance harness with apply-ordering assertions + - Add an assertion that no entry's state mutation or reactions occur before that entry commits + - Add an assertion that re-applying an already-committed entry is idempotent + - Observable: the conformance harness run reports both new assertions passing alongside its existing safety checks + - _Requirements: 10.1, 10.3_ + - _Depends: 6.3, 6.4_ + +- [ ] 7.2 Add a live-cluster regression scenario that kills the primary mid-fact-stream + - Drive a continuous worker-fact stream, kill the primary partway through, and let a new primary take over + - Assert no phantom slot-frees, no committed-state divergence, and no notification for a fact that never committed + - Observable: the scenario completes with zero divergence and zero phantom reactions across repeated runs + - _Requirements: 10.2, 10.4_ + - _Depends: 6.3, 6.4_ + +- [ ] 7.3 Re-run the full existing automated test suite and update only the tests tied to the old ordering + - Run the existing unit, integration, and HA soak suites + - Update only the specific tests that asserted the old apply-before-commit ordering to assert the new ordering instead + - Observable: the full suite passes with zero committed-job loss and zero state divergence across the existing failover soak + - _Requirements: 11.1, 11.2, 11.3_ + - _Depends: 7.1, 7.2_ + +- [ ] 7.4 Measure sustained fact-stream throughput against the pre-feature baseline + - Record a throughput baseline on the pre-feature code, then drive the same sustained high-volume worker-fact stream against the completed feature + - Confirm no per-fact synchronous commit barrier was introduced and throughput does not regress + - Observable: a recorded throughput comparison shows no regression relative to the pre-feature baseline + - _Requirements: 11.4_ + - _Depends: 7.3_