From 40196be2e5de8057eb1abb7ce461195d73a41631 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 7 Jul 2026 07:14:15 +0000 Subject: [PATCH] perf(raft-apply-decoupling): fix O(n^2) apply-pump WAL rescan, measure throughput (task 7.4) Measured sustained worker-fact throughput against the pre-feature baseline. Commit throughput is unregressed (~2.5% delta, within noise) and no per-fact synchronous commit barrier exists, confirming Req 11.4's core claim. But apply/observable throughput -- the new hop this spec introduces -- regressed 56% with clearly superlinear scaling (2x volume -> 5.2x elapsed). Root cause: apply_pump_read_entry has no memory across calls -- every LSN lookup re-lists WAL segments and rescans its segment from byte 0 -- while its only caller applies committed entries one at a time in strict ascending order, turning a sequential access pattern into O(n^2) total work. Fixed with a minimal segment-read cache on persist_t (fd + offset + expected next LSN, guarded by the existing apply_pump_lock): a fast path reads the next expected LSN directly at a remembered offset, falling back unchanged to full rediscovery on any miss. Correctness never depends on the cache, only performance does. Invalidated on init, fini, recovery, and WAL truncation -- the four points a stale cursor could otherwise misread from. Two review rounds: round 1 caught a missing lock guard in the recovery invalidation path and an inaccurate risk claim about the cache/trim interaction (a real, bounded behavior change deferred to the existing APPLY_DIVERGENT fence, not "no worse than before") -- both corrected and re-verified in round 2. Post-fix apply-throughput scales sublinearly (1.46x elapsed for 2x volume) and exceeds the pre-feature baseline. Zero new test failures across the pump, correctness-verification, recovery, and truncation groups. An intermittent, pre-existing client hang in the synchronous jobsub path and a trim-floor/apply-lag architectural gap were surfaced but are explicitly out of scope for this fix -- flagged for separate follow-up. --- .kiro/specs/raft-apply-decoupling/tasks.md | 3 +- src/controller/persist.c | 147 ++++++++++++++++++++- src/include/persist.h | 46 +++++++ 3 files changed, 192 insertions(+), 4 deletions(-) diff --git a/.kiro/specs/raft-apply-decoupling/tasks.md b/.kiro/specs/raft-apply-decoupling/tasks.md index 6906834f..73f83eb5 100644 --- a/.kiro/specs/raft-apply-decoupling/tasks.md +++ b/.kiro/specs/raft-apply-decoupling/tasks.md @@ -179,7 +179,7 @@ - _Requirements: 11.1, 11.2, 11.3_ - _Depends: 7.1, 7.2_ -- [ ] 7.4 Measure sustained fact-stream throughput against the pre-feature baseline +- [x] 7.4 Measure sustained fact-stream throughput against the pre-feature baseline - Record a throughput baseline on the pre-feature code, then drive the same sustained high-volume worker-fact stream against the completed feature - Confirm no per-fact synchronous commit barrier was introduced and throughput does not regress - Observable: a recorded throughput comparison shows no regression relative to the pre-feature baseline @@ -196,6 +196,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**. +- **7.4**: closes out task-group 7 and the spec. Measured sustained `jfinish` fact-stream throughput at the pre-feature baseline commit vs. the feature-complete commit on a single-node HA cluster, distinguishing two signals since facts are fire-and-forget: commit throughput (durable-append + majority-registration only) and apply/observable throughput (commit + the ordered apply pump's mutation, the new hop this spec introduces). Commit throughput: unregressed (~3111/s baseline vs ~3033/s feature, ~2.5% delta, within noise) — Req 11.4's "no per-fact synchronous commit barrier" claim independently confirmed both by this measurement and by code inspection (`commit_submit(COMMIT_MODE_RETAIN, ...)` in `commit.c` returns immediately, byte-identical mechanism to pre-feature code). Apply/observable throughput: **initially found regressed 56%** (2831/s baseline vs 1237/s feature) with clearly superlinear scaling (2x fact volume → 5.2x elapsed time) — a genuine, in-scope defect this task's own benchmark surfaced, not a false alarm, correctly reported BLOCKED rather than silently accepted or patched outside boundary. Root-caused (fresh debug investigation) to `apply_pump_read_entry` (`persist.c`, introduced this spec's task 6.1): the function has zero memory across calls — every single LSN lookup re-runs a full `opendir`/`readdir`/segment-sort and rescans its segment linearly from byte offset 0 — and its only caller, `persist_apply_pump_run`, calls it once per LSN in strict ascending order with no batching, so applying N contiguous entries does O(N²) total work (entry k's read rescans ~k prior headers). Fixed with a minimal segment-read cache on `persist_t` (`apply_pump_cache_fd`/`_seg_lsn`/`_off`/`_next_lsn`, guarded entirely by the existing `apply_pump_lock`): a fast path `pread`s the next expected LSN directly at a remembered offset on an already-open fd (O(1) hit, same CRC verification as the slow path), falling back unchanged to the original full-rediscovery logic on ANY miss (gap, first call, post-truncation, post-recovery) — correctness never depends on the cache, only performance does. Invalidated at all four lifecycle points a stale cursor could otherwise leak or misread from: `persist_init` (starts invalid), `persist_fini` (closes if open), `persist_recover` (resets before any pump kick can trust a pre-recovery cursor), and `persist_truncate_wal_above` (closes inside the existing real-truncation gate, since a truncation can `ftruncate`/`unlink` segment bytes a stale cached fd/offset might otherwise still successfully, and wrongly, read). **Round-1 review REJECTED** on two findings, both remediated in round 2: (1) `persist_recover`'s cache invalidation initially mutated the new fields without holding `apply_pump_lock`, contradicting the fields' own documented invariant (fixed: wrapped in lock/unlock matching `persist_truncate_wal_above`'s existing pattern; independently re-verified deadlock-free by tracing every call site of both `persist_recover` and `apply_pump_lock` itself — the lock is touched in exactly 3 places, all private to `persist.c`, no re-entrant path exists); (2) the initial fix's "no worse than before" characterization of the cache/trim interaction understated a real, bounded risk — the OLD code failed loudly and immediately (`-ENOENT`) if a needed segment was already trimmed, while the NEW cache can (POSIX unlink-keeps-fd-alive) silently advance `last_applied_lsn` past already-deleted data if the pump is lagging when `trim_wal_segments` fires, a reachable scenario since apply-lag-behind-commit is this feature's entire premise — bounded by the pre-existing `APPLY_DIVERGENT` boot fence (tasks 5.1/5.2) catching it at next restart (deferred loud fencing, not silent corruption, but a real behavior change, not literally risk-free) — corrected via an accurate doc comment, no redesign of `trim_wal_segments` attempted or needed. **Verified end-to-end** (implementer + two independent review rounds): post-fix apply-throughput scales sublinearly (1.46x elapsed for 2x volume, vs. the pre-fix 7.4x reproduced independently on a stashed pre-fix binary in the same environment) and exceeds both the regressed 1237/s and the original 2831/s pre-feature baseline; `test_apply_pump` 5/5, `test_apply_pump_integration` 4/4, `test_apply_correctness_verification` 11/11, `test_persist_recovery` 7/7, `test_persist_truncate` 8/8, plus a broader sweep (`test_demotion_convergence`, `test_ha_core_safety`, `test_commit_resolve`, `test_repl_apply`, `test_commit_barrier_reply`) all pass with zero new failures, independently re-run by both reviewers, not just trusted from the implementer's report. **Two items explicitly flagged as out-of-scope, not fixed here**: (1) an intermittent (~2 of 9 attempts) client hang in the synchronous `jobsub` (`COMMIT_MODE_WAIT`) path shortly after a fresh single-node election, reproduced identically on both pre-fix and post-fix binaries (confirmed unrelated to this fix), resembling but not confirmed identical to the closed bug-1416 boot-race class — worked around via retry for benchmarking only, needs separate root-cause investigation; (2) `trim_wal_segments`'s retention floor has no awareness of the local apply pump's own lag (a distinct, pre-existing architectural gap, same family as an already-tracked standby-cursor variant) — out of scope for this targeted perf fix, flagged for a future spec/task rather than redesigned here. - **7.3**: closes out task-group 7's regression gate. Full re-run confirmed exactly 2 of 33 tests in `test_commit_barrier_reply.c` failing — `test_jobsubmit_returns_before_commit_and_registers_pending_apply` and `test_jobsubmit_array_mixed_outcome_no_phantom_job_for_failed_elements` — both asserting task 3.2's own explicitly-documented interim contract ("no independently observable behavior until task 6 completes it") that tasks 6.1/6.2 (already committed, ordered apply pump) closed by construction. Updated both to assert the new deterministic settled state (poll for reply, then assert success/job-exists), mirroring the idiom already used by the file's other, already-updated passing tests — no production code touched. One judgment call flagged and independently confirmed by reviewer: in the mixed-outcome array test, a committed sibling now applies (job visible) independently of a differently-failing sibling, while the batch's single client-visible reply still reflects the failure — this is the documented, intended interim behavior (collapsing array submission into one atomic WAL record is explicitly out-of-scope, owned by the separate `job-array-single-wal-record` spec), not a regression or scope violation. Verified (implementer + independent reviewer, both fresh runs): `test_commit_barrier_reply` 33/33 pass (reviewer ran 3x to rule out flakiness); full 79-group/926-test suite 0 failures including both bare-binary runs (reviewer ran the full unfiltered `controller_unit_test` binary directly and it passed clean, even through the two tests flagged elsewhere in this file's notes as environment-flaky, bug-1406/1407 — no regression signal, strengthens confidence); `/xce-test-ha --sequential 5` HA soak PASS twice independently (implementer's own run + reviewer's fresh re-run in a separate scratch dir), zero job loss, zero divergence, all 3 nodes led both times. - **7.2**: added `--factstream` mode to `run-ha-soak.sh` (kill-mid-fact-stream against a live 3-node cluster, real `jstart`/`jfinish`/`jfail` wire traffic via `xce_test`'s `worker` REPL context, extended in this task since libxce already had the calls but the REPL didn't expose them) and a phantom-fact checker that reads `waldump`'s decoded `state_raw` bits (extended in this task to decode every worker-fact WAL type, not just `WAL_JOB_SUBMIT`) rather than inferring "did START apply" from the pretty-printed job state string — `job_state_str()`'s RUN-checked-before-EXIT priority ordering hides co-set EXIT/PERR bits on a fail-without-finish job, a genuine phantom-check false-positive caught and fixed here (pre-existing, minor, display-only quirk in `job_state_str` itself is out of boundary, not touched). **Round 2 found a real production bug this task's own scenario was needed to surface**: bug-1416, a boot-time TOCTOU in `persist_verify_apply_correctness()` — a returning node's `persist_recover()` replay+verify pass can race the live apply pump and the standby live-replication apply path (both write the same `last_applied_lsn`/job-table state `persist_verify_apply_correctness` re-reads live), producing a false `APPLY_DIVERGENT` that permanently, incorrectly fences a healthy node (task 5.3's sticky fence never self-clears on a false positive) — reproduced 10/10 across ASAN/release builds, 8-20 job counts, only on the RESTART/catch-up path (a clean 3-node cold boot with no kill never hit it). Fixed with a new `_Atomic bool persist_t.recovery_complete` gate (`persist.h`/`persist.c`): `persist_apply_pump_kick()` no-ops while closed; `ha_apply_single_entry`/`ha_apply_term_entry_via_library` (`ha.c`) skip only the in-memory `apply_wal_entry()` call while closed — the durability write, replication ACK, and `applied_lsn` advance are never gated (mirrors the bug-1043 durability/materialization-decoupling precedent, confirmed in `.wolf/cerebrum.md` since bug-1043 predates the current buglog retention window). `persist_recover()` closes the gate at entry, reopens it plus issues one deliberate catch-up kick right before returning, unconditionally of verdict — a genuine divergence still fences exactly as before, this flag only prevents the *comparison itself* from racing. **A second, harder-to-find gap surfaced only via live verification, not code review**: a node stuck retrying its own losing/timing-out election as a candidate can receive and durably apply live-replicated entries through the *ungated* (`persist_init` default=`true`) path for tens of seconds before `cluster_init_phase2()` ever calls `persist_recover()` for the first time — reproducing the identical false-DIVERGENT symptom through an entry point `persist_recover()`'s own bracket can't see. Closed with `persist_mark_recovery_pending(&c.persist)` in `myapp_run()` (`myapp.c`), called immediately after `persist_init()`/`persist_start()` succeed and before `ha_start_election_async()`/`cluster_run()` can process any network traffic. `persist_init()`'s default stays `true` so none of this spec's ~20 direct-`persist_t`-construction unit fixtures are affected — `persist_mark_recovery_pending` is called only from `myapp.c`. **Round-2 review independently verified, not just re-reported**: ran `test_apply_correctness_verification` itself (11/11, including all 3 real-divergence fence tests, confirming detection wasn't accidentally suppressed), confirmed the ACK/durability paths are genuinely ungated by reading the `ha.c` call sites directly, confirmed the `myapp.c` fix is load-bearing (not noise) by reverting only that hunk and reproducing the original failure live (2 of 3 nodes fenced), confirmed bug-1043 precedent against `.wolf/cerebrum.md` (not in `.wolf/buglog.json` — predates current retention). Verified: live 3-node `--factstream` soak (2 rounds, kill+restart both) PASS, zero `APPLY_DIVERGENT` across 5 `persist_recover()` calls; broader `controller_unit_test` sweep (23 groups) zero new failures (only the 2 pre-existing task-6.1/6.2-documented `test_commit_barrier_reply.c` failures reserved for task 7.3 remain). **Do-not-repeat lesson**: when gating a live path behind a "recovery in progress" flag, the flag's own bracket around the recovery function itself is insufficient if OTHER code paths (election retry, live replication) can mutate the same state before that function is ever first called — the gate must close at the earliest point in the real process lifecycle traffic could possibly begin, not just around the specific function whose output it protects. - **6.4**: closes out task-group 6. Approved first round, zero production changes — group ends 2-for-4 on real bugs found (6.1, 6.2), 2-for-4 clean confirmations (6.3, 6.4). Extended `test_apply_pump_integration.c` (task 6.3) with 2 tests covering the MIXED facts+commands scenario 6.3 explicitly deferred here. **Test 3**: genuinely interleaved COMMAND/FACT/COMMAND/FACT/COMMAND sequence through a real `repl_remove_standby` outage, polls for zero commit/apply/reply throughout, restores quorum, and asserts STRICT original-log-order catch-up via a real append-only sequence spy (not a set-membership check) — an adversarial probe (corrupting the expected slot-0 jobid) was independently reproduced by the reviewer to confirm the assertion genuinely discriminates reordering, not just typos. **Test 4 — the highest-value test in this task**: wires a REAL 4-vcpu `vtaskmgr_t` (mirroring task 6.2's bug-1415 regression harness) to drive 4 commands + 2 facts through ONE covering ACK, independently traced by the reviewer to confirm this genuinely creates 5 vtasks (4 continuation dispatches + 1 apply-pump walk) competing for 4 real worker threads — true N-way concurrent racing of task 6.2's self-healing fix, not just one isolated forced pair like 6.2's own test. The "exactly once" check is per-jobid (not an aggregate total), so a skewed double-fire-on-one/hang-on-another distribution would be caught even though the aggregate count would look fine. Adversarial probe (corrupting expected reply count) independently reproduced. **Two honestly-scoped, explicitly non-blocking caveats, not gaps requiring fixes**: (1) task 6.2's own reviewer flagged a narrow, nanosecond-scale TOCTOU residual (unlocked self-heal check vs. locked list-link in `persist_register_pending_apply()`) as a non-blocking fast-follow — Test 4 does NOT deliberately stress this specific window (no injected synchronization primitive forces it, unlike 6.2's own barrier-based test); 50 clean runs across two independent builds are consistent with "didn't hit it by chance," not proof it's closed — stated explicitly here so it isn't assumed covered by this task's "several concurrent races" framing. (2) Test 4 doesn't directly re-assert per-command reply-after-apply ordering the way Test 1 (task 6.3) does via `reply_already_enqueued` — this property holds BY CONSTRUCTION (both the walk's take-call and the self-heal's `lsn <= last_applied_lsn` check only ever fire after `last_applied_lsn` already reflects that entry, verified in the actual code) and is independently covered by task 6.2's own dedicated test, so this is non-blocking, not a real gap. **Task-group 6 final summary**: the ordered apply pump is now live, activated (6.1), correctly hands off command replies (6.2), proven correct for both isolated (6.3) and mixed/bursty (6.4) command+fact scenarios under both normal and quorum-loss conditions — with 2 real, independently-verified, production-severity bugs found and fixed along the way (6.1's job-state corruption from stale receipt-time computation; 6.2's client-hang race from unordered async dispatch), both closed with dedicated regression coverage, the first ever in this spec to wire a real multi-worker `vtaskmgr_t`. **Optional fast-follow noted, not blocking**: a dedicated stress test that deliberately widens the `persist_register_pending_apply()` check-to-lock window (e.g. via a test-only injection point) to reliably exercise 6.2's disclosed residual TOCTOU gap, since neither 6.2's nor 6.4's tests reliably reach it today. diff --git a/src/controller/persist.c b/src/controller/persist.c index 487603d7..19d857f4 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -656,6 +656,15 @@ int persist_init(persist_t* p, persist_config_t* cfg, cluster_t* c) pthread_mutex_init(&p->apply_pump_lock, NULL); atomic_store(&p->apply_pump_scheduled, 0); + /* Apply-pump segment-lookup cache starts invalid (raft-apply-decoupling + * task 7.4 perf-fix): no segment/fd cached yet. -1 is always a safe + * fallback trigger for apply_pump_read_entry's fast path (see persist.h + * doc comment). */ + p->apply_pump_cache_fd = -1; + p->apply_pump_cache_seg_lsn = 0; + p->apply_pump_cache_off = 0; + p->apply_pump_cache_next_lsn = 0; + /* Create WAL and snapshot directories */ if (mkdir_p(p->wal_path, 0700) != 0) return -errno; if (mkdir_p(p->snap_path, 0700) != 0) return -errno; @@ -785,6 +794,15 @@ void persist_fini(persist_t* p) * first, same precondition every other persist_t resource here already * assumes). */ pthread_mutex_destroy(&p->apply_pump_lock); + + /* raft-apply-decoupling task 7.4 perf-fix: close any cached segment fd + * left open by apply_pump_read_entry. No walk can be in flight here + * (same precondition as apply_pump_lock's destroy just above), so no + * lock is needed. */ + if (p->apply_pump_cache_fd >= 0) { + close(p->apply_pump_cache_fd); + p->apply_pump_cache_fd = -1; + } } /* Forward declaration — defined after checkpoint internals */ @@ -3004,6 +3022,72 @@ static int apply_pump_read_entry(persist_t* p, uint64_t lsn, if (!p || lsn == 0 || !out_type || !out_payload || !out_len) return -EINVAL; + /* Fast path (raft-apply-decoupling task 7.4 perf-fix): the sole caller + * (persist_apply_pump_run) calls this once per LSN in strict ascending + * order with no gaps in the common case, and holds apply_pump_lock + * across the entire walk -- so the cache fields below need no lock of + * their own here (see persist.h doc comment). A cache hit reads the next + * header directly at the remembered offset on the already-open fd, + * giving amortized O(1) per entry instead of a fresh + * opendir/readdir/segment-sort plus a scan-from-zero on every single + * call (the O(n^2) regression this fixes). ANY miss below just drops the + * cache and falls through to the unchanged slow path -- correctness + * never depends on the cache being right, only performance does. */ + if (p->apply_pump_cache_fd >= 0 && p->apply_pump_cache_next_lsn == lsn) { + wal_entry_hdr_t hdr; + ssize_t hn = pread(p->apply_pump_cache_fd, &hdr, sizeof(hdr), + (off_t)p->apply_pump_cache_off); + bool hit = (hn == (ssize_t)sizeof(hdr)) && (hdr.lsn == lsn); + uint8_t* payload = NULL; + + if (hit && hdr.payload_len > 0) { + payload = malloc(hdr.payload_len); + if (!payload) { + hit = false; + } else { + ssize_t pn = pread(p->apply_pump_cache_fd, payload, + hdr.payload_len, + (off_t)(p->apply_pump_cache_off + + sizeof(hdr))); + if (pn != (ssize_t)hdr.payload_len) { + free(payload); + payload = NULL; + hit = false; + } + } + } + if (hit) { + uint32_t expected_crc = + crc32(0L, (const Bytef*)&hdr, WAL_HDR_CRC_SIZE); + if (hdr.payload_len > 0) + expected_crc = crc32(expected_crc, (const Bytef*)payload, + hdr.payload_len); + if (hdr.crc != expected_crc) { + free(payload); + payload = NULL; + hit = false; + } + } + + if (hit) { + *out_type = hdr.type; + *out_payload = payload; + *out_len = hdr.payload_len; + p->apply_pump_cache_off += + (uint32_t)(sizeof(hdr) + hdr.payload_len); + p->apply_pump_cache_next_lsn = lsn + 1; + return 0; + } + + /* Miss (short read/EOF meaning the entry rolled into the next + * segment, header mismatch, CRC failure, whatever): don't try to be + * clever about advancing to the next segment inline. Close and fall + * through to the EXISTING slow path unchanged -- it already + * correctly finds the right segment from scratch. */ + close(p->apply_pump_cache_fd); + p->apply_pump_cache_fd = -1; + } + DIR* d = opendir(p->wal_path); if (!d) return -ENOENT; @@ -3090,12 +3174,29 @@ static int apply_pump_read_entry(persist_t* p, uint64_t lsn, *out_payload = payload; *out_len = hdr.payload_len; rc = 0; + + /* raft-apply-decoupling task 7.4 perf-fix: populate the + * segment-lookup cache with this fd positioned just past the + * entry we just returned, BEFORE closing it below, so the + * next expected LSN (lsn+1) hits the fast path above instead + * of re-scanning from segment start next time. Every OTHER + * branch (error paths, not-found-in-this-segment + * continuation) closes fd exactly as before -- caching only + * ever happens on this success branch. */ + p->apply_pump_cache_fd = fd; + p->apply_pump_cache_seg_lsn = seg_lsns[si]; + p->apply_pump_cache_off = + off + (uint32_t)(sizeof(hdr) + hdr.payload_len); + p->apply_pump_cache_next_lsn = lsn + 1; + fd = -1; /* ownership transferred to the cache -- the + * close(fd) below becomes a guarded no-op. */ break; } off += (uint32_t)(sizeof(hdr) + hdr.payload_len); } - close(fd); + if (fd >= 0) + close(fd); if (rc == 0 || rc == -ENOMEM || rc == -EIO) break; /* found it, or hit a genuine error inside the segment @@ -3709,6 +3810,29 @@ int persist_recover(persist_t* p) * whose own recovery never finished), not a new hazard. */ atomic_store_explicit(&p->recovery_complete, false, memory_order_release); + /* raft-apply-decoupling task 7.4 perf-fix (round 2): invalidate the apply + * pump's segment-lookup cache before replay -- the cached fd/offset are + * only valid relative to the pre-recovery on-disk state, and replay below + * can change checkpoint_lsn/current_lsn. Doing this first covers every + * return path below (including the early I/O-failure returns), so the + * first post-recovery pump kick always falls back to full segment + * rediscovery from the correct, recovered position -- never trusting a + * stale pre-recovery cursor. Wrapped in apply_pump_lock (matching + * persist_truncate_wal_above's identical invalidation site below) so the + * cache fields' documented invariant -- "guarded ENTIRELY by + * apply_pump_lock, never p->lock" (persist.h) -- holds unconditionally + * here too, rather than resting on the implicit fact that no live pump + * walk can be racing this boot-time call. No caller of persist_recover() + * holds apply_pump_lock (its sole production call site, cluster.c boot + * sequence, runs before the pump is ever kicked), so this cannot + * deadlock. */ + pthread_mutex_lock(&p->apply_pump_lock); + if (p->apply_pump_cache_fd >= 0) { + close(p->apply_pump_cache_fd); + p->apply_pump_cache_fd = -1; + } + pthread_mutex_unlock(&p->apply_pump_lock); + /* --- Step 1: find latest snapshot (highest LSN filename) --- */ char snap_file[4096] = ""; uint64_t snap_lsn = 0; @@ -5115,8 +5239,25 @@ int persist_truncate_wal_above(persist_t* p, uint64_t cutoff_lsn) * releasing p->lock: the callback (the commit engine) takes only * ce->lock, preserving the cycle-free p->lock -> ce->lock edge already * established by the durable-advance wake. */ - if (rc == 0 && after < before && p->on_truncate != NULL) { - p->on_truncate(p->truncate_cb_ctx, cutoff_lsn); + if (rc == 0 && after < before) { + /* raft-apply-decoupling task 7.4 perf-fix: a real truncation may + * ftruncate()/unlink() segment files the apply-pump's segment-lookup + * cache still points into -- invalidate it so a subsequent + * apply_pump_read_entry can never read truncated-away data. p->lock + * is already released above and apply_pump_lock is never held by + * this call path (persist_apply_pump_run is the only place that + * holds it, during its own walk), so no lock-order hazard here. */ + pthread_mutex_lock(&p->apply_pump_lock); + if (p->apply_pump_cache_fd >= 0) { + close(p->apply_pump_cache_fd); + p->apply_pump_cache_fd = -1; + } + p->apply_pump_cache_next_lsn = 0; + pthread_mutex_unlock(&p->apply_pump_lock); + + if (p->on_truncate != NULL) { + p->on_truncate(p->truncate_cb_ctx, cutoff_lsn); + } } return rc; } diff --git a/src/include/persist.h b/src/include/persist.h index fb149173..14d02e7c 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -433,6 +433,52 @@ struct persist_s { * pump; see design.md "Restart Replay & Verification"). */ pthread_mutex_t apply_pump_lock; _Atomic int apply_pump_scheduled; + + /* raft-apply-decoupling task 7.4 perf-fix: apply_pump_read_entry's + * segment-lookup cache. apply_pump_read_entry's only caller + * (persist_apply_pump_run) calls it once per LSN in strict ascending + * order with no gaps in the common case; without this cache every single + * lookup redid a full opendir/readdir/segment-sort plus a pread scan + * from byte offset 0, making N contiguous applies O(N^2). These fields + * cache the currently-open segment fd + byte offset positioned to read + * whichever LSN the pump is expected to ask for next, giving amortized + * O(1) per entry on the common contiguous-advance path. + * + * Guarded ENTIRELY by apply_pump_lock (never p->lock) -- the pump's sole + * caller already holds apply_pump_lock across the whole walk, including + * every apply_pump_read_entry call, so no additional locking is needed + * here. apply_pump_cache_fd == -1 means "no cache" -- this is always a + * safe, correct fallback trigger (falls through to full segment + * rediscovery), never a bug to fall back to. Reset to -1 in persist_init, + * persist_fini, persist_recover (a stale pre-recovery cursor must never + * be trusted), and persist_truncate_wal_above (a real truncation may + * ftruncate()/unlink() the segment the cache still points into). + * + * Known, accepted, bounded tradeoff vs. checkpoint-driven trim + * (trim_wal_segments): trim only unlinks segments below its own trim + * floor, and POSIX unlink-keeps-fd-alive semantics mean an fd this cache + * already has open into a segment stays readable even after trim unlinks + * it on disk. If the pump is lagging behind checkpoint/trim when trim + * fires -- reachable in this feature, since local-apply-lagging-behind- + * commit is raft-apply-decoupling's entire premise -- the cache can keep + * successfully reading that stale, deleted-on-disk data and silently + * advance last_applied_lsn past it. Before this cache existed, every + * lookup did a fresh opendir/readdir, so the equivalent situation failed + * loudly and immediately with -ENOENT on the very next lookup. This is a + * real behavior change -- NOT "no worse than before" -- but it is bounded: + * a crash before the next checkpoint leaves in-memory apply state ahead + * of what disk can prove on restart, and the existing APPLY_DIVERGENT + * boot fence (tasks 5.1/5.2, persist_recover) catches exactly this at the + * next restart and fences the node loudly rather than serve wrong state. + * So the loud failure is deferred from "immediate, at read-time" to + * "delayed, at next-restart-boot-time," not eliminated. Redesigning + * trim/cache invalidation to close this gap at read-time is a distinct, + * pre-existing architectural item (tracked separately) and explicitly + * out of scope for this perf fix. */ + int apply_pump_cache_fd; + uint64_t apply_pump_cache_seg_lsn; + uint32_t apply_pump_cache_off; + uint64_t apply_pump_cache_next_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