feat(ha-entity-snapshots): track in-flight snapshot transfer LSN per peer (task 4.1)
Adds snapshot_inflight_lsn[MAX_HA_CANDIDATES] to ha_state_t, alongside the existing snapshot_inflight[] flag it mirrors exactly: set to the checkpoint's LSN at ha_snapshot_send_dispatch before a transfer's first byte goes out, cleared to 0 at every completion/failure path (snapshot_lane_fn's normal completion, and all 3 early-return failure sites: malloc failure, slowlane dedup-refuse, slowlane submit failure). This is the foundation task 4.3 will consult from rotate_snapshots to avoid deleting a checkpoint file a peer is still mid-transfer reading from -- the one genuinely new retention guard this feature needs (the existing write_snapshot-before-trim_wal_segments sequencing already satisfies the WAL-trim side of Requirement 3). Proven via a new gated unit test (test_ha_liveness.c) that observes the tracked LSN mid-flight while the send body is parked at a test gate, not just before/after -- confirms the lifecycle ordering, not just the endpoints. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDYFbrgyDHbvNYcxApKhrX
This commit is contained in:
parent
eac7bb8ad8
commit
8d1ee8b761
4 changed files with 108 additions and 3 deletions
|
|
@ -50,7 +50,7 @@
|
||||||
- _Boundary: Persistence Layer (write_snapshot)_
|
- _Boundary: Persistence Layer (write_snapshot)_
|
||||||
|
|
||||||
- [ ] 4. Core: protect an in-flight checkpoint transfer from rotation
|
- [ ] 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
|
- 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
|
transfer begins, clear it when the transfer completes
|
||||||
- Observable: a unit test confirms the tracked position is set before a transfer's first byte
|
- 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.
|
- (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.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.
|
- (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.
|
||||||
|
|
|
||||||
|
|
@ -2778,7 +2778,12 @@ static void snapshot_lane_fn(void *arg)
|
||||||
ha_snapshot_do_send(c, sx->ep, peer_idx);
|
ha_snapshot_do_send(c, sx->ep, peer_idx);
|
||||||
|
|
||||||
/* Clear in-flight LAST so a concurrent heartbeat tick cannot enqueue a
|
/* 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,
|
atomic_store_explicit(&c->ha.snapshot_inflight[peer_idx], 0,
|
||||||
memory_order_release);
|
memory_order_release);
|
||||||
free(sx);
|
free(sx);
|
||||||
|
|
@ -2810,8 +2815,19 @@ static void ha_snapshot_send_dispatch(ha_state_t *ha, cluster_t *c,
|
||||||
return;
|
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));
|
snapshot_send_ctx_t *sx = malloc(sizeof(*sx));
|
||||||
if (!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,
|
atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0,
|
||||||
memory_order_release);
|
memory_order_release);
|
||||||
return;
|
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
|
* refused — release the flag we set so no clear is lost, and let the
|
||||||
* next tick retry. */
|
* next tick retry. */
|
||||||
free(sx);
|
free(sx);
|
||||||
|
atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], 0,
|
||||||
|
memory_order_release);
|
||||||
atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0,
|
atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0,
|
||||||
memory_order_release);
|
memory_order_release);
|
||||||
return;
|
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 "
|
WARN("HA repl: slowlane_submit snapshot peer[%d] failed: %d — retry "
|
||||||
"next cycle", peer_idx, srv);
|
"next cycle", peer_idx, srv);
|
||||||
free(sx);
|
free(sx);
|
||||||
|
atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], 0,
|
||||||
|
memory_order_release);
|
||||||
atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0,
|
atomic_store_explicit(&ha->snapshot_inflight[peer_idx], 0,
|
||||||
memory_order_release);
|
memory_order_release);
|
||||||
return;
|
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. */
|
||||||
}
|
}
|
||||||
|
|
||||||
/* ───────────────────────────────────────────────────────────────────────
|
/* ───────────────────────────────────────────────────────────────────────
|
||||||
|
|
|
||||||
|
|
@ -300,6 +300,25 @@ typedef struct {
|
||||||
* set, the heartbeat decision loop /
|
* set, the heartbeat decision loop /
|
||||||
* promotion enqueues no duplicate config
|
* promotion enqueues no duplicate config
|
||||||
* push. 0 at init (ha_init memset). */
|
* 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;
|
} ha_state_t;
|
||||||
|
|
||||||
/* ── Public API ────────────────────────────────────────────── */
|
/* ── Public API ────────────────────────────────────────────── */
|
||||||
|
|
|
||||||
|
|
@ -1969,6 +1969,68 @@ static void test_snapshot_offload_dedups_per_peer(void)
|
||||||
printf("test_snapshot_offload_dedups_per_peer: PASS\n");
|
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).
|
* 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_offload_runs_on_lane);
|
||||||
RUN_TEST(test_snapshot_inline_when_flag_off);
|
RUN_TEST(test_snapshot_inline_when_flag_off);
|
||||||
RUN_TEST(test_snapshot_offload_dedups_per_peer);
|
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) */
|
/* 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_offload_runs_on_lane);
|
||||||
RUN_TEST(test_configpush_inline_when_flag_off);
|
RUN_TEST(test_configpush_inline_when_flag_off);
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue