diff --git a/.kiro/specs/ha-entity-snapshots/tasks.md b/.kiro/specs/ha-entity-snapshots/tasks.md index 48335ea..1457624 100644 --- a/.kiro/specs/ha-entity-snapshots/tasks.md +++ b/.kiro/specs/ha-entity-snapshots/tasks.md @@ -50,7 +50,7 @@ - _Boundary: Persistence Layer (write_snapshot)_ - [ ] 4. Core: protect an in-flight checkpoint transfer from rotation -- [ ] 4.1 (P) Track which checkpoint position each in-flight peer transfer references +- [x] 4.1 (P) Track which checkpoint position each in-flight peer transfer references - Add a per-peer tracking array alongside the existing in-flight-transfer flag; set it when a transfer begins, clear it when the transfer completes - Observable: a unit test confirms the tracked position is set before a transfer's first byte @@ -131,3 +131,4 @@ - (2.2) First attempt ALSO touched `load_snapshot`/`persist.h` to make task 2.1's round-trip tests pass end-to-end, but review found it advanced `current_lsn`/`checkpoint_lsn` BEFORE the entity-decode loop ran -- a live silent-divergence hazard reachable through the unmodified `ha_handle_snapshot_chunk` production path (reachable today, not hypothetical, since `write_snapshot` now always emits entity-bearing checkpoints). REMEDIATED by fully reverting `load_snapshot`/`persist.h` to pre-2.2 state (byte-identical to commit `0f344da`) and rewriting the round-trip tests to parse `write_snapshot`'s raw on-disk bytes directly (new `snapent_read_checkpoint_jobs` helper using `read_Job`/`decode_Job`) instead of round-tripping through `persist_load_snapshot`. Task 5.2 must implement `load_snapshot`'s entity-decode from scratch per design.md's restore-before-advance-position contract -- do not assume any decode scaffolding exists yet. - (3.1) `snapent_read_checkpoint_queues`'s job-section skip loop duplicates `snapent_read_checkpoint_jobs`'s (task 2.2) skip logic verbatim rather than sharing a helper -- minor, non-blocking; consider factoring a shared "skip past job section" reader if task 5.x's test coverage grows the file further. - (3.2) Pre-existing ASan leak in the test fixture itself (`snapent_make` -> `makeInstance`/`makeJob`/`makeQueue`/`xkvmgr_add`, ~18KB/113 allocations), unrelated to any production code -- present since task 1.1, masked by the project's standing `ASAN_OPTIONS=detect_leaks=0` convention for this suite. Not blocking any task; worth a dedicated fixture-teardown cleanup pass if this test file keeps growing. +- (4.1) `snapshot_inflight_lsn[peer_idx]` clears to 0 BEFORE `snapshot_inflight[peer_idx]` clears to 0 in every path (completion + all 3 failure sites in `ha_snapshot_send_dispatch`). Task 4.3's `rotate_snapshots` consultation must therefore treat `snapshot_inflight_lsn[i] != 0` as the "protected" signal directly (checkpoint LSNs are never 0), not "check flag==1 first, then trust LSN" -- there's a narrow window where the flag is still 1 but the LSN has already cleared. diff --git a/src/controller/ha.c b/src/controller/ha.c index 43d3171..a8974f0 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -2778,7 +2778,12 @@ static void snapshot_lane_fn(void *arg) ha_snapshot_do_send(c, sx->ep, peer_idx); /* Clear in-flight LAST so a concurrent heartbeat tick cannot enqueue a - * second snapshot for this peer until this one has fully finished. */ + * second snapshot for this peer until this one has fully finished. + * Clear the tracked LSN (Req 3.1-3.3) alongside the flag — same + * lifecycle, one more field — so rotate_snapshots never sees a stale + * in-flight LSN for a transfer that has already completed. */ + atomic_store_explicit(&c->ha.snapshot_inflight_lsn[peer_idx], 0, + memory_order_release); atomic_store_explicit(&c->ha.snapshot_inflight[peer_idx], 0, memory_order_release); free(sx); @@ -2810,8 +2815,19 @@ static void ha_snapshot_send_dispatch(ha_state_t *ha, cluster_t *c, return; } + /* Record the checkpoint position this transfer references (Req 3.1-3.3) + * at the SAME point the flag above is set — before the first byte of the + * transfer goes out — so a reader that observes snapshot_inflight[peer_idx] + * == 1 can find a meaningful LSN alongside it. rotate_snapshots (task 4.3) + * consults this to avoid deleting the checkpoint file this transfer is + * still reading from. */ + atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], + c->persist.checkpoint_lsn, memory_order_release); + snapshot_send_ctx_t *sx = malloc(sizeof(*sx)); if (!sx) { + atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], 0, + memory_order_release); atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0, memory_order_release); return; @@ -2827,6 +2843,8 @@ static void ha_snapshot_send_dispatch(ha_state_t *ha, cluster_t *c, * refused — release the flag we set so no clear is lost, and let the * next tick retry. */ free(sx); + atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], 0, + memory_order_release); atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0, memory_order_release); return; @@ -2838,11 +2856,14 @@ static void ha_snapshot_send_dispatch(ha_state_t *ha, cluster_t *c, WARN("HA repl: slowlane_submit snapshot peer[%d] failed: %d — retry " "next cycle", peer_idx, srv); free(sx); + atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], 0, + memory_order_release); atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0, memory_order_release); return; } - /* 0 or -EAGAIN: enqueued exactly once; snapshot_lane_fn clears the flag. */ + /* 0 or -EAGAIN: enqueued exactly once; snapshot_lane_fn clears the flag + * and the tracked LSN. */ } /* ─────────────────────────────────────────────────────────────────────── diff --git a/src/controller/ha.h b/src/controller/ha.h index 9e17fd8..00c1d2e 100644 --- a/src/controller/ha.h +++ b/src/controller/ha.h @@ -300,6 +300,25 @@ typedef struct { * set, the heartbeat decision loop / * promotion enqueues no duplicate config * push. 0 at init (ha_init memset). */ + /* ── New field added by ha-entity-snapshots spec (Task 4.1) ──────── */ + _Atomic uint64_t snapshot_inflight_lsn[MAX_HA_CANDIDATES]; /* Per-peer + * checkpoint LSN referenced by an + * in-flight snapshot transfer (Req + * 3.1-3.3): set in + * ha_snapshot_send_dispatch at the + * SAME point snapshot_inflight[peer_idx] + * is set to 1 (before dispatch), + * cleared at the same points the flag + * is cleared to 0 (on completion or + * CAS-release failure) — identical + * lifecycle, one more field. Consulted + * by rotate_snapshots (task 4.3) to + * skip deleting a checkpoint file a + * peer transfer is still reading from. + * Explicitly zeroed on every clear so + * a stale value can never be mistaken + * for a live in-flight LSN. 0 at + * init (ha_init memset). */ } ha_state_t; /* ── Public API ────────────────────────────────────────────── */ diff --git a/src/controller/testsuite/test_ha_liveness.c b/src/controller/testsuite/test_ha_liveness.c index d96bc09..59cb505 100644 --- a/src/controller/testsuite/test_ha_liveness.c +++ b/src/controller/testsuite/test_ha_liveness.c @@ -1969,6 +1969,68 @@ static void test_snapshot_offload_dedups_per_peer(void) printf("test_snapshot_offload_dedups_per_peer: PASS\n"); } +/* ─────────────────────────────────────────────────────────────────────── + * ha-entity-snapshots Task 4.1 (Req 3.1-3.3): the in-flight-LSN tracking + * array must be set to the checkpoint's position BEFORE a transfer's first + * byte goes out, and cleared once the transfer completes -- proven with the + * same gate mechanism the dedup test above uses to observe the tracker + * mid-flight (parked), not just before/after. + * ─────────────────────────────────────────────────────────────────────── */ +static void test_snapshot_inflight_lsn_set_before_send_cleared_after(void) +{ + cluster_t c; + memset(&c, 0, sizeof(c)); + ha_init(&c.ha, &g_cfg, 0); + TEST_ASSERT_EQUAL_INT(0, slowlane_init(&c.slowlane, 2, 4)); + c.persist.checkpoint_lsn = 42; + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + + snap_reset_probe(); + atomic_store(&g_snap_gate_on, 1); /* lane body parks at the gate */ + ha_repl_send_snapshot_hook = snap_probe; + ha_set_snapshot_offload_for_test(1); + + TEST_ASSERT_EQUAL_UINT64_MESSAGE(0, + atomic_load_explicit(&c.ha.snapshot_inflight_lsn[0], memory_order_acquire), + "Req 3.1-3.3: no transfer in flight yet -- tracked LSN must be unset"); + + TEST_ASSERT_EQUAL_INT(0, snap_dispatch_once(&c, &ep, 0)); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, snap_wait_parked(2000), + "the snapshot body must reach the gate (running on the lane)"); + + TEST_ASSERT_EQUAL_UINT64_MESSAGE(42, + atomic_load_explicit(&c.ha.snapshot_inflight_lsn[0], memory_order_acquire), + "Req 3.1-3.3: while the transfer is in flight (parked before its " + "first byte is released), the tracked LSN must equal the checkpoint " + "position this transfer references"); + + snap_release_gate(); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, snap_wait_runs(1, 2000), + "the parked snapshot body must complete after release"); + + /* Bounded poll: snapshot_lane_fn clears the LSN then the flag after the + * send body returns -- give it a moment to finish its own cleanup. */ + uint64_t cleared = 1; + for (int i = 0; i < 2000; i++) { + cleared = atomic_load_explicit(&c.ha.snapshot_inflight_lsn[0], + memory_order_acquire); + if (cleared == 0) break; + struct timespec ts = { .tv_sec = 0, .tv_nsec = 1000000L }; + nanosleep(&ts, NULL); + } + TEST_ASSERT_EQUAL_UINT64_MESSAGE(0, cleared, + "Req 3.1-3.3: once the transfer completes, the tracked LSN must be " + "cleared so rotate_snapshots never sees a stale in-flight LSN"); + + ha_set_snapshot_offload_for_test(0); /* restore production default (OFF) */ + ha_repl_send_snapshot_hook = NULL; + slowlane_fini(&c.slowlane); + ha_fini(&c.ha); + printf("test_snapshot_inflight_lsn_set_before_send_cleared_after: PASS\n"); +} + /* ─────────────────────────────────────────────────────────────────────── * Task 3.3 — requester-less ConfigPush slow-lane offload (Req 1.1, 1.5; bug-1198). * @@ -2214,6 +2276,8 @@ void test_ha_liveness(void) RUN_TEST(test_snapshot_offload_runs_on_lane); RUN_TEST(test_snapshot_inline_when_flag_off); RUN_TEST(test_snapshot_offload_dedups_per_peer); + /* ha-entity-snapshots Task 4.1 — in-flight snapshot LSN tracking (Req 3.1-3.3) */ + RUN_TEST(test_snapshot_inflight_lsn_set_before_send_cleared_after); /* raft-drive-async Task 3.3 — requester-less ConfigPush slow-lane offload (Req 1.1, 1.5, 6.x) */ RUN_TEST(test_configpush_offload_runs_on_lane); RUN_TEST(test_configpush_inline_when_flag_off);