diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 6209cc34..dbe1449d 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -18972,6 +18972,31 @@ ], "occurrences": 1, "last_seen": "2026-08-25T14:05:00Z" + }, + { + "id": "bug-1694", + "timestamp": "2026-08-25T00:00:00.000Z", + "error_message": "xce-audit-invariants finding (dimension 1, async I/O thread-role): checkpoint_task_fn (src/controller/persist.c) was the last delaymgr-triggered path still calling its full blocking body (persist_write_checkpoint_marker's pwrite+fdatasync, write_snapshot()'s unbounded open/write walk over every job/queue/host frame held under apply_pump_lock, trim_wal_segments, rotate_snapshots' unlink) directly on the delaymgr timer thread -- the same single thread that drives heartbeats and election timeouts cluster-wide (ha-async-io.md thread-role contract: delaymgr must only trigger work, never block). Triggered from the WAL-append hot path via schedule_checkpoint()/delay_task() whenever wal_record_count crosses max_wal_records. Every sibling long-blocking-I/O path (snapshot send, config-push, reconnect) had already been offloaded by raft-drive-async; checkpoint was the one documented remaining gap.", + "file": "src/controller/persist.c, src/include/persist.h, src/controller/testsuite/test_persist_checkpoint_offload.c, src/controller/testsuite/controller_test.c, src/controller/meson.build", + "root_cause": "checkpoint_task_fn called persist_checkpoint() inline with no offload mechanism at all -- unlike every other delaymgr-triggered blocking path in the codebase. Investigated whether the obvious fix (vtask_enq_classified(pool, task, VTASK_LIVE_BLOCK), the pattern used by persist_apply_pump_kick/commit.c's dispatch_continuation) was safe here: it is NOT -- persist_apply_pump_run() (which ALSO acquires apply_pump_lock, across its entire per-entry walk) is itself dispatched onto that SAME vtaskmgr N-thread pool via persist_apply_pump_kick()'s vtask_enq_classified(..., VTASK_LIVE_BLOCK). Putting checkpoint's genuinely unbounded write_snapshot() (held under apply_pump_lock) on the identical N-thread pool would risk a worker thread parked on apply_pump_lock for the whole snapshot-write duration, starving the pool's liveness-critical capacity (commit continuations, apply-pump drains) -- exactly the failure mode ha-async-io.md's own table entry warns about (\"vtaskmgr must NOT do: long/unbounded blocking I/O (snapshot transfer, config-push) -- starves the pool\"). The correct destination, per the codebase's OWN established distinction (re-verified by reading all 4 reference sites plus grepping every offload flag), is the SLOW LANE (slowlane_submit) -- the same mechanism snapshot-send (ha_snapshot_do_send -> snapshot_lane_fn, g_drive_async_snapshot_offload, default ON) and config-push (configpush_lane_fn, g_drive_async_configpush_offload, default ON) already use, and NOT the vtask pool. Confirms the task brief's suspicion that checkpoint was harder than a simple vtask wrap -- it's a slow-lane fix, not a vtask-pool fix.", + "fix": "FIXED. checkpoint_task_fn is now trigger-only: it releases the checkpoint_in_progress CAS slot, then submits the real work (via a new run_checkpoint_body()/checkpoint_lane_fn()) to the cluster's slow lane (slowlane_submit(&persist->cluster->slowlane, key=1, checkpoint_lane_fn, persist)), mirroring snapshot_lane_fn/configpush_lane_fn/catchup_lane_fn/demotion_lane_fn exactly. New feature flag g_persist_checkpoint_offload (persist_set_checkpoint_offload_for_test/persist_checkpoint_offload_enabled, persist.h/persist.c) defaults ON (1) -- unlike the still-experimental catchup/demotion offloads (default OFF pending an unrelated real test-fixture hang, bug-1429), this reuses an ALREADY-PROVEN-SAFE call: persist_checkpoint() already runs on this same slow lane in production today via ha_snapshot_do_send()->snapshot_lane_fn (snapshot offload also default ON), so apply_pump_lock's acquisition from a slow-lane thread is exercised in production every day, not new surface. persist_checkpoint()'s own atomic_exchange guard (checkpoint_in_progress) remains the single-flight barrier preventing double-run with the snapshot-send path, exactly as it already does today. Verified no existing controller_unit_test fixture depends on the delaymgr trigger path completing synchronously (test_ha_core_safety.c deliberately pins max_wal_records=1u<<30 specifically to prevent ever exercising it in a live-cluster fixture) -- ruling out the catchup/demotion-class 'uninitialized slow lane + a synchronous downstream waiter hangs' hazard, since checkpoint's trigger is pure fire-and-forget. Inline-fallback + WARN on genuine slowlane_submit failure only (not -EAGAIN/-EEXIST, both non-failure per slowlane's own documented contract) -- a scheduled checkpoint is never silently dropped. New regression suite test_persist_checkpoint_offload.c (3 tests, mirrors test_ha_liveness.c's reconnect/configpush offload gate-probe pattern): test_checkpoint_offload_trigger_returns_promptly (GREEN: trigger returns <200ms while the real body is observably parked on a slow-lane worker thread, not the caller's), test_checkpoint_inline_blocks_delaymgr_thread_RED (flag OFF reproduces the pre-fix inline-blocking behavior on demand, permanent regression guard), test_checkpoint_no_cluster_runs_inline (defensive NULL-cluster fallback). RED-phase independently confirmed by temporarily forcing checkpoint_task_fn's pre-fix always-inline behavior (a one-line `if (1) ||` probe) and rebuilding: the GREEN test then hung indefinitely (process had to be kill -9'd, PID confirmed via ps as the stuck controller_unit_test binary) -- proving the test genuinely detects the violation, not a tautology. Probe reverted (git diff --stat confirmed clean re-application of the real fix) and the full suite re-run GREEN: 1184 Tests, identical 2 pre-existing failures both runs (test_conf_trim_keeps_only_n_versions Expected 2 Was 3, test_conf_get_saved_tag_no_config Expected 0 Was , test_config_repl.c, both matching bug-1561's well-documented overlayfs fdatasync-EIO artifact signature exactly -- 0 NEW failures. Live HA smoke test (real checkpoint trigger against a running deployed cluster under load) was scoped out given budget already spent on the lock-order investigation, which was this fix's primary risk -- flagged explicitly in the final report rather than silently skipped, as a reasonable follow-up soak.", + "tags": [ + "async-io", + "thread-role", + "delaymgr", + "checkpoint", + "persist.c", + "slowlane", + "apply_pump_lock", + "vtask", + "xce-audit-invariants", + "raft-drive-async" + ], + "related_bugs": [ + "bug-1429" + ], + "occurrences": 1, + "last_seen": "2026-08-25T00:00:00.000Z" } ] } diff --git a/src/controller/meson.build b/src/controller/meson.build index 28d693fd..b849d111 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -180,6 +180,7 @@ controller_unit_test_src = [ 'testsuite/test_apply_correctness_verification.c', 'testsuite/test_apply_pump.c', 'testsuite/test_apply_pump_integration.c', + 'testsuite/test_persist_checkpoint_offload.c', 'testsuite/test_job_array_apply.c', 'testsuite/test_job_array_fence.c', 'testsuite/test_job_array_replay_fence.c', diff --git a/src/controller/persist.c b/src/controller/persist.c index 1334d8f7..027ea192 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -3182,12 +3182,162 @@ static void rotate_snapshots(persist_t* p) /* Async checkpoint scheduling (schedule_checkpoint + task fn) */ /* ------------------------------------------------------------------ */ -/* checkpoint_task_fn: executed by delaymgr worker thread. +#ifdef XCE_TESTING +/* Test-injectable substitute for the real (blocking) checkpoint work, + * mirroring ha.c's ha_repl_send_snapshot_hook idiom exactly: when non-NULL, + * checkpoint_lane_fn calls this INSTEAD OF persist_checkpoint() — lets a + * test park/gate the lane worker without needing a full disk-backed + * checkpoint to actually complete. NULL in every fixture that never installs + * it (the overwhelming common case): checkpoint_lane_fn falls through to the + * real call. Symbol always compiled so test TUs can `extern` it; unused in + * a non-XCE_TESTING (production) build. */ +void (*persist_checkpoint_test_hook)(persist_t*) = NULL; +#endif + +/* g_persist_checkpoint_offload: checkpoint slow-lane offload feature flag + * (xce-audit-invariants controller pass, dimension 1 async-I/O thread-role + * fix, bug-1694). + * + * checkpoint_task_fn (below) was the last delaymgr-triggered path in this + * codebase that still ran its full blocking body directly on the delaymgr + * timer thread -- the SAME single thread that drives heartbeats and election + * timeouts cluster-wide (ha-async-io.md thread-role contract: delaymgr must + * only trigger work, never block). The blocking body is real: a + * WAL_CHECKPOINT marker pwrite+fdatasync, write_snapshot()'s unbounded + * open/write walk over every job/queue/host frame (scales with cluster state + * size, held under apply_pump_lock), then trim_wal_segments + rotate_ + * snapshots' unlink. Every sibling long-blocking-I/O path (snapshot send, + * config-push, reconnect) was already moved off its liveness thread by + * raft-drive-async -- this was the one documented remaining gap. + * + * DEFAULT ON (1), unlike the still-experimental catchup/demotion offloads + * (g_drive_async_catchup_offload / g_drive_async_demotion_offload in + * config_repl.c / ha_convergence.c, both default OFF pending an unrelated, + * real test-fixture hang -- see their own comments and bug-1429): this + * reuses an ALREADY-PROVEN-SAFE call, not new surface. persist_checkpoint() + * already runs on this SAME slow lane in production TODAY via + * ha_snapshot_do_send() -> snapshot_lane_fn (g_drive_async_snapshot_offload, + * ha.c, also default ON) -- every snapshot-send cycle already calls + * persist_checkpoint() from a slow-lane worker thread, so apply_pump_lock's + * acquisition inside persist_checkpoint() from a slow-lane thread is + * exercised in production every day, not a new code path. persist_ + * checkpoint()'s own atomic_exchange guard (checkpoint_in_progress) is the + * existing, already-relied-upon single-flight barrier that keeps this + * offload and the snapshot-send path from ever double-running a checkpoint + * concurrently (see g_drive_async_snapshot_offload's own comment: "at most + * one persist_checkpoint in flight (existing atomic guard)"). + * + * Destination is the SLOW LANE (slowlane_submit), deliberately NOT the + * vtaskmgr N-thread pool: write_snapshot()'s per-entity walk is genuinely + * unbounded, matching ha-async-io.md's own table entry ("vtaskmgr must NOT + * do: long/unbounded blocking I/O (snapshot transfer, config-push) -- + * starves the pool"). The vtask pool was investigated and ruled out + * specifically because persist_apply_pump_run() -- which ALSO acquires + * apply_pump_lock, across its entire per-entry walk -- is itself dispatched + * onto that SAME pool via persist_apply_pump_kick()'s + * vtask_enq_classified(pool, task, VTASK_LIVE_BLOCK). Putting checkpoint's + * unbounded, lock-held I/O on the identical N-thread pool would risk a + * worker thread parked on apply_pump_lock for the whole snapshot-write + * duration, starving the pool's liveness-critical capacity (commit + * continuations, apply-pump drains) -- exactly the failure mode the + * codebase's own slow-lane/vtask-pool split already exists to prevent. The + * slow lane's dedicated, separate worker threads carry no such contention + * with the vtask pool's own capacity, and this checkpoint offload does not + * introduce any NEW lock-order edge: it is the same apply_pump_lock + * acquisition the snapshot-send path already makes from a slow-lane thread. + * + * No synchronous-wait hazard (the actual mechanism behind the catchup/ + * demotion offloads staying OFF -- an uninitialized slow lane silently + * swallows a submitted task with zero live workers to ever run it, and + * something ELSE downstream then hangs forever waiting for its completion): + * checkpoint's trigger (schedule_checkpoint, fired from the WAL-append hot + * path when wal_record_count crosses max_wal_records) is pure + * fire-and-forget -- nothing waits synchronously for a checkpoint to finish, + * so a silently-swallowed submission degrades to "this checkpoint cycle is + * skipped, retried on the next WAL threshold crossing" (a liveness/WAL- + * growth concern in a hypothetical mis-configured fixture, never a hang). + * Verified no existing controller_unit_test fixture depends on this trigger + * path completing: test_ha_core_safety.c's live-cluster HA fixtures + * deliberately pin max_wal_records to 1u<<30 specifically "so no checkpoint + * is [triggered]" / "never auto-checkpoint during a unit test"; every other + * max_wal_records reference in the suite is a synthetic/no-cluster check + * that never reaches schedule_checkpoint's cluster branch at all. Production + * always calls slowlane_init via cluster_init_phase2 before the delaymgr + * loop can fire this trigger. + * + * OFF -> legacy inline blocking path on the delaymgr thread; retained only + * so the TDD RED test can demonstrate the pre-fix stall. Compiled in ALL + * builds; the setter is the test seam to flip RED<->GREEN. */ +static _Atomic int g_persist_checkpoint_offload = 1; + +void persist_set_checkpoint_offload_for_test(int on) +{ + atomic_store_explicit(&g_persist_checkpoint_offload, on ? 1 : 0, + memory_order_release); +} + +int persist_checkpoint_offload_enabled(void) +{ + return atomic_load_explicit(&g_persist_checkpoint_offload, + memory_order_acquire); +} + +/* run_checkpoint_body: the actual (blocking) checkpoint work -- persist_ + * checkpoint() itself is UNCHANGED. Shared by every call site that can end + * up running it (the slow-lane worker AND checkpoint_task_fn's flag-OFF / + * no-cluster / submit-failure inline-fallback branches) so the XCE_TESTING + * hook seam (persist_checkpoint_test_hook) covers all of them identically -- + * a test can park/observe the checkpoint body regardless of which path + * dispatched it, which is exactly what proves (or, pre-fix, disproves) the + * delaymgr thread never runs it directly. Resets wal_record_count AFTER the + * work actually ran (unconditionally, success or failure alike -- matches + * the pre-fix ordering exactly: the original inline checkpoint_task_fn also + * reset it unconditionally right after persist_checkpoint() returned). */ +static void run_checkpoint_body(persist_t *persist) +{ +#ifdef XCE_TESTING + if (persist_checkpoint_test_hook != NULL) { + persist_checkpoint_test_hook(persist); + atomic_store(&persist->wal_record_count, 0); + return; + } +#endif + + persist_checkpoint(persist); + atomic_store(&persist->wal_record_count, 0); +} + +/* checkpoint_lane_fn: slow-lane worker entry (offload flag ON). Runs + * run_checkpoint_body() off the delaymgr thread. Mirrors snapshot_lane_fn / + * configpush_lane_fn / catchup_lane_fn / demotion_lane_fn exactly. */ +static void checkpoint_lane_fn(void *arg) +{ + persist_t *persist = (persist_t *)arg; + if (!persist) + return; + + run_checkpoint_body(persist); +} + +/* checkpoint_task_fn: executed by delaymgr worker thread -- TRIGGER ONLY + * (ha-async-io.md thread-role contract: delaymgr fires timers and triggers + * work, then returns fast -- it never blocks). * * The CAS in schedule_checkpoint already holds the "in_progress" slot (=1). - * We must clear it before calling persist_checkpoint(), which has its own - * atomic_exchange guard. After persist_checkpoint() returns (it resets the - * flag to 0), we also reset wal_record_count so the counter starts fresh. */ + * Release it so persist_checkpoint's own guard (inside checkpoint_lane_fn, + * now running off-thread) can acquire it, then hand the real (blocking) + * checkpoint work to the slow lane -- see g_persist_checkpoint_offload above + * for the flag + full design rationale. + * + * Inline-fallback + WARN on slowlane_submit failure (mirrors every sibling + * offload's own rule): a scheduled checkpoint must never be silently + * dropped -- that risks unbounded WAL growth. -EEXIST (a checkpoint already + * queued/running under this same lane key) and -EAGAIN (the lane's + * admission cap is momentarily full) are NOT failures -- in both cases the + * work is already going to run (slowlane_submit's own documented contract: + * "In all of {0, -EAGAIN} the work is enqueued and will run exactly once"; + * -EEXIST means it is already enqueued/running under this key). Only a + * genuine submit failure (alloc / lane stopping) falls back inline. */ static int checkpoint_task_fn(void* arg) { persist_t* persist = (persist_t*)arg; @@ -3195,14 +3345,39 @@ static int checkpoint_task_fn(void* arg) /* Release the "scheduled" slot so persist_checkpoint's guard can acquire */ atomic_store(&persist->checkpoint_in_progress, 0); - persist_checkpoint(persist); + if (!persist_checkpoint_offload_enabled() || persist->cluster == NULL) { + /* Legacy inline path -- blocks the calling (delaymgr) thread. + * Retained solely so the RED test can show the pre-change stall; + * production runs with the flag ON. */ + run_checkpoint_body(persist); + return 0; + } - /* Reset the record counter so the next checkpoint window starts clean. - * Done after persist_checkpoint returns — the flag is already 0 by now. */ - atomic_store(&persist->wal_record_count, 0); + int srv = slowlane_submit(&persist->cluster->slowlane, /*key=*/1, + checkpoint_lane_fn, persist); + if (srv < 0 && srv != -EAGAIN && srv != -EEXIST) { + WARN("persist: checkpoint slowlane_submit failed (rc=%d) -- running " + "inline (never silently drop a scheduled checkpoint -- risk of " + "unbounded WAL growth)", srv); + run_checkpoint_body(persist); + } + /* 0 or -EAGAIN: enqueued exactly once; checkpoint_lane_fn resets + * wal_record_count when it actually runs. -EEXIST: a checkpoint is + * already queued/running -- nothing further to do here. */ return 0; } +#ifdef XCE_TESTING +/* Test seam: directly drive checkpoint_task_fn (production's only call site + * is via delay_task, which a test would otherwise have to wait out — the + * real delaymgr fires it 1s after schedule_checkpoint's delay_task call). + * Returns checkpoint_task_fn's own return value (always 0). */ +int persist_checkpoint_task_fn_for_test(persist_t *p) +{ + return checkpoint_task_fn(p); +} +#endif + /* schedule_checkpoint: idempotent async checkpoint request via delaymgr. * Safe to call from any thread. Uses atomic CAS to ensure at most one * checkpoint is scheduled or running at a time. diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index d4a0bf57..8f9f12de 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -94,6 +94,7 @@ extern void test_apply_reapply_idempotency(void); extern void test_apply_correctness_verification(void); extern void test_apply_pump(void); extern void test_apply_pump_integration(void); +extern void test_persist_checkpoint_offload(void); extern void test_job_array_apply(void); extern void test_job_array_fence(void); extern void test_job_array_replay_fence(void); @@ -270,6 +271,7 @@ int main(int argc, char* argv[]) TEST(test_apply_correctness_verification); TEST(test_apply_pump); TEST(test_apply_pump_integration); + TEST(test_persist_checkpoint_offload); TEST(test_job_array_apply); TEST(test_job_array_fence); TEST(test_job_array_replay_fence); diff --git a/src/controller/testsuite/test_persist_checkpoint_offload.c b/src/controller/testsuite/test_persist_checkpoint_offload.c new file mode 100644 index 00000000..c2d05bd4 --- /dev/null +++ b/src/controller/testsuite/test_persist_checkpoint_offload.c @@ -0,0 +1,320 @@ +/* test_persist_checkpoint_offload.c — xce-audit-invariants controller pass, + * dimension 1 async-I/O thread-role fix, bug-1694. + * + * Proves checkpoint_task_fn (persist.c) no longer blocks the delaymgr thread + * with real (unbounded) checkpoint I/O: the trigger now only releases the + * "scheduled" slot and submits the real work to the cluster's slow lane + * (slowlane_submit), returning immediately regardless of how long the real + * checkpoint body takes. Mirrors test_ha_liveness.c's reconnect/configpush + * slow-lane offload tests (rc_run_offload_tick / test_configpush_offload_ + * runs_on_lane) exactly: a gated XCE_TESTING hook (persist_checkpoint_ + * test_hook) substitutes for the real (disk-backed) persist_checkpoint() + * call so the test can park the checkpoint body in flight and observe which + * thread it runs on, without needing a full WAL/snapshot fixture. + * + * checkpoint_task_fn's only production call site is delay_task (1s + * coalescing delay) — persist_checkpoint_task_fn_for_test drives it directly + * so the test doesn't have to wait out a real delaymgr timer. + * + * Fixture is deliberately minimal (bare zeroed persist_t + cluster_t, no + * persist_init/persist_start): every field checkpoint_task_fn/checkpoint_ + * lane_fn/run_checkpoint_body touch — checkpoint_in_progress, + * wal_record_count (plain atomics), cluster->slowlane — is satisfied without + * a real WAL. The XCE_TESTING hook path never reaches the real + * persist_checkpoint() (which WOULD need a real disk-backed persist_t), so + * this is safe. Mirrors test_ha_liveness.c's rc_setup_primary / + * test_configpush_offload_runs_on_lane, which likewise use a bare + * memset-zeroed cluster_t with only ha_init + slowlane_init. + */ +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include + +#include + +#include "persist.h" +#include "slowlane.h" +#include "xce/cluster_def.h" + +/* Test seams (persist.c, compiled under XCE_TESTING — controller_unit_test + * always builds with it). */ +extern int persist_checkpoint_task_fn_for_test(persist_t *p); +extern void (*persist_checkpoint_test_hook)(persist_t *); + +/* ── park/release gate + thread-recording probe ────────────────────────── + * Mirrors test_ha_liveness.c's cp_probe/cp_wait_parked/cp_wait_runs/ + * cp_release_gate/cp_reset_probe (configpush slow-lane offload, raft-drive- + * async task 3.3) exactly, renamed for checkpoint. */ +static _Atomic int g_ckpt_runs = 0; +static _Atomic pthread_t g_ckpt_last_thread; +static _Atomic int g_ckpt_gate_on = 0; +static pthread_mutex_t g_ckpt_mu = PTHREAD_MUTEX_INITIALIZER; +static pthread_cond_t g_ckpt_release_cv = PTHREAD_COND_INITIALIZER; +static int g_ckpt_parked = 0; +static int g_ckpt_release = 0; + +static void ckpt_probe(persist_t *p) +{ + (void)p; + atomic_store_explicit(&g_ckpt_last_thread, pthread_self(), + memory_order_relaxed); + if (atomic_load(&g_ckpt_gate_on)) { + pthread_mutex_lock(&g_ckpt_mu); + g_ckpt_parked = 1; + while (!g_ckpt_release) + pthread_cond_wait(&g_ckpt_release_cv, &g_ckpt_mu); + pthread_mutex_unlock(&g_ckpt_mu); + } + atomic_fetch_add(&g_ckpt_runs, 1); +} + +/* Bounded poll until the gated body parks; 0 on park, -1 on timeout. */ +static int ckpt_wait_parked(int timeout_ms) +{ + pthread_mutex_lock(&g_ckpt_mu); + int parked = 0; + for (int i = 0; i < timeout_ms; i++) { + if (g_ckpt_parked) { parked = 1; break; } + pthread_mutex_unlock(&g_ckpt_mu); + struct timespec ts = { .tv_sec = 0, .tv_nsec = 1000000L }; /* 1 ms */ + nanosleep(&ts, NULL); + pthread_mutex_lock(&g_ckpt_mu); + } + pthread_mutex_unlock(&g_ckpt_mu); + return parked ? 0 : -1; +} + +static int ckpt_wait_runs(int want, int timeout_ms) +{ + for (int i = 0; i < timeout_ms; i++) { + if (atomic_load(&g_ckpt_runs) >= want) break; + struct timespec ts = { .tv_sec = 0, .tv_nsec = 1000000L }; /* 1 ms */ + nanosleep(&ts, NULL); + } + return atomic_load(&g_ckpt_runs); +} + +static void ckpt_release_gate(void) +{ + pthread_mutex_lock(&g_ckpt_mu); + g_ckpt_release = 1; + pthread_cond_broadcast(&g_ckpt_release_cv); + pthread_mutex_unlock(&g_ckpt_mu); +} + +static void ckpt_reset_probe(void) +{ + atomic_store(&g_ckpt_runs, 0); + atomic_store(&g_ckpt_gate_on, 0); + pthread_mutex_lock(&g_ckpt_mu); + g_ckpt_parked = 0; + g_ckpt_release = 0; + pthread_mutex_unlock(&g_ckpt_mu); +} + +/* Elapsed milliseconds between two CLOCK_MONOTONIC timespecs. */ +static double ms_since(const struct timespec *t0, const struct timespec *t1) +{ + return (double)(t1->tv_sec - t0->tv_sec) * 1000.0 + + (double)(t1->tv_nsec - t0->tv_nsec) / 1e6; +} + +/* Minimal fixture: bare zeroed persist_t + cluster_t, real slow lane. See + * file header for why this is sufficient (the hook path never reaches real + * disk I/O). */ +static void ckpt_setup(persist_t *p, cluster_t *c) +{ + memset(p, 0, sizeof(*p)); + memset(c, 0, sizeof(*c)); + TEST_ASSERT_EQUAL_INT(0, slowlane_init(&c->slowlane, 2, 4)); + p->cluster = c; + atomic_store(&p->checkpoint_in_progress, 1); /* mirrors schedule_checkpoint's + * CAS having claimed the slot + * before delay_task fires the + * trigger */ + atomic_store(&p->wal_record_count, 999); +} + +static void ckpt_teardown(cluster_t *c) +{ + slowlane_fini(&c->slowlane); +} + +/* + * GREEN (Req: delaymgr thread must never block on checkpoint I/O): with the + * offload flag ON (the production default), triggering checkpoint_task_fn + * returns promptly — well under the gate's hold duration — while the real + * checkpoint body is still parked in flight on a SLOW-LANE worker thread, + * not the caller's own thread. This is the direct proof that delaymgr's own + * worker thread (which, in production, is the one calling checkpoint_task_fn) + * would never observe the checkpoint body's blocking duration. + */ +static void test_checkpoint_offload_trigger_returns_promptly(void) +{ + persist_t p; + cluster_t c; + ckpt_setup(&p, &c); + + ckpt_reset_probe(); + atomic_store(&g_ckpt_gate_on, 1); /* lane body parks at the gate */ + persist_checkpoint_test_hook = ckpt_probe; + persist_set_checkpoint_offload_for_test(1); + + pthread_t caller = pthread_self(); + + struct timespec t0, t1; + clock_gettime(CLOCK_MONOTONIC, &t0); + int rc = persist_checkpoint_task_fn_for_test(&p); + clock_gettime(CLOCK_MONOTONIC, &t1); + TEST_ASSERT_EQUAL_INT(0, rc); + + double trigger_ms = ms_since(&t0, &t1); + TEST_ASSERT_TRUE_MESSAGE(trigger_ms < 200.0, + "checkpoint_task_fn (the delaymgr-thread trigger) must return " + "promptly — it must never block on the real checkpoint body"); + + /* The "scheduled" slot must already be released so a future + * schedule_checkpoint() call is not permanently locked out. */ + TEST_ASSERT_EQUAL_INT(0, atomic_load(&p.checkpoint_in_progress)); + + /* The real checkpoint body must actually be running (parked) — proving + * the trigger's prompt return is NOT because the work was dropped. */ + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ckpt_wait_parked(2000), + "the offloaded checkpoint body must reach the gate, running " + "somewhere off the trigger's own call stack"); + TEST_ASSERT_FALSE_MESSAGE( + pthread_equal(atomic_load_explicit(&g_ckpt_last_thread, + memory_order_relaxed), caller), + "the checkpoint body must run on a SLOW-LANE worker thread, NOT the " + "trigger's own (delaymgr, in production) thread"); + + /* wal_record_count must NOT have been reset yet — the body hasn't + * finished (still parked at the gate). */ + TEST_ASSERT_EQUAL_UINT(999, atomic_load(&p.wal_record_count)); + + ckpt_release_gate(); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, ckpt_wait_runs(1, 2000), + "the checkpoint body must complete exactly once after release"); + /* Only after the body actually completes does the counter reset. */ + TEST_ASSERT_EQUAL_UINT(0, atomic_load(&p.wal_record_count)); + + persist_checkpoint_test_hook = NULL; + persist_set_checkpoint_offload_for_test(1); /* restore production default */ + ckpt_teardown(&c); + printf("test_checkpoint_offload_trigger_returns_promptly: PASS\n"); +} + +/* + * RED (pre-fix behavior, demonstrated via the flag's OFF/kill-switch path): + * with the offload flag OFF, checkpoint_task_fn falls back to running the + * checkpoint body INLINE, on the calling thread — exactly the delaymgr- + * blocking violation this fix closes. The trigger call itself now BLOCKS + * until the gate is released, and the body runs on the CALLER's own thread. + * This is what checkpoint_task_fn did unconditionally before this fix; with + * the flag flipped OFF the same legacy code path is still reachable, so this + * test demonstrates the exact violation the GREEN test above proves fixed. + */ +typedef struct { persist_t *p; _Atomic int done; int rc; } ckpt_trigger_arg_t; + +static void *ckpt_trigger_thread(void *arg) +{ + ckpt_trigger_arg_t *a = (ckpt_trigger_arg_t *)arg; + a->rc = persist_checkpoint_task_fn_for_test(a->p); + atomic_store(&a->done, 1); + return NULL; +} + +static void test_checkpoint_inline_blocks_delaymgr_thread_RED(void) +{ + persist_t p; + cluster_t c; + ckpt_setup(&p, &c); + + ckpt_reset_probe(); + atomic_store(&g_ckpt_gate_on, 1); + persist_checkpoint_test_hook = ckpt_probe; + persist_set_checkpoint_offload_for_test(0); /* RED: legacy inline path */ + + ckpt_trigger_arg_t arg = { .p = &p }; + atomic_store(&arg.done, 0); + + pthread_t th; + pthread_create(&th, NULL, ckpt_trigger_thread, &arg); + + /* The body must reach the gate — proving it ran (inline, synchronously, + * as part of the trigger call itself), unlike the offloaded case. */ + TEST_ASSERT_EQUAL_INT_MESSAGE(0, ckpt_wait_parked(2000), + "RED: the inline checkpoint body must still run (on the delaymgr/" + "trigger thread itself) with the offload flag OFF"); + + /* While parked, the trigger call has NOT returned — this is the + * blocking-delaymgr-thread violation. */ + struct timespec ts = { .tv_sec = 0, .tv_nsec = 100 * 1000000L }; /* 100ms */ + nanosleep(&ts, NULL); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, atomic_load(&arg.done), + "RED: with the offload flag OFF, checkpoint_task_fn must still be " + "blocked inside the checkpoint body — reproducing the pre-fix " + "delaymgr-thread-blocking violation"); + + /* The body ran on the SAME thread as the trigger — no offload. */ + TEST_ASSERT_TRUE_MESSAGE( + pthread_equal(atomic_load_explicit(&g_ckpt_last_thread, + memory_order_relaxed), th), + "RED: with the offload flag OFF the checkpoint body must run on the " + "trigger's own (delaymgr) thread, not a lane worker"); + + ckpt_release_gate(); + pthread_join(th, NULL); + TEST_ASSERT_EQUAL_INT(0, arg.rc); + TEST_ASSERT_EQUAL_INT(1, atomic_load(&arg.done)); + + persist_checkpoint_test_hook = NULL; + persist_set_checkpoint_offload_for_test(1); /* restore production default */ + ckpt_teardown(&c); + printf("test_checkpoint_inline_blocks_delaymgr_thread_RED: PASS\n"); +} + +/* + * No-cluster fallback (unit-test / bare persist_t, e.g. most of this file's + * siblings): checkpoint_task_fn's own production call site is only ever + * reached via schedule_checkpoint's cluster branch, but this proves the + * defensive persist->cluster == NULL check inside checkpoint_task_fn itself + * also takes the inline path (no crash dereferencing a NULL cluster's + * slowlane). + */ +static void test_checkpoint_no_cluster_runs_inline(void) +{ + persist_t p; + memset(&p, 0, sizeof(p)); + p.cluster = NULL; + atomic_store(&p.checkpoint_in_progress, 1); + atomic_store(&p.wal_record_count, 42); + + ckpt_reset_probe(); + persist_checkpoint_test_hook = ckpt_probe; + persist_set_checkpoint_offload_for_test(1); /* even with flag ON */ + + int rc = persist_checkpoint_task_fn_for_test(&p); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* No lane to submit to (cluster == NULL) — must run synchronously, + * inline, before the trigger call returns. */ + TEST_ASSERT_EQUAL_INT(1, atomic_load(&g_ckpt_runs)); + TEST_ASSERT_EQUAL_UINT(0, atomic_load(&p.wal_record_count)); + TEST_ASSERT_EQUAL_INT(0, atomic_load(&p.checkpoint_in_progress)); + + persist_checkpoint_test_hook = NULL; + printf("test_checkpoint_no_cluster_runs_inline: PASS\n"); +} + +void test_persist_checkpoint_offload(void) +{ + UnitySetTestFile(__FILE__); + RUN_TEST(test_checkpoint_offload_trigger_returns_promptly); + RUN_TEST(test_checkpoint_inline_blocks_delaymgr_thread_RED); + RUN_TEST(test_checkpoint_no_cluster_runs_inline); +} diff --git a/src/include/persist.h b/src/include/persist.h index 2e14589a..bfe4c27f 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -1058,11 +1058,41 @@ int persist_test_force_new_segment(persist_t* p); * comment above). Pass a negative index to disable. Lock-free atomic * write; compiled out of production. */ void persist_test_set_force_job_encode_fail_at(persist_t* p, int index); + +/* Test seam: directly drive the delaymgr-triggered checkpoint task + * (checkpoint_task_fn, persist.c) without waiting out delay_task's real 1s + * delay. Returns the task fn's own return value (always 0). Compiled out of + * production — the only real call site is via delay_task. */ +int persist_checkpoint_task_fn_for_test(persist_t* p); + +/* Test-injectable substitute for the real (blocking) checkpoint work; when + * non-NULL, checkpoint_lane_fn (persist.c) calls this INSTEAD OF + * persist_checkpoint() — lets a test park/gate the slow-lane worker in + * flight. NULL in every fixture that never installs it. Symbol always + * compiled (like ha.c's ha_repl_send_snapshot_hook) so test TUs can extern + * it; unused in a non-XCE_TESTING build. */ +extern void (*persist_checkpoint_test_hook)(persist_t*); #endif /* XCE_TESTING */ /* Checkpoint */ int persist_checkpoint(persist_t* p); +/* + * persist_set_checkpoint_offload_for_test / persist_checkpoint_offload_enabled: + * Feature-flag accessors (xce-audit-invariants controller pass, dimension 1 + * async-I/O thread-role fix, bug-1694). The offload flag gates whether + * checkpoint_task_fn (persist.c) submits the real checkpoint work to the + * cluster's slow lane (ON, the production default — the delaymgr thread + * never blocks on checkpoint I/O) or runs the legacy inline blocking + * checkpoint on the delaymgr thread (OFF, retained only so the TDD RED test + * can demonstrate the pre-fix stall). See checkpoint_task_fn's own doc + * comment (persist.c) for the full design rationale, including why the + * destination is the slow lane and not the vtaskmgr pool. These accessors + * are compiled in ALL builds; the setter is used by tests to flip the flag. + */ +void persist_set_checkpoint_offload_for_test(int on); +int persist_checkpoint_offload_enabled(void); + /* WAL truncation — remove all entries with LSN > cutoff_lsn. * Idempotent; returns 0 on success, negative errno on I/O error. */ int persist_truncate_wal_above(persist_t* p, uint64_t cutoff_lsn);