chore(ha-prevote): resolve log-reason nit; track elect_ctx leak as follow-up spec

- 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) <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-06-16 07:56:07 +00:00
parent 47f733891b
commit 5503c7a176
5 changed files with 100 additions and 2 deletions

View file

@ -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`.

View file

@ -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
}

View file

@ -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.)

View file

@ -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 <suite>`; 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.

View file

@ -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 |