From b47cc660ba10758e3a81bbc008f793243e07d17f Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 15 Jul 2026 03:54:20 +0000 Subject: [PATCH] feat(job-array-single-wal-record): add live, self-healing apply resource fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new atomic LSN field on persist_t (distinct from the existing boot-only unrecoverable-fence flag) plus three primitives: persist_apply_resource_fenced(), persist_mark_apply_resource_fenced(), persist_clear_apply_resource_fence(). Wired into persist_apply_pump_run(): engages on task 4.1's halt condition, clears unconditionally on the walk's next successful advance — provably equivalent to "clears only when the same stuck LSN's retry succeeds" since the walk is strictly sequential and never advances past a failed position. Known residual gap, out of this task's boundary (documented in tasks.md Implementation Notes): replay_wal() during boot/restart replay does not yet share this protection, so a node crashing while fenced can silently re-pass the same failure on restart. Flagged for a follow-up task, not fixed here. Task 4.2 of job-array-single-wal-record (Requirements 9.4, 9.5). --- .../job-array-single-wal-record/tasks.md | 3 +- src/controller/persist.c | 71 +++++++- src/controller/testsuite/test_apply_pump.c | 153 ++++++++++++++++++ src/include/persist.h | 71 ++++++++ 4 files changed, 296 insertions(+), 2 deletions(-) diff --git a/.kiro/specs/job-array-single-wal-record/tasks.md b/.kiro/specs/job-array-single-wal-record/tasks.md index b955fea..849f55b 100644 --- a/.kiro/specs/job-array-single-wal-record/tasks.md +++ b/.kiro/specs/job-array-single-wal-record/tasks.md @@ -88,7 +88,7 @@ - _Requirements: 9.4_ - _Boundary: Live Apply Resource Fence_ -- [ ] 4.2 Add the live, self-healing fence +- [x] 4.2 Add the live, self-healing fence - Add a new piece of state (distinct from the existing boot-only unrecoverable-fence flag) recording "a position is currently stuck failing to apply" - Engage it when 4.1's halt condition triggers; clear it automatically, from inside the apply walk itself, the moment a retry of that same stuck position succeeds - Observable: after a forced failure the new state reads "fenced"; after the same position later succeeds on retry, the state reads "clear" without any external action @@ -176,3 +176,4 @@ - **Task 3.3**: `apply_wal_entry()` (persist.c:2574) is dead code in production — confirmed via exhaustive grep (zero call sites outside its own declaration/definition/comments) and via `ha.c:8024`'s `ha_standby_advance_committed_and_apply` (dated 2026-07-10), whose own doc comment explicitly says standby entity materialization now goes through the SAME `apply_committed_entry()`/`persist_apply_pump_kick` pump the primary uses, not `apply_wal_entry()`. The persist.c:3722 comment claiming otherwise predates that refactor and was never updated (design.md corrected accordingly). Requirement 3.1/3.2/4.1 are fully delivered by task 3.2 alone. Task 3.3's code was kept (not discarded) as defensive parity with `WAL_JOB_SUBMIT`'s existing, also-dead case in the same switch — it is tested and correct, just not currently reachable in production. If `apply_wal_entry()` is ever revived or removed entirely in a future spec, this task's case should move or be deleted with it. - **Task 3.1**: `restore_job_array()`'s atomicity has one narrow, documented exception (design.md updated to match): the final bulk-insert-into-`c->jobmgr` pass can itself fail on allocation/growth exhaustion (`c->jobmgr` grows via `realloc()`; no capacity-reservation primitive exists anywhere in `kv.c`/`kvmgr.c`/`zarray.c` to pre-check this before mutating). On this specific failure, the function never silently claims success — it returns a negative errno — but an already-inserted prefix `[0, inserted)` can be left genuinely, permanently visible in `c->jobmgr` (no `key_del` exists to undo it), and that prefix is NEVER included in `out_new_jobs`, so `HOOK_DETAIL_JOB_NEW` never fires for it, not on this attempt or any later successful retry of the same LSN. **Tasks 3.2/4.x must account for this**: those elements are individually addressable (status/cancel/completion work normally) but permanently un-pushed to the scheduler/API server unless something later re-derives and re-notifies them — this is a real gap in the current design, not yet solved, and task 4.x's Live Apply Resource Fence design should explicitly decide whether/how to handle it (e.g. detecting this residual state and re-notifying on self-heal) rather than only handling the construction-phase failure case. - **Task 4.1**: `persist_apply_pump_run()` now halts the walk (no `last_applied_lsn` advance) on ANY negative return from `apply_committed_entry()`, not just the new array type — this is a general fix. Investigated whether this newly exposes a hazard for the pre-existing job-state fact entries (`WAL_JOB_START/FINISH/FAIL/UPDATE`), which have their own real `-ENOENT` branch when the target jobid isn't in `c->jobmgr`: confirmed NOT a hazard, because `c->jobmgr` (`xkvmgr_type`) has no `key_del` (jobs are never removed for the process lifetime, corroborated by `job.c:428,455,645`), and this task's own strict sequential-no-skip apply means a fact's LSN is only reached after its causally-prior submit already applied. So a live `-ENOENT` on a fact can now only mean genuine corruption/divergence — halting is correct, not a new footgun. Independent reviewer confirmed this via `test_term_safety_adapter.c:2039-2043`'s synthetic (non-production-representative) trigger. design.md's "Existing Architecture Analysis" table row overclaims ("every existing WAL type... returns 0 or positive in practice") — technically inaccurate re: the fact-ENOENT branch — left as-is since the persist.c code comment at the site already documents the exception correctly; low-severity, non-blocking. +- **Task 4.2**: New `_Atomic uint64_t apply_resource_fenced_lsn` field on `persist_t` (0 = clear, nonzero = the stuck LSN) plus `persist_apply_resource_fenced()`/`persist_mark_apply_resource_fenced()`/`persist_clear_apply_resource_fence()`, wired into `persist_apply_pump_run()`'s existing halt branch (engage) and unconditionally after every successful advance (clear). The unconditional clear is provably equivalent to "clear only on the fenced LSN's own successful retry" because `last_applied_lsn` has exactly 3 writers codebase-wide, the other two (`replay_wal`, `persist_recover`) are boot-only and temporally disjoint from the live pump, and a failed walk never advances past the stuck LSN — so the next success by construction IS that LSN's retry. **Known residual gap (out of this task's boundary, flagged by review, NOT yet fixed)**: `replay_wal()` (boot/restart replay, persist.c:3875-3888) still discards `apply_committed_entry()`'s failure return the same way the live pump did before task 4.1, and `persist_init()` unconditionally zeroes the fence field on every restart. A node that crashes while fenced and re-hits the identical resource-exhaustion condition during boot replay will silently pass it and look fully promotable while actually divergent — task 4.1/4.2's protection does not yet extend to the boot-replay path. This is a pre-existing gap (not introduced by this feature) but is now inconsistent with the live path's new guarantee; worth a dedicated follow-up task before anything downstream places full trust in this fence across a restart. diff --git a/src/controller/persist.c b/src/controller/persist.c index 45e9e67..f1d81bb 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -623,6 +623,15 @@ int persist_init(persist_t* p, persist_config_t* cfg, cluster_t* c) p->apply_verdict = APPLY_CLEAN; atomic_store(&p->last_applied_lsn, 0); + /* Live Apply Resource Fence starts clear (job-array-single-wal-record + * task 4.2, Req 9.4): a fresh store has nothing stuck failing to apply. + * Set by persist_mark_apply_resource_fenced on a resource-exhaustion + * apply failure; cleared by persist_clear_apply_resource_fence the + * instant a retry of the same LSN succeeds -- see persist_t's field doc + * comment for why this is deliberately separate from + * recovery_unrecoverable just above. */ + atomic_store(&p->apply_resource_fenced_lsn, 0); + /* bug-1416: defaults to true (nothing to gate) so every fixture that * calls persist_init() but never persist_recover() -- most of this * spec's own test suite -- sees identical, ungated apply-pump/live- @@ -3374,8 +3383,16 @@ int persist_apply_pump_run(persist_t* p) "committed entry lsn=%" PRIu64 " wal_type=%" PRIu32 " (rc=%d) -- stopping this pass at this position; the " "entry stays committed and will be retried, from the same " - "position, on the next kick", + "position, on the next kick; Live Apply Resource Fence " + "ENGAGED (job-array-single-wal-record Req 9.4)", next, wal_type, apply_rc); + /* job-array-single-wal-record task 4.2 (design.md "Live Apply + * Resource Fence"): engage the NEW, live, self-healing fence -- + * distinct from recovery_unrecoverable -- so the promotion gate + * (task 4.3) can refuse this node while it stays alive and + * keeps retrying. Cleared below, from inside this same walk, + * the instant a retry of this exact LSN succeeds (Req 9.5). */ + persist_mark_apply_resource_fenced(p, next); free(payload); pthread_mutex_unlock(&p->apply_pump_lock); return apply_rc; @@ -3385,6 +3402,21 @@ int persist_apply_pump_run(persist_t* p) atomic_store_explicit(&p->last_applied_lsn, next, memory_order_relaxed); applied++; + /* job-array-single-wal-record task 4.2 (design.md "Live Apply + * Resource Fence", Req 9.5): self-heal. Called unconditionally on + * EVERY successful apply, not just after a known-fenced LSN -- + * persist_clear_apply_resource_fence() is a cheap no-op when nothing + * is fenced (the overwhelming common case). This is correct and + * sufficient because the walk above is strictly sequential and + * returns immediately on any failure (see the halt branch just + * above): it can never advance PAST a fenced LSN without that exact + * LSN applying successfully first, so "the next entry to apply + * successfully after a fence engages" and "a retry of the SAME + * stuck LSN succeeding" are the same event by construction. No + * election, promotion check, or external/admin action is involved -- + * the fence clears purely because this walk itself made progress. */ + persist_clear_apply_resource_fence(p); + /* raft-apply-decoupling task 6.2 (Req 1.1, 2.1-2.3): consume this * LSN's command hand-off, if any (task 3.2's * persist_register_pending_apply(), e.g. handle_jobsubmit's @@ -4357,6 +4389,43 @@ void persist_clear_recovery_unrecoverable(persist_t* p) } } +/* job-array-single-wal-record task 4.2 (design.md "Live Apply Resource + * Fence", Req 9.4/9.5). See the apply_resource_fenced_lsn field's doc + * comment (persist.h) and this trio's own doc comment (persist.h) for the + * full rationale and the deliberate distinction from + * persist_recovery_unrecoverable/persist_clear_recovery_unrecoverable just + * above -- that boot-only mechanism is NOT reused here on purpose. */ +bool persist_apply_resource_fenced(persist_t* p) +{ + return p && atomic_load_explicit(&p->apply_resource_fenced_lsn, + memory_order_acquire) != 0; +} + +void persist_mark_apply_resource_fenced(persist_t* p, uint64_t lsn) +{ + if (!p) + return; + atomic_store_explicit(&p->apply_resource_fenced_lsn, lsn, + memory_order_release); +} + +void persist_clear_apply_resource_fence(persist_t* p) +{ + if (!p) + return; + /* Clear only on a genuine transition so the self-heal log fires once per + * episode, not on every ordinary successful apply (the overwhelming + * common case: nothing was fenced, so this is a cheap no-op). */ + uint64_t prev = atomic_exchange_explicit(&p->apply_resource_fenced_lsn, 0, + memory_order_acq_rel); + if (prev != 0) { + INFO("persist_apply_pump_run: Live Apply Resource Fence RELEASED — " + "lsn=%" PRIu64 " applied successfully on retry; promotable " + "again (job-array-single-wal-record Req 9.5)", + prev); + } +} + recover_decision_t persist_recover_loud_fence_if_unrecoverable(persist_t* p, const recover_policy_inputs_t* in) diff --git a/src/controller/testsuite/test_apply_pump.c b/src/controller/testsuite/test_apply_pump.c index 6f9a447..11a14ae 100644 --- a/src/controller/testsuite/test_apply_pump.c +++ b/src/controller/testsuite/test_apply_pump.c @@ -767,6 +767,157 @@ static void test_apply_pump_run_halts_at_failed_lsn_without_advancing(void) "PASS\n"); } +/* ── 2c. Task 4.2 (job-array-single-wal-record, design.md "Live Apply + * Resource Fence"): a resource-exhaustion apply failure that halts the walk + * (task 4.1) must additionally ENGAGE a new, live, self-healing fence -- + * persist_apply_resource_fenced()/persist_mark_apply_resource_fenced() -- + * deliberately DISTINCT from the boot-only sticky recovery_unrecoverable + * flag (Req 9.4). Reuses the same force-fail seam and array-record shape as + * test 2b above, but asserts the NEW fence accessor rather than the halt + * behavior itself (already covered by test 2b). */ + +static void test_apply_pump_run_engages_live_fence_on_failed_lsn(void) +{ + tap_setup(); + +#ifdef XCE_TESTING + job_array_apply_test_clear_force_fail(); +#endif + + bool fenced_before = persist_apply_resource_fenced(&g_tap_c.persist); + + const uint64_t base_jobid = 95000; + const uint64_t array_size = 3; + job_submit_data_t spec; + memset(&spec, 0, sizeof(spec)); + spec.queue = (char*)"q"; + spec.user = (char*)"tester"; + spec.mintasks = 1; + spec.maxtasks = 1; + spec.job_name = (char*)"array-fence-test"; + + uint8_t* array_payload = NULL; + size_t array_len = 0; + int enc_rc = tap_encode_job_submit(base_jobid, array_size, &spec, + &array_payload, &array_len); + + /* Fail at scratch-build index 1 of 3 -- identical seam to test 2b. */ +#ifdef XCE_TESTING + job_array_apply_test_set_force_fail_at(1); +#endif + + int rc_array = persist_wal_async(&g_tap_c.persist, WAL_JOB_ARRAY_SUBMIT, + array_payload, (uint32_t)array_len); + free(array_payload); + uint64_t array_lsn = g_tap_c.persist.current_lsn - 1; + + atomic_store(&g_tap_c.persist.committed_lsn, array_lsn); + + int walk_rc = persist_apply_pump_run(&g_tap_c.persist); + + bool fenced_after_failure = + persist_apply_resource_fenced(&g_tap_c.persist); + +#ifdef XCE_TESTING + job_array_apply_test_clear_force_fail(); +#endif + + tap_teardown(); + + TEST_ASSERT_EQUAL_INT(0, enc_rc); + TEST_ASSERT_EQUAL_INT(0, rc_array); + TEST_ASSERT_FALSE_MESSAGE(fenced_before, + "a fresh persist_t must start with the Live Apply Resource Fence " + "CLEAR -- nothing has failed yet, and this is a distinct piece of " + "state from the boot-only recovery_unrecoverable flag"); + TEST_ASSERT_TRUE_MESSAGE(walk_rc < 0, + "the forced construction failure must still halt the walk (task " + "4.1 regression guard)"); + TEST_ASSERT_TRUE_MESSAGE(fenced_after_failure, + "the Live Apply Resource Fence must ENGAGE the moment the apply " + "walk halts on a resource-exhaustion apply failure (Req 9.4)"); + + printf("test_apply_pump_run_engages_live_fence_on_failed_lsn: PASS\n"); +} + +/* ── 2d. Task 4.2 self-heal: the fence must CLEAR itself, from inside the + * apply walk, the instant a retry of the SAME stuck LSN succeeds -- with NO + * external/admin action (Req 9.5). Mirrors test 2b's engage-then-retry + * shape but asserts the fence accessor at each stage instead of job + * visibility. */ + +static void test_apply_pump_run_self_clears_fence_on_successful_retry(void) +{ + tap_setup(); + +#ifdef XCE_TESTING + job_array_apply_test_clear_force_fail(); +#endif + + const uint64_t base_jobid = 96000; + const uint64_t array_size = 3; + job_submit_data_t spec; + memset(&spec, 0, sizeof(spec)); + spec.queue = (char*)"q"; + spec.user = (char*)"tester"; + spec.mintasks = 1; + spec.maxtasks = 1; + spec.job_name = (char*)"array-fence-selfheal-test"; + + uint8_t* array_payload = NULL; + size_t array_len = 0; + int enc_rc = tap_encode_job_submit(base_jobid, array_size, &spec, + &array_payload, &array_len); + + /* Fail at scratch-build index 1 of 3 -- identical seam to tests 2b/2c. */ +#ifdef XCE_TESTING + job_array_apply_test_set_force_fail_at(1); +#endif + + int rc_array = persist_wal_async(&g_tap_c.persist, WAL_JOB_ARRAY_SUBMIT, + array_payload, (uint32_t)array_len); + free(array_payload); + uint64_t array_lsn = g_tap_c.persist.current_lsn - 1; + + atomic_store(&g_tap_c.persist.committed_lsn, array_lsn); + + /* First attempt: forced failure engages the fence (proven by test 2c; + * re-checked here as this test's own precondition). */ + int walk_rc_fail = persist_apply_pump_run(&g_tap_c.persist); + bool fenced_after_failure = + persist_apply_resource_fenced(&g_tap_c.persist); + + /* Clear the injected fault and retry the SAME stuck LSN -- no external + * or admin action, purely the apply walk retrying on its own. */ +#ifdef XCE_TESTING + job_array_apply_test_clear_force_fail(); +#endif + int walk_rc_retry = persist_apply_pump_run(&g_tap_c.persist); + bool fenced_after_retry = + persist_apply_resource_fenced(&g_tap_c.persist); + + tap_teardown(); + + TEST_ASSERT_EQUAL_INT(0, enc_rc); + TEST_ASSERT_EQUAL_INT(0, rc_array); + TEST_ASSERT_TRUE_MESSAGE(walk_rc_fail < 0, + "the forced failure must still halt the walk (task 4.1 regression " + "guard)"); + TEST_ASSERT_TRUE_MESSAGE(fenced_after_failure, + "precondition: the fence must be engaged before this test can prove " + "self-heal"); + TEST_ASSERT_TRUE_MESSAGE(walk_rc_retry >= 0, + "once the fault clears, the retry of the same stuck LSN must " + "succeed"); + TEST_ASSERT_FALSE_MESSAGE(fenced_after_retry, + "the Live Apply Resource Fence must CLEAR itself, from inside the " + "apply walk, the moment the retry of the SAME stuck position " + "succeeds -- with no external/admin action (Req 9.5)"); + + printf("test_apply_pump_run_self_clears_fence_on_successful_retry: " + "PASS\n"); +} + /* ── 3. Hazard 1 guard precision: job_ctrl's own live append must survive ── */ static void test_apply_pump_guard_does_not_break_job_ctrl_live_append(void) @@ -1492,6 +1643,8 @@ void test_apply_pump(void) RUN_TEST(test_apply_pump_kick_applies_committed_burst_in_ascending_lsn_order); RUN_TEST(test_apply_pump_run_applies_mixed_submit_and_fact_entries); RUN_TEST(test_apply_pump_run_halts_at_failed_lsn_without_advancing); + RUN_TEST(test_apply_pump_run_engages_live_fence_on_failed_lsn); + RUN_TEST(test_apply_pump_run_self_clears_fence_on_successful_retry); RUN_TEST(test_apply_pump_guard_does_not_break_job_ctrl_live_append); RUN_TEST(test_apply_pump_fail_racing_finish_merges_against_current_state); RUN_TEST(test_apply_pump_late_pending_apply_registration_self_heals); diff --git a/src/include/persist.h b/src/include/persist.h index cee80be..231c293 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -486,6 +486,41 @@ struct persist_s { uint64_t apply_pump_cache_seg_lsn; uint32_t apply_pump_cache_off; uint64_t apply_pump_cache_next_lsn; + + /* job-array-single-wal-record task 4.2 (design.md "Live Apply Resource + * Fence"): a LIVE, self-healing fence recording "an LSN is currently + * stuck failing to apply" -- deliberately a SEPARATE mechanism from the + * sticky, boot-only `recovery_unrecoverable` flag above. The two must + * never be conflated: recovery_unrecoverable is set only during boot + * replay/recovery and cleared only via the WAL-durability catch-up path + * (persist_clear_recovery_unrecoverable), which is wrong for THIS + * condition -- a resource-exhaustion apply failure (e.g. OOM mid array + * expansion, Req 9.4) can happen at ANY time on a long-lived running + * primary or standby, has nothing to do with WAL durability catch-up, + * and must clear itself the instant a RETRY of the exact same stuck LSN + * succeeds, not on some unrelated periodic check. + * + * Semantics: 0 = clear (not fenced); non-zero = the LSN currently stuck + * failing to apply. Storing the actual LSN (rather than a bare bool) + * gives the loud ERROR log and any diagnostic reader (e.g. ctrlstatus) + * the exact stuck position for free. LSN 0 is reserved/never a real + * committed entry (first real entry is LSN 1), so 0 is a safe "clear" + * sentinel needing no separate has-value flag. + * + * Concurrency: set ONLY from persist_apply_pump_run()'s single-threaded + * walk on a negative apply_committed_entry() return (mirrors task 4.1's + * halt branch); cleared ONLY from that same walk, the instant a retry of + * the SAME LSN applies successfully -- never by an election, a promotion + * check, or any other periodic/external path (Req 9.5). Both set and + * clear happen under apply_pump_lock (the walk's own serialization), so + * the atomic ops here are belt-and-suspenders for readers (ha.c's + * promotion-gate call sites, task 4.3) that consult this without holding + * that lock. Not persisted -- like last_applied_lsn itself, recomputed + * by construction: a crash/restart re-derives "stuck" the next time + * replay/live-apply actually fails to apply the same LSN again; it is + * not itself a correctness-critical persisted fact. Defaults to 0 + * (clear) in persist_init -- a fresh store has nothing stuck. */ + _Atomic uint64_t apply_resource_fenced_lsn; #ifdef XCE_TESTING /* Fault-injection seam (ha-commit-durability task 2.1 error-path test): * when non-zero, every fdatasync site treats the sync as FAILED — it @@ -798,6 +833,42 @@ persist_recover_loud_fence_if_unrecoverable(persist_t* p, bool persist_recovery_unrecoverable(persist_t* p); void persist_clear_recovery_unrecoverable(persist_t* p); +/* persist_apply_resource_fenced / persist_mark_apply_resource_fenced / + * persist_clear_apply_resource_fence (job-array-single-wal-record task 4.2, + * design.md "Live Apply Resource Fence", Req 9.4/9.5). + * + * A LIVE, self-healing fence recording "an LSN is currently stuck failing + * to apply" -- deliberately SEPARATE from persist_recovery_unrecoverable + * above (see the apply_resource_fenced_lsn field's doc comment on `persist_t` + * for the full rationale: that flag is boot-only and clears on WAL-durability + * catch-up, which is the wrong trigger for a resource-exhaustion apply + * failure that can occur at any time on a long-lived node). + * + * persist_mark_apply_resource_fenced(p, lsn) is called ONLY from + * persist_apply_pump_run()'s own halt branch, the moment a negative + * apply_committed_entry() return stops the walk at `lsn` -- mirroring + * persist_recover_loud_fence_if_unrecoverable's "log loud, then set the + * sticky flag" shape, but scoped to this narrower, live condition and with + * no abort()/exit() anywhere (Process Lifecycle invariant, + * src/controller/CLAUDE.md): the node stays alive, refuses promotion + * (task 4.3 wires the promotion-gate read), and keeps retrying. + * + * persist_clear_apply_resource_fence(p) is called ONLY from inside that same + * apply-pump walk, unconditionally after ANY entry applies successfully -- + * safe and correct because the walk is strictly sequential and never + * advances past a fenced LSN without that exact LSN applying first, so "the + * next successful apply" and "a retry of the SAME stuck LSN succeeding" are + * the same event by construction. No election, promotion check, or any + * other periodic/external path may ever clear this (Req 9.5) -- the node + * self-heals purely by the apply pump itself eventually succeeding. + * + * persist_apply_resource_fenced(p) is the read-side accessor (task 4.3's + * promotion-gate call sites, ctrlstatus, tests). All three are safe on a + * NULL p (accessor returns false; mutators no-op). */ +bool persist_apply_resource_fenced(persist_t* p); +void persist_mark_apply_resource_fenced(persist_t* p, uint64_t lsn); +void persist_clear_apply_resource_fence(persist_t* p); + /* persist_recovery_complete (bug-1416, raft-apply-decoupling task 7.2 round * 2) -- true unless a persist_recover() call is CURRENTLY in progress on * this persist_t (see the recovery_complete field doc comment). Consulted