From c0b0c7d1deb52687f7d39ac8293af97ce11c76fd Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 26 Jul 2026 04:11:27 +0000 Subject: [PATCH] fix(repl): harden standby slot allocation against WAL-retention guard clobber (bug-1474) repl_add_standby()'s "find first empty slot" scan treated any ep==NULL slot as available, with no awareness that a disconnected slot can still hold an unexpired bug-1147 WAL-retention guard (last_disconnected_lsn, set by repl_remove_standby, meant to be cleared only by repl_expire_disconnect_slots once snap_interval_s genuinely elapses). The scan could pick a guard-holding slot over a genuinely virgin one purely by lower index, then unconditionally memset it -- destroying the guard before trim_wal_segments/repl_trim_lsn could ever honor it. Confirmed via 2/2 reproducible live 3-node soak runs (config-upgrade- snapshot task 8.9's new --config-upgrade-wal-trim mode): a killed standby's still-needed WAL segment got trimmed out from under it. Now a two-pass scan: prefer a genuinely virgin slot; only fall back to reusing a guard-holding slot (oldest last_disconnect_time first, to discard the least remaining protection) when no virgin slot exists -- logged via WARN() so the discarded guard is operator-visible rather than silent. Defense-in-depth at the slot-allocation layer, independent of whatever triggers a given repl_add_standby call (a separate, not-fully-root-caused ha.c reconnect-sweep defect is the specific trigger seen in the soak, out of this fix's scope). Independently reviewed across two rounds: round 1 flagged the exhausted-fallback path silently discarding a guard with zero logging; remediated with a WARN() matching this file's existing repl_broadcast convention, empirically confirmed to fire only on the fallback path with correct content. Suite: 1028 -> 1030 tests, 0 failures (net of this fix; final combined total with bug-1472 landing concurrently: 1033 tests, 0 failures). --- src/controller/repl.c | 60 +++++++++- src/controller/testsuite/test_repl_unit.c | 131 ++++++++++++++++++++++ 2 files changed, 189 insertions(+), 2 deletions(-) diff --git a/src/controller/repl.c b/src/controller/repl.c index c7e8dd6..4dc9b54 100644 --- a/src/controller/repl.c +++ b/src/controller/repl.c @@ -93,15 +93,60 @@ int repl_add_standby(replicator_t *r, repl_ep_t ep, const char *host, uint32_t p return -ENOSPC; } - /* Find first empty slot */ + /* Slot allocation (bug-1474 hardening): prefer a genuinely virgin slot + * over one still holding an unexpired bug-1147 disconnect guard + * (last_disconnected_lsn != 0) for a DIFFERENT peer. repl_add_standby + * cannot know whether this call is a legitimate new connection or a + * duplicate/late-joiner reconnect for an already-connected peer (that + * classification defect lives in ha.c, out of scope here) — but either + * way, clobbering an unexpired guard is never correct: it silently + * defeats repl_trim_lsn's WAL retention floor for whichever standby + * the guard was protecting (see .wolf/buglog.json bug-1474). + * + * Pass 1: any slot with ep==NULL && last_disconnected_lsn==0 (virgin — + * never used, or its guard already expired via + * repl_expire_disconnect_slots, which fully zeroes the slot). Always + * safe to take; first such slot by index. + * + * Pass 2 (fallback, only when Pass 1 finds nothing): among slots with + * ep==NULL && last_disconnected_lsn!=0 (guard-holding), reuse the one + * with the OLDEST last_disconnect_time. Deliberate policy: that guard + * is closest to its own natural expiry (repl_expire_disconnect_slots' + * snap_interval_s window), so reusing it discards the LEAST remaining + * WAL-retention protection of any guard-holding slot. With + * MAX_STANDBYS==3 it is possible — under a chaotic multi-disconnect + * scenario with no virgin slot free — for every non-primary slot to be + * guard-holding simultaneously; refusing this connect (-ENOSPC) would + * trade a bounded, already-degraded WAL-retention risk for an outright + * connection failure, which is worse for cluster availability. This + * fallback does NOT touch the -ENOSPC path above, which remains + * reserved for the genuinely-impossible case of more distinct live + * peers than slots exist. */ int idx = -1; for (int i = 0; i < MAX_STANDBYS; i++) { - if (r->standbys[i].ep == NULL) { + if (r->standbys[i].ep == NULL && r->standbys[i].last_disconnected_lsn == 0) { idx = i; break; } } + /* Pass 1 found nothing -- about to fall back to Pass 2 (guard-holding + * slot reuse) below. Tracked so the WARN below fires only for the + * genuine fallback case, not the common virgin-slot path. */ + int used_fallback_slot = (idx < 0); + + if (idx < 0) { + for (int i = 0; i < MAX_STANDBYS; i++) { + if (r->standbys[i].ep != NULL) { + continue; + } + if (idx < 0 || + r->standbys[i].last_disconnect_time < r->standbys[idx].last_disconnect_time) { + idx = i; + } + } + } + if (idx < 0) { /* Should not happen given nstandbys guard above, but be safe */ mylock_unlock(&r->lock); @@ -109,6 +154,17 @@ int repl_add_standby(replicator_t *r, repl_ep_t ep, const char *host, uint32_t p } standby_conn_t *s = &r->standbys[idx]; + if (used_fallback_slot) { + /* bug-1474 review finding: this silently destroys the slot's + * WAL-retention guard below (via memset) -- log it loudly, mirroring + * repl_broadcast's existing "degraded but continuing" WARN + * convention (e.g. "standby %s:%u evicted", line ~314 above), so the + * discarded protection is operator-visible instead of silent. */ + WARN("repl_add_standby: reusing guard-holding slot %d (host=%s:%u," + " last_disconnected_lsn=%lu) -- no virgin slot available," + " WAL-retention protection discarded", + idx, s->host, s->port, (unsigned long)s->last_disconnected_lsn); + } memset(s, 0, sizeof(*s)); s->ep = ep; s->port = port; diff --git a/src/controller/testsuite/test_repl_unit.c b/src/controller/testsuite/test_repl_unit.c index 0daff18..7659777 100644 --- a/src/controller/testsuite/test_repl_unit.c +++ b/src/controller/testsuite/test_repl_unit.c @@ -583,6 +583,134 @@ static void test_repl_registration_lifecycle(void) { printf("test_repl_registration_lifecycle: PASS\n"); } +/* ============================================================ + * bug-1474: repl_add_standby slot allocation must never clobber an + * unexpired bug-1147 disconnect guard (last_disconnected_lsn) belonging to + * a DIFFERENT, still-disconnected peer, when a genuinely virgin + * (never-used) slot is available. See .wolf/buglog.json bug-1474 for the + * full forensic trace: a spurious/duplicate reconnect for an unrelated, + * already-connected peer picked the lower-indexed, guard-holding slot over + * a higher-indexed virgin slot and memset it to zero, silently destroying + * the guard repl_trim_lsn relies on to protect WAL segments a genuinely + * disconnected-but-reconnecting standby still needs. + * ============================================================ */ + +/* RED (pre-fix): repl_add_standby's old "first empty slot by lowest index" + * scan picks idx_a (guard-holding, index 0) over the virgin slot (index 2) + * purely because of index order, destroying A's guard. Req: the guard must + * survive and the new peer must land in the virgin slot instead. */ +static void test_repl_add_standby_prefers_virgin_slot_over_guard(void) { + replicator_t r; + int ep_a = 0, ep_b = 1, ep_c = 2; + repl_init(&r); + + int idx_a = repl_add_standby(&r, (void *)&ep_a, "standby-a", 7001); + int idx_b = repl_add_standby(&r, (void *)&ep_b, "standby-b", 7002); + TEST_ASSERT_TRUE(idx_a >= 0); + TEST_ASSERT_TRUE(idx_b >= 0); + TEST_ASSERT_NOT_EQUAL(idx_a, idx_b); + + /* A acks up to LSN 42, then disconnects: leaves a bug-1147 guard at + * idx_a with last_disconnected_lsn == 42. The third slot (whichever + * index neither idx_a nor idx_b used) remains genuinely virgin. */ + repl_on_ack(&r, (void *)&ep_a, 42); + repl_remove_standby(&r, (void *)&ep_a); + TEST_ASSERT_NULL(r.standbys[idx_a].ep); + TEST_ASSERT_EQUAL_UINT64(42, r.standbys[idx_a].last_disconnected_lsn); + + /* A third, genuinely NEW peer (unrelated to A) connects while idx_a's + * guard is still unexpired and a virgin slot is also available. */ + int idx_c = repl_add_standby(&r, (void *)&ep_c, "standby-c", 7003); + TEST_ASSERT_TRUE(idx_c >= 0); + + /* The new peer must land in the virgin slot, NOT the guard-holding one, + * even though idx_a may be the lower index. */ + TEST_ASSERT_NOT_EQUAL_MESSAGE(idx_a, idx_c, + "new peer must not be allocated the guard-holding slot when a" + " virgin slot exists (bug-1474)"); + + /* Load-bearing assertion: A's guard must survive completely untouched. + * Before the fix, repl_add_standby's unconditional memset(s, 0, ...) + * on the chosen slot would have wiped last_disconnected_lsn to 0 here. */ + TEST_ASSERT_NULL_MESSAGE(r.standbys[idx_a].ep, + "guard-holding slot must remain disconnected (bug-1474)"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(42, r.standbys[idx_a].last_disconnected_lsn, + "bug-1147 guard LSN must survive an unrelated peer's" + " repl_add_standby call (bug-1474)"); + + repl_fini(&r); + printf("test_repl_add_standby_prefers_virgin_slot_over_guard: PASS\n"); +} + +/* RED (pre-fix, and proves the DELIBERATE fallback policy): with + * MAX_STANDBYS==3, exhaust all three slots, then disconnect two of them + * (both now guard-holding, one older-disconnected than the other) so NO + * virgin slot remains. A fourth genuinely new peer must still be admitted + * (never -ENOSPC for this case -- refusing a legitimate reconnect would be + * worse for cluster availability than an early guard loss), and the + * fallback must deterministically reuse the OLDER-disconnected guard slot + * (closest to its own natural repl_expire_disconnect_slots expiry, so + * reusing it discards the LEAST remaining WAL-retention protection) -- + * not merely the lowest-indexed guard slot. */ +static void test_repl_add_standby_exhausted_guards_prefers_oldest_disconnect(void) { + replicator_t r; + int ep_a = 0, ep_b = 1, ep_c = 2, ep_d = 3; + repl_init(&r); + + int idx_a = repl_add_standby(&r, (void *)&ep_a, "standby-a", 7001); + int idx_b = repl_add_standby(&r, (void *)&ep_b, "standby-b", 7002); + int idx_c = repl_add_standby(&r, (void *)&ep_c, "standby-c", 7003); + TEST_ASSERT_TRUE(idx_a >= 0); + TEST_ASSERT_TRUE(idx_b >= 0); + TEST_ASSERT_TRUE(idx_c >= 0); + /* All MAX_STANDBYS(3) slots now occupied. */ + + /* B disconnects first; force its disconnect timestamp strictly OLDER + * than A's (below) so the two guards are unambiguously ordered by + * time, independent of their slot index (idx_a < idx_b by add order -- + * deliberately the OPPOSITE of the time order, so a naive + * "lowest-index guard slot" fallback would pick the wrong one). */ + repl_on_ack(&r, (void *)&ep_b, 20); + repl_remove_standby(&r, (void *)&ep_b); + r.standbys[idx_b].last_disconnect_time -= 100; + + /* A disconnects second, with its real (newer) "now" timestamp. */ + repl_on_ack(&r, (void *)&ep_a, 10); + repl_remove_standby(&r, (void *)&ep_a); + + TEST_ASSERT_TRUE_MESSAGE( + r.standbys[idx_b].last_disconnect_time < r.standbys[idx_a].last_disconnect_time, + "test setup invariant: B must be the OLDER disconnect"); + + /* C is still connected; nstandbys == 1 (< MAX_STANDBYS), so the + * "too many distinct live peers" -ENOSPC guard does not trip -- but + * BOTH empty slots (idx_a, idx_b) are guard-holding: no virgin slot + * exists anywhere in the registry. */ + TEST_ASSERT_EQUAL_INT(1, r.nstandbys); + + /* A fourth, genuinely new peer connects into the exhausted state. */ + int idx_d = repl_add_standby(&r, (void *)&ep_d, "standby-d", 7004); + TEST_ASSERT_TRUE_MESSAGE(idx_d >= 0, + "exhausted-guard fallback must still admit a legitimate new peer," + " never -ENOSPC (bug-1474 policy)"); + + /* Deliberate policy: reuse the OLDER-disconnected guard slot (idx_b), + * not idx_a (which is both newer-disconnected AND lower-indexed -- + * ruling out an accidental "first guard by index" fallthrough). */ + TEST_ASSERT_EQUAL_INT_MESSAGE(idx_b, idx_d, + "exhausted fallback must reuse the OLDEST-disconnected guard slot," + " not merely the lowest-indexed one (bug-1474 policy)"); + + /* idx_a's guard (the one NOT chosen) must remain completely untouched. */ + TEST_ASSERT_NULL_MESSAGE(r.standbys[idx_a].ep, + "non-selected guard slot must remain disconnected"); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(10, r.standbys[idx_a].last_disconnected_lsn, + "non-selected guard slot's LSN must survive untouched"); + + repl_fini(&r); + printf("test_repl_add_standby_exhausted_guards_prefers_oldest_disconnect: PASS\n"); +} + void test_repl_unit(void) { UnitySetTestFile(__FILE__); RUN_TEST(test_repl_init_zeroes_nstandbys); @@ -608,4 +736,7 @@ void test_repl_unit(void) { RUN_TEST(test_wal_nack_does_not_update_acked_lsn); /* Task 5.3: registration lifecycle — Req 1.1, 1.2, 1.3 */ RUN_TEST(test_repl_registration_lifecycle); + /* bug-1474: slot allocation must not clobber an unexpired guard */ + RUN_TEST(test_repl_add_standby_prefers_virgin_slot_over_guard); + RUN_TEST(test_repl_add_standby_exhausted_guards_prefers_oldest_disconnect); }