wdgzyjx/myxce
1
0
Fork 0
forked from lei.chen/myxce
Code Pull requests Activity

fix(ha): close permanent standby catch-up wedge after a WAL_CHECKPOINT boundary (bug-1475)

Browse source 
A restarted standby could get permanently stuck unable to catch up,
with the primary re-streaming the identical doomed AppendEntries range
every heartbeat with zero progress -- discovered via a live 3-node
config-upgrade-snapshot soak. Liveness-only; no safety invariant
(Election Safety, Leader Completeness, State Machine Safety, Commit
Durability) was ever violated.

Three-layer root cause, all now closed:

1. ha_stream_wal_to_standby (config_repl.c) used to skip advancing its
   prev_streamed_lsn/prev_streamed_term tracker across WAL_CHECKPOINT
   entries (a local-only, never-replicated marker that still consumes
   a real LSN slot). Now advances across them too, while still never
   shipping a catchup entry for the checkpoint itself.

2. ha_apply_term_entry_via_library (ha.c) now JIT-resyncs
   ha->raft.log_len from persist_durable_floor() before consuming an
   AE, to correct for persist_write_checkpoint_marker advancing
   current_lsn without going through raft_log_append/record_entry_term
   (pre-existing gap, bug-1466, not itself fixed here).

3. The actual root cause, found via real instrumentation after layers
   1-2 alone still left a regression test red: record_entry_term's
   array-growth realloc zero-filled new slots, silently reclassifying
   a checkpoint's deliberately-never-recorded gap slot from "absent"
   to "present, term=0/WAL_LEGACY_TERM" the instant any later LSN got
   recorded -- a time-dependent read where a permanent fact was
   assumed. Fixed with a TERM_INDEX_UNSET (UINT64_MAX) sentinel that
   growth now fills gap slots with instead of zero, so entry_term_locked
   can distinguish "never explicitly written" from "explicitly
   recorded as legacy term 0" regardless of later array growth.

The same determinism fix exposed a second, independent instance of the
identical wedge via the snapshot-install catch-up path instead of
WAL-streaming catch-up: persist_reconcile_checkpoint_term recorded a
wire checkpoint_term of 0 as a raw, explicit write (bypassing the
growth-fill sentinel entirely), so a snapshot-installed standby read
the checkpoint LSN back as WAL_LEGACY_TERM(1) forever while the leader
now correctly read its own checkpoint LSN as absent(0) -- the same
permanent logOk mismatch, a different code path. Fixed by substituting
TERM_INDEX_UNSET for a wire value of exactly 0 before recording; a
wire 0 is unambiguous (persist_entry_term's own contract never returns
raw 0 for a present entry, so 0 can only mean genuine absence).

Independently reviewed across three rounds: sentinel mechanism
verified via real RED->GREEN revert-and-rebuild; a documentation
claim that config_repl.c/ha.c were "comment-only" corrected; the
snapshot-path gap independently traced, confirmed real via its own
RED->GREEN cycle, and fixed; two stale doc comments (persist.h's
public contract, ha.c's call-site rationale) still describing the
disproven pre-fix legacy-0 convention corrected. Suite: 1028 -> 1035
tests, 0 failures.
This commit is contained in:
Rongsong Shen 2026-07-28 04:48:53 +00:00
parent 7d31fce27a
commit ce8df99037
5 changed files with 650 additions and 45 deletions
Download patch file Download diff file Expand all files Collapse all files

68
src/controller/ha.c
View file

@ -2996,6 +2996,64 @@ 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
* 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
* call path — never touching persist.c or adding any new cross-thread
* mutation of ha->raft (ha_apply_wal_catchup_entry / this function already
* own that field on this thread, same as every other s->log_len write in
* the AE-receive path below).
*
* Root cause: persist_write_checkpoint_marker (a LOCAL-only, never-
* replicated WAL_CHECKPOINT entry written by ANY role — primary or
* standby — whenever a committed WAL_CONFIG_MARKER triggers an immediate
* checkpoint) advances persist_t's current_lsn directly, WITHOUT going
* through raft_log_append — so s->log_len silently falls one behind the
* true physical WAL tip for every checkpoint written since the last
* resync (documented gap, bug-1466). Once behind, the very next AE for
* the entry immediately following the checkpoint carries a prev_log_index
* equal to the checkpoint's own LSN (config_repl.c's streamer correctly
* advances its tracker across the skipped checkpoint) — but a stale
* s->log_len makes logOk's `prev_log_index <= s->log_len` bound fail
* forever, permanently wedging this standby's catch-up (every subsequent
* heartbeat re-streams the identical doomed range with zero progress).
*
* persist_durable_floor is a lock-free atomic read of durable_lsn (the
* flusher's own fsync-confirmed watermark, which DOES count a checkpoint's
* consumed LSN slot, same as at boot) — safe to call from any thread. The
* write below (raising ha->raft.log_len) happens on the same thread that
* already owns every other s->log_len mutation in this call chain
* (raft_on_append_entries_request), so no new lock is introduced.
* Monotonic: only ever raises log_len, mirroring
* ha_sync_raft_log_from_recovery exactly. Deliberately does NOT touch the
* term index (on_get_entry_term(checkpoint_lsn) stays 0/absent on every
* member, by design — see persist_write_checkpoint_marker's own comment)
* — recording a synthetic term here would risk a cross-member mismatch
* (each member's checkpoint stamps ITS OWN current term at write time,
* which need not agree with a peer's), whereas leaving it absent keeps
* the logOk equality check (0==0) trivially and SAFELY consistent on
* every member.
*
* bug-1475 follow-up: the "stays absent on every member" claim above only
* holds because persist.c's term index now uses a TERM_INDEX_UNSET
* sentinel to keep a never-explicitly-recorded LSN (like a checkpoint's)
* reading back absent PERMANENTLY. Before that fix, persist.c's array-
* growth zero-fill silently reclassified the checkpoint's gap slot as
* present-with-WAL_LEGACY_TERM the instant any LATER lsn was recorded —
* which happens immediately on the sender (it streams after its own
* later entries exist) but not yet on a receiver that hasn't appended
* past its own checkpoint, so the two sides read DIFFERENT terms for the
* SAME checkpoint LSN and logOk permanently rejected. See
* TERM_INDEX_UNSET's comment (persist.c) and
* test_bug1475_catchup_survives_midstream_checkpoint (test_config_repl.c)
* for the full mechanism and live-instrumentation evidence. */
if (p) {
uint64_t recovered_tip = persist_durable_floor(p);
if (recovered_tip > ha->raft.log_len)
ha->raft.log_len = recovered_tip;
}
/* Wrap the single entry in an AE container whose TOP-LEVEL fields carry the
* prev tuple + term that ha_consume_append_entries reads, with entries[0]
* pointing at the entry itself (the same shape repl_send_append_entries /
@ -3488,10 +3546,12 @@ int ha_handle_snapshot_chunk(ha_state_t *ha, persist_t *p,
* rather than silently surviving the install. Applies identically to
* both a legacy (position-only) and an entity-bearing checkpoint — the
* term reconcile is orthogonal to entity content. chunk->checkpoint_term
* == 0 means a legacy sender never stamped the wire field; persist's
* existing legacy-0 -> WAL_LEGACY_TERM convention (applied uniformly at
* read-back by persist_entry_term) already covers that case, so no
* special-casing is needed here. */
* == 0 is unambiguous and means the checkpoint LSN is genuinely absent
* from the sender's own term index, never "legacy term 0, present"
* (bug-1475 Finding 2 — see persist_reconcile_checkpoint_term's own doc
* comment, persist.h). persist_reconcile_checkpoint_term itself
* special-cases a wire 0 to TERM_INDEX_UNSET before recording, so the
* receiver reads this LSN back as absent too, matching the sender. */
persist_reconcile_checkpoint_term(p, chunk->snapshot_lsn,
chunk->checkpoint_term);