test(raft-apply-decoupling): durable fact retention under sustained quorum loss (task 4.2)
Add test_fact_state_unchanged_throughout_quorum_loss_outage to test_commit_barrier_reply.c (30->31), no production changes. Drives a real repl_remove_standby quorum-loss window through all 4 fact-receipt handlers, polls live state and committed_lsn at 5 time-spread points (not once), asserts a real hook-spy never sees AFTER_EVENT fire, and confirms durable WAL presence via direct on-disk read. Documents an architectural asymmetry for task 6 planning: facts have no commit-timeout concept (COMMIT_MODE_RETAIN registers no continuation, "never honors the timeout") unlike commands' commit_submit_async -- the apply pump must discover newly-committed retained facts purely via committed_lsn advancement, not any per-fact signal. Closes out task-group 4 (fact apply-after-commit split).
This commit is contained in:
parent
23cbfb48e1
commit
665b29f5b0
2 changed files with 202 additions and 2 deletions
|
|
@ -90,7 +90,7 @@
|
|||
- _Requirements: 2.1, 2.3_
|
||||
- _Depends: 3.2_
|
||||
|
||||
- [ ] 4. Fact apply-after-commit split
|
||||
- [x] 4. Fact apply-after-commit split
|
||||
- [x] 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
|
||||
|
|
@ -99,7 +99,7 @@
|
|||
- _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
|
||||
- [x] 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
|
||||
|
|
@ -193,6 +193,7 @@
|
|||
- **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**.
|
||||
- **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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue