feat(raft-apply-decoupling): commit engine resolve primitive + majority-ack wiring (task 1.2)

commit_resolve() delivers a registered continuation's outcome exactly
once (idempotent re-resolve), dispatched off-thread via
vtask_enq_classified(VTASK_LIVE_BLOCK) so it never runs inline on the
replication-ACK or iomgr path. Wired into all 6 commit.c-internal
paths that can advance committed_lsn/writable_lsn.

Review caught a real task-graph gap, not a defect: 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
cond_timedwait deadline). Nothing yet arms an equivalent timeout for
async-registered continuations, and nothing wires demotion/log-truncation
detection to commit_resolve(FAILED, ...) -- left as-is, a pending async
command during quorum loss would hang forever instead of failing,
which is worse than today. Split into new tasks 1.3 (timeout arming)
and 1.4 (demotion/truncation wiring), renumbered the old concurrency
test to 1.5, and made 3.2 depend on both since its failure/timeout
observable is meaningless without them. design.md's File Structure
Plan updated to reflect that ha.c/persist.c need a narrow touch for
1.4's detection hooks.
This commit is contained in:
Rongsong Shen 2026-07-03 09:03:50 +00:00
parent 10d157f8c5
commit fd7a0746eb
5 changed files with 558 additions and 19 deletions

View file

@ -131,9 +131,10 @@ This design has a hard build order, not just a hard conceptual one:
## 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/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). Also adds the timeout-arming mechanism for registered continuations: removing the blocking wait also removes the only thing that ever detected a commit timing out (the caller's own `pthread_cond_timedwait` deadline), so an explicit timeout must be armed at registration and swept/fired independently.
- `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/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. Also adds a call from the existing WAL-truncation point into the commit engine's `commit_resolve()` so any pending registered continuation for a truncated LSN resolves as failed instead of hanging.
- `src/controller/ha.c` — Adds a call from the existing demotion/role-transition point into the commit engine's `commit_resolve()` so any pending registered continuation invalidated by losing leadership resolves as failed instead of hanging. This is in addition to `ha.c`'s other listed change (wiring the apply pump, below).
- `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).

View file

@ -8,18 +8,35 @@
- _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
- [x] 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
- 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
- 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 Unit test the commit engine's resolve behavior under concurrent in-flight commits
- Cover exactly-once resolution across majority ack, timeout, demotion, and truncation
- [ ] 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
@ -65,7 +82,7 @@
- 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_
- _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
@ -173,3 +190,4 @@
- **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.