myxce/.kiro/specs/raft-apply-decoupling/research.md

# 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