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

fix(raft-drive-async): resolve snapshot chunk-drop (bug-1386) and chunk misordering (bug-1389)

Browse source 
ha_send_snapshot_to_standby ignored the network send's return value in its
chunk loop, silently truncating a transfer on a mid-stream error while still
logging "complete" (bug-1386). Fixing the silent drop exposed a second,
independent bug in the same pipeline: ha_handle_snapshot_chunk appended
received chunks in arrival order instead of at their wire-carried
chunk_offset, so out-of-order delivery across the vtask pool corrupted the
reassembled file even though every chunk's bytes were individually correct
(bug-1389). Fixed by checking every fallible call in the send loop and
switching the receiver to pwrite-at-offset with a count-based completion
tracker.

Adds snapverify/snapdiff xce_test commands (offline checkpoint-file
structural validator and byte-level comparator) used to diagnose bug-1389 and
kept as standing diagnostic tools for future snapshot-transfer triage.

g_drive_async_configpush_offload flipped back ON after both fixes, validated
via 6-round chaotic + 3/3 force-snapshot soak (raft-drive-async task 3.3).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SDWVNQLKxqTVHh42cH7rV3
This commit is contained in:
Rongsong Shen 2026-07-03 02:40:12 +00:00
parent 02e46f6ba1
commit 5cb36794d2
14 changed files with 1553 additions and 43 deletions
Download patch file Download diff file Expand all files Collapse all files

148
src/controller/ha.c
View file

@ -2588,33 +2588,78 @@ int ha_handle_snapshot_chunk(ha_state_t *ha, persist_t *p,
snprintf(part_path, sizeof(part_path), "%s/snap.part", snap_dir);
/* Req 4.4: a transfer always restarts from chunk 0 — discard any stale
* partial file from an interrupted previous transfer. */
if (chunk->chunk_index == 0 && unlink(part_path) != 0 && errno != ENOENT)
WARN("ha_handle_snapshot_chunk: unlink stale snap.part failed: %s",
strerror(errno));
* partial file from an interrupted previous transfer, and reset the
* completion bookkeeping below for the fresh attempt. */
pthread_mutex_lock(&ha->lock);
if (chunk->chunk_index == 0) {
if (unlink(part_path) != 0 && errno != ENOENT)
WARN("ha_handle_snapshot_chunk: unlink stale snap.part failed: %s",
strerror(errno));
ha->snapshot_recv_count = 0;
ha->snapshot_recv_total = 0;
}
pthread_mutex_unlock(&ha->lock);
/* Append chunk payload to snap.part */
/* bug-1389 triage: crc32 of the exact bytes this chunk arrived with,
* taken right after decode and before any local write — correlate
* against ha_send_snapshot_to_standby's send-side crc32 log
* (config_repl.c) by (snapshot_lsn, chunk_index) to localize whether a
* mismatch is introduced before send, in the wire transport, or during
* receive-side assembly. */
if (payload_bytes && payload_len > 0)
TRACE("ha_handle_snapshot_chunk: chunk crc32 "
"checkpoint_lsn=%" PRIu64 " chunk_index=%u len=%zu crc32=%08x",
chunk->snapshot_lsn, chunk->chunk_index, payload_len,
(unsigned)crc32(0L, (const Bytef *)payload_bytes, (uInt)payload_len));
/* bug-1389: chunks of one transfer are dispatched across the vtask pool
* and are NOT guaranteed to arrive/execute in chunk_index order (a live
* soak observed e.g. chunk_index 2 processed AFTER 3 and 4) — a live
* crc32 correlation confirmed every chunk's CONTENT survives the wire
* intact, but the OLD append-in-arrival-order write put reordered
* chunks' bytes at the wrong file position. pwrite() at the chunk's own
* chunk_offset makes assembly order-independent and safe for concurrent
* writers (positioned writes to disjoint byte ranges of the same fd
* don't race each other). */
if (payload_bytes && payload_len > 0) {
FILE *f = fopen(part_path, "ab");
if (!f) {
int fd = open(part_path, O_CREAT | O_WRONLY, 0600);
if (fd < 0) {
int err = errno;
ERROR("ha_handle_snapshot_chunk: cannot open snap.part: %s",
strerror(err));
return -err;
}
size_t written = fwrite(payload_bytes, 1, payload_len, f);
fclose(f);
if (written != payload_len) {
ssize_t written = pwrite(fd, payload_bytes, payload_len,
(off_t)chunk->chunk_offset);
close(fd);
if (written != (ssize_t)payload_len) {
ERROR("ha_handle_snapshot_chunk: short write to snap.part "
"(%zu of %zu bytes)", written, payload_len);
"(%zd of %zu bytes) at offset %" PRIu64,
written, payload_len, chunk->chunk_offset);
return -EIO;
}
}
if (!chunk->is_last)
/* bug-1389: finalize once every chunk index 0..N-1 has actually been
* written, not merely once THIS call happens to carry is_last — under
* out-of-order arrival, is_last can be processed before an earlier
* chunk. snapshot_recv_total is claimed (reset to 0) by whichever
* thread wins the race so a late straggler chunk can never trigger a
* second finalize. */
pthread_mutex_lock(&ha->lock);
ha->snapshot_recv_count++;
if (chunk->is_last)
ha->snapshot_recv_total = chunk->chunk_index + 1;
int should_finalize = (ha->snapshot_recv_total != 0 &&
ha->snapshot_recv_count >= ha->snapshot_recv_total);
if (should_finalize)
ha->snapshot_recv_total = 0;
pthread_mutex_unlock(&ha->lock);
if (!should_finalize)
return 0;
/* isLast=true: verify size, rename, load, set applied_lsn */
/* All chunks accounted for: verify size, rename, load, set applied_lsn */
/* Verify total file size equals chunk->total_size */
struct stat st;
@ -4333,33 +4378,68 @@ int ha_snapshot_offload_enabled(void)
* DEFAULT OFF (0): the requester-less config push runs INLINE on the caller
* (io_worker/drive) thread — byte-for-byte the legacy behavior, i.e.
* push_config_transport_only's fan-out followed by its blocking
* wait_all_with_timeout(CONF_PUSH_TIMEOUT_MS). This task lands DORMANT,
* exactly like the Task 3.1/3.2 flags, because the offload borrows standby
* endpoints it does not own (bug-1197 family — push_config_transport_only
* reads c->ha.standby_endpoints): moving the push to a lane thread while the
* io_worker may RDHUP-free those endpoints is a cross-thread UAF. Today (flag
* OFF) the push and the endpoint teardown are serialized on the single
* io_worker, so they cannot overlap.
* wait_all_with_timeout(CONF_PUSH_TIMEOUT_MS).
*
* STILL DEFAULT OFF, unlike the snapshot (3.2) and peer-control (3.1) flags,
* which flipped ON once their single-endpoint hazard closed via endpoint_ref/
* endpoint_unref. push_config_transport_only's hazard is DIFFERENT and NOT
* yet closed by that fix: it grabs raw endpoint_t* pointers for ALL standbys
* under c->ha.lock, releases the lock, then spawns one pthread per standby
* that reads/writes those endpoints unprotected for up to CONF_PUSH_TIMEOUT_MS
* — a race that exists TODAY regardless of this flag (the pthread-per-standby
* fan-out is not the io_worker, so "runs inline on io_worker" does not
* actually serialize it against a concurrent RDHUP teardown). Logged as
* bug-1381 (open). Needs its own fix (ref every standby ep for the fan-out's
* duration, or route through the per-ep block primitive) before flipping ON
* — tracked separately, not fixed by this task. Until then this offload is
* exercised only by unit tests with a stable (or absent) endpoint set.
* Landed DORMANT alongside the Task 3.1/3.2 flags because the offload borrows
* standby endpoints it does not own (bug-1197 family — push_config_transport_only
* reads c->ha.standby_endpoints): moving the push to a lane thread while the
* io_worker may RDHUP-free those endpoints would be a cross-thread UAF.
* push_config_transport_only's hazard was DISTINCT from the snapshot (3.2)
* lane's borrowed-ep hazard — it grabbed raw endpoint_t* pointers for ALL
* standbys under c->ha.lock, released the lock, then spawned one pthread per
* standby that read/wrote those endpoints unprotected for up to
* CONF_PUSH_TIMEOUT_MS, a race that existed regardless of this flag's value
* (the pthread-per-standby fan-out is not the io_worker, so "runs inline on
* io_worker" never actually serialized it against a concurrent RDHUP
* teardown). Logged as bug-1381, **fixed 2026-07-02**: push_config_transport_only
* (config_repl.c) now takes its own endpoint_ref() on each standby ep at the
* same point the pointer is snapshotted under c->ha.lock, with a matching
* endpoint_unref() from push_thread (or from this function's own cleanup if a
* thread never started) — mirroring this same file's snapshot-lane fix.
*
* **Flip attempted 2026-07-02, REVERTED to OFF**: bug-1381 alone did not clear
* the bar. Flag-ON force-snapshot soak went 0/3 pass ("no snapshot install" /
* "did not reconverge to 3-of-3" on the freshly-rebuilt xce.build binary — NOT
* a stale-binary artifact); a flag-OFF control on the identical build went
* 1/2 pass with the SAME failure signature, so the underlying defect is
* PRE-EXISTING and orthogonal to this flag (root cause: `ha_send_snapshot_to_standby`,
* config_repl.c, never checks `xce_snapshot_chunk_req_raw`'s return in its
* chunk loop, so a chunk that fails to enqueue — e.g. ECONNRESET during the
* empty-rejoin reconnect window — is silently dropped and the sender still
* logs "transfer complete" and returns 0; logged as a new bug, see buglog).
* Even so, flag-ON's 0/3 vs flag-OFF's 1/2 is a small enough sample that a
* TIMING interaction (configpush lane traffic narrowing the window the
* snapshot lane has to complete before reconnect) can't be ruled out either.
* Flag stayed OFF until the chunk-loss bug (bug-1386) AND a second,
* independently-discovered bug (bug-1389) were fixed and re-validated.
*
* **bug-1386 FIXED 2026-07-02** (config_repl.c): the send-side chunk loop now
* checks xce_snapshot_chunk_req_raw's return and stops+propagates on failure
* instead of silently continuing and reporting "complete".
*
* **bug-1389 FIXED 2026-07-02** (this file + ha.h): re-validating bug-1386's
* fix surfaced a SECOND, independent defect — force-snapshot chunks are
* dispatched across the vtask pool with no ordering guarantee (a live soak
* showed arrival order 0,1,3,4,2,5,... across cpu#0-3 workers), but
* ha_handle_snapshot_chunk appended every chunk's bytes to snap.part in
* ARRIVAL order instead of at its own chunk_offset — reordered chunks landed
* at the wrong file position (same total size, wrong content). A live crc32
* correlation between send and receive proved every chunk's CONTENT survived
* the wire intact, ruling out capnp/transport entirely. Fixed via
* pwrite()-at-chunk_offset (order-independent) plus a
* snapshot_recv_count/snapshot_recv_total completion tracker (guarded by
* ha->lock) so finalize fires only once every chunk has actually arrived,
* not merely because the current call happens to carry is_last.
*
* **Flag re-flipped to ON 2026-07-02** after both bugs closed: full controller
* suite green, live-validated 3/3 --force-snapshot PASS (genuine snapshot
* installs confirmed) + 6-round --chaotic PASS on the flag-ON binary.
*
* ONLY the requester-less call sites (post-election promotion, heartbeat-driven
* HA_REPL_PUSH_CONFIG) route through this flag; the admin-upgrade barrier path
* (push_config_to_standbys) is unaffected. Compiled in ALL builds; the setter
* is the test seam to flip RED<->GREEN. */
static _Atomic int g_drive_async_configpush_offload = 0;
static _Atomic int g_drive_async_configpush_offload = 1;
void ha_set_configpush_offload_for_test(int on)
{