docs: promote raft-apply-decoupling spec family from /kiro-discovery
Adds the Raft Apply-Ordering & Hook Decoupling roadmap phase: canonical pre-hooks -> raft commit -> apply -> notify ordering, with the raft protocol step lifted out of the notification hook fan-out (root of the bug-795/828/ 851/1060 divergence family). Decision: apply-after-commit via a single shared apply_committed_entry() seam reused by online apply and restart replay. - raft-drive-async: full spec docs (brief/requirements/research/design) committed alongside its already-tracked tasks.md; async-drive prerequisite for the apply-decoupling work, sequenced to start first (threatens failover liveness today). - raft-apply-decoupling: new spec (brief + facts-in-raft.md) covering the apply seam and the facts (WAL_MODE_RETAIN) apply-timing question (open decision D1). - job-array-single-wal-record: promoted from backlog to a tracked spec, sequenced after raft-apply-decoupling. - ha-consistency-invariants: add criterion 2.9 (Facts-Apply invariant), documenting the current on-receipt fact-reaction deviation this family will reconcile. - raft-commit-safety / raft-log-replication: reconcile spec.json phase to "implemented" (both were mislabeled tasks-generated despite full 18/18 and 24/24 task completion + raft_sim GO). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SDWVNQLKxqTVHh42cH7rV3
This commit is contained in:
parent
9318f9977e
commit
5f307e297b
14 changed files with 1075 additions and 16 deletions
86
.kiro/specs/raft-apply-decoupling/brief.md
Normal file
86
.kiro/specs/raft-apply-decoupling/brief.md
Normal file
|
|
@ -0,0 +1,86 @@
|
|||
# Brief: raft-apply-decoupling
|
||||
|
||||
## Problem
|
||||
|
||||
The controller's Raft consensus protocol is **embedded inside the notification hook fan-out**, and in-memory state is mutated **before** the entry is committed. This couples three concerns that canonical Raft keeps separate (log → commit → apply → notify):
|
||||
|
||||
- `persist_job_hook_fun` is registered as an `AFTER_EVENT` hook, and *inside* that callback it performs the WAL append (`ha_primary_log_append` → `raft_log_append`) **and** blocks on the synchronous majority-commit barrier (`repl_wait_majority_ack`). The raft protocol step is therefore literally a side effect of running a hook (`persist.c:2532`, `persist.c:2198`, `ha.c:3080`).
|
||||
- `handle_jobsubmit` mutates the in-memory job table via `create_job()` **before** the WAL/commit step (`job.c:558` runs before the AFTER hook at `job.c:593`). A commit that fails or times out does **not** cleanly revert that optimistic mutation, leaving a window where in-memory state diverges from the committed log.
|
||||
- Because apply happens before commit, every downstream consumer (scheduler push, client reply) must defensively re-gate on a commit watermark (`sched.c:38` `job_submit_committed`) to avoid acting on not-yet-committed state — scattering commit-awareness across the codebase instead of centralizing it at one apply point.
|
||||
|
||||
This is the structural root behind the bug-795/828/851/1060 divergence family: the controller has no single, ordered "apply committed entry to the state machine" seam.
|
||||
|
||||
## Current State
|
||||
|
||||
- Hooks (`src/lib/hook.c`) are a typed pub/sub fan-out (`xce_run_hooks`) with `HOOK_BEFORE_EVENT` / `HOOK_AFTER_EVENT` situations. Subscribers: `persist_job_hook_fun` (durability — does WAL+commit), `sched_job_hook_fun` (scheduler push), apiserver SSE callbacks.
|
||||
- Order today: `BEFORE hook → create_job() (state mutation) → AFTER hook { WAL append + majority commit barrier } → client reply → scheduler push (commit-gated)`.
|
||||
- The raft library (raft-log-replication, raft-commit-safety) already OWNS the replicated log and exposes pure callbacks (`on_persist_entry`, `on_commit_advanced`, etc.). The library half is the right protocol core; the controller half is what is mis-layered.
|
||||
- Restart replay (`persist_recover` → `replay_wal` → `apply_wal_entry`, `persist.c:1950/1735/1606`) validates only the CRC chain. There is **no** post-replay check that materialized in-memory state equals the committed log.
|
||||
|
||||
## Desired Outcome
|
||||
|
||||
The controller follows canonical Raft ordering with one explicit apply seam:
|
||||
|
||||
1. **pre-hooks** (validation / preparation only — no authoritative state change)
|
||||
2. **raft step** — append entry → replicate → majority commit (an explicit protocol step, NOT a hook callback)
|
||||
3. **apply** — a single `apply_committed_entry()` seam mutates the in-memory state machine *after* commit
|
||||
4. **after-hooks** = pure notification delivery (scheduler push, client reply/SSE, apiserver) — they observe already-committed, already-applied state and no longer carry WAL/commit responsibility
|
||||
|
||||
A commit that never reaches majority therefore never applies, eliminating the optimistic-divergence window. On restart, the same `apply_committed_entry()` seam is reused by replay, and a verification step proves materialized state == committed log.
|
||||
|
||||
## Approach
|
||||
|
||||
Apply-after-commit (canonical), chosen 2026-06-24. Invert the submit path so the raft commit gates the apply:
|
||||
|
||||
- **KEYSTONE PRE-REQUIREMENT (must land first):** convert the protocol drive from blocking to event-driven. `repl_wait_majority_ack()` today blocks on `pthread_cond_timedwait` (commit.c:359) — a parked thread per in-flight commit. Build its async counterpart: append → register `pending(LSN)` → resolve on majority-ACK/timeout/demotion → drive apply + fire reply continuation. B (apply self-deadlock), C (reply lifecycle), and vtask-pool exhaustion are all downstream of this one conversion. See `facts-in-raft.md` § Keystone. The rest of this approach assumes the drive no longer blocks.
|
||||
|
||||
- Extract WAL append + commit barrier out of `persist_job_hook_fun` into an explicit controller-side raft step invoked directly by command handlers (not via the hook system).
|
||||
- Introduce a single `apply_committed_entry()` function that the commit path calls **after** `on_commit_advanced`, and that WAL replay reuses on restart — guaranteeing online-apply and replay-apply are the same code (no divergence-by-two-implementations).
|
||||
- Demote the hook fan-out to notification-only: after-hooks fire from the apply step, observing committed state. Remove the now-redundant `job_submit_committed` watermark gating where the new ordering makes it unnecessary.
|
||||
- **Commands vs facts policy:** make commands (`WAL_MODE_WAIT`: job submit, config) strictly apply-after-commit. Facts (`WAL_MODE_RETAIN`: worker-reported start/finish/fail/rusage) are applied on receipt today because the controller reacts immediately. **Direction set 2026-06-25: route facts through commit-then-apply as well** — candidate plan and 15-step list in `facts-in-raft.md` (this spec dir), formalized as the **Facts-Apply invariant** (criterion 2.9) in `ha-consistency-invariants/requirements.md`. After the change, commands and facts share the one `apply_committed_entry()` seam and differ only in ingress + commit-wait/failure policy (see the comparison table in `facts-in-raft.md`). The one remaining open decision is **D1** (quorum-loss reaction policy: block-but-durable vs local-apply allowlist) — resolve before Phase C.
|
||||
- **Restart raft-log-applied validation (item #4, in-scope):** add a post-replay verification that the materialized entity set matches the committed log tip (e.g., count/hash of applied entities vs WAL-derived expectation), surfacing a CLEAN/DIVERGENT verdict alongside the existing CRC verdict.
|
||||
|
||||
## Scope
|
||||
|
||||
- **In**:
|
||||
- Decouple the raft protocol step from the hook fan-out (item #1).
|
||||
- Canonical ordering: pre-hooks → commit → apply → after-hooks-as-notification (item #2).
|
||||
- Single shared `apply_committed_entry()` seam used by both online commit and restart replay.
|
||||
- Post-restart validation that committed raft log was correctly applied to materialized state (item #4).
|
||||
- Requirement + regression test design for the above (item #5): unit tests for ordering/rollback, plus conformance assertions extended in `raft-sim-conformance`.
|
||||
- **Out**:
|
||||
- Job-array single-record collapse — owned by the sibling `job-array-single-wal-record` spec.
|
||||
- Changes to the raft library protocol core (owned by raft-log-replication / raft-commit-safety).
|
||||
- Entity-bearing snapshots / config-upgrade snapshot (separate specs).
|
||||
- Read-consistency / leader-lease optimizations.
|
||||
|
||||
## Boundary Candidates
|
||||
|
||||
- The explicit controller raft step (append + commit barrier) extracted from `persist_job_hook_fun`.
|
||||
- The single `apply_committed_entry()` apply seam (shared online + replay).
|
||||
- The hook system's demotion to notification-only (after-hook subscribers re-pointed at the apply step).
|
||||
- The commands-vs-facts apply policy.
|
||||
- The post-restart apply-correctness verification.
|
||||
|
||||
## Out of Boundary
|
||||
|
||||
- The raft library callbacks/protocol (consumed, not modified here).
|
||||
- Array-submission batching.
|
||||
- Scheduler framework policy changes.
|
||||
|
||||
## Upstream / Downstream
|
||||
|
||||
- **Upstream**: raft-log-replication + raft-commit-safety (library log/commit core providing `on_persist_entry` / `on_commit_advanced`); ha-term-safety (controller-side raft integration umbrella — this spec restructures the same submit→commit path ha-term-safety wires, so it must land in coordination with it); ha-commit-durability (durable-commit semantics, unchanged).
|
||||
- **Downstream**: `job-array-single-wal-record` (builds the single atomic array record on top of the clean apply seam); worker-rejoin-resync and any future state-machine change benefit from the centralized apply point.
|
||||
|
||||
## Existing Spec Touchpoints
|
||||
|
||||
- **Extends / sequences with**: `ha-term-safety` (controller raft integration umbrella — disjoint-but-adjacent edits to the submit/commit path; coordinate ordering to avoid conflicting refactors).
|
||||
- **Adjacent**: `raft-sim-conformance` (item #5 regression assertions land here — apply-after-commit ordering + no-apply-on-failed-commit invariants); `ha-commit-barrier-rollout` (commit classification table — the WAIT vs RETAIN distinction this spec must respect); `raft-log-replication` / `raft-commit-safety` (library callbacks).
|
||||
|
||||
## Constraints
|
||||
|
||||
- Must not block the iomgr thread (async-I/O invariant in `ha-async-io.md`): the explicit commit barrier must run off the iomgr thread, as the current `persist_wal_sync_append` already arranges.
|
||||
- "Controllers never self-terminate" — a divergent restart verdict is a loud, non-promotable fence, never an `exit`/`abort`.
|
||||
- Preserve persist-before-ACK durability and the synchronous commit barrier as policy (do not canonicalize them away).
|
||||
- The shared apply seam must be deterministic so online-apply and replay-apply produce identical materialized state.
|
||||
150
.kiro/specs/raft-apply-decoupling/facts-in-raft.md
Normal file
150
.kiro/specs/raft-apply-decoupling/facts-in-raft.md
Normal file
|
|
@ -0,0 +1,150 @@
|
|||
# Design Note: Routing Facts Through Raft (apply-after-commit for RETAIN events)
|
||||
|
||||
**Status:** candidate plan (2026-06-25). Resolves the "commands vs facts apply policy" open question in `brief.md`. **D1 resolved 2026-06-25: block-but-durable (retain-only) under quorum loss.** Design-risk triage complete — see `## Design Risks & Tricky Cases`.
|
||||
|
||||
## Premise
|
||||
|
||||
Today XCE splits state changes into two paths:
|
||||
|
||||
- **Commands** (`WAL_MODE_WAIT`: job submit, config) — client-initiated; synchronous commit before reply.
|
||||
- **Facts** (`WAL_MODE_RETAIN`: worker-reported job start/finish/fail/rusage) — worker-initiated; **applied + reacted to on receipt**, WAL-appended non-blocking, committed later via a covering standby ACK (`wal_hook_append_classified`, `persist.c:2488`).
|
||||
|
||||
Facts were made apply-on-receipt for liveness: a worker fact (job finished) must promptly free the slot and trigger the next dispatch, and the worker has already moved on (fire-and-forget, zero redelivery — bug-396).
|
||||
|
||||
**"Raft handling facts" means:** a fact becomes a normal committed log entry. Its *state-machine mutation* (job RUN/DONE/FAIL) and its *reactions* (slot free, dispatch trigger, client/apiserver notify) move to the post-commit `apply_committed_entry()` seam. Only the **durable append** stays on-receipt (so the fact is never lost). This makes online-apply ≡ replay-apply for facts too, and eliminates phantom reactions on uncommitted facts.
|
||||
|
||||
## Do we still handle requests and facts differently? (yes — but only at the edges)
|
||||
|
||||
Once both requests (commands) and facts route through commit-then-apply, **the apply step unifies** — both go through the same `apply_committed_entry()` seam. What stays type-specific is the *ingress* and the *commit-wait / failure* policy, not the apply:
|
||||
|
||||
| Aspect | Request / command (was `WAL_MODE_WAIT`) | Fact (`WAL_MODE_RETAIN`) |
|
||||
|---|---|---|
|
||||
| Origin / ingress | client; non-leader arrivals redirected via the `libxce-failover` taxonomy | worker; non-leader arrivals rejected/redirected, lost-window recovered by `worker-rejoin-resync` |
|
||||
| Commit wait | **synchronous** — handler blocks on `repl_wait_majority_ack`; client reply gated on commit (criteria 2.1/2.2) | **async** — append, return immediately; commit via the group-commit batch; never a per-fact blocking barrier (high volume) |
|
||||
| If commit fails / times out | **report FAILED** to the waiting requester (criterion 2.2) | **nobody to tell** — retain + retry while leader (criterion 2.8) |
|
||||
| Quorum loss | block/fail (CP) | retained-and-retried; decision **D1** decides any local-react escape hatch |
|
||||
| State mutation + reaction (apply) | **shared** `apply_committed_entry()`, post-commit | **shared** `apply_committed_entry()`, post-commit |
|
||||
|
||||
**The key shift:** the `WAIT` vs `RETAIN` classification's *meaning narrows*. Today it conflates two things — "apply-after-commit vs apply-on-receipt" **and** "synchronous-confirm vs async". After this change the apply timing is the **same** for both (after commit); the classification governs **only** "does a requester block and get a failure reply" vs "is it async-retained". So we still *route* them differently at the edges, but they *converge* at the one place correctness depends on: the apply seam.
|
||||
|
||||
## Why this is correct, not just possible
|
||||
|
||||
- A fact is an external input to the replicated state machine. Raft already commits external inputs (that's what `ClientRequest` is). Nothing about a fact requires apply-on-receipt at the *log* level.
|
||||
- Reactions are state-machine **outputs**. Canonical Raft produces outputs at apply time (post-commit), on the leader, idempotently. Today's on-receipt reaction violates this — it can fire on a leader that then loses the entry.
|
||||
- The latency cost (one commit round-trip before slot-free/dispatch) is the same round-trip commands already pay, and the group-commit flusher (`ha-commit-durability`) already amortizes the high fact volume. The iomgr thread is never blocked: `on_commit_advanced` drives apply asynchronously.
|
||||
|
||||
## Decisions to lock before building
|
||||
|
||||
- **D1 — Quorum-loss fact policy — RESOLVED 2026-06-25: (a) block-but-durable, retain-only.** When the primary cannot reach a majority, a fact is appended durably but NOT applied (no state mutation, no reaction) until its entry commits. No local-apply allowlist. Rationale: option (b) (local-apply allowlist) directly contradicts criterion 2.4 (provisional state must not drive irrevocable exports such as dispatch) and reopens the bug-795/828/851 divergence family. Cost of (a) is bounded and self-healing — only *new* dispatch stalls during the (already-degraded) quorum-loss window; running jobs continue (worker autonomy); retained facts commit+apply in order on quorum restore; primary-death-in-window recovered by `worker-rejoin-resync`. Consistent with CP + "controllers never self-terminate". Refinements: no carve-outs (allowlists rot); accept the documented stale-read deviation; emit a metric/WARN on retained-fact backlog age; facts inherit Requirement 7's quorum-loss posture.
|
||||
- Rejected: **(b) local-apply allowlist** — buys availability the operator should fix at the quorum level, at the cost of the core safety invariant.
|
||||
- **D2 — Reaction timing.** Confirm ALL fact reactions (slot free, scheduler dispatch, client/apiserver notify) are safe to defer to post-commit. Expectation: yes — the worker is fire-and-forget; nothing external blocks on the controller's reaction.
|
||||
- **D3 — Idempotency key.** Fact apply must be idempotent (replay, worker-rejoin-resync re-report, retry). Key on `(jobid, monotonic job-state)` so a re-applied or out-of-order fact is a no-op, never a regression of state. Reuse the `mark_job_*` idempotency guards (shared need with `ha-consistency-invariants`).
|
||||
|
||||
## The list — follow in order
|
||||
|
||||
### Phase A — Preconditions (shared with raft-apply-decoupling core)
|
||||
1. Land the single `apply_committed_entry()` seam; route BOTH commands and facts through it.
|
||||
2. Add/confirm `mark_job_*` idempotency guards (D3) so apply is replay/resync/retry safe.
|
||||
3. Ensure the async group-commit flusher (`ha-commit-durability` `fsync_worker`) wakes a post-commit apply pump via `on_commit_advanced(lsn)` — no iomgr blocking.
|
||||
|
||||
### Phase B — Reclassify facts onto the committed-apply path
|
||||
4. Change fact ingestion (the `WAL_MODE_RETAIN` path in `wal_hook_append_classified`, `persist.c:2488`) so on receipt it ONLY: validate → `ha_primary_log_append` (append entry) → return. **Remove the on-receipt state mutation and reactions.**
|
||||
5. Move the fact state mutation (`restore_job_state`-equivalent: RUN/DONE/FAIL/rusage) into `apply_committed_entry()`, keyed on committed LSN.
|
||||
6. Move the reactions (slot free, scheduler dispatch trigger, client/apiserver notify) to fire from the apply step, after the mutation, leader-only.
|
||||
7. Implement the apply pump: `on_commit_advanced(lsn)` applies all newly-committed entries (facts + commands) in LSN order exactly once.
|
||||
|
||||
### Phase C — Failover / liveness correctness
|
||||
8. Reject/redirect facts arriving at a non-leader (workers must target the leader; reuse the `libxce-failover` redirect taxonomy `E_XCE_REDIRECT`/`E_XCE_NOMASTER`/...).
|
||||
9. Wire `worker-rejoin-resync` as the backstop for facts lost in the appended-but-not-committed or never-arrived window.
|
||||
10. Implement D1 (RESOLVED: block-but-durable — append durably, no apply/react until commit, no allowlist); gate it, emit the retained-fact backlog metric, and test the quorum-loss-then-restore path.
|
||||
|
||||
### Phase D — Restart / replay parity
|
||||
11. Restart replay applies fact entries through the SAME `apply_committed_entry()` → online-apply ≡ replay-apply.
|
||||
12. Extend the brief's post-restart apply-validation (item #4) to cover fact-derived final states, not just submits.
|
||||
|
||||
### Phase E — Verification / regression (item #5)
|
||||
13. `raft-sim-conformance`: assert no fact reaction (slot-free/dispatch/notify) occurs before that fact's entry commits; assert idempotent re-apply; assert State-Machine-Safety across interleaved fact+command logs.
|
||||
14. Live soak (`/xce-test-ha`): kill the primary mid-fact-stream; assert no phantom slot-frees, no divergence, no final-state loss beyond what `worker-rejoin-resync` recovers.
|
||||
15. Performance regression: high worker-churn fact stream must not regress — verify group-commit keeps fact commit amortized (NO per-fact synchronous barrier; facts never use the command `repl_wait_majority_ack` blocking path).
|
||||
|
||||
## Keystone Pre-requirement: async, non-blocking commit drive *(must land FIRST)*
|
||||
|
||||
Everything in this spec is downstream of one conversion. The protocol **drive** is currently **blocking**: `repl_wait_majority_ack()` → `pthread_cond_timedwait(&ce->cond, &ce->lock, &dl)` (commit.c:359, bounded by `DEFAULT_SYNC_TIMEOUT_MS`=500ms, config.h:93) **parks a thread until a majority of WalAcks arrive**. This is *blocking-offloaded* (the wait was moved off the iomgr thread) but NOT *event-driven* — it honors the letter of "never block iomgr" while still parking one worker thread per in-flight commit.
|
||||
|
||||
**Pre-requirement — convert the commit engine from `wait` to `resolve`** (do NOT build a parallel registry; reuse the commit engine's existing per-LSN majority tracking — today it `cond_broadcast`s, replace that with `resolve()`):
|
||||
- append → register `pending(LSN)` on the existing per-LSN commit structure, return immediately (no park).
|
||||
- on each WalAck event: if majority reached → `resolve(lsn, COMMITTED)`; on timeout/demotion/truncation → `resolve(lsn, FAILED)`.
|
||||
- `resolve(lsn, outcome)` → advance commit_index → drive apply (off the ACK thread) → fire the reply continuation (C) and any apply-time proposal callbacks (B).
|
||||
- the `cond_timedwait` is **deleted, not wrapped**.
|
||||
|
||||
**Why it is the keystone:** B (apply→propose self-deadlock), C (reply continuation), and pool-exhaustion (parked workers starving the bounded vtask pool under a slow/degraded majority) are **all downstream of the blocking drive**. One conversion dissolves all three: with nothing parked, there is no thread to self-deadlock, replies become natural callbacks, and the pool can't be starved by commits. The apply-after-commit work sits on top of this — so the async drive is a **hard prerequisite**, sequenced before Phase A.
|
||||
|
||||
The blocking call is simple and obviously correct; the event-driven version is a state machine across async events (append-complete, each ACK, timeout, demotion, disconnect) carrying the same exactly-once + completeness burden as C, now on the protocol critical path. That cost is why "driving raft" — not the raft algorithm (already library-verified) — is the hardest part of this effort.
|
||||
|
||||
### Two distinct prerequisites — the commit barrier is NOT the only blocking violation
|
||||
|
||||
A broad async-violation audit (2026-06-25) found the commit barrier is actually the *least* severe blocking class (it blocks a pooled vtask worker — bounded, degrades gracefully). **Three CRITICAL violations block the iomgr thread itself** — the single epoll loop driving ALL heartbeats/votes/ACKs/client I/O. Root cause: `peer_ctrl_msg_handler` is registered **directly as `ep->msg_handler`** (NOT handed to the vtask pool like normal cluster messages), so anything it does synchronously runs on the iomgr event loop. When iomgr stalls → heartbeats stop → peers declare the leader dead → election storm. This is a *separate defect class* from the apply refactor: it threatens protocol **liveness today**, independent of apply-after-commit, and is a likely root of the bug-851 chaotic-failover family.
|
||||
|
||||
**CRITICAL (block iomgr / the heartbeat-driver thread):**
|
||||
1. **Snapshot send on iomgr** — `ha_dispatch_repl_action_for_standby` (ha.c:2702) → `persist_checkpoint` (fdatasync + dir scan + segment unlinks) + `ha_send_snapshot_to_standby` (`fopen`/`fread` chunk loop, config_repl.c:2446-2503), all on iomgr. A large snapshot freezes every heartbeat/vote/client request for its whole duration. Worse than the commit barrier.
|
||||
2. **Config push wait on iomgr** — `push_config_transport_only` → `wait_all_with_timeout` busy-polls `usleep(5000)` + `pthread_join` (config_repl.c:1332,1340), called from `peer_ctrl_msg_handler` paths (ha.c:1172,1177,2672,2677,3973), up to `CONF_PUSH_TIMEOUT_MS`. Code comments (ha.c:75-77) acknowledge the safe-vs-unsafe caller split but unsafe paths exist.
|
||||
3. **Blocking DNS + connect on the delaymgr (heartbeat) thread** — `endpoint_connect` → `gethostbyname` + blocking `connect()` (netio.c:289,302) via `ha_connect_to_standby` from `primary_heartbeat_fn`. One slow/unresolvable peer stalls ALL heartbeats/election-timeouts for seconds → false failover. (`set_fd_nonblocking` is applied AFTER connect, netio.c:716 — the connect is inherently blocking.)
|
||||
|
||||
**MEDIUM (block a vtask pool worker — same class as the commit barrier):**
|
||||
- `repl_wait_majority_ack` for `OPCode_upgradeConfigAck` (config_repl.c:1459); 30-second `usleep` checkpoint-wait poll in `do_upgrade_config_with_bytes` (config_repl.c:1661); `ha_persist_term` fsync on a vote grant (ha.c:3407).
|
||||
|
||||
**Correctly event-driven (confirmed):** RequestVote fan-out, heartbeat fan-out, and WAL replication streaming all use non-blocking `xce_request()` enqueue; standby-side replicated WAL write + its fdatasync run on a vtask worker, not iomgr.
|
||||
|
||||
**Verdict: the protocol drive is NOT fully event-driven.** Two separable prerequisites result:
|
||||
- **P1 — commit-barrier async** (this doc's keystone): unblocks the apply/reply/facts work (B, C, pool-exhaustion). Prerequisite for `raft-apply-decoupling`.
|
||||
- **P2 — drive hardening** (move snapshot send, config-push wait, and peer DNS/connect OFF the iomgr/delaymgr threads): independently valuable, independently testable, threatens liveness today. **Recommended home: its own spec `raft-drive-async` (or fold into `election-iomgr-refactor`, which already touches this thread model) — NOT buried inside `raft-apply-decoupling`.**
|
||||
|
||||
**Offload mechanism — "trigger, don't execute" (delaymgr/iomgr only schedule; blocking work runs in a pool with admission headroom):**
|
||||
- The timer/event thread enqueues a task and returns immediately; the blocking work runs on a worker. The danger is relocating the stall: dumping blocking work into the bounded vtask pool can starve request handling. **XCE already solved this for the commit barrier — `COMMIT_WAIT_RESERVED` caps concurrent blocking waits at `max(1, n_vcpus − RESERVED)`, reserving headroom (commit.c:195-203).** Any offloaded blocking task MUST live under the same admission discipline.
|
||||
- **API gap:** the existing enqueue `vtask_handle_msg` (vtaskmgr.c:356) is endpoint+inbound-packet-bound (`vtype=VTASK_IO`, requires `ep->fsm_in.pkt`). A reconnect has no inbound packet → P2 needs a **general (non-IO) task-submit path** (`vtask_enq` with custom data), or generalize the API.
|
||||
- **Lane by duration:** short bounded tasks (**reconnect**, ≤200ms connect cap) → vtask pool **under an admission cap** (COMMIT_WAIT_RESERVED-style). Long tasks (**snapshot send, config-push**, seconds) → a **dedicated slow-lane executor**, NOT the request pool (a few concurrent multi-second tasks head-of-line-block request handling even with reservation).
|
||||
- **Coordination per offloaded task:** (1) idempotent in-flight dedup (`pending[peer]` atomic) so periodic timers don't pile up duplicates; (2) refcount the endpoint at enqueue, release at completion (arg outlives the tick); (3) marshal completion (epoll fd registration) back to the **iomgr** owning thread (bug-646); (4) resolve the outbound-vs-inbound connection race.
|
||||
- **DNS:** `gethostbyname` is LOW real-world risk (HPC peers are IP/`/etc/hosts`) — **defer unless measured**. The `connect` fix is mostly reordering an existing pattern: non-blocking connect + bounded `poll` timeout already exists in `xce_probe_config_tag` (config_repl.c:1753-1761); the buggy path just sets non-blocking AFTER connect (netio.c:716).
|
||||
|
||||
**Heartbeat-problem completeness (does the offload fix it? — partially):** Heartbeat liveness spans BOTH threads — delaymgr *sends* heartbeats + fires election timeouts; iomgr *processes* incoming ACKs/votes/replies (`peer_ctrl_msg_handler`). Connect-offload protects the delaymgr (sender) side and election-timeout firing — a real win — but is **necessary-not-sufficient**: the snapshot-send-on-iomgr blocker (#1) freezes the *receiver* loop, so the leader stops hearing its peers regardless of how cleanly it sends. **Heartbeat is safe only when BOTH delaymgr AND iomgr never block → P2 must land all three CRITICALs, not just connect.** Residual: delaymgr ticks at 200ms precision (delay.c) — a timer-granularity floor offload doesn't touch; set the failover-detection timeout with margin above 200ms (tuning, not blocking).
|
||||
|
||||
**Missing regression test:** `/xce-test-ha` only *kills* the primary (process-dead → connect fails fast with RST; does NOT reproduce this). The real test is a **partition variant** — make one peer *unreachable* (blackhole packets so connect hangs and snapshot-send stalls) and assert (a) heartbeats to survivors keep flowing and (b) no spurious election. This specifically exercises the *unreachable*-not-*dead* case the blocking findings care about — the bug-851 chaotic-failover family.
|
||||
|
||||
## Design Risks & Tricky Cases
|
||||
|
||||
Triaged 2026-06-25. After triage, only **A** and **B** are genuinely architectural; **C** was reclassified to request-handling correctness; **D–G** are correctness/cost refinements.
|
||||
|
||||
### A — Reactions are not replayable → re-derive from committed state on recovery *(architectural; RESOLVED by existing resync)*
|
||||
A committed log perfectly records *what state should be*, but is a bad script for *what side effects to re-emit*: replaying a job-finish must rebuild DONE state but must NOT re-notify the client or re-dispatch. So reactions originate from **only two places**: (1) **live-leader apply** of newly-committed entries, and (2) the **full-state resync on consumer reconnect**. Recovery never replays reactions — it rebuilds committed state, and the inevitable reconnect (failover / primary-restart / promotion all look identical to a consumer) re-derives the world. Three apply contexts: leader-live = state + react; standby = state only; recovery/replay = state only, then re-derive via resync.
|
||||
|
||||
**This is solved by XCE's existing reconnect-and-resync** (scheduler/apiserver full-state push on reconnect = the re-derivation step). Conditions for it to hold:
|
||||
1. Resync must push **committed** state only (post-replay), never provisional (criterion 2.7).
|
||||
2. **Full reconcile**, not delta (consumer discards old view) — guard against the "lightweight re-attach" optimization eroding this.
|
||||
3. Resync re-reports flow through **commit-then-apply** too (a rejoining worker's re-reported fact is appended→committed→applied idempotently, never apply-on-receipt — same D1/2.9 gate).
|
||||
4. Order resync **after** replay (re-reports above the recovered tip → append+commit; already-committed → idempotent no-op).
|
||||
|
||||
**HARD prerequisite:** the worker→controller half depends on `worker-rejoin-resync`, which is **not yet implemented** (workers send only `hostattrs` today, no job state — bug-396). Without it, a fact lost in a failover window has no redelivery path and A reopens for facts. → upgrade the `worker-rejoin-resync` dependency in `brief.md`/roadmap from "designated backstop" to **HARD**.
|
||||
|
||||
### B — Apply → propose reentrancy (deadlock risk) *(architectural; OPEN)*
|
||||
Apply runs inside the commit-advance callback. A reaction that produces *new cluster state* must **enqueue a fresh proposal**, never mutate inline and never block waiting for its commit (you'd deadlock inside the commit path). Rule: **apply may enqueue proposals; apply may never block on consensus.** The vtask-continuation discipline below (register-don't-block) is the same mechanism and covers B.
|
||||
|
||||
### C — Reply ownership *(RECLASSIFIED: request-handling correctness, NOT architecture; low risk)*
|
||||
"One request → exactly one reply" is a **pre-existing** request-handling invariant. Apply-decoupling only **defers** the reply to commit; it adds no new architecture. The genuinely architectural sub-concerns are already solved by the existing design:
|
||||
- **Send race / socket-epoll ownership (bug-646 class):** solved — `xce_reply` is non-blocking (it *enqueues* to the endpoint's output queue; the io worker owns the wire). Reply is generated by the vtask handler-continuation (completion marshaled to the iomgr thread), enqueued, and delivered by the io worker.
|
||||
- **Endpoint lifetime (bug-1077 class):** solved — the io socket is owned by the endpoint, which is **refcounted**; a continuation holds a ref, so a mid-flight disconnect discards the reply against a dead endpoint rather than a UAF.
|
||||
|
||||
**Residual (request-handling correctness, single-threaded if produced on the iomgr thread):**
|
||||
1. **Exactly-once deferred-reply resolution** per request across `{commit-success, commit-timeout, leadership-loss, uncommitted-tail truncation}` — an atomic "claimed" flag on the obligation; centralize all exits through one `resolve(lsn, outcome)`.
|
||||
2. **Bounded in-flight** (the one system-level item, not per-request): the async model loses the blocking model's free thread-pool backpressure, so add admission control on the accept path.
|
||||
|
||||
### D — Which facts need consensus? *(refinement)*
|
||||
Job *lifecycle* (start/finish/fail) → RETAIN-through-commit (authoritative). `rusage`/telemetry → consider **LOCAL** (soft, re-derivable, highest-volume — don't commit-amplify it). Re-audit the `wal_classify` table.
|
||||
|
||||
### E — Idempotency must be state-monotonic, not LSN-dedupe *(refinement; mandatory)*
|
||||
Compare job-state rank (PEND<RUN<DONE) and no-op regressions/duplicates — not LSN dedupe. **Resync amplifies this** (it floods re-reported facts), so E is mandatory, not optional. (Implements D3.)
|
||||
|
||||
### F — Strict LSN-ordered apply *(refinement)*
|
||||
Serial, in-LSN-order apply pump, never reordered/parallelized — this is what makes "fact for a not-yet-created job" races vanish by construction. Lock it as a `raft-sim-conformance` assertion.
|
||||
|
||||
### G — Notify reliability is unchanged *(note, not a regression)*
|
||||
Moving after-hooks to notify-only does NOT make delivery reliable (still best-effort, bug-396). Resync backstops loss **across a reconnect**, not per-message. Don't let the cleaner architecture imply guaranteed delivery.
|
||||
Loading…
Add table
Add a link
Reference in a new issue