docs: log task 3.3's config-push endpoint race as bug-1381

Cross-references bug-1381 in tasks.md and the ha.c configpush flag comment.
No code behavior change -- push_config_transport_only's race (raw standby
endpoint pointers read/written by unprotected pthread_create workers,
independent of the offload flag) is a distinct, still-open blocker from
bug-1197/bug-1376 (both closed in the prior commit).
This commit is contained in:
Rongsong Shen 2026-07-02 07:24:21 +00:00
parent 1ce007814d
commit 02e46f6ba1
3 changed files with 996 additions and 958 deletions

View file

@ -154,10 +154,10 @@ Tasks mirror the design's 4-phase migration. The slow lane is foundational (ever
- **Part A (DORMANT, `g_drive_async_snapshot_offload` DEFAULT 0/OFF)**: `ha_snapshot_send_dispatch` routes `HA_REPL_SEND_SNAPSHOT` inline (flag OFF — byte-for-byte legacy, send on io_worker) or to `slowlane_submit`+`snapshot_lane_fn` (flag ON). Send body factored into `ha_snapshot_do_send` (shared by both paths, holds the XCE_TESTING hook). Per-peer single-flight via `ha_state_t.snapshot_inflight[MAX_HA_CANDIDATES]` CAS + slowlane key `peer_idx+1`; flag cleared on all 4 terminal paths (alloc-fail, -EEXIST, hard-error, lane-fn completion) exactly like reconnect (2.1). Tests: `test_snapshot_offload_runs_on_lane`, `test_snapshot_inline_when_flag_off`, `test_snapshot_offload_dedups_per_peer` (test_ha_liveness.c).
- **WHY IT WAS DORMANT, AND HOW IT CLOSED**: `snapshot_lane_fn` captures a BORROWED standby endpoint (unlike reconnect, which owns its ep). With the flag ON the send runs on a lane thread while the io_worker may RDHUP-`endpoint_fini` that ep → cross-thread UAF (bug-1197 family). Flag OFF kept send+teardown serialized on the io_worker. Caught + fixed pre-commit early on: the offload flag was initially authored DEFAULT-ON (would have shipped the UAF) → corrected to OFF in both ha.c and the ha.h accessor doc (bug-1210). **Closed 2026-07-02**: `ha_snapshot_send_dispatch`/`snapshot_lane_fn` now bracket the lane dispatch with `endpoint_ref`/`endpoint_unref` (reusing the general bug-1298 refcount system), so the borrowed-endpoint hazard cannot recur regardless of thread timing — no lane-serialized-teardown redesign needed. `g_drive_async_snapshot_offload` and `g_drive_async_peerctrl_dispatch` both now default ON together (bug-1376, a distinct cold-start self-deadlock unmasked by flipping both, is also fixed).
- **3.3 (landed DORMANT — checkbox `[~]` until 3.4 gate)**: requester-less config-push transport offload, flag `g_drive_async_configpush_offload` **DEFAULT 0/OFF** (toggle `ha_set_configpush_offload_for_test`). `ha_config_push_offloadable(c, bytes, blen, sha256, new_tag)` routes inline (flag OFF — verbatim `push_config_transport_only`, caller still owns+frees `bytes`) or to `slowlane_submit`+`configpush_lane_fn` (flag ON — DEEP-COPIES `bytes`+sha into a ctx the lane frees). Single cluster-wide single-flight via scalar `ha_state_t.configpush_inflight` CAS + lane key=1 (config push is a cluster-wide fan-out, not per-peer); flag cleared on all terminal paths (alloc-fail×2, -EEXIST, hard-error, lane completion). Send body factored into `ha_config_push_do_send` (production → `push_config_transport_only`; XCE_TESTING → `ha_configpush_probe` stub since cluster.c/transport isn't linked in test). THREE requester-less drive-thread call sites converted: `raft_cb_become_leader` (promotion, both bytes/NULL branches), the `HA_REPL_PUSH_CONFIG` heartbeat case, and the direct-CTRL_PRIMARY FSM promotion branch (~ha.c:4476). UNCHANGED: `push_config_to_standbys` admin-upgrade barrier (has a waiting requester) and `join.c`. Tests: `test_configpush_offload_runs_on_lane`, `test_configpush_inline_when_flag_off`, `test_configpush_offload_dedups` (test_ha_liveness.c). Same BORROWED-ENDPOINT/bug-1197 dormancy rationale as 3.2 (lane reads `c->ha.standby_endpoints`); **3.4 flips this ON together with snapshot + peerctrl flags**. cpp-review: no CRITICAL/HIGH/MEDIUM; controller suite EXIT=0.
- **3.3 (landed DORMANT — checkbox `[~]`, blocked on bug-1381)**: requester-less config-push transport offload, flag `g_drive_async_configpush_offload` **DEFAULT 0/OFF** (toggle `ha_set_configpush_offload_for_test`). `ha_config_push_offloadable(c, bytes, blen, sha256, new_tag)` routes inline (flag OFF — verbatim `push_config_transport_only`, caller still owns+frees `bytes`) or to `slowlane_submit`+`configpush_lane_fn` (flag ON — DEEP-COPIES `bytes`+sha into a ctx the lane frees). Single cluster-wide single-flight via scalar `ha_state_t.configpush_inflight` CAS + lane key=1 (config push is a cluster-wide fan-out, not per-peer); flag cleared on all terminal paths (alloc-fail×2, -EEXIST, hard-error, lane completion). Send body factored into `ha_config_push_do_send` (production → `push_config_transport_only`; XCE_TESTING → `ha_configpush_probe` stub since cluster.c/transport isn't linked in test). THREE requester-less drive-thread call sites converted: `raft_cb_become_leader` (promotion, both bytes/NULL branches), the `HA_REPL_PUSH_CONFIG` heartbeat case, and the direct-CTRL_PRIMARY FSM promotion branch (~ha.c:4476). UNCHANGED: `push_config_to_standbys` admin-upgrade barrier (has a waiting requester) and `join.c`. Tests: `test_configpush_offload_runs_on_lane`, `test_configpush_inline_when_flag_off`, `test_configpush_offload_dedups` (test_ha_liveness.c). Blocked on **bug-1381** (open, DIFFERENT from bug-1197/bug-1376 — `push_config_transport_only` itself, not the lane fn, races raw standby endpoint pointers via unprotected `pthread_create` workers). cpp-review: no CRITICAL/HIGH/MEDIUM; controller suite EXIT=0.
- **3.4a (bug-1197 endpoint-quiescence fix — committed; flags STILL default OFF)**: makes the flag-ON peer-control dispatch path UAF-free so `raft_cb_become_follower` (running as a pool vtask) can free peer vote-eps while other pool workers process peer-control on the same eps. Mechanism (no libxce-wide endpoint refcount; reuses the existing per-ep FIFO + busy serialization):
- **Deferred teardown-as-vtask**: the loser loop, when `ha_peerctrl_dispatch_enabled() && ha->pool`, does per vote-ep `endpoint_mark_tearing_down(ep)` → `iomgr_unmonitor(ep)` → enqueue a `VTASK_IO`/`VTASK_LIVENESS` teardown vtask bound to that ep (`vote_ep_teardown_vtask_entry`). Per-ep busy FIFO makes it run strictly AFTER any in-flight/queued same-ep peer-control vtask; it NULLs `iod->ep` before `endpoint_fini`+`free(ep)` to disarm `run_task`'s post-body `endpoint_unblock_tasks` (which would otherwise touch the freed `ep->lock`). OFF / pool-unwired path is byte-for-byte the legacy `endpoint_fini` (no free).
- **RCU-like drain closes the TOCTOU window**: a new `_Atomic int endpoint_t.dispatch_inflight` (+ `tearing_down:1` bit). The ON dispatch brackets its flag-check+enqueue: `fetch_add(seq_cst)` → if `tearing_down` drop pkt + `fetch_sub` → else enqueue → `fetch_sub`. `endpoint_mark_tearing_down` sets the flag under `ep->lock`, releases, then spins `while(load(dispatch_inflight,seq_cst)>0) sched_yield()`. become_follower marks (drains) BEFORE enqueuing the teardown vtask, so any dispatch that read the flag false is FIFO-before teardown; any later dispatch drops. Could NOT hold `ep->lock` across the enqueue — it inverts with the taskq lock (dispatcher takes taskq→ep in `vtask_eligible`); the counter is the lock-free handshake. Drain is bounded (the held window is just calloc+non-blocking enqueue on the single io_worker).
- Files: `src/include/netio.h` (`tearing_down:1`, `dispatch_inflight`, mark/is helpers), `src/lib/netio.c` (helpers + drain + `do_endpoint_init` zero-init), `src/controller/ha.c` (`vote_ep_teardown_*`, loser-loop async branch, dispatch bracket + late-drop, test seams), `src/controller/ha.h`. Tests: `test_ha_vep_teardown.c` (5 ASan tests: ordering, late-drop, disarm, OFF-path, deterministic drain-blocks). xcelib_test 282/0; controller EXIT=0; ASan-clean. Adversarial cpp-review: no CRITICAL/HIGH; handshake formally sound (SC total order on the counter + monotone `tearing_down` under `ep->lock`); flag-ON path proven race-free.
- **STILL OPEN for the prod flip (4.x)**: the borrowed-ep hazard in `snapshot_lane_fn`/`configpush_lane_fn` is a DISTINCT race (slowlane thread vs io_worker RDHUP teardown, not the vtask-pool FIFO) — needs its own per-standby-ep refcount/lane-serialized teardown before `g_drive_async_snapshot_offload`/`configpush_offload` go ON in production. 3.4b integration tests may enable the flags in-test (no mid-transfer ep teardown, so the lane race isn't exercised).
- **STILL OPEN for `configpush_offload`'s prod flip**: `snapshot_lane_fn`'s borrowed-ep hazard closed 2026-07-02 (endpoint_ref/unref, see task 3.2's note). `configpush_lane_fn`'s hazard is a DIFFERENT, still-open race — logged as **bug-1381**: `push_config_transport_only` (not `configpush_lane_fn` itself) grabs raw standby endpoint pointers and spawns unprotected `pthread_create` workers for the fan-out, a race that exists independent of the offload flag. Needs its own per-standby-ep refcount (or lane-serialized teardown) before `g_drive_async_configpush_offload` goes ON in production. 3.4b integration tests may enable the flag in-test (no mid-transfer ep teardown, so the race isn't exercised there).

File diff suppressed because it is too large Load diff

View file

@ -4349,11 +4349,11 @@ int ha_snapshot_offload_enabled(void)
* 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.
* 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.
*
* ONLY the requester-less call sites (post-election promotion, heartbeat-driven
* HA_REPL_PUSH_CONFIG) route through this flag; the admin-upgrade barrier path