fix(job-array-single-wal-record): extend Live Apply Resource Fence to boot replay (task 4.2's gap)
replay_wal() now halts logical apply and engages persist_mark_apply_resource_fenced() on a negative apply_committed_entry() return during boot/restart replay, mirroring persist_apply_pump_run()'s existing live-path halt-and-fence behavior (task 4.1). Physical scan bookkeeping (current_lsn, CRC chain) stays unconditional per entry, consistent with persist_recover()'s own established current_lsn/last_applied_lsn divergence precedent. Went through one remediation round: round 1 exercised the wrong ha.c gate (periodic re-alert timer); round 2 corrected the test to drive the real election-time gate (raft_cb_become_leader_with_cluster_for_test()), with teeth independently confirmed via a temporary fence-clear RED/GREEN cycle. Round-2 reviewer APPROVED. Documents a real, NOT-fixed residual gap found during this task: a node's very first-ever promotion evaluation in a fresh process runs before that node's own first persist_recover()/replay_wal() call, so this fence is invisible to that one promotion check (ha.c:1303-1408 gate runs before ha.c:1427-1439's first-boot recovery). Every promotion evaluation after a process's first one is protected. Out of this task's persist.c-only boundary; flagged for a dedicated ha.c follow-up.
This commit is contained in:
parent
5a8136263e
commit
ea8d9d3c4f
5 changed files with 669 additions and 53 deletions
|
|
@ -197,7 +197,7 @@
|
|||
- _Requirements: 2.3, 9.4, 9.6_
|
||||
- _Boundary: Array Apply Expansion (`src/lib/kvmgr.c`, `src/lib/zarray.c`, `src/controller/job.c`'s `restore_job_array()`, `src/controller/persist.c`'s `apply_committed_entry()` `WAL_JOB_ARRAY_SUBMIT` case) — touches shared library code (`kvmgr.c`/`zarray.c`) used far beyond this feature; new functions only, zero changes to existing function bodies_
|
||||
|
||||
- [ ] 7.2 Extend the Live Apply Resource Fence's protection across a boot/restart replay (task 4.2's self-flagged gap)
|
||||
- [x] 7.2 Extend the Live Apply Resource Fence's protection across a boot/restart replay (task 4.2's self-flagged gap)
|
||||
- `replay_wal()` (boot/restart replay path) currently captures `apply_committed_entry()`'s return value only for the job-count tally (task 6.0a) — it does not halt or engage the Live Apply Resource Fence on a negative return the way `persist_apply_pump_run()` (the live path, task 4.1) already does
|
||||
- Add the same halt-and-fence behavior to `replay_wal()`: on a negative `apply_committed_entry()` return during replay, stop the replay walk at that LSN without advancing past it, log loud, and call the SAME `persist_mark_apply_resource_fenced()` primitive the live path uses — reuse, do not reimplement
|
||||
- Verify (and document, not just assume) that this engages BEFORE the node could ever be considered for promotion during its own boot sequence — trace the actual boot ordering to confirm `persist_recover()`/`replay_wal()` completes before `ha.c`'s promotion-gate logic could evaluate this node as a candidate
|
||||
|
|
@ -238,3 +238,6 @@
|
|||
- **`/kiro-validate-impl` feature-level gate (2026-07-16)**: Ran 4 parallel validation dimensions. Requirements coverage: 38 ACs, 33 FULL, 5 PARTIAL (2.3, 8.3, 9.4, 9.5, 9.6 — all tracing to task 3.1's already-known bulk-insert partial-visibility gap plus 8.3's admitted commit-barrier-count inference), 0 outright unclaimed gaps. Design/boundary alignment: ALIGNED, no drift, no boundary violations, all 5 Out-of-Boundary items confirmed untouched, task 5.3b/6.0a's mid-run insertions confirmed surgically scoped to their own narrow fixes. Cross-task integration: SEAMS_SOUND — wire format, `base_jobid + i` arithmetic, `apply_rc` contract, and fence naming all independently verified consistent across every producer/consumer in the chain (5 wire consumers, 3 independent arithmetic implementations in C controller/C scheduler/Go apiserver, 3 tasks sharing the `apply_rc` contract). Mechanical checks: full suite clean, TBD/secrets grep clean, and a genuine live smoke boot (fresh single-node deploy, real `jobsub -a 5` via `xce_test`, exactly one `JOB_ARRAY_SUBMIT` WAL record, all 5 elements individually queryable) — PASS.
|
||||
The requirements-coverage pass sharpened task 3.1's known gap: it is a literal, code-proven violation of Requirement 2.3 and 9.4 (not just a soft nuance) — a bulk-insert-phase resource-exhaustion failure can permanently orphan array elements that exist and are individually addressable but never reach `HOOK_DETAIL_JOB_NEW`, so they are never scheduled or pushed, not on this attempt or any later successful retry of the same LSN. User decision on final GO/NO-GO: **GO, with the gap filed as a tracked bug (`bug-1443`, `.wolf/buglog.json`)** rather than blocking the spec on a fast-follow fix or holding NO-GO — the trigger condition is narrow (a resource-exhaustion failure at the exact moment of the bulk-insert pass) and every other validation dimension came back clean. Closing `bug-1443` requires either a new `key_del`/capacity-reservation primitive in `kv.c`/`kvmgr.c` (shared library, its own small design effort) or a re-derive-and-renotify recovery path — explicitly out of this spec's approved task list, not a quick follow-up task.
|
||||
- **Task 7.1**: Added `kvmgr_reserve()`/`xzarray_reserve()` (new, purely-additive primitives in shared `kvmgr.c`/`zarray.c`, following the existing `kvmgr_capture_keys()` template — bare extern function, no RTTI vtable slot, "caller must hold write lock" contract), wired a reservation call into `restore_job_array()`'s bulk-insert phase, and made a genuinely-inserted partial prefix always get notified (`out_new_jobs` populated + `apply_committed_entry()` fires `HOOK_DETAIL_JOB_NEW`) even when the function still fails overall. **Genuine off-by-one found in the existing, unmodified `xzarray_ensure_items()`**: its sufficiency check is strict less-than (`mincap < array->caps`), so reserving exactly `array_size` and then performing `array_size` real stores still triggers one more growth cycle on the LAST store (`mincap == caps` fails the strict check). Independently re-derived and confirmed by the reviewer with concrete arithmetic: reserving `array_size + 1` (not literally `array_size` as this task's text above says) is exactly the minimal correct compensation — not over- or under-provisioning. This is a deliberate, verified deviation from this task's literal wording, documented at the call site. Confirmed the fix narrows (does not fully eliminate) the residual gap as designed: `kvtab_add()`'s own small per-entry hash-chain-node allocation and a content-based `-EEXIST` collision remain theoretically possible even after reservation — both non-N-scaling and far rarer than the closed failure mode, honestly documented as such. Retry-double-notify risk (does a retry's own `out_new_jobs` ever re-include an already-notified prefix) independently traced and confirmed NOT present — the pre-existing per-index idempotent-skip logic excludes already-inserted indices from any subsequent attempt's `built` list. `apply_wal_entry()`'s dead-code standby path confirmed genuinely untouched (zero behavior change, still passes `NULL` for `out_new_jobs`). New build-architecture finding: `libxce.a` is compiled exactly once without `-DXCE_TESTING` and shared by every consumer, so the new test-support definitions in `zarray.c` had to be left unconditionally compiled (only their declarations in `zarray.h` stay `#ifdef XCE_TESTING`-gated) — confirmed zero production risk (force-fail flag defaults false/inert, no production translation unit can reach the ungated symbols without the gated declaration). One non-blocking coverage gap: no single test composes the full engage→notify-prefix→clear-seam→retry→no-double-notify flow end-to-end (the property is provably true by combining two already-passing tests, but not directly demonstrated in one).
|
||||
- **Task 7.2**: Restructured `replay_wal()`'s per-entry loop into a three-way branch (`apply_halted`/`apply_rc<0`/success) mirroring `persist_apply_pump_run()`'s (task 4.1) halt-and-fence behavior — on a negative `apply_committed_entry()` return during boot/restart replay, logical apply (tally, `last_applied_lsn` advance, `replayed++`) halts at that LSN and `persist_mark_apply_resource_fenced()` engages (reused verbatim, no new mechanism), while PHYSICAL scan bookkeeping (`current_lsn`, CRC chain) deliberately continues unconditionally for every entry — verified safe against `persist_recover()`'s own pre-existing "Round-2 fix" precedent, which already establishes `current_lsn` and `last_applied_lsn` are expected to diverge. This closes the boot-replay half of task 4.2's self-flagged gap: a node that crashes while fenced and re-hits the identical condition on restart now correctly re-engages the fence during replay instead of silently booting clean.
|
||||
**Went through one remediation round.** Round 1 was REJECTED not on the `persist.c` fix's correctness (verified sound both rounds) but because task 7.2's own explicit deliverable — "verify and document" that the fence engages before any promotion decision — was left unmet, and the new test exercised the wrong `ha.c` gate (the periodic re-alert timer, not the actual election-time check). Round 2 fixed both: the test now drives the real election-time gate (`raft_cb_become_leader_with_cluster_for_test()`, the same seam task 4.3's own test uses for a live-pump-engaged fence — this is the first test to drive it for a fence engaged by boot replay specifically), with teeth independently reproduced twice (temporarily clearing the fence before the gate call reliably flips the test RED, both in isolation and in the full suite).
|
||||
**CONFIRMED, NOT FIXED, real first-boot promotion-ordering gap (independently verified twice — implementer, then two separate reviewers)**: `raft_cb_become_leader()`'s election-time eligibility gate (the `if (c != NULL) {...}` block, `ha.c:1303-1408`, reading `persist_apply_resource_fenced()` at `ha.c:1337`) runs strictly BEFORE the `is_first_boot` block (`ha.c:1427-1439`) that calls `cluster_init_phase2()` → `persist_recover()` → `replay_wal()` (`cluster.c:1482`) — the function this task modified. There is no re-check of `persist_apply_resource_fenced()` between `cluster_init_phase2()` returning and the unconditional `CTRL_PRIMARY` role assignment a few lines later. Net effect: this task's fix protects every promotion evaluation AFTER a node's first one in a given process's life (a later election, or the periodic re-alert timer) — but a node's very FIRST-EVER promotion evaluation, on its first election win in a freshly-started process, is decided before its own first `persist_recover()`/`replay_wal()` call has even run, so a resource-exhaustion condition that same first replay is about to hit is invisible to that one gate check. This is exactly the "all nodes restart simultaneously" race this whole task exists to guard against, and it is not fully closed by task 7.2 alone. Trigger requires BOTH (a) a node winning its very first election in a fresh process AND (b) that same node's first-ever boot replay hitting the identical resource-exhaustion condition that previously fenced it — narrow but real. Documented via a code comment in `persist.c` (citing these exact file:line references) plus this entry; fixing it requires an `ha.c` change (out of this task's `persist.c`-only boundary) — either re-checking `persist_apply_resource_fenced()` immediately after `cluster_init_phase2()` returns inside `raft_cb_become_leader()`, or reordering recovery to run and be checked before the promotion decision finalizes. Recommend a small dedicated follow-up task against `ha.c` before anything downstream places full trust in this fence across a first-ever-boot promotion race specifically (restarts/re-elections after that first one are already safe).
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue