diff --git a/src/controller/ha.c b/src/controller/ha.c index 01d2a82d..603c7a50 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -3295,7 +3295,11 @@ static int ha_apply_term_entry_via_library(ha_state_t *ha, persist_t *p, cluster_t *c, struct endpoint_t *ep, wal_catchup_entry_t *e) { - /* bug-1475 / bug-1466: resync the raft library's log_len from this node's + /* bug-1870: checkpoints no longer write WAL_CHECKPOINT markers, so new + * logs have no such holes; this resync is kept for WALs written by older + * binaries, whose markers still sit in the log. + * + * bug-1475 / bug-1466: resync the raft library's log_len from this node's * OWN recovered/durable WAL tip before consuming the AE, exactly mirroring * ha_sync_raft_log_from_recovery's boot-time computation (persist_durable_ * floor), but applied live, JUST-IN-TIME, on this same single AE-receive diff --git a/src/controller/persist.c b/src/controller/persist.c index 0f36e6aa..8a9e49dc 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -2275,9 +2275,24 @@ uint64_t persist_test_durable_lsn_on_disk(persist_t* p) /* Checkpoint internals */ /* ------------------------------------------------------------------ */ -/* Write a WAL_CHECKPOINT marker to the current segment. - * Never calls repl_broadcast() — checkpoint markers are local only. */ -static int persist_write_checkpoint_marker(persist_t* p) +/* bug-1870: checkpoints no longer write a WAL_CHECKPOINT marker. The marker + * was local-only (never replicated) yet consumed an LSN of the replicated log + * (current_lsn++), so a checkpoint taken by one member only -- the periodic + * max-wal-records one, or the one before a snapshot send -- left a hole the + * other members never had: the leader's next entry carried prev_log_index = + * the marker's LSN, logOk failed on every standby and writes stalled. Even a + * config-marker-triggered checkpoint, taken on every member, stamped the + * marker with each member's own current_lsn at apply time, which need not + * agree. Nothing reads the marker back except to skip it (recovery, + * catch-up streaming), so it is simply not written any more; those skip + * paths stay for markers already present in existing WALs. */ + +#ifdef XCE_TESTING +/* bug-1870: the WAL_CHECKPOINT marker writer checkpoints used before the fix, + * kept for tests only, to produce the on-disk state of a WAL written by an + * older binary (a local marker that consumed an LSN) -- recovery and catch-up + * streaming must still handle such WALs. Never called in production. */ +int persist_test_write_legacy_checkpoint_marker(persist_t* p) { pthread_mutex_lock(&p->lock); @@ -2320,6 +2335,7 @@ static int persist_write_checkpoint_marker(persist_t* p) pthread_mutex_unlock(&p->lock); return 0; } +#endif /* XCE_TESTING */ /* Entity-bearing checkpoint payload (ha-entity-snapshots task 2.2/3.2, Req * 1.1-1.4). Physical layout appended after the existing legacy header @@ -3581,10 +3597,8 @@ int persist_checkpoint(persist_t* p) int rc = 0; - /* 1. Write WAL_CHECKPOINT marker (local only — never replicated) */ - rc = persist_write_checkpoint_marker(p); - if (rc != 0) - goto done; + /* 1. (bug-1870) no WAL_CHECKPOINT marker any more -- see the bug-1870 + * note above the checkpoint payload layout comment. */ /* 2. Record checkpoint_lsn = the APPLIED cursor (last_applied_lsn), not * the physical WAL append tip (current_lsn). write_snapshot() below @@ -3666,9 +3680,8 @@ static int persist_checkpoint_marker_triggered(persist_t* p, if (atomic_exchange(&p->checkpoint_in_progress, 1) != 0) return -EBUSY; - int rc = persist_write_checkpoint_marker(p); - if (rc == 0) - rc = persist_checkpoint_write_locked(p, marker_lsn); + /* bug-1870: no WAL_CHECKPOINT marker (see persist_checkpoint). */ + int rc = persist_checkpoint_write_locked(p, marker_lsn); if (rc == 0) { trim_wal_segments(p); diff --git a/src/controller/testsuite/test_apply_correctness_verification.c b/src/controller/testsuite/test_apply_correctness_verification.c index 7eef76fc..70c3bf31 100644 --- a/src/controller/testsuite/test_apply_correctness_verification.c +++ b/src/controller/testsuite/test_apply_correctness_verification.c @@ -365,6 +365,10 @@ static void test_checkpoint_tail_with_no_snapshot_is_apply_clean(void) * seam to stop persist_checkpoint() midway, so the snapshot is * deleted immediately below to reach the identical post-crash * on-disk state. */ + /* bug-1870: checkpoints no longer write a marker; reproduce a WAL + * written by an older binary (marker, then snapshot). */ + TEST_ASSERT_EQUAL_INT(0, + persist_test_write_legacy_checkpoint_marker(p)); TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(p)); marker_checkpoint_lsn = p->checkpoint_lsn; diff --git a/src/controller/testsuite/test_config_repl.c b/src/controller/testsuite/test_config_repl.c index a1a2a151..79ffd502 100644 --- a/src/controller/testsuite/test_config_repl.c +++ b/src/controller/testsuite/test_config_repl.c @@ -5497,9 +5497,15 @@ static void test_bug1475_catchup_survives_midstream_checkpoint(void) * synchronous flush right after so persist_durable_floor() (which the * ha.c bug-1475 fix reads) reflects it immediately rather than waiting on * the background fsync_worker's own timer (cfg.wal_sync_ms=5000). */ - TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&leader->persist)); /* lsn=2 */ + /* bug-1870: checkpoints no longer write a marker; this test now covers a + * WAL written by an older binary, whose checkpoint marker took lsn=2. */ + TEST_ASSERT_EQUAL_INT(0, + persist_test_write_legacy_checkpoint_marker(&leader->persist)); /* lsn=2 */ + TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&leader->persist)); TEST_ASSERT_EQUAL_INT(0, persist_flush(&leader->persist)); - TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&recv->persist)); /* lsn=2 */ + TEST_ASSERT_EQUAL_INT(0, + persist_test_write_legacy_checkpoint_marker(&recv->persist)); /* lsn=2 */ + TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&recv->persist)); TEST_ASSERT_EQUAL_INT(0, persist_flush(&recv->persist)); /* bug-1466 (unfixed, deliberately NOT touched by this fix): the checkpoint @@ -5558,6 +5564,79 @@ static void test_bug1475_catchup_survives_midstream_checkpoint(void) printf("test_bug1475_catchup_survives_midstream_checkpoint: PASS\n"); } +/* bug-1870: a checkpoint taken by ONE member only -- the primary's periodic + * max-wal-records checkpoint, or the one before a snapshot send -- must not + * leave a hole in the replicated log. persist_write_checkpoint_marker used to + * consume an LSN for its local-only WAL_CHECKPOINT marker, so the leader's + * next real entry carried prev_log_index = the marker's LSN, which no standby + * holds: logOk failed forever and writes stalled (--force-snapshot soak). */ +static void test_bug1870_leader_only_checkpoint_leaves_no_log_hole(void) +{ + char ltmp[256], rtmp[256]; + strcpy(ltmp, "/tmp/xce_cr_b1870_leader_XXXXXX"); + strcpy(rtmp, "/tmp/xce_cr_b1870_recv_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(ltmp)); + TEST_ASSERT_NOT_NULL(mkdtemp(rtmp)); + + cluster_t *leader = t42_make_cluster(ltmp); + cluster_t *recv = t42_make_cluster(rtmp); + TEST_ASSERT_NOT_NULL(leader); + TEST_ASSERT_NOT_NULL(recv); + + enum { TERM = 3 }; + t42_make_leader(leader, TERM, /*n_peers=*/3); + t_bug1475_make_follower(recv, TERM, /*n_peers=*/3); + + /* identical prefix through lsn=1 on both */ + persist_set_term(&leader->persist, TERM); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(&leader->persist, WAL_JOB_UPDATE, NULL, 0)); /* lsn=1 */ + leader->ha.raft.log_len = 1; + persist_set_term(&recv->persist, TERM); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(&recv->persist, WAL_JOB_UPDATE, NULL, 0)); /* lsn=1 */ + recv->ha.raft.log_len = 1; + atomic_store_explicit(&recv->ha.applied_lsn, 1, memory_order_release); + + /* ONLY the leader checkpoints; it must not take a log slot */ + uint64_t before = (uint64_t)leader->persist.current_lsn; + TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&leader->persist)); + TEST_ASSERT_EQUAL_INT(0, persist_flush(&leader->persist)); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(before, + (uint64_t)leader->persist.current_lsn, + "bug-1870: a checkpoint must not consume a replicated-log LSN"); + + /* leader appends two real entries after its checkpoint */ + persist_set_term(&leader->persist, TERM); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(&leader->persist, WAL_JOB_UPDATE, NULL, 0)); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(&leader->persist, WAL_JOB_UPDATE, NULL, 0)); + uint64_t tip = (uint64_t)leader->persist.current_lsn - 1; + atomic_store_explicit(&leader->persist.committed_lsn, tip, + memory_order_release); + + /* real catch-up streamer -> real receive path */ + g_bug1475_recv = recv; + xce_wal_stream_request_hook = bug1475_replay_hook; + int rc = ha_stream_wal_to_standby(/*ep=*/NULL, leader->persist.wal_path, + &leader->persist, /*leader_term=*/TERM, + /*standby_lsn=*/1, /*committed_lsn=*/tip); + xce_wal_stream_request_hook = NULL; + g_bug1475_recv = NULL; + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_UINT64_MESSAGE(tip, + atomic_load_explicit(&recv->ha.applied_lsn, memory_order_acquire), + "bug-1870: a standby that never checkpointed must still catch up " + "past the leader's checkpoint"); + TEST_ASSERT_EQUAL_UINT64(tip, recv->ha.raft.log_len); + TEST_ASSERT_EQUAL_UINT64(TERM, persist_entry_term(&recv->persist, tip)); + + t42_teardown(leader); + t42_teardown(recv); +} + /* * bug-1028 — the catch-up streamer must source each streamed entry's term and * the AppendEntries prev_log_term from the AUTHORITATIVE per-LSN term index @@ -5809,7 +5888,10 @@ static void test_bug1475_finding2_snapshot_install_checkpoint_term_absent(void) TEST_ASSERT_EQUAL_UINT64(2, (uint64_t)leader->persist.current_lsn); atomic_store_explicit(&leader->persist.last_applied_lsn, 2, memory_order_release); - TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&leader->persist)); /* lsn=2 */ + /* bug-1870: legacy (pre-fix) marker at lsn=2, as an older binary wrote. */ + TEST_ASSERT_EQUAL_INT(0, + persist_test_write_legacy_checkpoint_marker(&leader->persist)); /* lsn=2 */ + TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(&leader->persist)); TEST_ASSERT_EQUAL_INT(0, persist_flush(&leader->persist)); TEST_ASSERT_EQUAL_UINT64_MESSAGE(2, leader->persist.checkpoint_lsn, "precondition: checkpoint_lsn must coincide with the checkpoint " @@ -7447,6 +7529,7 @@ void test_config_repl(void) RUN_TEST(test_wal_stream_empty_range_no_batches); RUN_TEST(test_wal_stream_skips_checkpoint_marker); RUN_TEST(test_bug1475_catchup_survives_midstream_checkpoint); /* bug-1475 */ + RUN_TEST(test_bug1870_leader_only_checkpoint_leaves_no_log_hole); /* bug-1870 */ RUN_TEST(test_wal_stream_term_from_index_not_raw_hdr); /* bug-1028 */ /* Task 3.5: snapshot chunk transfer (sender side) */ RUN_TEST(test_snapshot_transfer_two_chunks); diff --git a/src/controller/testsuite/test_persist_checkpoint.c b/src/controller/testsuite/test_persist_checkpoint.c index 4f1722ce..2d5ee89c 100644 --- a/src/controller/testsuite/test_persist_checkpoint.c +++ b/src/controller/testsuite/test_persist_checkpoint.c @@ -171,23 +171,32 @@ static void test_checkpoint_lsn_tracks_last_applied_not_current_lsn(void) } /* ------------------------------------------------------------------ */ -/* Test 3: checkpoint writes a WAL_CHECKPOINT marker (advances lsn) */ +/* Test 3 (bug-1870): a checkpoint writes no WAL_CHECKPOINT marker and */ +/* consumes no LSN -- a local-only marker in the replicated log left a */ +/* hole standbys never had and stalled replication. */ /* ------------------------------------------------------------------ */ -static void test_checkpoint_writes_marker_to_wal(void) +static void test_checkpoint_writes_no_marker_to_wal(void) { char tmpdir[256]; persist_t* p = make_ckpt_persist(tmpdir, sizeof(tmpdir)); TEST_ASSERT_NOT_NULL(p); + persist_wal_async(p, WAL_JOB_UPDATE, "z", 1); uint64_t lsn_before = p->current_lsn; + atomic_store_explicit(&p->last_applied_lsn, lsn_before - 1, + memory_order_relaxed); TEST_ASSERT_EQUAL_INT(0, persist_checkpoint(p)); - /* current_lsn must have advanced — WAL_CHECKPOINT marker was written */ - TEST_ASSERT_TRUE(p->current_lsn > lsn_before); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(lsn_before, p->current_lsn, + "a checkpoint must not consume a replicated-log LSN"); + + /* the next real entry takes the very next LSN -- no hole */ + persist_wal_async(p, WAL_JOB_UPDATE, "z", 1); + TEST_ASSERT_EQUAL_UINT64(lsn_before + 1, p->current_lsn); cleanup_ckpt_persist(p); - printf("test_checkpoint_writes_marker_to_wal: PASS\n"); + printf("test_checkpoint_writes_no_marker_to_wal: PASS\n"); } /* ------------------------------------------------------------------ */ @@ -247,7 +256,7 @@ void test_persist_checkpoint(void) RUN_TEST(test_checkpoint_creates_snapshot_file); RUN_TEST(test_checkpoint_sets_checkpoint_lsn); RUN_TEST(test_checkpoint_lsn_tracks_last_applied_not_current_lsn); - RUN_TEST(test_checkpoint_writes_marker_to_wal); + RUN_TEST(test_checkpoint_writes_no_marker_to_wal); RUN_TEST(test_checkpoint_busy_guard); RUN_TEST(test_checkpoint_rotation_keeps_snap_keep_n); } diff --git a/src/include/persist.h b/src/include/persist.h index d9c6b87a..2fbc75c7 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -1092,6 +1092,12 @@ extern void (*persist_checkpoint_test_hook)(persist_t*); /* Checkpoint */ int persist_checkpoint(persist_t* p); +#ifdef XCE_TESTING +/* bug-1870: test-only -- write a pre-fix WAL_CHECKPOINT marker (consumes an + * LSN) to reproduce a WAL written by an older binary. */ +int persist_test_write_legacy_checkpoint_marker(persist_t* p); +#endif + /* * persist_set_checkpoint_offload_for_test / persist_checkpoint_offload_enabled: * Feature-flag accessors (xce-audit-invariants controller pass, dimension 1