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

fix(raft-drive-async): fix bug-1376 cold-start self-deadlock, flip peerctrl+snapshot flags ON

Browse source 
Root cause: peer_ctrl_msg_handler held ep->dispatch_inflight incremented
across the entire peer_ctrl_dispatch_to_pool call, including its inline
fallback (the common cold-start shape when ha->pool isn't wired yet). That
inline body can reach raft_cb_become_follower, which -- now that
cluster_init_phase2 ran synchronously earlier in the same call chain and
wired the pool -- tears down its own loser vote-ep (the very ep whose reply
triggered this call) via the deferred path (vote_ep_teardown_async), whose
endpoint_mark_tearing_down spins waiting for dispatch_inflight==0 on that
SAME ep. That count is held only by this exact call, on this exact thread,
and can't reach 0 until the spin itself returns -- a permanent self-deadlock.

Confirmed live via a gdb thread backtrace: added a GDB_WRAP debug seam to
run-ha-soak.sh (launches a named node directly under `gdb --batch` so it
becomes ptrace-legal as gdb's own direct child under yama scope 1, bypassing
a live-attach restriction), then SIGINT to gdb once the log went stale for a
bounded interrupt + full backtrace. The frame dump showed pool=0x0 at
peer_ctrl_dispatch_to_pool, and the same ep pointer in the nested
vote_ep_teardown_async -> endpoint_mark_tearing_down frame, spinning on
sched_yield().

Fix: moved the dispatch_inflight release into peer_ctrl_dispatch_to_pool
itself, at the exact right point per exit path -- released BEFORE calling
the inline body on every fallback path (pool==NULL, OOM, enqueue failure),
since io_worker is single-threaded and the inline path never enqueues
anything (no pool-worker race left to protect against once inline execution
is committed to); released only AFTER the non-blocking enqueue completes on
the success path, preserving the original protection for the genuinely
concurrent pool-worker case.

New unit test test_vep_self_teardown_does_not_deadlock uses a bounded
pthread_timedjoin_np so a regression fails loud instead of hanging the
suite. RED confirmed via a temporary manual revert (ETIMEDOUT) before
restoring the fix.

Live-verified with both flags ON: --force-snapshot soak (the exact repro
shape) completes without hanging across repeated runs, confirms real
snapshot installs, zero divergence, zero job loss; a 6-round --chaotic soak
also passed clean; full controller suite green throughout.

g_drive_async_snapshot_offload and g_drive_async_peerctrl_dispatch_enabled
now default ON in production, closing raft-drive-async tasks 3.1 and 3.2.
g_drive_async_configpush_offload stays OFF -- a distinct, still-open hazard
in push_config_transport_only's endpoint handling (task 3.3, unrelated to
this bug).

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 07:18:42 +00:00
parent 10f92a5d49
commit 1ce007814d
5 changed files with 272 additions and 70 deletions
Download patch file Download diff file Expand all files Collapse all files

156
src/controller/ha.c
View file

@ -4292,30 +4292,29 @@ int ha_reconnect_offload_enabled(void)
/* raft-drive-async task 3.2: SnapshotSend feature flag.
*
* 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
* DEFAULT ON (1): the snapshot transfer runs on a slow-lane worker, off the
* caller (io_worker/drive) thread. The bug-1197-family borrowed-endpoint
* 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.
*
* 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.
* Coupled to g_drive_async_peerctrl_dispatch (bug-1198 requires both ON
* together — see that flag's comment) and flips ON with it. The cold-start
* pool-wiring self-deadlock (bug-1376) found flipping both ON together in a
* live force-snapshot soak is root-caused and fixed — see
* g_drive_async_peerctrl_dispatch_enabled's comment.
*
* 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
* in ALL builds; the setter is the test seam to flip RED↔GREEN. */
static _Atomic int g_drive_async_snapshot_offload = 0;
static _Atomic int g_drive_async_snapshot_offload = 1;
void ha_set_snapshot_offload_for_test(int on)
{
@ -4376,34 +4375,46 @@ int ha_configpush_offload_enabled(void)
/* raft-drive-async task 3.1: PeerCtrlDispatch feature flag.
*
* 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.
* DEFAULT ON (1): the registered peer-control handler hands the decoded
* packet to the vtask pool and returns promptly, instead of running its
* routing body inline on the iomgr io_worker thread.
*
* 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).
* Gate items closed: the task 3.4 deadlock-freedom proof (LivenessAdmission,
* task 1.6), the bug-1197 endpoint-quiescence fix (3.4a), bug-1198 (closed by
* the snapshot-offload flag's endpoint_ref/endpoint_unref fix — see that
* flag's comment), and bug-1376 (below).
*
* 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 (logged as
* bug-1376). 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.
* bug-1376 (self-deadlock, root-caused + fixed): a live --force-snapshot soak
* (a genuinely EMPTY node cold-starting and rejoining — the one scenario
* chaotic mode never exercises, since it restarts nodes with WAL intact) hung
* with both flags ON. Root cause: peer_ctrl_msg_handler held ep->dispatch_
* inflight across the ENTIRE peer_ctrl_dispatch_to_pool call, including its
* inline fallback when ha->pool is not yet wired (the common cold-start
* shape — a fresh node's own self-election can resolve, via an inbound vote
* reply, before cluster_init_phase2 finishes wiring the pool). That inline
* body can reach raft_cb_become_follower, which — now that cluster_init_
* phase2 has run synchronously earlier in the SAME call chain and wired the
* pool — tears down its own loser vote-eps via the deferred path
* (vote_ep_teardown_async), whose endpoint_mark_tearing_down spins waiting
* for dispatch_inflight==0 on THIS SAME ep. That count is held by this exact
* call, on this exact thread, and cannot reach 0 until the spin itself
* returns — a permanent self-deadlock. Fixed: peer_ctrl_dispatch_to_pool now
* releases dispatch_inflight at the exact right point for each of its 3 exit
* paths — before calling the inline body on every fallback path (pool==NULL,
* OOM, enqueue failure), since io_worker is single-threaded and the inline
* path never enqueues anything, so there is no pool-worker race left to
* protect against once inline execution is committed to; only after the
* (non-blocking) enqueue completes on the success path, preserving the
* original protection for the genuinely concurrent pool-worker case. Unit
* test: test_vep_self_teardown_does_not_deadlock (bounded-timeout join,
* RED->GREEN). Live-verified: --force-snapshot soak (the exact repro shape)
* now completes without hanging, confirmed a real snapshot install,
* committed_divergence=0, committed_job_loss=0; a 6-round --chaotic soak
* with both flags ON also passed clean.
*
* Compiled in ALL builds (production reads it on every peer-control message);
* the setter is the test seam to flip ON for testing. */
static _Atomic int g_drive_async_peerctrl_dispatch_enabled = 0;
* the setter is the test seam to flip OFF for a regression comparison. */
static _Atomic int g_drive_async_peerctrl_dispatch_enabled = 1;
void ha_set_peerctrl_dispatch_for_test(int on)
{
@ -5549,14 +5560,14 @@ static int peer_ctrl_vtask_entry(vtask_t *task)
* │ occupy every liveness worker and starve the acks that advance │
* │ committed_lsn. │
* │ │
* │ 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. │
* │ CLOSED: 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 now default ON (bug-1376, the cold-start │
* │ pool-wiring self-deadlock found flipping both ON, is root-caused + fixed │
* │ — see g_drive_async_peerctrl_dispatch_enabled's comment). │
* └──────────────────────────────────────────────────────────────────────────┘
*/
static vtask_class_t peer_ctrl_classify(ha_state_t *ha, io_packet_t *pkt)
@ -5710,6 +5721,31 @@ void ha_vote_ep_teardown_for_test(ha_state_t *ha, endpoint_t *ep)
* │ deferred vote-ep teardown block below and vote_ep_teardown_async. │
* └──────────────────────────────────────────────────────────────────────────┘
*/
/*
* bug-1376 (self-deadlock, found investigating the raft-drive-async flag
* flip): the caller (peer_ctrl_msg_handler) holds ep->dispatch_inflight
* incremented across this whole call. That counter exists to stop a POOL
* WORKER thread from touching a queued vtask's ep concurrently with a
* teardown — it protects the brief window between the tearing_down check
* and a (non-blocking) enqueue. It must NOT still be held while running the
* INLINE fallback body: peer_ctrl_msg_handler_inline can recursively reach
* raft_cb_become_follower -> vote_ep_teardown_async -> endpoint_mark_
* tearing_down on THIS SAME ep (a node's own vote-request ep receiving the
* very reply that makes it step down), which spins waiting for
* dispatch_inflight==0 — a count only this call itself holds, on the same
* thread, unable to ever reach 0 until this call returns. Permanent hang
* (io_worker is single-threaded, so nothing else could ever be racing this
* ep while we run inline — dispatch_inflight has nothing left to protect
* once we've committed to the inline path).
*
* Fix: release dispatch_inflight at the exact right point for whichever of
* the 3 exit paths fires below, instead of the caller holding it across the
* whole call. The ENQUEUE-success path still releases only after the
* (non-blocking) enqueue completes, preserving the original protection for
* the genuinely concurrent pool-worker case; every INLINE-fallback path
* releases BEFORE calling the inline body, since no pool worker can ever be
* racing an ep whose message we are about to process synchronously
* ourselves. */
static int peer_ctrl_dispatch_to_pool(endpoint_t *ep, io_packet_t *pkt)
{
vote_ep_ctx_t *vctx = (vote_ep_ctx_t *)ep->data;
@ -5719,6 +5755,7 @@ static int peer_ctrl_dispatch_to_pool(endpoint_t *ep, io_packet_t *pkt)
if (pool == NULL) {
WARN("peer_ctrl_dispatch_to_pool: vtask pool not wired — running "
"inline (stay alive, never drop a peer-control message)");
atomic_fetch_sub_explicit(&ep->dispatch_inflight, 1, memory_order_seq_cst);
return peer_ctrl_msg_handler_inline(ep, pkt);
}
@ -5729,6 +5766,7 @@ static int peer_ctrl_dispatch_to_pool(endpoint_t *ep, io_packet_t *pkt)
"inline (stay alive)");
free(data);
free(task);
atomic_fetch_sub_explicit(&ep->dispatch_inflight, 1, memory_order_seq_cst);
return peer_ctrl_msg_handler_inline(ep, pkt);
}
@ -5755,9 +5793,15 @@ static int peer_ctrl_dispatch_to_pool(endpoint_t *ep, io_packet_t *pkt)
"— running inline (stay alive)", rc);
free(data);
free(task);
atomic_fetch_sub_explicit(&ep->dispatch_inflight, 1, memory_order_seq_cst);
return peer_ctrl_msg_handler_inline(ep, pkt);
}
/* Enqueued: release now that the (non-blocking) enqueue itself has
* completed. The queued vtask is what a pool worker will process; a
* concurrent teardown drain now correctly waits on the per-ep busy/FIFO
* gate (bind_vtask) for that vtask, not on this call. */
atomic_fetch_sub_explicit(&ep->dispatch_inflight, 1, memory_order_seq_cst);
return 0; /* returns promptly; the body runs on a pool worker */
}
@ -5818,15 +5862,11 @@ static int peer_ctrl_msg_handler(void *ep_raw)
ha_peerctrl_predispatch_probe(ep);
#endif
/* NOTE (drain liveness): this call runs while dispatch_inflight is held, so
* a concurrent endpoint_mark_tearing_down on this ep spins until it returns.
* peer_ctrl_dispatch_to_pool only ever enqueues (non-blocking) or, on OOM,
* runs peer_ctrl_msg_handler_inline — which must also stay non-blocking
* (the vtask-handler no-block invariant). Keep it bounded or the drain is. */
int rc = peer_ctrl_dispatch_to_pool(ep, pkt);
atomic_fetch_sub_explicit(&ep->dispatch_inflight, 1, memory_order_seq_cst);
return rc;
/* bug-1376: peer_ctrl_dispatch_to_pool owns the matching fetch_sub now
* (released at the exact right point for whichever of its 3 exit paths
* fires — see its own comment) instead of a single unconditional
* decrement here after it returns. See that function for why. */
return peer_ctrl_dispatch_to_pool(ep, pkt);
}
#ifdef XCE_TESTING

86
src/controller/testsuite/test_ha_vep_teardown.c
View file

@ -538,6 +538,91 @@ static void test_vep_mark_drains_inflight_dispatch(void)
printf("test_vep_mark_drains_inflight_dispatch: PASS\n");
}
/* ── Test (f): bug-1376 SELF-TEARDOWN — must not self-deadlock ─────────────
*
* A node's OWN in-flight peer-control dispatch can, via its routing body,
* decide to tear down the VERY SAME ep the dispatch is currently processing
* (the live signature: a node's own vote-request ep receives the reply that
* makes it become follower, and the loser loop closes that same ep). Before
* the bug-1376 fix, peer_ctrl_msg_handler held ep->dispatch_inflight across
* the ENTIRE inline body call; vote_ep_teardown_async's endpoint_mark_
* tearing_down then spun forever waiting for that same count — held by this
* same thread, unable to decrement until the spin itself returns — a
* permanent self-deadlock. Reproduced with a bounded-timeout join: the
* dispatch runs on a helper thread; the test FAILS (rather than hanging the
* whole suite) if it does not return within the timeout.
*/
static void *self_teardown_dispatch_thread(void *p)
{
dispatch_arg_t *a = (dispatch_arg_t *)p;
io_packet_t *pkt = repl_catchup_done_pkt_for_test();
peer_ctrl_msg_handler_for_test(a->ep, pkt);
atomic_store(&a->done, 1);
return NULL;
}
static ha_state_t *g_self_teardown_ha = NULL;
static endpoint_t *g_self_teardown_ep = NULL;
/* Fires at the top of the inline routing body (before role dispatch) — tears
* down the SAME ep this dispatch is currently processing, the self-referential
* case the fix must resolve. */
static void self_teardown_body_probe(endpoint_t *ep)
{
(void)ep;
ha_vote_ep_teardown_for_test(g_self_teardown_ha, g_self_teardown_ep);
}
static void test_vep_self_teardown_does_not_deadlock(void)
{
reset_oracles();
ha_state_t ha;
TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 1));
/* CTRL_INIT → the inline body hits the default case (drop + free pkt)
* after the probe runs, so the stand-in vote_ep_ctx_t is sufficient. */
atomic_store_explicit(&ha.role, (int32_t)CTRL_INIT, memory_order_release);
ha.pool = NULL; /* matches the live cold-start repro (bug-1376) */
endpoint_t *ep = endpoint_get();
TEST_ASSERT_NOT_NULL(ep);
ep->iomgr = NULL;
ep->fd = -1;
struct mini_vctx *v = calloc(1, sizeof(*v));
TEST_ASSERT_NOT_NULL(v);
v->ha = &ha;
ep->data = v;
ha_set_peerctrl_dispatch_for_test(1);
g_self_teardown_ha = &ha;
g_self_teardown_ep = ep;
ha_peerctrl_body_probe = self_teardown_body_probe;
dispatch_arg_t darg = { .ep = ep };
atomic_store(&darg.done, 0);
pthread_t t;
TEST_ASSERT_EQUAL_INT(0,
pthread_create(&t, NULL, self_teardown_dispatch_thread, &darg));
struct timespec deadline;
clock_gettime(CLOCK_REALTIME, &deadline);
deadline.tv_sec += 5;
int jrc = pthread_timedjoin_np(t, NULL, &deadline);
TEST_ASSERT_EQUAL_INT_MESSAGE(0, jrc,
"bug-1376: self-teardown (an ep's own dispatch tearing down the same "
"ep) must not deadlock — dispatch_inflight must be released before "
"the inline body runs, not held across it");
TEST_ASSERT_EQUAL_INT_MESSAGE(1, atomic_load(&darg.done),
"the dispatch thread must have completed");
ha_peerctrl_body_probe = NULL;
ha_set_peerctrl_dispatch_for_test(0);
g_self_teardown_ha = NULL;
g_self_teardown_ep = NULL;
ha_fini(&ha);
printf("test_vep_self_teardown_does_not_deadlock: PASS\n");
}
/* ── Entry point ─────────────────────────────────────────────────────────── */
void test_ha_vep_teardown(void)
@ -547,4 +632,5 @@ void test_ha_vep_teardown(void)
RUN_TEST(test_vep_teardown_vtask_disarms_unblock);
RUN_TEST(test_vep_off_path_uses_plain_fini);
RUN_TEST(test_vep_mark_drains_inflight_dispatch);
RUN_TEST(test_vep_self_teardown_does_not_deadlock);
}