From 5503c7a17616cc52c8950a7760ed8f67cfcb58f3 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 16 Jun 2026 07:56:07 +0000 Subject: [PATCH] chore(ha-prevote): resolve log-reason nit; track elect_ctx leak as follow-up spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Mark the PreVote denial log-reason nit RESOLVED in ha-prevote tasks notes (47f7338). - File ha-elect-ctx-lifecycle spec scaffold (spec.json + brief.md) capturing the pre-existing, bounded, UAF-sensitive election-carrier leak and the refcount/ teardown-ordered-free approach across both election and PreVote paths. Deferred by decision — not folded into ha-prevote to avoid a risky lifetime refactor. Co-Authored-By: Claude Opus 4.8 (1M context) --- .kiro/specs/ha-elect-ctx-lifecycle/brief.md | 62 ++++++++++++++++++++ .kiro/specs/ha-elect-ctx-lifecycle/spec.json | 22 +++++++ .kiro/specs/ha-prevote/tasks.md | 4 +- .wolf/cerebrum.md | 1 + .wolf/memory.md | 13 ++++ 5 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 .kiro/specs/ha-elect-ctx-lifecycle/brief.md create mode 100644 .kiro/specs/ha-elect-ctx-lifecycle/spec.json diff --git a/.kiro/specs/ha-elect-ctx-lifecycle/brief.md b/.kiro/specs/ha-elect-ctx-lifecycle/brief.md new file mode 100644 index 00000000..1f051420 --- /dev/null +++ b/.kiro/specs/ha-elect-ctx-lifecycle/brief.md @@ -0,0 +1,62 @@ +# Discovery Brief: ha-elect-ctx-lifecycle + +_Filed 2026-06-16 as a tracked follow-up from the ha-prevote feature. Not yet specced — run `/kiro-spec-requirements ha-elect-ctx-lifecycle` to begin._ + +## Problem + +The HA election **carrier** (`ha_elect_ctx_t`, allocated as `mc` in both +`ha_start_election_async` and `ha_start_prevote_async`, stored in +`ha->elect_ctx`) is **never freed**. Each failover/election/PreVote round +`calloc`s a fresh carrier and the prior one is deliberately leaked: + +```c +/* ha.c ~3746 */ +/* Do NOT free a prior ctx (live vote endpoints may hold a bare + * pointer to it — same UAF caution as the rdhup path). */ +atomic_store_explicit(&ha->elect_ctx, mc, memory_order_release); +``` + +Root cause: per-peer vote endpoints hold a bare pointer to the carrier +(`vctx->ctx = ectx`). Those `vctx` are freed asynchronously by `endpoint_fini` +(FREEUP) on the iomgr thread when the vote endpoint closes — at a time the +round-starting thread cannot cheaply observe. There is **no actual +`ha_election_cleanup()`** (only an aspirational comment at ha.c:3504). So +freeing the carrier when a new round starts would risk a **use-after-free** if +an endpoint callback still references it. + +## Why it's not urgent + +- **Pre-existing**: the real-election path (`ha_start_election_async`) has the + identical leak; ha-prevote reused the same carrier pattern and did **not** + worsen the per-failover rate (a suppressed disruptive rejoin now leaks *one* + PreVote carrier where it previously caused an election storm leaking several). +- **Bounded**: one small struct (`sizeof(ha_elect_ctx_t) + n_peers*votes[]`, + hundreds of bytes for a 3-node cluster) per failover/PreVote event. Failover + is a rare event, not steady-state — no unbounded growth under normal operation. +- Surfaced as ~174KB / ~1548 allocs under ASAN during the full controller test + suite (many simulated rounds); `detect_leaks=0` is used in the test command. + +## Scope (when specced) + +- Reference-count the carrier (increment per `vctx` that captures it, decrement + on each `vctx` free, free at zero) **or** a teardown-ordered deferred free + tied to confirmed vote-endpoint release. +- Apply to **both** the real-election and PreVote carrier paths — they share the + `ha->elect_ctx` field and the same UAF hazard; fixing one without the other + leaves the hazard. +- Must not regress failover correctness (bug-339/bug-340) or introduce a UAF on + the async endpoint-teardown path. Validate under ASAN with leak detection + **enabled** (the success criterion is zero carrier leaks across many rounds). + +## Out of scope + +- The PreVote protocol itself (shipped and validated in `ha-prevote`). +- Any change to election/voting *semantics* — this is purely memory lifetime. + +## Pointers + +- `src/controller/ha.c`: `ha_start_election_async`, `ha_start_prevote_async` + (carrier alloc + the "do NOT free prior ctx" comments ~3503, ~3746), + `raft_cb_send_vote_request` / `ha_send_pre_vote_request` (set `vctx->ctx`), + endpoint teardown via `endpoint_fini` (frees `vctx`). +- `src/controller/ha.h`: `ha_elect_ctx_t`, `vote_ep_ctx_t`. diff --git a/.kiro/specs/ha-elect-ctx-lifecycle/spec.json b/.kiro/specs/ha-elect-ctx-lifecycle/spec.json new file mode 100644 index 00000000..398f70fa --- /dev/null +++ b/.kiro/specs/ha-elect-ctx-lifecycle/spec.json @@ -0,0 +1,22 @@ +{ + "feature_name": "ha-elect-ctx-lifecycle", + "created_at": "2026-06-16T03:10:00Z", + "updated_at": "2026-06-16T03:10:00Z", + "language": "en", + "phase": "initialized", + "approvals": { + "requirements": { + "generated": false, + "approved": false + }, + "design": { + "generated": false, + "approved": false + }, + "tasks": { + "generated": false, + "approved": false + } + }, + "ready_for_implementation": false +} diff --git a/.kiro/specs/ha-prevote/tasks.md b/.kiro/specs/ha-prevote/tasks.md index 35b98b4a..d144249e 100644 --- a/.kiro/specs/ha-prevote/tasks.md +++ b/.kiro/specs/ha-prevote/tasks.md @@ -95,6 +95,6 @@ - (1.1) Wire opcodes `preVoteRequest @38` / `preVoteReply @39` reuse existing `RequestVoteRequest`/`RequestVoteReply` structs and union arms (`requestvotemsg @27` / `requestvotereplymsg @14`). Generated bindings live under `src/xce.build/lib/controller.capnp.h` (build artifacts, not source-committed). - (1.2) Raft PreVote scaffolding in `raft.h`: outcome codes `RAFT_PRE_VOTE_WON/LOST/PENDING` = 7/8/9; tally fields `pre_vote_in_progress`, `pre_votes_granted`, `pre_vote_replies[RAFT_MAX_PEERS]` appended to `raft_state_t` (after `my_priority`); optional `on_send_pre_vote_request` callback (NULL-checkable). New `#define RAFT_MAX_PEERS 8` bounds the per-peer dedup array — safe: production cap is `MAX_HA_CANDIDATES=4` (src/include/config.h:78), and it matches `RAFT_SIM_MAX_NODES=8`. Tasks 2.1/2.2/2.3 supply the function bodies and MUST keep them pure (no `term`/`voted_for`/`role` mutation, no `on_persist`). Build/test the raft layer with `cd src && nix develop --command bash tools/xce-build.sh lib`. - (3.1) IMPORTANT for controller tasks: `cluster.c` is NOT linked into the unit-test binary (`controller_unit_test_src` in `src/controller/testsuite/meson.build`) — it's daemon-only (`controller_src`). So dispatch cases in cluster.c can't be unit-tested directly; test their composed primitives + contract asserts, and defer end-to-end to scenario tasks 5.2/5.3. `ha.c` IS unit-test-linked (the `test_ha_*` suites exercise it). Controller build/test: `cd src && nix develop --command bash tools/xce-build.sh controller`; the registered meson test name is `"controller test"` (single Unity binary). Known pre-existing `test_commit_barrier_reply` 300s TIMEOUT makes the full suite exit non-zero — validate via the targeted run `nix develop --command meson test -C xce.build --no-rebuild --print-errorlogs "controller test"` and check for zero `:FAIL` (Timeout:1 is the known one). The new prevote opcodes' generated constants are `OPCode_preVoteRequest=38` / `OPCode_preVoteReply=39`. -- (3.1, LOW nit) cluster.c PreVote denial LOG reason uses elimination order (stale_term→primary→catch_up→fresh_heartbeat) which differs from `raft_cb_vote_eligible`'s gate order (primary→fresh_heartbeat→catch_up); in the rare STANDBY catch-up+fresh-heartbeat overlap the log string may say `catch_up` while the gate denied on `fresh_heartbeat`. Decision is identical (deny); log-string only. Optional future polish: reorder to match. +- (3.1, LOW nit) cluster.c PreVote denial LOG reason uses elimination order (stale_term→primary→catch_up→fresh_heartbeat) which differs from `raft_cb_vote_eligible`'s gate order (primary→fresh_heartbeat→catch_up); in the rare STANDBY catch-up+fresh-heartbeat overlap the log string may say `catch_up` while the gate denied on `fresh_heartbeat`. Decision is identical (deny); log-string only. RESOLVED (commit 47f7338): exposed pure `ha_heartbeat_is_fresh` and rewrote the elimination to mirror the gate's exact order (stale_term → primary → fresh_heartbeat → catch_up). - (4.1) ha.c now has `ha_start_prevote_async`, `pre_vote_reply_dispatch`, `ha_enter_real_election` (WON branch = the factored truncate+CTRL_ELECTING+ha_start_election_async helper), a `raft_cb_send_pre_vote_request` callback (registered in `s_real_cbs`), and a round-timeout task. KEY for task 5.1: `pre_vote_reply_dispatch(ha, peer_idx, granted)` takes the ABSOLUTE cluster peer index (matches `raft_begin_pre_vote`'s send loop), NOT the real-vote local `votes[]` index. `pre_vote_reply_dispatch` is `__attribute__((used))` with NO production caller yet — task 5.1's cluster.c `OPCode_preVoteReply` case must call it (replacing the `xce_*` test seam in production). Triggers (rdhup/liveness/heartbeat) are still UNMODIFIED — task 4.2 rewires them to call `ha_start_prevote_async`. -- (4.2) Triggers now rewired: `ha_on_primary_ctrl_fd_rdhup` + `liveness_poll_fn` → `ha_begin_failover` → `ha_start_prevote_async` (PreVote first; truncate+CTRL_ELECTING deferred to `ha_enter_real_election` on WON). `ha_on_heartbeat_received` cancels an in-progress round (only when `pre_vote_in_progress`) + re-attaches CTRL_STANDBY, no term change. LOAD-BEARING: `ha_start_prevote_async` checks `raft_begin_pre_vote`'s return and on immediate `RAFT_PRE_VOTE_WON` (single-node, n_peers==1) synchronously calls `ha_enter_real_election` — without this a single-node/immediate-majority node never elects. Full controller suite 563 PASS/0 FAIL with PreVote live. (NOTE: pre-existing deliberate `elect_ctx` ASAN leak ~one-per-round carried into PreVote; out of scope, future cleanup.) +- (4.2) Triggers now rewired: `ha_on_primary_ctrl_fd_rdhup` + `liveness_poll_fn` → `ha_begin_failover` → `ha_start_prevote_async` (PreVote first; truncate+CTRL_ELECTING deferred to `ha_enter_real_election` on WON). `ha_on_heartbeat_received` cancels an in-progress round (only when `pre_vote_in_progress`) + re-attaches CTRL_STANDBY, no term change. LOAD-BEARING: `ha_start_prevote_async` checks `raft_begin_pre_vote`'s return and on immediate `RAFT_PRE_VOTE_WON` (single-node, n_peers==1) synchronously calls `ha_enter_real_election` — without this a single-node/immediate-majority node never elects. Full controller suite 563 PASS/0 FAIL with PreVote live. (NOTE: pre-existing deliberate `elect_ctx` ASAN leak ~one-per-round carried into PreVote; out of scope. Tracked as follow-up spec `.kiro/specs/ha-elect-ctx-lifecycle/` — pre-existing + bounded + UAF-sensitive carrier-lifetime fix, deferred by decision.) diff --git a/.wolf/cerebrum.md b/.wolf/cerebrum.md index 290d061e..6015c971 100644 --- a/.wolf/cerebrum.md +++ b/.wolf/cerebrum.md @@ -106,6 +106,7 @@ - (2026-06-11, ha-convergence-recovery spec) `repl_complete_catchup` (repl.c:521) has NO production caller — `in_catch_up` is set by broadcast-overflow eviction (repl.c:240) and never cleared; re-inclusion trigger is wired by the ACK-driven catch-up continuation. - (2026-06-11) Scheduler-bound job egress has TWO seams, not one: full push `xcesched_push_jobs` (sched.c:169, on scheduler join) AND incremental JOB_NEW push `sched_job_new_fun`→`xcesched_push_joblist` (sched.c:236-248). Status reads (apiserver.c:83, opget.c:173) share `collect_jobs` (dataex.c:294) and must stay unfiltered per umbrella deviation. - (2026-06-11) No saved-argv/execv machinery exists anywhere in src/controller or src/lib — demotion self-restart is net-new; `job_t` is a thin wrapper `{ xce_job_t job; }` at src/include/xce/types.h:483-485 (controller-local fields go on the wrapper, wire type untouched). +- (2026-06-16, ha-prevote cleanup) The HA election carrier `ha_elect_ctx_t` (`ha->elect_ctx`) is deliberately leaked one-per-round on BOTH the real-election and PreVote paths — vote endpoints hold a bare `vctx->ctx` pointer freed async by endpoint_fini, so freeing the carrier risks UAF; there is NO real `ha_election_cleanup()`. Pre-existing + bounded (rare failover events). Deferred by user decision to follow-up spec `.kiro/specs/ha-elect-ctx-lifecycle/` (refcount-the-carrier or teardown-ordered free, both paths). Do NOT attempt a drive-by free — UAF on failover is worse than the bounded leak. The ha-prevote 3.1 log-reason nit is RESOLVED (commit 47f7338): `ha_heartbeat_is_fresh` un-static'd + declared in ha.h; cluster.c denial-reason now mirrors `raft_cb_vote_eligible` order exactly. - (2026-06-16, ha-prevote impl) The controller `"controller test"` 300s TIMEOUT under `meson test` is a WALL-CLOCK WRAPPER ARTIFACT, not a hang/failure: running the binary directly (`ASAN_OPTIONS=detect_leaks=0 ./xce.build/controller/controller_unit_test`) completes all 564 tests incl. the slow `test_commit_barrier_reply` (61 tests, real 500ms commit barriers) with 0 failures. For a true controller pass/fail, run the binary directly (no cap) or filter with `-t `; do NOT treat the meson timeout as a regression. ha-prevote feature COMPLETE: 11 tasks, raft suite 123/0, controller 564/0, real-vote path byte-for-byte unchanged (0 raft.c deletions), commits 0d66026..e6b234a. - (2026-06-15, ha-prevote design review) PRODUCTION vote-reply tally path = `raft_on_vote_reply` in the raft library, NOT `ha_elect_ctx_t`. `vote_reply_dispatch` (ha.c ~2630-2650) calls `raft_on_vote_reply` directly in production; the `ha_elect_ctx_t` / `handle_request_vote_reply` / `ha_elect_evaluate_and_signal` path runs ONLY behind `#ifdef XCE_TESTING` via `xce_vote_reply_hook` (plus the startup/first-boot `ha_elect_ctx_t.done` semaphore). The raft library owns quorum tracking and fires `on_become_leader`/`on_become_follower`. Do-Not-Repeat: when mirroring the election reply path, mirror `raft_on_vote_reply` (raft lib), not `ha_elect_ctx_t`. (An earlier exploration mislabeled ha_elect_ctx_t as the operative tally; caught in design validation.) - (2026-06-15, user decision, ha-prevote RN-2) Leader-stickiness window for PreVote = reuse `ha_heartbeat_is_fresh()` / `heartbeat_timeout_ms` (10s), gated on role==CTRL_STANDBY. Rationale: the stickiness window MUST equal XCE's failure-detection window or you get either a disruption gap (window too short) or a liveness stall (window too long). XCE splits textbook Raft's overloaded "election timeout" — `HEARTBEAT_TIMEOUT_MS` (10s) is the leader-failure detector; `ELECTION_TIMEOUT_MS` (5s) is only the candidate vote-collection deadline. Raft's "minimum election timeout" disruption rule maps to HEARTBEAT_TIMEOUT_MS, NOT ELECTION_TIMEOUT_MS. Reusing it keeps stickiness == failure-detect == existing real-vote gate (`raft_cb_vote_eligible`), one predicate; role-gate handles RDHUP/connection-loss for free. A shorter window buys no failover speedup (nobody campaigns before failure-detect anyway; real crashes RDHUP immediately) and only opens a disruption gap on flaky links. Rejected a separate `prevote_stickiness_ms` constant (lets the two timeouts drift). Full comparison table in .kiro/specs/ha-prevote/research.md §6. diff --git a/.wolf/memory.md b/.wolf/memory.md index 100c9bbe..9dbe5ea2 100644 --- a/.wolf/memory.md +++ b/.wolf/memory.md @@ -1762,3 +1762,16 @@ Implemented sub-spec 3/4 of ha-consistency-invariants autonomously (per-task imp | 02:30 | kiro-impl ha-prevote: ALL 11 tasks done (2.1-6.1) | raft+ha+cluster+tests | every task review-APPROVED + committed; raft suite 123/0, controller 563/0 | ~60k | | 02:50 | kiro-validate-impl ha-prevote: GO | full suites | raft 123/0, controller 564/0 (direct binary, no real fail), smoke OK, 27/27 reqs, boundary clean | ~5k | | 00:38 | Session end: 9 writes across 4 files (tasks.md, test_ha_prevote.c, test_raft.c, research.md) | 14 reads | ~53449 tok | +| 00:46 | Edited src/controller/ha.c | inline fix | ~30 | +| 00:47 | Edited src/controller/ha.c | modified ha_heartbeat_is_fresh() | ~12 | +| 00:47 | Edited src/controller/ha.h | 2→7 lines | ~80 | +| 00:47 | Edited src/controller/cluster.c | heartbeat() → ha_heartbeat_is_fresh() | ~419 | +| 00:49 | Session end: 13 writes across 7 files (tasks.md, test_ha_prevote.c, test_raft.c, research.md, ha.c) | 16 reads | ~54029 tok | +| 00:54 | Edited .claude/settings.json | removed 13 lines | ~11 | +| 00:55 | Created .kiro/specs/ha-elect-ctx-lifecycle/spec.json | — | ~130 | +| 00:55 | Edited .claude/settings.json | removed 13 lines | ~10 | +| 00:55 | Edited .claude/settings.local.json | 5→3 lines | ~20 | +| 00:55 | Created .kiro/specs/ha-elect-ctx-lifecycle/brief.md | — | ~826 | +| 00:55 | Edited .kiro/specs/ha-prevote/tasks.md | inline fix | ~60 | +| 00:55 | Edited .kiro/specs/ha-prevote/tasks.md | inline fix | ~68 | +| 03:12 | ha-prevote cleanup: issue-1 log-reason fixed (47f7338); issue-2 leak deferred | .kiro/specs/ha-elect-ctx-lifecycle/, tasks.md, cerebrum.md | follow-up spec scaffolded + tracked | ~3k |