lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(repl): harden standby slot allocation against WAL-retention guard clobber (bug-1474)

Browse source 
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).
This commit is contained in:
Rongsong Shen 2026-07-26 04:11:27 +00:00
parent 7547a8f14b
commit c0b0c7d1de
2 changed files with 189 additions and 2 deletions
Download patch file Download diff file Expand all files Collapse all files

60
src/controller/repl.c
View file

@ -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;

131
src/controller/testsuite/test_repl_unit.c
View file

@ -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);
}