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

fix(raft-drive-async): close snapshot-lane borrowed-endpoint hazard + catch-up checkpoint-marker bug

Browse source 
Both fixes are real and independently verified; the drive-async flags
(g_drive_async_snapshot_offload, g_drive_async_peerctrl_dispatch_enabled)
stay at their production-default OFF -- see the tasks.md session note for
why the flip itself doesn't land in this commit.

1. Snapshot-lane borrowed-endpoint hazard (task 3.2, bug-1197 family):
   ha_snapshot_send_dispatch now takes endpoint_ref(ep) before submitting to
   the slow lane, released on every non-lane terminal path and by
   snapshot_lane_fn on completion. Reuses the general bug-1298 endpoint
   refcount system (standby-ep teardown already routes through
   endpoint_unref via cluster.c's endpoint_killer) -- a concurrent RDHUP
   teardown can now only drop the owner's reference while a transfer is in
   flight, never free the ep out from under it. New test:
   test_snapshot_offload_holds_endpoint_ref_across_transfer (RED->GREEN).

2. Catch-up streamer WAL_CHECKPOINT bug (found while verifying #1 via a live
   soak, pre-existing, unrelated to the drive-async flags): ha_stream_wal_
   to_standby had no filter for WAL_CHECKPOINT markers -- local-only
   bookkeeping that still consumes a real LSN slot via persist_write_
   checkpoint_marker's own current_lsn++. It shipped one to a catching-up
   standby, which correctly refused it (persist_wal_write_replicated
   rejects type==WAL_CHECKPOINT) and got stuck retrying the same LSN
   forever. Fixed by skipping WAL_CHECKPOINT entirely in the streamer loop,
   mirroring the existing replay_wal/term-index precedent of treating
   checkpoint-marker LSNs as invisible gaps. New test:
   test_wal_stream_skips_checkpoint_marker (RED->GREEN).

Attempted flipping both drive-async flags ON after landing these fixes (the
"full fix, defer prod flip" decision's prerequisite -- 4.1+4.2 are green).
A 6-round --chaotic soak passed clean, but a --force-snapshot soak (a
genuinely empty node cold-start rejoin, the one scenario chaotic mode never
exercises) surfaced a THIRD, not-yet-root-caused issue: the freshly-wiped
node's own self-election appears to race cluster_init_phase2's vtask-pool
wiring, then the process hangs silently (no crash, no ASAN report) until
killed. Reverted both flags to OFF rather than ship an unproven state --
production behavior is unchanged by this commit. Full controller suite
green and both soaks (--force-snapshot, 6-round --chaotic) pass clean with
flags OFF, confirming no regression from the two logic fixes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014dZVSx3CdBjys9BB6hqpxP
This commit is contained in:
Rongsong Shen 2026-07-02 04:54:17 +00:00
parent 1439e2655e
commit f3b6e6ae3b
5 changed files with 315 additions and 64 deletions
Download patch file Download diff file Expand all files Collapse all files

163
src/controller/ha.c
View file

@ -2844,17 +2844,15 @@ static void ha_snapshot_do_send(cluster_t *c, endpoint_t *ep, int peer_idx)
* in-flight flag (success or failure) so the dedup never wedges and a later
* heartbeat can retry. Frees the ctx.
*
* ⚠ BORROWED-ENDPOINT LIFETIME HAZARD (bug-1197 family — why this path is
* gated OFF by default). `ep` here is a standby endpoint shared with the
* io_worker epoll loop, NOT one this lane owns (contrast reconnect_lane_fn,
* which owns the ep it creates). If the standby RDHUP-disconnects while this
* transfer runs, the io_worker can endpoint_fini/free `ep` concurrently → UAF.
* Today (flag OFF) the send runs inline ON the io_worker, so teardown and send
* are serialized on one thread and cannot overlap. Flipping
* g_drive_async_snapshot_offload ON is therefore only safe once Task 3.4
* establishes the endpoint-lifetime discipline for the whole pool/lane
* dispatch model (refcount or lane-serialized teardown). Until then this code
* lands DORMANT and is exercised only by unit tests with a stable endpoint. */
* BORROWED-ENDPOINT LIFETIME (bug-1197 family): `ep` here is a standby
* endpoint shared with the io_worker epoll loop, NOT one this lane owns
* (contrast reconnect_lane_fn, which owns the ep it creates). ha_snapshot_
* send_dispatch takes an endpoint_ref() on `ep` before submitting (mirrors
* every vtask-bound ep — vtask_enq +1 / run_task -1) so a concurrent standby
* teardown (RDHUP -> zarray_del -> endpoint_killer -> endpoint_unref, the
* general bug-1298 refcount system) can only drop the OWNER's reference
* while this lane transfer is in flight — it cannot free `ep` out from under
* the send. This function's endpoint_unref below is the matching release. */
static void snapshot_lane_fn(void *arg)
{
snapshot_send_ctx_t *sx = (snapshot_send_ctx_t *)arg;
@ -2873,6 +2871,7 @@ static void snapshot_lane_fn(void *arg)
memory_order_release);
atomic_store_explicit(&c->ha.snapshot_inflight[peer_idx], 0,
memory_order_release);
endpoint_unref(sx->ep);
free(sx);
}
@ -2921,6 +2920,13 @@ static void ha_snapshot_send_dispatch(ha_state_t *ha, cluster_t *c,
}
sx->c = c; sx->ep = ep; sx->peer_idx = peer_idx;
/* Borrowed-endpoint lifetime (bug-1197 family): take our own reference
* before the lane ever sees `ep`, mirroring vtask_enq's +1 for a
* pool-bound ep. This is the ONLY ref taken across the 3 submit-outcome
* branches below — release it on every path that does NOT hand off to
* snapshot_lane_fn (which owns the matching unref on completion). */
endpoint_ref(ep);
uint64_t key = (uint64_t)(peer_idx + 1); /* +1: key 0 disables dedup */
int srv = slowlane_submit(&c->slowlane, key, snapshot_lane_fn, sx);
if (srv == -EEXIST) {
@ -2929,6 +2935,7 @@ static void ha_snapshot_send_dispatch(ha_state_t *ha, cluster_t *c,
* lane has not yet removed. Our CAS saw 0 and set 1, but the submit is
* refused — release the flag we set so no clear is lost, and let the
* next tick retry. */
endpoint_unref(ep);
free(sx);
atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], 0,
memory_order_release);
@ -2942,6 +2949,7 @@ static void ha_snapshot_send_dispatch(ha_state_t *ha, cluster_t *c,
* and release the guard so the next tick retries; never self-terminate. */
WARN("HA repl: slowlane_submit snapshot peer[%d] failed: %d — retry "
"next cycle", peer_idx, srv);
endpoint_unref(ep);
free(sx);
atomic_store_explicit(&ha->snapshot_inflight_lsn[peer_idx], 0,
memory_order_release);
@ -4284,20 +4292,25 @@ int ha_reconnect_offload_enabled(void)
/* raft-drive-async task 3.2: SnapshotSend feature flag.
*
* DEFAULT OFF (0): the snapshot transfer runs INLINE on the caller (io_worker/
* drive) thread — byte-for-byte the legacy behavior. This task lands DORMANT,
* exactly like the Task 3.1 peer-control dispatch flag, because the offload
* captures a BORROWED standby endpoint (see snapshot_lane_fn's bug-1197 hazard
* note): moving the transfer to a lane thread while the io_worker may
* RDHUP-free that endpoint is a cross-thread UAF. Today (flag OFF) the send
* and the endpoint teardown are serialized on the single io_worker, so they
* cannot overlap.
* STILL DEFAULT OFF (0) — see below for why, even though the ORIGINAL
* borrowed-endpoint hazard this flag was gated on is now CLOSED. The
* bug-1197-family hazard (snapshot_lane_fn's `ep` is shared with the
* io_worker epoll loop, not lane-owned) is fixed: ha_snapshot_send_dispatch
* takes an endpoint_ref() before submit and snapshot_lane_fn releases it via
* endpoint_unref() on completion — the general bug-1298 refcount system
* (every heap-ep teardown site already routes through endpoint_unref, see
* cluster.c's endpoint_killer) means a concurrent RDHUP teardown can only
* drop the owner's reference while the transfer is in flight, never free the
* ep out from under it. Verified: dedicated unit test
* (test_snapshot_offload_holds_endpoint_ref_across_transfer), full controller
* suite green.
*
* Flipping ON is the job of Task 3.4 (receiver-side integration), which
* establishes the endpoint-lifetime discipline for the pool/lane dispatch
* model and proves the transfer no longer stalls other peers' heartbeats/votes
* with the pool near saturation. Until then, bug-1198 (the repl-ack body is
* still unbounded when run inline) remains open by design.
* STAYS OFF because this flag is coupled to g_drive_async_peerctrl_dispatch
* (bug-1198 requires both ON together) — see that flag's comment for the
* cold-start pool-wiring race discovered flipping BOTH ON together in a live
* force-snapshot soak (a freshly-wiped empty node's self-election can race
* cluster_init_phase2's vtask-pool wiring, then hang silently). Until that
* race is root-caused and fixed, this flag cannot flip ON either.
*
* Keyed per standby (Req 1.5) so at most one snapshot per peer is in flight;
* at most one persist_checkpoint in flight (existing atomic guard). Compiled
@ -4329,10 +4342,19 @@ int ha_snapshot_offload_enabled(void)
* OFF) the push and the endpoint teardown are serialized on the single
* io_worker, so they cannot overlap.
*
* Flipping ON is the job of Task 3.4, which establishes the endpoint-lifetime
* discipline for the whole pool/lane dispatch model and flips this together
* with the snapshot + peer-control dispatch flags. Until then this offload is
* exercised only by unit tests with a stable (or absent) endpoint set.
* 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). 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.
*
* ONLY the requester-less call sites (post-election promotion, heartbeat-driven
* HA_REPL_PUSH_CONFIG) route through this flag; the admin-upgrade barrier path
@ -4354,14 +4376,32 @@ int ha_configpush_offload_enabled(void)
/* raft-drive-async task 3.1: PeerCtrlDispatch feature flag.
*
* DEFAULT OFF (0): the registered peer-control handler runs its routing body
* INLINE on the iomgr io_worker thread — byte-for-byte the pre-task behavior,
* so THIS COMMIT DOES NOT CHANGE PRODUCTION BEHAVIOR. ON (1): the handler
* hands the decoded packet to the vtask pool and returns promptly. The flag
* stays OFF until the task 3.4 deadlock gate proves the dispatch path safe.
* STILL DEFAULT OFF (0): the registered peer-control handler runs its
* routing body INLINE on the iomgr io_worker thread — byte-for-byte the
* pre-task behavior, so production behavior is unchanged. ON (1): the
* handler hands the decoded packet to the vtask pool and returns promptly.
*
* The originally-scoped gate items are closed: the task 3.4 deadlock-freedom
* proof (LivenessAdmission, task 1.6), the bug-1197 endpoint-quiescence fix
* (3.4a), and bug-1198 (closed by the snapshot-offload flag's endpoint_ref/
* endpoint_unref fix, this session — see that flag's comment). A 6-round
* chaotic HA soak with BOTH flags ON passed clean (zero divergence, zero job
* loss, full controller suite green).
*
* BUT a live --force-snapshot soak (a genuinely EMPTY node cold-starting and
* rejoining, a scenario chaotic mode never exercises — it restarts nodes
* with WAL intact) with both flags ON HUNG: the freshly-wiped node's own
* self-election raced cluster_init_phase2's vtask-pool wiring (log shows
* peer_ctrl_dispatch_to_pool warning "vtask pool not wired — running inline"
* immediately followed by cluster_init_phase2 wiring the pool while
* raft_cb_become_follower's deferred-teardown vtasks were being enqueued),
* then the process went silent with no further log output until killed — no
* crash, no ASAN report, just a stall. NOT YET ROOT-CAUSED. Flag stays OFF
* until this cold-start race is understood and fixed; do not flip ON based
* on the chaotic soak alone — it does not cover this startup shape.
*
* Compiled in ALL builds (production reads it on every peer-control message);
* the setter is the test seam to flip RED<->GREEN. */
* the setter is the test seam to flip ON for testing. */
static _Atomic int g_drive_async_peerctrl_dispatch_enabled = 0;
void ha_set_peerctrl_dispatch_for_test(int on)
@ -5498,25 +5538,24 @@ static int peer_ctrl_vtask_entry(vtask_t *task)
* config-push/snapshot ops are offloaded to the SlowLane in tasks 3.2/3.3. So
* everything this handler dispatches is liveness work today.)
*
* ┌─ HIGH FLAG-ON HAZARD (raft-drive-async finding-2; bug-1198) ─────────────┐
* │ LIVENESS is correct ONLY once the snapshot send runs OFF the pool. The │
* ┌─ FLAG-ON HAZARD CLOSED (raft-drive-async finding-2; bug-1198) ───────────┐
* │ LIVENESS is correct only once the snapshot send runs OFF the pool. The │
* │ CTRL_PRIMARY repl-ack path can reach HA_REPL_SEND_SNAPSHOT → │
* │ persist_checkpoint (a BLOCKING fdatasync) in │
* │ repl_ack_decode_and_dispatch → ha_dispatch_repl_action_for_standby. │
* │ Running that blocking op while tagged VTASK_LIVENESS (never gated) breaks │
* │ the deadlock-freedom reservation — a burst of snapshot sends could occupy │
* │ every liveness worker and starve the acks that advance committed_lsn. │
* │ Running that blocking op while tagged VTASK_LIVENESS (never gated) would │
* │ break the deadlock-freedom reservation — a burst of snapshot sends could │
* │ occupy every liveness worker and starve the acks that advance │
* │ committed_lsn. │
* │ │
* │ Task 3.2 has LANDED the slow-lane offload (ha_snapshot_send_dispatch), │
* │ but it is DORMANT: g_drive_async_snapshot_offload defaults OFF (the send │
* │ still runs INLINE) because the offload borrows a standby endpoint whose │
* │ lifetime is not yet pool-safe (bug-1197). So bug-1198 is NOT yet closed │
* │ in the default build. Task 3.4 is the single gate that flips BOTH │
* │ g_drive_async_snapshot_offload AND g_drive_async_peerctrl_dispatch ON │
* │ together (offload bounds the body → LIVENESS correct) and proves the │
* │ deadlock-freedom + endpoint-lifetime properties under load. │
* │ DO NOT flip g_drive_async_peerctrl_dispatch_enabled ON until task 3.4's │
* │ gate is green WITH the snapshot offload also ON. │
* │ CLOSED (logic-wise): the borrowed-endpoint lifetime hazard (bug-1197) is │
* │ fixed by an endpoint_ref/endpoint_unref bracket around the lane dispatch │
* │ (see ha_snapshot_send_dispatch / snapshot_lane_fn), so WHENEVER both │
* │ flags are ON together the repl-ack path's snapshot send is ALWAYS │
* │ offloaded before peer_ctrl_classify ever tags it VTASK_LIVENESS on the │
* │ pool — bug-1198 cannot recur. Both flags STILL default OFF, though — see │
* │ g_drive_async_peerctrl_dispatch_enabled's comment for a SEPARATE, │
* │ not-yet-fixed cold-start pool-wiring race found flipping both ON. │
* └──────────────────────────────────────────────────────────────────────────┘
*/
static vtask_class_t peer_ctrl_classify(ha_state_t *ha, io_packet_t *pkt)
@ -5657,21 +5696,17 @@ void ha_vote_ep_teardown_for_test(ha_state_t *ha, endpoint_t *ep)
* (pool unwired, OOM, or admission reject) we fail loud and run the body INLINE
* rather than drop a consensus message — staying alive and correct.
*
* ┌─ CRITICAL FLAG-ON HAZARD (raft-drive-async finding-1; bug-1197) ──────────┐
* │ Enabling this dispatch (g_drive_async_peerctrl_dispatch_enabled == 1) │
* │ introduces an endpoint use-after-free that does NOT exist while the body │
* │ runs inline. raft_cb_become_follower (ha.c, see its endpoint_fini loop) │
* │ runs on a POOL worker and calls endpoint_fini() on ALL peer vote-eps — │
* │ freeing each ep->data (vctx) via FREEUP. Meanwhile the single io_worker │
* │ thread can be concurrently inside peer_ctrl_msg_handler_inline(ep_j, ...) │
* │ for a same-batch queued event on another ep, reading ep_j->data → races a │
* │ freed vctx (null-deref / UAF). Safe TODAY only because the flag is OFF and │
* │ the io_worker processes peer-control sequentially. │
* │ FIX REQUIRED BEFORE task 3.4 FLIPS THE FLAG ON: every pool-reachable │
* │ teardown that frees a peer ep (raft_cb_become_follower's endpoint_fini │
* │ loop) MUST iomgr_unmonitor(ep) BEFORE endpoint_fini(ep) so the io_worker │
* │ can never deliver a new event on a freed ep->data. Do NOT enable dispatch │
* │ until that ordering is in place. │
* ┌─ FLAG-ON HAZARD CLOSED (raft-drive-async finding-1; bug-1197, task 3.4a) ─┐
* │ With this dispatch enabled, raft_cb_become_follower (pool worker) tearing │
* │ down peer vote-eps could race the single io_worker thread reading the │
* │ same ep's data for a same-batch queued event — a freed-vctx UAF. Fixed by │
* │ deferred teardown-as-vtask: become_follower marks each loser vote-ep │
* │ tearing_down + iomgr_unmonitor's it BEFORE enqueuing a per-ep-bound │
* │ teardown vtask (vote_ep_teardown_vtask_entry), so the pool's per-ep busy │
* │ serialization guarantees it runs strictly after any in-flight/queued │
* │ same-ep peer-control vtask. An RCU-like dispatch_inflight drain closes the │
* │ TOCTOU window between the flag-check and enqueue. See the bug-1197 │
* │ deferred vote-ep teardown block below and vote_ep_teardown_async. │
* └──────────────────────────────────────────────────────────────────────────┘
*/
static int peer_ctrl_dispatch_to_pool(endpoint_t *ep, io_packet_t *pkt)