myxce/.kiro/specs/raft-apply-decoupling/tasks.md
Rongsong Shen 2fa955b92e feat(raft-apply-decoupling): ordered live apply pump activates apply_committed_entry (task 6.1)
Build persist_apply_pump_run()/persist_apply_pump_kick() -- the
ordered walk from last_applied_lsn+1 through committed_lsn, applying
every entry via apply_committed_entry() regardless of commit mode.
This is the first live (non-replay) consumer of that dispatch
function. Kick is wired into commit_resolve_pending() (the single
choke point all 6 internal committed_lsn-advancing paths funnel
through, discovered in task 1.2), dispatched via
vtask_enq_classified(VTASK_LIVE_BLOCK) with a CAS-coalesced
fire-and-forget trigger.

Fixes the two hazards flagged since task 4.1 as required before
going live:

1. Double-append: a thread-local call-provenance guard
   (t_apply_seam_notifying) prevents persist_job_hook_fun/
   persist_queue_hook_fun/persist_worker_hook_fun from re-appending
   entries the apply pump itself just applied, while preserving
   job_ctrl's legitimate live command-path appends (a naive
   detail-based filter would have broken SIGCONT/SIGSTOP). Also
   closes an undocumented instance of the same hazard in the
   queue/worker hook functions.

2. Merge-staleness: record_job_fail_fact/postdone's fact recording
   previously baked a receipt-time-computed final job state into the
   WAL entry, which goes stale under deferred apply -- a FINISH
   followed shortly by an ordinary post-exec FAIL could silently
   clobber a correct DONE completion with a stale RUN|EXIT|PERR, no
   guard catching it. Fixed by repurposing wal_jobstate_t's unused
   _pad field to carry just enough data (FAIL's stage) to recompute
   against the job's actual current state at apply time via the real
   apply_job_fail_transition()/new apply_job_postdone_transition()
   guard cores, instead of overwriting with a stale absolute value.
   Backward compatible: _pad==0 (all prior data) routes to the same
   legacy overwrite behavior.

Also fixes bug-1409 (unbounded busy-loop in apply_pump_drain when
committed_lsn advances without a corresponding on-disk entry) and a
genuine test-flakiness hazard (a second, independent apply-pump
kicker from the fsync_worker thread that existing tests didn't
account for).

Two pre-existing tests in test_commit_barrier_reply.c now fail as
expected -- their own assertions state "no job may be constructed
until task 6's apply pump exists," now correctly invalidated by this
task landing; updating them is task 7.3's explicit job.
2026-07-04 19:00:43 +00:00

67 KiB

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 Build the generic resolve primitive and wire the majority-ack (committed) outcome

    • Implement commit_resolve(): invoke a registered continuation with a committed or failed outcome, exactly once, idempotent on re-resolve
    • On majority ack, invoke the continuation with a committed outcome
    • 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 majority-ack in a test resolves the registered continuation exactly once with a committed outcome
    • Requirements: 1.3, 9.3, 9.4
    • Note: real timeout/demotion/truncation detection (as opposed to the generic resolve primitive) is 1.3/1.4 — see Implementation Notes
  • 1.3 Arm a timeout for registered async continuations

    • commit_submit_async() no longer parks a thread that would otherwise notice its own pthread_cond_timedwait deadline expire — converting away from the blocking wait removes the ONLY mechanism that ever detected a commit timing out. Add an explicit timeout (e.g., a delaymgr-driven check, or a deadline recorded at registration and swept periodically) that resolves a still-pending registered continuation as FAILED with -ETIMEDOUT if majority is not reached within the caller-supplied timeout
    • Preserve the existing default timeout value and error code so callers observe identical failure timing/vocabulary to today's blocking path — this is a "no regression" requirement (11.1), not just a new capability
    • Observable: an async-registered continuation for an LSN that never reaches majority is resolved as FAILED with -ETIMEDOUT within the configured timeout window, with no thread blocked to detect it, verified by a test that lets real time (or a mocked clock) elapse past the deadline without ever calling commit_resolve() manually
    • Requirements: 1.3, 1.4, 9.3, 9.4
    • Boundary: Commit Engine Resolve — commit.c only
  • 1.4 Wire demotion and log-truncation detection to resolve pending continuations as failed

    • When this node is demoted (loses leadership) or truncates its log tail (conflict reconcile), any still-pending registered continuation for an LSN affected by that transition must resolve as FAILED — otherwise a demoted/truncated node leaves requesters permanently unresolved
    • Find the existing demotion/role-transition point and the existing log-truncation point and add a call into the commit engine's resolve path for every pending LSN they invalidate
    • Observable: forcing a demotion or a truncation while a continuation is registered for an affected LSN resolves it as FAILED exactly once, with the existing error-code vocabulary
    • Requirements: 1.3, 1.4, 9.3, 9.4
    • Boundary: Commit Engine Resolve integration points — this task is a narrow, explicit exception to the "commit.c only" boundary used by 1.1-1.3: it may touch the demotion/role-transition site and the log-truncation site (wherever they currently live) in addition to commit.c, but must not touch job.c or any command-handler code
    • Depends: 1.3
  • 1.5 Unit test the commit engine's resolve behavior under concurrent in-flight commits

    • Cover exactly-once resolution across majority ack, timeout, demotion, and truncation, exercised through the real detection paths built in 1.3/1.4 (not by calling commit_resolve() directly, which 1.2's own tests already cover)
    • 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
    • Depends: 1.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, 1.3, 1.4
  • 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

Implementation Notes

  • 1.1: commit_submit_async() reuses the exact wait_admit_locked/wait_release_locked/inflight_waits admission-cap discipline as COMMIT_MODE_WAIT — task 1.2's commit_resolve() MUST call wait_release_locked() when resolving an async-registered entry, or the shared admission cap leaks. Continuation registry is calloc(n_vcpus, ...), sized so n_continuations >= wait_cap(ce) always holds; lsn == 0 is the free-slot sentinel.
  • 1.1 review: bare controller_unit_test (no --run filter) segfaults deterministically in this environment (SSL_CTX_free/iomgr_fini crash, test_ha_vep_teardown.c) — reproduced identically on the pre-task base commit, confirmed pre-existing and unrelated. Logged as bug-1406. Also test_commit_advance_retry_clears_push_before_commit_fence (test_config_repl.c) fails independently of this feature, also reproduced on base — logged as bug-1407, flagged because it's adjacent to commit_advance_locked, which task 1.2 modifies. Always validate with a filtered --run <group> invocation or the tightly-coupled commit-engine cluster, not the bare unfiltered binary, until 1406/1407 are separately fixed.
  • 1.2: implemented commit_resolve() + wired it into all 6 commit.c-internal paths that can advance committed_lsn/writable_lsn (the COMMITTED/majority-ack outcome). Chose VTASK_LIVE_BLOCK for dispatch (never admission-gated, unlike VTASK_REQ_BLOCK which can fast-reject — unacceptable for "exactly-once, never unresolved"). Reviewer caught a real task-graph gap, not a defect in 1.2 itself: converting COMMIT_MODE_WAIT away from a blocking wait also removes the only mechanism that ever detected a commit timing out (the caller's own pthread_cond_timedwait deadline) — nothing yet arms an equivalent timeout for async-registered continuations, and nothing yet wires demotion/log-truncation detection to call commit_resolve(FAILED, ...). Without this, a pending async command during quorum loss would hang forever instead of failing — worse than today's behavior. Split out as new tasks 1.3 (timeout arming) and 1.4 (demotion/truncation wiring), inserted before the renumbered 1.5 (was 1.3, concurrency test — now exercises the real detection paths from 1.3/1.4 instead of calling commit_resolve() directly). Task 3.2 now also depends on 1.3/1.4, since its own "failure/timeout outcome replies" observable is meaningless without them.
  • 1.3: added a dedicated per-commit-engine sweep thread (not the shared delaymgr — would violate boundary + delaymgr's 200ms poll granularity vs. DEFAULT_SYNC_TIMEOUT_MS=500ms would add timing jitter). Race rule: majority-satisfied always wins over an already-expired deadline when both are true under the same lock hold. Round 1 REJECTED: commit_sweep_once() computed the next-deadline snapshot under ce->lock, unlocked to dispatch, then slept on the stale snapshot — a commit_submit_async() registration/broadcast landing during that unlocked dispatch window was lost (no thread parked on the condvar to receive it), so the sweep thread could sleep on an absent/late deadline indefinitely (worst case: unbounded delay in a low-traffic quorum-loss scenario, violating the "identical failure timing" no-regression bar). Fixed by holding ce->lock continuously across scan→wait decisions (releasing only for the unlocked dispatch step) and always re-scanning under the lock immediately before ever calling cond_wait/cond_timedwait — the standard recheck-then-wait idiom. Round 2 reviewer independently reproduced the round-1 bug shape to confirm the fix actually closes it, not just narrows it. Precedent for 1.4: any future code that needs to interrupt the sweep thread's wait (e.g., a demotion/truncation handler resolving a pending LSN out-of-band) must go through the same lock+broadcast discipline commit_submit_async() uses, or it risks the identical lost-wakeup class of bug.
  • 1.4: added commit_resolve_pending_above(ce, lsn_floor, rc) (snapshot-then-commit_resolve()-per-entry, no parallel bookkeeping). Truncation: a single new on_truncate/truncate_cb_ctx callback slot on persist_t (same inversion-of-control shape as the existing on_durable_advance hook) fires from persist_truncate_wal_above after releasing p->lock, only when a truncation actually removed entries — this ONE wrapper covers all three real truncation call sites (raft_cb_become_leader's post-election truncate, the library's on_truncate_from conflict-reconcile, ha_demotion_convergence_fn's post-demotion truncate), verified by the reviewer reading all three. Demotion: raft_cb_become_follower calls commit_resolve_pending_above(&c->commit, 0, -XCE_ERR_STANDBY) (lsn_floor=0 → fails everything — a demoted node can never authoritatively confirm anything pending; verified against the CP posture). Known residual, non-blocking: a fourth internal truncation path (persist.c inside persist_wal_write_replicated's tip-conflict-repair branch, STANDBY replicated-apply only) calls truncate_wal_above_locked() directly, bypassing the new wrapper — safe today only because demotion resolves ALL pending continuations before role flips to STANDBY (so nothing can still be registered by the time that path runs), but this is an implicit ordering invariant, not an asserted one. Flag for whoever next touches the STANDBY replicated-apply path: add a defensive comment/assertion there, or route it through the wrapper too, before relying on it further. Also: bug-1407 was believed closed at this point (70/70 across 3 runs) — see 1.5's note below, this was a false reading, not a real fix.
  • 6.1: the most consequential task in the entire spec — this is what finally makes apply_committed_entry() live (previously only restart replay, task 2.3, ever called it). Built persist_apply_pump_run() (walks [last_applied_lsn+1..committed_lsn] strictly ascending, calls apply_committed_entry() for every entry regardless of commit mode) and persist_apply_pump_kick() (off-thread trigger via vtask_enq_classified(VTASK_LIVE_BLOCK), inline fallback for pool-less fixtures, CAS-coalesced so at most one caller runs the walk at a time — the loser hands off asynchronously, by design, to whichever thread wins). Design deviation, verified correct and a strict superset: wired the trigger into commit_resolve_pending() (commit.c) rather than only ha.c's on_commit_advanced — task 1.2 already discovered commit_resolve_pending is the single choke point all 6 internal committed_lsn-advancing paths funnel through (verified: exactly 6 call sites, commit.c:353/558/1042/1060/1099/1116, all firing after ce->lock releases), while on_commit_advanced covers only 1 of them; wiring only there would silently starve the pump for single-node/self-quorum clusters. design.md should be updated to reflect this as the actual integration point. Fixed Hazard 1 (double-append, flagged since task 4.1): a naive detail==HOOK_DETAIL_JOB_UPDATE filter would have broken job_ctrl's legitimate live SIGCONT/SIGSTOP append (same hook detail, still-legitimate command path) — fixed instead with a _Thread_local bool t_apply_seam_notifying call-provenance guard set only around apply_committed_entry()'s own hook-firing calls; persist_job_hook_fun/persist_queue_hook_fun/persist_worker_hook_fun early-return when set. Also caught and fixed an UNDOCUMENTED instance of the identical hazard class in the queue/worker hook functions (neither had ANY mode-based early-return before this, unlike job's pre-existing dead WAL_MODE_WAIT branch) — would have double-appended+re-triggered a nested majority-commit wait for every live-applied WAL_QUEUE_CTRL/WAL_WORKER_CTRL entry. Took 2 review rounds. Round 1 REJECTED — the most severe finding of the entire session, a real data-corruption bug, not a doc/log issue: Hazard 2 (merge-staleness, also flagged since 4.1) was investigated and ESCALATED as requiring wire-format changes beyond scope, while still landing the pump that activates it. Round-1 review proved with field-level precision this was wrong to ship: job state DONE, correctly baked by a FINISH fact — followed shortly after (an entirely ordinary HPC lifecycle event, not adversarial) by a POST_EXEC-stage FAIL fact whose receipt-time snapshot is STILL stale (RUN, since FINISH hadn't applied yet, apply now being deferred) — bakes RUN|EXIT|PERR into its WAL entry. Both apply in correct LSN order (FINISH then FAIL), but FAIL's stale absolute overwrite wins over the now-correct DONE, because the terminal/non-terminal guard doesn't catch it (both values happen to carry a terminal-ish bit). Final result: a job's correct DONE completion silently replaced by RUN|EXIT|PERR — a Req 11.3 regression, and a real, operator-and-billing-visible corruption newly introduced by deferring apply (this feature's whole purpose), reachable under completely ordinary traffic. The reviewer also found a scoped fix contradicting the "needs wire-format enrichment" escalation: wal_jobstate_t's existing unused _pad field had room. Round 2 fixed it properly: _pad now carries a discriminator (WAL_JOB_FAIL_STAGE_FLAG/WAL_JOB_UPDATE_POSTDONE_MARK) plus the minimal data needed (FAIL's stage) to RECOMPUTE against the job's ACTUAL CURRENT state AT APPLY TIME via the real apply_job_fail_transition()/new apply_job_postdone_transition() guard cores — no more baking a stale absolute value at receipt time. WAL_JOB_START/WAL_JOB_FINISH are unchanged (confirmed safe on their own — deterministic, state-independent payloads that were never the problem, only the thing getting clobbered). Backward-compatible: _pad==0 (all pre-existing/legacy data) deterministically routes to the same absolute-overwrite behavior that was universal before this fix — no crash, no new corruption, correctly out of scope for further hardening since this feature has never been deployed. Bonus finding during round-2 review — a real test-flakiness hazard, root-caused and fixed on the spot, not just reported: a rare (initially estimated ~2%, actually verified ~7.5%) flake in the new regression test was root-caused to a genuine SECOND, independent kicker of the apply pump (commit_on_durable_advance, fired from the real fsync_worker background thread on every durable_lsn advance, entirely separate from repl_on_ack) that the test's own header comment didn't account for — this is by-design eventually-consistent behavior (task 6.2's continuation-firing will correctly rely on it), not a production bug, but the TEST needed a proper poll-for-applied-state helper instead of asserting immediately. Fixed (tap_wait_applied() polling helper added to 2 tests) and a SECOND, related flake found during the investigation in a third test, also fixed — 120/120 clean runs afterward. Also fixed a stale orphaned forward-declaration left over from the round-2 rename. Also fixed a genuine, unrelated bug found via regression sweep, logged as bug-1409: an early version of apply_pump_drain's retry loop didn't check persist_apply_pump_run's return code, causing an unbounded busy-loop (18.5M+ log lines) in test_demotion_convergence whenever committed_lsn advances without a corresponding real on-disk WAL entry (a legitimate scenario for commit-engine-focused unit fixtures) — fixed by returning immediately on a negative rc. Hand-off built for task 6.2: int persist_take_pending_apply(persist_t* p, uint64_t lsn, pending_apply_notify_fn* out_fn, void** out_ctx) — call AFTER apply_committed_entry() applies lsn; returns 1+populates once (atomically unlinking) when a registration exists (the command path, task 3.2), 0 otherwise (the common case, facts never register); a second call for the same LSN is guaranteed to return 0; safe to call unconditionally for every applied LSN. Task 6.2 should call this right after each apply inside/after the walk and invoke the returned continuation — deliberately NOT wired into the walk itself, per boundary. Two pre-existing tests in test_commit_barrier_reply.c now fail as EXPECTED (not a regression): their own assertion text literally says "no job may be constructed until task 6's apply pump exists" — now correctly invalidated by this task landing; explicitly task 7.3's job to update, per task 3.2's own Implementation Notes anticipating exactly this. Minor, non-blocking coverage gap for a fast-follow: no test explicitly replays a WAL_JOB_FAIL/WAL_JOB_UPDATE entry with a real _pad marker through the REPLAY path specifically (only through live apply) — low risk given apply_committed_entry() is the literal shared function both paths call, but untested for that specific combination.
  • 5.3: closes task-group 5. Took 3 review rounds — the heaviest task in the entire series, and the only one that surfaced a genuine CRITICAL production safety bug (not a doc/log issue like 5.1/5.2's rounds). Round 1 (test-only) REJECTED: while writing tests for "the fence stays alive" and "the fence clears on a subsequent consistent recovery," review discovered the sticky recovery_unrecoverable flag is trigger-agnostic — it has ZERO provenance about WHY it was set. Task 5.2 correctly reused this flag for the new apply_divergent trigger, but TWO PRE-EXISTING, live, running-process call sites in ha.c already clear this SAME flag based purely on STRUCTURAL conditions (never checking the apply verdict): promotion_fence_timeout_fn (a self-re-arming periodic timer) and the promotion-eligibility gate in raft_cb_become_leader. Concrete, reachable attack path: a node's local apply dispatch has a defect (materialized state diverges from the committed log) while its WAL bytes are otherwise fine (structurally healthy — orthogonal to the apply defect by construction). The apply-divergent fence sets correctly at boot. But WHILE THE PROCESS KEEPS RUNNING, the periodic timer sees the structural conditions were always fine and silently calls persist_clear_recovery_unrecoverable() — releasing a fence that was never actually resolved. If this node then wins a later election, the promotion gate now sees unrecoverable==false and promotes it to primary — a genuinely divergent node serving authoritative state, a direct Req 8.4 / CLAUDE.md "never serve divergent state" violation. Round 2 fixed the code: added apply_divergent = persist_apply_verdict(&c->persist) == APPLY_DIVERGENT as an additional guard ANDed into BOTH clear-call-site conditions — promotion_fence_timeout_fn (line ~876) and raft_cb_become_leader's gate (line ~1130) — so an apply-divergence-triggered fence can now ONLY be cleared by a fresh process restart's clean persist_recover() (since apply_verdict has exactly two production write sites: persist_init()'s reset-to-APPLY_CLEAN and persist_recover()'s boot-time computation — no live path resets it). Verified via full trace + RED→GREEN for site 1. Round 2 REJECTED anyway: the fix was logically sound, but review PROVED site 2's guard had ZERO test coverage — by deleting only that guard and re-running the ENTIRE 298-test regression sweep across 13 groups, every single test still passed. An unverified defense-in-depth guard on promotion/election-safety code doesn't meet the bar. Round 3 (test-only) closed it: added a test that structurally isolates site 2 from site 1 — proven not to be exercising the already-covered sticky-flag path, since ha_promotion_gate_decision() never populates apply_divergent in its own inputs, so ONLY the round-2-added standalone guard at the call site can catch this scenario. RED→GREEN independently reproduced by two separate reviewers reverting the guard and confirming the exact predicted test failure. Final closed picture, verified end-to-end across all 3 rounds: (a) apply-divergence sets the fence correctly (5.2); (b) survives the structural timer's periodic re-check (round 2); (c) survives the promotion gate's re-check even in the hypothetical where the sticky flag itself somehow reads false (round 3); (d) only clears via a fresh process restart replaying cleanly — confirmed by round 1's own two-separate-persist_t-instances test. Forward note for task 6, already flagged in-repo (test_apply_correctness_verification.c comments), not a defect today: this entire safety picture rests on apply_verdict only ever being recomputed once, at boot. If task 6's live apply pump ever adds a live re-verification of apply-correctness (not currently planned, but worth checking), the "restart-only" assumption these 3 rounds of guards depend on would need re-examination at that time. Lesson for the whole series: reusing an existing mechanism correctly at the logic/decision level (5.2's apply_divergent short-circuit) does not automatically make every OTHER consumer of that same mechanism's output (ha.c's two independent, pre-existing live-clear call sites, written before this feature existed and with no reason to have considered a future trigger) still behave safely — a shared, trigger-agnostic sticky flag is exactly the kind of implicit coupling that needs an explicit audit of ALL its readers/writers whenever a new trigger is added, not just the one you're actively wiring.
  • 5.2: routed APPLY_DIVERGENT (task 5.1) into the EXISTING loud-fence mechanism as a new INPUT, not a new fencing path — added bool apply_divergent to the existing recover_policy_inputs_t; persist_recover_decision() now unconditionally short-circuits to the pre-existing RECOVER_DECISION_FENCE_UNRECOVERABLE enum value when it's true (correct and design-mandated: a divergent local materialization can't be trusted for a peer/catch-up comparison either, so it must always fence regardless of peer state); reuses the same sticky recovery_unrecoverable flag and the same persist_recover_loud_fence_if_unrecoverable() function — confirmed by both review rounds to be the identical pre-existing symbols, zero new fence mechanism. Confirmed the existing promotion-gate consultation in ha.c (~line 1078, persist_recovery_unrecoverable(&c->persist)) already independently reads this flag — zero ha.c changes needed. Zero new self-termination paths (verified by grep both rounds) — stays alive, keeps retrying, per the CLAUDE.md process-lifecycle invariant. Took 2 review rounds. Round 1 REJECTED — real, test-reproduced defect, not documentation: the operator-facing ERROR() log's fixed boilerplate clause ("no healthy peer reachable to catch up; durable_floor_lsn=X < cluster committed high-water hwm=Y") fired UNCONDITIONALLY for every fence decision, including the new apply_divergent trigger — but for that trigger the fields are either hardcoded placeholders or explicitly-set values that made the clause LITERALLY FALSE in test output ("100 < 100", "2 < 0" — numerically impossible claims presented as fact to an operator during a real incident). Before this task, RECOVER_DECISION_FENCE_UNRECOVERABLE was only reachable via the structural "no healthy peer" branch, so the clause was truthful by construction; the new unconditional short-circuit broke that invariant. This directly undermines Req 8.3's "loud, operator-visible" requirement — a log an operator can't trust during an incident is worse than no log. Round 2 fixed by branching the ERROR() call itself (not just the inner reason string) on apply_divergent — that branch now states only the real, verified reason with no peer/HWM claim; the structural-fence branch is byte-identical to before. New regression test uses a genuine (non-mocked) stdout-capture mechanism relying on controller_unit_test never calling tlog_init() (verified true across all ~85 files in the test binary — confirmed the vendored tinylog fallback path used is a real raw write(STDOUT_FILENO,...), not a stub) — both review rounds independently reverted the fix and reproduced the exact false-clause failure before re-confirming GREEN. Lesson, consistent with task 2.2's earlier pattern: reusing an existing decision path correctly at the LOGIC level doesn't automatically make every SIDE EFFECT of that path (like a log message assuming context from the old, narrower set of triggers) still valid for a newly-added trigger — check accompanying log/error text for assumptions that were true only because of the old, narrower reachability, not just the returned enum/decision value itself.
  • 5.1: added a new apply_verdict_t (APPLY_CLEAN/APPLY_DIVERGENT) computed alongside the EXISTING, untouched structural recover_verdict_t (CRC) verdict — a dual-check design: (1) an entity-count check comparing WAL_JOB_SUBMIT entries scanned during replay against the live c->jobmgr table's actual growth (count_jobmgr_entries()), genuinely independent since it queries live state rather than trusting apply_committed_entry()'s return code — verified real by both review rounds via a forced job-count-mismatch test; (2) a position check comparing an "expected" LSN against p->last_applied_lsn (new _Atomic uint64_t field, per design.md, forward-looking infrastructure for task 6's live pump). Took 2 review rounds. Round 1 REJECTED — real, live-today bug, not documentation: the position check's "expected" value was computed as recovered_tip = current_lsn - 1 (the PHYSICAL WAL-scan tip, which replay_wal() advances for every entry including WAL_CHECKPOINT markers and unknown types) instead of the LOGICAL apply cursor. persist_checkpoint() durably fsyncs the checkpoint marker in one step, then persists the advanced checkpoint_lsn to the on-disk snapshot in a LATER step — a crash between those two steps (a real, unavoidable crash-safety window this codebase already engineers hard for, cf. bug-661/ha-commit-durability) leaves a trailing checkpoint marker as the highest-LSN entry on next boot, causing recovered_tip to exceed the true last_applied_lsn and falsely report APPLY_DIVERGENT on a perfectly clean recovery — a direct Req 8.2 violation, reachable TODAY on any node that crashes at exactly the wrong moment during a checkpoint. Round 2 fixed by reading p->last_applied_lsn directly (the value replay_wal() already correctly advances only for entries actually dispatched through apply_committed_entry(), skipping checkpoint-marker/unknown-type entries) instead of recovered_tip; recovered_tip itself is untouched and still correctly feeds durable_lsn, which does need the physical tip. New regression test genuinely reproduces the trigger condition (real persist_checkpoint(), then unlink()s the exact snapshot file it wrote, landing on-disk state in the identical place a real crash would) — both review rounds independently reverted the fix and reproduced the false-DIVERGENT failure before re-confirming GREEN. Known, disclosed, accepted scope gap (not a defect, flagged for whoever extends this check): the entity-count check covers jobs only. Queues are correctly excluded (config-derived, WAL_QUEUE_CTRL never creates a new one, verified by both review rounds). Workers are NOT covered but CAN diverge: worker_from_capnp()/restore_worker() genuinely creates a new worker on a WAL_WORKER_CTRL/HOOK_DETAIL_WORKER_JOIN entry when absent from workermgr — a worker-creation divergence is currently invisible to this verification. Extend persist_verify_apply_correctness() with a worker-count check (mirroring count_jobmgr_entries()) if/when this needs to be closed. Self-disclosed, accepted limitation: because replay_wal()'s single-threaded caller in persist_recover() derives both the "expected" and "actual" position values from the same atomic within moments of each other on the same thread, the position check is diagnostically tautological FOR THIS SPECIFIC CALL SITE today (though the entity-count half remains genuinely independent) — this will change once task 6's live apply pump gives expectation and actual state a real chance to diverge across threads/time; task 5.1's boundary does not require solving that now.
  • 4.2: pure test-only close-out of task-group 4 — one new test test_fact_state_unchanged_throughout_quorum_loss_outage in test_commit_barrier_reply.c (30→31), approved first round, zero production changes. Drives a genuine repl_remove_standby-based quorum-loss window through all 4 real fact-receipt handlers, polls live job state + committed_lsn at 5 time-spread points (real 20ms sleeps, ~100ms total) rather than once, asserts a real hook-spy never sees AFTER_EVENT fire, and confirms durable WAL presence via direct on-disk segment read (cbrp_count_jobstate_entries) — combining and strengthening two pre-existing tests that each covered only half (4.1's own single-point-in-time check under normal conditions; an older pre-4.1 test that proved WAL durability but never inspected live state). Static/inspection half of Req 6.2 confirmed independently by the reviewer, re-reading 4.1's landed diff: none of the four record_*_fact functions call apply_committed_entry/restore_job/mutate the real job object — record_job_fail_fact specifically operates on a throwaway job_t scratch = *job shallow copy. Architecturally significant finding for task 6 planning: facts have NO commit-timeout/failure concept at the receipt layer, unlike commands (task 3.2's commit_submit_async). persist_submit_job_fact() → wal_hook_append_classified(WAL_MODE_RETAIN) → commit_submit(..., COMMIT_MODE_RETAIN, 0), and COMMIT_MODE_RETAIN's branch in commit.c is an immediate, unconditional return with NO continuation registered (explicit comment: "RETAIN never honors the timeout") — commit_resolve() never fires for a retained fact. A fact's outage is therefore open-ended by construction; its eventual commit is discovered only when commit_advance_locked()/commit_on_ack notices committed_lsn can advance past it via the ACK-registry watermarks, purely. Task 6's apply pump must rely entirely on committed_lsn advancement to discover newly-eligible retained facts — there is no per-fact signal to wait on. Confirmed neither of 4.1's two flagged hazards (double-append, merge-staleness) is triggered by this test, since it never drives apply. Task-group 4 (fact apply-after-commit split) is now complete: 4.1 (receipt converted to durable-append-only, two critical hazards discovered and documented for task 6) → 4.2 (retention proven under sustained quorum loss, fact-vs-command architectural asymmetry documented). Both task-groups 1-4 are now done; task 6 (ordered apply pump) is the critical next milestone that must resolve, before going live: the double-append hazard, the merge-staleness hazard, AND design around facts' lack of a per-entry commit signal (commit_advance/committed_lsn-driven discovery only). Groups 5 (restart verification) and 6 (apply pump) remain; 5.1 depends only on 2.3 (already done) so could run in parallel with 6, per tasks.md's dependency graph — continuing in numeric order (5 next) unless reprioritized.
  • 4.1: converted the four worker-fact receipt handlers (on_job_start, on_job_finish, on_job_fail, on_job_postdone — NOT literally named on_job_update, task text's naming was approximate) to validate-and-durably-append-only: BEFORE_EVENT validation hook unchanged, AFTER_EVENT (post-mutation) hook no longer fires at receipt, live state mutation removed. Key discovery: the durable WAL append was NOT a separate call — it lived INSIDE the very persist_job_hook_fun AFTER_EVENT subscriber this task needed to silence, so a new persist_submit_job_fact() (persist.c) was added that calls the same unmodified wal_hook_append_classified(WAL_MODE_RETAIN, wal_jobstate_t{jobid,state}, ...) directly — verified wire-format-identical to before, apply_committed_entry()'s existing switch (task 2.2) routes it unchanged. mark_job_start/mark_job_finish (single caller each, verified via diff against 41d3620) renamed in place to record_job_start_fact/record_job_finish_fact; mark_job_fail/mark_job_update (shared with job_ctrl's OUT-OF-BOUNDARY client-command path, 2 and 3 additional call sites) left untouched, with new durable-append-only siblings record_job_fail_fact/record_job_update_fact added instead — job_ctrl/do_jobcontrol_op confirmed byte-identical to pre-task by the reviewer. Approved first round. Real, permanent regression accepted, not fixed (Req 5.1 required the removal): on_job_postdone's finish_time = time(NULL) anomaly backstop is gone with no wire-format-level equivalent — concretely, a job hitting this rare no-prior-exit-status path now has finish_time stuck at 0 forever, which job_should_retain() (persist.c) evaluates as immediately GC-eligible instead of retained for the configured window. Narrow, rare-path, but real. TWO CRITICAL HAZARDS DISCOVERED BY REVIEW, MUST BE RESOLVED BEFORE TASK 6 WIRES apply_committed_entry() INTO A LIVE PATH — currently inert only because replay runs before persist_start() registers hooks (cluster.c: persist_recover() at line ~1451 vs persist_start() at ~1694):
    1. Latent double-append: persist_job_hook_fun is STILL registered on (XCTL_HOOK_JOB, HOOK_AFTER_EVENT) with no detail filter for HOOK_DETAIL_JOB_UPDATE, and does NOT early-return for that detail (it only early-returns for the WAIT-mode submit path). The moment task 6 wires apply_committed_entry() into live commit-advance, EVERY fact apply_committed_entry() applies will fire HOOK_DETAIL_JOB_UPDATE, which persist_job_hook_fun will catch and append AGAIN — a duplicate, mis-typed WAL entry per live-applied fact. Task 6.1/6.2 must retire or detail-filter persist_job_hook_fun's fact-handling branch before going live, since no live receipt path fires AFTER_EVENT for facts anymore (this task removed the only other source).
    2. Merge-staleness on deferred apply: record_job_fail_fact/record_job_update_fact precompute a FINAL target state at receipt time from a snapshot of current job state and bake it verbatim into the WAL entry (their own doc comments claim this "can never drift from what apply time will eventually compute" — that claim was TRUE only when receipt and apply were the same event; it becomes FALSE the moment they're separated by a commit round-trip, which is this entire feature's purpose). apply_committed_entry() routes all job-state facts through the generic apply_job_update_transition, which does a blind job->job.state = state overwrite — not the type-specific |= merge apply_job_fail_transition uses. Once two facts for the same job are in flight simultaneously (now newly possible, since apply is deferred), the second one to apply can silently clobber bits the first one set. Root cause is task 2.2's wire-format/generic-routing reality, not this task's diff — but no task in the current plan (4.2, 5.x, 6.x) tests or fixes this. Task 6 planning must resolve this before wiring live apply — likely requires either wire-format enrichment (baking the merge operation itself into the WAL entry, not just the precomputed result) or an apply-time re-derivation strategy.
  • 3.3: pure test-only close-out of task-group 3 — 4 new tests added to test_commit_barrier_reply.c (25→29), approved first round, zero production changes. Closes two gaps 3.2 flagged: (1) an explicit-FAILED test via persist_truncate_wal_above/commit_on_truncate/commit_resolve_pending_above(-ENOENT), distinct from the existing timeout (-ETIMEDOUT) path; (2) the array_size > 1 mixed-outcome gap — one element ACKed to real majority while two time out, proving the committed sibling's persist_register_pending_apply hand-off isn't blocked by failing siblings, zero phantom jobs for the failures, and exactly one batch-level failure reply (via jobsubmit_reply_failure_once's ref-counted first-failure-wins gate). Also added two repeated-failure loop tests (5 independent cycles each, fresh jobid/LSN per iteration) proving the no-phantom-job guarantee isn't a one-shot fluke. Confirmed correct, non-obvious harness detail: this file's fixture wires job-submit continuations through a standalone commit_engine_t g_hce, NOT cluster_t's embedded c->commit field that demotion (raft_cb_become_follower) resolves against — so truncation, not demotion, is the only deterministic way to force an explicit FAILED (non-timeout) outcome in this specific test file. Demotion-forces-FAILED already has its own dedicated coverage in test_demotion_convergence.c (tasks 1.4/1.5) via a different fixture — not a gap, a file-boundary distinction. Non-blocking observation carried forward, not fixed here (pre-existing since 3.2, correctly out of scope for a test-only task): the single batch-level failure reply carries no per-element granularity — a client can't tell "1 of 3 committed" from "all 3 failed" from the reply alone. Task-group 3 (command apply-after-commit ordering) is now complete: 3.1 (job construction deferred past commit, bug-1408 caught+fixed) → 3.2 (converted to real async continuation-based commit, majority-ack safety independently verified intact, several documented design deviations) → 3.3 (failure-path guarantee proven under repetition and array mixed-outcomes). Standing operational blocker carried from 3.2, still true: no live path applies a successfully-committed job submission until task 6 exists — this branch must not be soak-tested or deployed against live traffic yet.
  • 3.2: converted handle_jobsubmit() from 3.1's still-synchronous commit to a real async registration against task 1's continuation API. New persist_submit_job_payloads_async() (persist.c) appends the WAL entries via the same ha_primary_log_append/persist_wal_async primitive the pre-existing RETAIN-fact path (wal_hook_append_classified) already uses, then calls commit_submit_async() directly on p->repl.ce — bypassing repl_wait_majority_ack/repl_submit_majority_commit in repl.c, which is what design.md's text actually specifies as the integration site. Independently verified safe (reviewer traced the full ACK→commit_resolve() path in commit.c): majority-ack detection is driven entirely by (ce, lsn) events inside the commit engine, agnostic to which wrapper registered the continuation — no false-commit risk. But it's a real, permanent design deviation, not just an implementation detail: this creates a THIRD parallel "append+track a commit" implementation (alongside the old sync one, now alive only in 3 test files, and the RETAIN-fact one), narrowing the reusable primitive design.md wanted — a future command handler (e.g. a config-change command) gets no shared plumbing from this work and would need its own bespoke path. On FAILED/timeout, replies failure immediately via jobsubmit_reply_failure_once(), no job ever constructed. On COMMITTED, calls new persist_register_pending_apply(p, lsn, on_applied, ctx) — a hand-off linked list on persist_t for task 6.1 to consume — and does NOT apply or reply, per the task's explicit instruction. Deleted (not preserved-dead) task 3.1's bug-1408 create_job-loop/notify block: verified correct — apply_committed_entry()'s existing WAL_JOB_SUBMIT case (task 2.2) already fires HOOK_DETAIL_JOB_NEW per-entry independently, structurally immune to bug-1408's batch-suppression class; resurrecting the old loop in task 6 would double-construct/double-notify. Original code fully recoverable from git show 6f1fa97:src/controller/job.c if ever needed. CRITICAL OPERATIONAL CALLOUT — this branch must NOT be soak-tested or deployed against live traffic until task 6 lands: there is currently NO live code path that applies a successfully-committed job submission (only restart replay, task 2.3, does) — every successful job submission produces a durably-committed WAL entry but ZERO reply to the client, forever, until task 6.1/6.2 exist to consume the pending-apply hand-off and complete the reply. This is exactly what the task text sanctions ("no independently observable behavior until task 6"), not a bug, but it means this commit range is only safe in isolated unit-test form right now. Hand-off mechanism for task 6.1's briefing: persist_register_pending_apply()/persist_has_pending_apply() exist; there is NO "take and remove" accessor yet — task 6.1 must build the consumer side itself. The per-batch jobsubmit_async_ctx_t (ref-counted across array elements, frees only once every element has either failed-and-replied or been applied via jobsubmit_apply_complete) will not free for any batch containing a committed element until task 6 exists — an accepted, bounded-by-test-scope leak today, but would be unbounded growth under any live/soak run before task 6 lands. Outcome encoding uses a plain int rc (not commit_outcome_t+rc) to avoid a persist.h→commit.h circular dependency — verified unambiguous today (every real FAILED call site in commit.c passes non-zero rc) but not compiler-enforced; fragile by convention, not by construction. New regression found by review, not yet fixed, non-blocking: array_size == 0 now hangs the client forever and leaks one jobsubmit_async_ctx_t per request (3.1's version replied synchronously even for this input; 3.2's async loop never runs so the ctx never releases) — confirmed NOT reachable via any current legitimate client (Go apiserver always sets arraySize >= 1; xce_test's cmd_jobsub clamps arsize <= 0 to 1), so flagged as a defense-in-depth hardening gap for a future task rather than fixed here. Also no array_size > 1 mixed-outcome test exists yet (per-element independent resolution verified correct by code inspection, not by test) — fast-follow recommended.
  • 3.1: reordered handle_jobsubmit() (job.c) so job objects are only constructed via create_job() AFTER the (still-synchronous) WAL commit succeeds — extracted jobid_range_exists() (fast-fail pre-check, no WAL work), build_job_submit_payloads() (WAL-payload construction with no jobmgr insertion), and a new persist.c sibling persist_submit_job_payloads_and_commit() (operates on raw xce_job_t* instead of pre-existing job_t*, single production call site in job.c — legitimately within handle_jobsubmit's call chain, not a boundary spillover; no task-1 continuation API dependency, genuinely still synchronous). Took 3 review rounds. Round 1 REJECTED — real defect, not documentation: the post-commit array-construction loop suppressed the AFTER_EVENT/HOOK_DETAIL_JOB_NEW notification for an ENTIRE array batch if even one sibling element failed to construct (e.g. a jobid collision slipping past jobid_range_exists()'s inherent TOCTOU window, or a plain allocation failure — no race needed) — stranding OTHER elements that were durably committed AND successfully constructed, invisible to the purely event-driven scheduler/apiserver until the next restart's replay. Logged as bug-1408. Round 2 fixed the defect (moved the construct_rc < 0 early-return to AFTER the jlist-build + hook-fire block, so every successfully-constructed sibling is always notified regardless of a sibling's failure; client reply still separately reports the batch's construct_rc) but was REJECTED again on a narrower, documentation-only ground: bug-1408's RED-phase writeup claimed a clean assertion FAIL, but under the standard full-group --run test_commit_barrier_reply invocation the failing assertion actually longjmps past the test's own cbrp_teardown() call (Unity TEST_ABORT semantics) and HANGS the next test — the same longjmp-past-teardown pathology already known from task 1.3's era. Round 3 (parent-handled directly, no code regression risk): corrected bug-1408's buglog.json description to accurately describe the hang mode, and hardened the new test itself — every assertion-relevant observable is now captured into a local, cbrp_teardown() is called unconditionally, and assertions run only after teardown, so a future regression in this exact test can no longer skip cleanup and deadlock sibling tests. No production code changed in round 3; job.c's fix from round 2 was independently verified correct by the round-2 reviewer (traced jlist-build/hook-fire at job.c:717-766 running unconditionally before the construct_rc check at line 775) and needed no further changes. Known low-severity residual, not blocking, flagged for a future task: apiserver_push_joblist (apiserver.c) lacks the nfiltered > 0 guard sched.c's equivalent has, so a total-batch construction failure (every array element fails, jlist ends up empty) now sends an empty/no-op jobs push to every connected apiserver — informationless, can't corrupt state, but a minor behavioral quirk introduced by firing the hook unconditionally.
  • 2.4: pure test-only close-out of task-group 2 — new test_apply_reapply_idempotency.c (3 tests), no production changes, approved first round. Covers three distinct cases beyond 2.2's existing coverage: (1) triple-reapplication of an identical WAL_JOB_SUBMIT through apply_committed_entry() with hook-fire-count AND pointer-identity assertions across all 3 calls (not just final state); (2) the JOB_TRANSITION_NOOP_DUPLICATE guard branch (state == current state) — a genuinely untested path, distinct from 2.2's forward-once and backward-rejected cases; (3) the task's named scenario, driven entirely through the real dispatch seam (not seeded/shortcut): real SUBMIT → real FINISH → stale out-of-order START (lower LSN, arriving after), asserting the job stays in its FINISH/terminal state and no third hook fires for the rejected START. Test assertions key on hook-fire COUNT and job STATE, never a specific HOOK_DETAIL_* value — so these tests stay valid even if 2.2's documented generic-hook-routing deviation is later corrected. Task-group 2 (shared apply seam) is now complete: 2.1 (guard cores extracted) → 2.2 (dispatch function built, generic job-state-family routing is a confirmed permanent deviation from design.md, tracked) → 2.3 (replay wired to the seam, duplicate-notification-on-restart hazard confirmed inert today but must be re-verified before 4.1/6.1/6.2/6.3) → 2.4 (idempotency proven through the real dispatch path). Next: task-group 3 (command apply-after-commit ordering), which is the first consumer of both this seam and task-group 1's commit-engine continuation API.
  • 2.3: rewired replay_wal() (persist.c) to call apply_committed_entry() (task 2.2's seam) instead of apply_wal_entry() for restart replay's per-entry apply. apply_wal_entry/apply_committed_entry are siblings, not wrapper/wrappee — apply_wal_entry is NOT retired, it remains ha.c's live standby catch-up/replicated-apply dispatch (untouched, different call sites, out of this task's boundary). Confirmed hazard, investigated to resolution by the reviewer (not left open): replay_wal(p, p->checkpoint_lsn, ...) replays the FULL WAL range since the last snapshot on every restart (last_applied_lsn is deliberately not persisted — recomputed at replay time per design.md), not just a narrow crash-gap tail. Since apply_committed_entry fires AFTER_EVENT hooks (unlike the old apply_wal_entry, which never did), this means every restart re-fires notification hooks for entries already applied+notified in a prior process life — a duplicate-notification hazard in principle. Independently traced and confirmed inert today for both existing hook consumers: scheduler hooks aren't registered until on_scheduler_join() (sched.c), which happens post-boot via network JOIN handshake, strictly after persist_recover() runs; apiserver hooks ARE registered before persist_recover() (apiserver_init() in cluster_init_phase2()), but every apiserver hook body gates its actual network notify on apiservers[i].ep != NULL, and the apiserver-join table starts empty each process life — a no-op except via a narrow reconnect race. MUST be re-verified before tasks 4.1/6.1/6.2/6.3 make this path unambiguously live and broad (e.g. if a future task registers apiserver connections earlier, or scheduler hooks get registered pre-replay) — at that point this either needs a replay-mode flag suppressing hook-firing, or an explicit accepted-idempotent-duplicate-notification decision recorded in design.md. A loud in-line comment cross-referencing this note now sits at the apply_committed_entry() call site in replay_wal() (persist.c). Also confirmed: job-state facts now replay through 2.1's guarded apply_job_update_transition() instead of the previously-unguarded restore_job_state() — intentional (Req 3/7 online-replay divergence fix), verified no existing test depended on the old unguarded semantics. apply_committed_entry's missing oversized-payload guard (vs. apply_wal_entry's MAX_WAL_PAYLOAD_BYTES/-EMSGSIZE check) confirmed inert — persist_wal_write_replicated() already rejects oversized payloads at write time, before anything reaches disk.
  • 2.2: implemented apply_committed_entry(persist_t*, uint32_t wal_type, uint64_t lsn, const void* payload, uint32_t len) in persist.c/persist.h, matching design.md's signature. Routes WAL_JOB_SUBMIT through restore_job() (idempotent on duplicate jobid — no-op, Req 7.1), WAL_QUEUE_CTRL/WAL_WORKER_CTRL through their existing restore primitives, and fires each type's post-mutation hook exactly once, after mutation. Confirmed permanent, non-cosmetic deviation from design.md's Apply Seam wording: ALL nine job-state fact WAL types (WAL_JOB_{START,FINISH,FAIL,STOP,RESUME,UPDATE,RUSAGE,CKPT,PREASON}) route through the single guard-only apply_job_update_transition() core from 2.1 — NOT the type-specific apply_job_{start,finish,fail}_transition cores — and fire one generic HOOK_DETAIL_JOB_UPDATE hook, not the type-specific HOOK_DETAIL_JOB_START/_EXIT/_FAIL. Root cause, independently verified by two review rounds: the on-disk wire format for every one of these types (wal_jobstate_t {jobid, state}) genuinely only carries the resulting state, not the richer job_pidinfo_t/xce_jobfinish_t/job_failinfo_t payload the type-specific cores/hooks require — this is a pre-existing wire-format reality (apply_wal_entry already collapsed these the same way before this task), not a shortcut invented here. design.md's Apply Seam section and Fact Ingestion → Event Contract section currently describe per-type hook dispatch that this wire format cannot support as written — this is a real, standing design/reality conflict, not yet reconciled in design.md itself. No task in the current plan (2.3, 4.1, 4.2, 6.1, 6.2, 6.3) enriches the wire format to close this gap — confirmed by reading all of them; task 4.1 is exclusively about removing receipt-time mutation/hook-firing, not payload enrichment. Explicit blocking-style call-out for whoever implements 4.1/6.2/6.3: wiring this dispatch function into a live fact-apply path will permanently change every worker-reported job start/finish/fail notification to a generic UPDATE detail instead of the type-specific one, unless the wire format is enriched first or design.md is corrected to formally accept this substitution — verify this is still the accepted behavior before wiring it live, don't assume it'll be fixed by then. Took 3 review rounds to land: round 1 rejected two doc-comment defects (a citation blaming the wrong crash site, and a dangling pointer to task 4.1 as the fix location); round 2's fix corrected the crash site (real one is sched.c's sched_job_start_fun/_exit_fun/_fail_fun, which unconditionally dereference info->jobid before any NULL check) but fabricated a nonexistent function name (encode_JobPIDInfo_ptr) as supporting evidence; round 3 replaced it with a narrower, fully source-verified claim (apiserver.c's equivalent functions store the raw pointer into a notify union without dereferencing it at that call site — real, checked line-by-line). Lesson: when a reviewer catches a citation error, the safest fix is the narrowest claim you can personally verify by reading the actual code, not a broader-sounding replacement — round 2's attempt to explain "why the alternative is fine" reached for an unverified downstream mechanism and got caught fabricating it.
  • 2.1: split mark_job_start/mark_job_finish/mark_job_fail/mark_job_update (job.c) into four hook-free cores (apply_job_{start,finish,fail,update}_transition, new job_transition_result_t enum in cluster.h) containing only the monotonic-rank guard + mutation, verified an unmodified lift line-by-line against pre-extraction code. Wrappers peek the same guard predicate to decide hook-firing, then delegate to the core — no unlock/relock gap between peek and delegated call, so no new race from the double-evaluation. mark_job_fail's pre-existing lack of a guard is preserved, not added or removed. New test_apply_seam.c (8 tests) calls the cores directly, bypassing on_job_* receipt handlers entirely — this is the form task 2.2 (dispatch function) and task 6.1 (apply pump) will call into. Non-blocking follow-up flagged by reviewer: the new doc-comments on apply_job_fail_transition/apply_job_update_transition overclaim a universal "caller holds RTTI_LOCK_WRITE" precondition — job_ctrl (job.c:396, pre-existing, untouched) calls mark_job_update/mark_job_fail under only RTTI_LOCK_READ. No new race (extraction added no unlock/relock), but whoever writes task 2.2/6.1 should not trust that comment's blanket claim — verify the actual lock held at each call site instead.
  • 1.5: pure test-only close-out of task-group 1 — 3 new genuine-multi-thread tests added across test_commit_resolve.c/test_demotion_convergence.c/test_persist_truncate.c (real pthreads, not sequential calls dressed as concurrent): an admission-cap burst with a concurrent monitor thread, a mixed majority-ack/timeout-sweep burst proving bounded dispatch-thread usage, and a genuine two-real-thread ack-vs-sweep-thread race (closing the "single-thread white-box only" gap both task-1.3 review rounds had flagged as a nice-to-have). No production code touched. bug-1407 saga, definitively resolved: it was NEVER a source bug. 1.4's "closed, 70/70" and 1.5's initial "reopened, 10/10 failing" were BOTH real, correct, contradictory measurements of the exact same source commit (9b4b805) — the difference was a stale/corrupted incremental-build artifact in the shared build/ directory, which had been ninja -C build-rebuilt incrementally across all 5 tasks without ever a fresh meson setup. Proven by isolating each measurement in its own throwaway git worktree add + fresh meson setup/ninja: pre-feature commit and the exact failing commit both pass cleanly (0 failures) on a fresh build; only the main worktree's stale incremental build/ produced the failure. Fixed with rm -rf build && meson setup build --buildtype=debug -Db_sanitize=address && ninja -C build; bug-1407 is now genuinely CLOSED with high confidence. Lesson for every remaining task in this series: distrust a surprising/contradictory ninja -C build incremental result before concluding a source regression — rebuild clean (or isolate via a throwaway worktree) first. Never use git checkout <sha> -- . to inspect an old commit in place — it silently reverts working-tree files against current HEAD; use git worktree add <path> <sha> instead. Both lessons are now in .wolf/cerebrum.md Do-Not-Repeat and .wolf/buglog.json bug-1407.