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

fix(controller): bug-1450/1451 offload delaymgr-inline blocking work to the slow lane

Browse source 
/xce-audit-invariants controller found two live violations of the async
I/O thread-role contract: ha_demotion_convergence_fn (ha_convergence.c)
ran WAL truncation + tlog_exit()+execv() directly on the delaymgr thread,
and ha_catchup_continuation_fn (config_repl.c) ran unbounded WAL-streaming
to a standby directly on delaymgr -- the same single thread that drives
heartbeats and election timeouts cluster-wide, so either blocks liveness
for the whole cluster and risks a spurious election.

A prior vtask-pool offload attempt (2026-07-10, bug-1429) deadlocked
controller_unit_test because c->ha.pool is never initialized by the
lightweight unit-test fixtures these functions run under, and was fully
reverted. This fix instead uses the codebase's established slow-lane
pattern (the same mechanism already used for snapshot-send, config-push,
and peer connect) -- a 4th and 5th instance, not a new mechanism.

New g_drive_async_demotion_offload / g_drive_async_catchup_offload flags
ship default OFF, matching the original rollout discipline of the three
reference offloads (shipped OFF, flipped ON later after a dedicated
compatibility audit). Flipping ON was tried during validation and
reproduced two distinct hazards against pre-existing fixtures that never
call slowlane_init: the already-known hang, and a new ASan SEGV in
commit_advance_locked (logged as bug-1452, not root-caused, deferred to
the future flip-to-ON audit). Both flags verified OFF and the full
976-test suite clean, then the committed state re-verified clean against
a live 10-round chaotic and 10-round sequential HA soak (zero job loss,
zero divergence, both PASS).
This commit is contained in:
Rongsong Shen 2026-07-19 08:14:00 +00:00
parent aeb6e37ce7
commit f88e4ef431
5 changed files with 877 additions and 55 deletions
Download patch file Download diff file Expand all files Collapse all files

156
src/controller/config_repl.c
View file

@ -2596,29 +2596,70 @@ typedef struct {
} ha_catchup_continuation_ctx_t;
/*
* ha_catchup_continuation_fn — the real catch-up work. Re-validate the
* endpoint against the registry, re-derive target, stream [acked_lsn+1,
* target], and clear catchup_inflight on EVERY exit path (success, ep-gone,
* nothing-to-stream) so a stuck flag never blocks future continuations (Req
* 5.2). Frees its own ctx (body-frees-own-ctx pattern, mirroring
* ha_demotion_convergence_run).
* g_drive_async_catchup_offload / ha_set_catchup_offload_for_test /
* ha_catchup_offload_enabled (xce-audit-invariants controller pass,
* thread-role violation 2 fix, 2026-07-18):
*
* Runs as a delaymgr task body (see ha_schedule_catchup_continuation below) —
* must run off the iomgr walAck thread, since catchup_stream_range ->
* ha_stream_wal_to_standby does blocking WAL file reads + socket writes.
* (2026-07-10: a VTASK_LIVE_BLOCK dispatch was attempted here to also get it
* off the delaymgr thread per ha-async-io.md's thread-role contract, but it
* deadlocked the controller_unit_test harness — reverted pending a proper
* fix; tracked as a known, still-open thread-role violation alongside the
* other raft-drive-async P2 items.)
* Gates whether ha_catchup_continuation_fn hands the WAL-streaming body off
* to the slow lane (ON — closes the thread-role violation) or runs it
* INLINE on the calling (delaymgr) thread (OFF, the PRODUCTION DEFAULT for
* now — see below). Catch-up can stream a variable, potentially large
* amount of WAL data, so leaving it inline on delaymgr — the SAME single
* thread that drives heartbeats and election timeouts cluster-wide — is an
* unbounded block and a real spurious-election risk, not just a theoretical
* one (ha-async-io.md thread-role contract). This is the SAME established
* slow-lane pattern already used for snapshot send, config-push, and peer
* connect in ha.c — see ha_set_demotion_offload_for_test (ha.h) for why NOT
* the vtask pool that deadlocked controller_unit_test on 2026-07-10
* (bug-1429) when tried here.
*
* OFF by default: ha_walack_on_ack / ha_catchup_continuation_fn is exercised
* by test_convergence_verification.c's pre-existing catch-up fixtures, which
* (like most controller_unit_test HA fixtures) never call slowlane_init.
* Flipping this flag ON by default was tried and reproduced a REAL hang —
* the streaming work is silently submitted to an uninitialized
* c->slowlane (queued, never run — no live workers), and downstream code
* waiting on that now-never-happening completion hangs. Matches the
* ORIGINAL rollout discipline of the snapshot/config-push/reconnect
* offloads (all shipped OFF-by-default, flipped ON only in a LATER,
* dedicated pass — raft-drive-async task 3.4 — once the full test suite was
* audited). This change lands the mechanism, correct and covered by its
* own targeted tests below; flipping the cluster-wide default to ON is the
* same kind of follow-up audit, out of scope for this fix (bug-1451,
* buglog.json). Compiled in ALL builds; the setter is the test seam to
* flip RED<->GREEN.
*/
int ha_catchup_continuation_fn(void *arg)
static _Atomic int g_drive_async_catchup_offload = 0;
void ha_set_catchup_offload_for_test(int on)
{
atomic_store_explicit(&g_drive_async_catchup_offload, on ? 1 : 0,
memory_order_release);
}
int ha_catchup_offload_enabled(void)
{
return atomic_load_explicit(&g_drive_async_catchup_offload,
memory_order_acquire);
}
/*
* catchup_lane_fn — slow-lane worker entry (and the legacy inline-path body,
* offload flag OFF). Re-validate the endpoint against the registry,
* re-derive target, stream [acked_lsn+1, target], and clear catchup_inflight
* on EVERY exit path (success, ep-gone, nothing-to-stream) so a stuck flag
* never blocks future continuations (Req 5.2). Frees its own ctx
* (body-frees-own-ctx pattern, mirroring ha_demotion_convergence_do_run).
*
* The blocking work itself — catchup_stream_range -> ha_stream_wal_to_standby
* (WAL file reads + socket writes to a standby) — is unchanged from before
* the offload; only WHERE it runs changed (see ha_catchup_continuation_fn).
*/
static void catchup_lane_fn(void *arg)
{
ha_catchup_continuation_ctx_t *ctx = (ha_catchup_continuation_ctx_t *)arg;
if (ctx == NULL || ctx->c == NULL) {
free(ctx);
return -EINVAL;
}
if (ctx == NULL)
return;
cluster_t *c = ctx->c;
repl_ep_t ep = ctx->ep;
int peer_idx = ctx->peer_idx;
@ -2648,6 +2689,83 @@ clear_and_done:
atomic_store_explicit(&c->ha.catchup_inflight[peer_idx], 0,
memory_order_release);
free(ctx);
}
/*
* ha_catchup_continuation_fn — delaymgr task body (the FIRST hop, off the
* iomgr walAck thread — see ha_schedule_catchup_continuation below). With
* the offload flag ON, this function does only fast, non-blocking admin
* work (slowlane_submit) and returns immediately — the SECOND hop, off the
* delaymgr thread itself, onto a slow-lane worker (catchup_lane_fn), which
* is where catchup_stream_range's blocking WAL reads + socket writes
* actually run. With the flag OFF (the current default — see
* g_drive_async_catchup_offload above), runs the legacy inline blocking
* path.
*
* c->ha.catchup_inflight[peer_idx] (set by ha_walack_on_ack BEFORE this task
* is even scheduled) remains the authoritative per-standby single-flight
* guard (Req 5.2); the lane's own per-key dedup (key = peer_idx+1) is a
* redundant backstop — same relationship ha_connect_to_peer's reconnect
* offload has with reconnect_inflight[] (ha.c).
*
* History: a VTASK_LIVE_BLOCK vtask-pool dispatch was attempted here on
* 2026-07-10 to get this off the delaymgr thread and deadlocked
* controller_unit_test — reverted (bug-1429). The slow lane (this fix) is a
* different, already-proven mechanism (raft-drive-async's snapshot/
* config-push/reconnect offloads) and does not share that failure mode: its
* worker threads start unconditionally in cluster_init_phase2, and
* slowlane_submit never blocks the caller even when a lightweight unit-test
* fixture never calls slowlane_init (it just queues work that will not run,
* rather than blocking waiting for a worker).
*/
int ha_catchup_continuation_fn(void *arg)
{
ha_catchup_continuation_ctx_t *ctx = (ha_catchup_continuation_ctx_t *)arg;
if (ctx == NULL || ctx->c == NULL) {
free(ctx);
return -EINVAL;
}
cluster_t *c = ctx->c;
int peer_idx = ctx->peer_idx;
if (!ha_catchup_offload_enabled()) {
/* Legacy inline path (flag OFF) — blocks the calling (delaymgr)
* thread. Retained solely so the RED test can show the pre-change
* stall; production runs with the flag ON. */
catchup_lane_fn(ctx);
return 0;
}
uint64_t key = (uint64_t)(peer_idx + 1);
int srv = slowlane_submit(&c->slowlane, key, catchup_lane_fn, ctx);
if (srv == -EEXIST) {
/* Lane still holds this peer's dedup key from a PRIOR continuation
* that already cleared catchup_inflight[peer_idx] (its last action)
* but whose key the lane has not yet removed. Release the guard so
* the next WAL ack retries cleanly (mirrors ha_connect_to_peer's
* -EEXIST handling in ha.c). */
if (peer_idx >= 0 && peer_idx < MAX_STANDBYS)
atomic_store_explicit(&c->ha.catchup_inflight[peer_idx], 0,
memory_order_release);
free(ctx);
return 0;
}
if (srv < 0 && srv != -EAGAIN) {
/* Hard submit failure (alloc / lane stopping). -EAGAIN is NOT a
* failure (still queued, will run). Only on a real failure release
* the guard and drop — the next WAL ack schedules a fresh
* continuation (never self-terminate; this event recurs on every
* ack, unlike the one-shot demotion-convergence task). */
WARN("HA catch-up continuation: slowlane_submit failed for "
"standby[%d]: %d — retry next ack", peer_idx, srv);
if (peer_idx >= 0 && peer_idx < MAX_STANDBYS)
atomic_store_explicit(&c->ha.catchup_inflight[peer_idx], 0,
memory_order_release);
free(ctx);
return 0;
}
/* 0 or -EAGAIN: enqueued exactly once; catchup_lane_fn clears the flag
* and frees ctx on completion. */
return 0;
}