Rongsong Shen
6529dcf08f
Replace the deliberate election-carrier leak with an atomic refcount: a bias reference held by ha->elect_ctx plus one reference per captured vote endpoint, released via a new endpoint_t.on_fini teardown-completion hook (fired once, before ep->data is freed). Both real-election and PreVote capture sites (raft_cb_send_vote_request, raft_cb_send_pre_vote_request) increment on capture; both publish sites (ha_start_election_async, ha_start_prevote_async) now atomic_exchange the outgoing carrier and release it instead of leaking or freeing outright, so a still-outstanding vote endpoint keeps the superseded carrier alive until its own teardown completes. Verified: 758+ controller tests green, isolated LeakSanitizer run shows zero ha_elect_ctx_t leaks across many election/PreVote rounds, no premature-free/UAF under supersession, and a live 10-round HA soak (36 jobs, zero loss, all 3 nodes served as primary).
56 lines
8.2 KiB
Markdown
56 lines
8.2 KiB
Markdown
# Research / Gap Analysis — ha-elect-ctx-lifecycle
|
||
|
||
## Gap Re-Audit (2026-06-18)
|
||
|
||
> **Prerequisite not met:** this spec has only `brief.md` — **no `requirements.md`** (phase=`initialized`, `ready_for_implementation=false`). A full requirements↔codebase gap map cannot be produced. Run `/kiro-spec-requirements ha-elect-ctx-lifecycle` to formalize EARS requirements, then re-run `/kiro-validate-gap ha-elect-ctx-lifecycle`. The note below is a **pre-requirements gap sketch** derived from `brief.md` + a current code check.
|
||
|
||
### Current State (verified 2026-06-18)
|
||
The deliberate-leak pattern the brief describes is **still present and unfixed** in `src/controller/ha.c`:
|
||
- `ha.c:4229` — "Do NOT free a prior ctx (live vote endpoints may hold a bare pointer to it…)" immediately before `atomic_store(&ha->elect_ctx, mc)`. Each election/PreVote round `calloc`s a fresh `ha_elect_ctx_t` and intentionally leaks the previous one.
|
||
- `ha.c:3978`, `ha.c:4830` — sibling "Do NOT free … may still reference / may already be running" comments on the async endpoint / delaymgr teardown paths (the same UAF caution).
|
||
- No `ha_election_cleanup()` exists (only the aspirational comment the brief cites). Carrier read sites: `ha.c:123-124`, `234-235`, `903-904`.
|
||
- Carrier captured by per-peer vote endpoints (`vote_ep_ctx_t.ctx = ectx`), freed asynchronously by `endpoint_fini` (FREEUP) on the iomgr thread — the root cause of the UAF hazard that makes a naïve free unsafe.
|
||
|
||
### Requirement-to-Asset Map (pre-requirements — tags: MET / Missing / Constraint)
|
||
| Intended capability (from brief) | Current code | Tag |
|
||
|----------------------------------|--------------|-----|
|
||
| Carrier (`ha_elect_ctx_t`) freed when no endpoint references it | leaked by design at `ha.c:4229` | **Missing** |
|
||
| Safe free vs async `endpoint_fini` (`vctx`) teardown | no refcount / no deferred-free ordering | **Missing** |
|
||
| Applies to BOTH real-election and PreVote carrier paths (shared `ha->elect_ctx`) | both paths leak identically | **Missing / Constraint** (fix one without the other leaves the hazard) |
|
||
| No regression of failover correctness (bug-339/bug-340) | current correctness intact; leak is memory-only | **Constraint** |
|
||
|
||
### Severity / Why deferred
|
||
- **Pre-existing + bounded**: one small struct (`sizeof(ha_elect_ctx_t) + n_peers*votes[]`, hundreds of bytes for 3 nodes) per *failover/PreVote event* — a rare event, not steady-state. No unbounded growth under normal operation.
|
||
- Surfaces as ~174 KB / ~1548 allocs under ASAN across the full controller suite's many simulated rounds; the test command uses `detect_leaks=0`, so it does not fail CI today.
|
||
|
||
### Implementation Approach Options (for the eventual design)
|
||
- **Option A — Reference-count the carrier.** Increment per `vctx` that captures it, decrement on each `vctx` free, free at zero. Clean, localized; needs an atomic refcount on `ha_elect_ctx_t` and disciplined inc/dec at every capture/free site. Effort **M**, Risk **Medium** (must cover every `vctx` lifecycle exit, incl. error paths).
|
||
- **Option B — Teardown-ordered deferred free.** Tie the free to confirmed vote-endpoint release (e.g. a generation/epoch the iomgr teardown signals back). Avoids per-capture bookkeeping but needs a reliable "all endpoints of round N released" signal. Effort **M–L**, Risk **Medium**.
|
||
- **Option C — Hybrid.** Refcount as the primitive, plus an epoch guard so a superseded round's carrier is reclaimed promptly once its endpoints drain. Effort **L**, Risk **Medium**.
|
||
|
||
### Research-Needed (carry to design)
|
||
- Enumerate **all** `vctx->ctx = ectx` capture sites and **all** `vctx` free sites (incl. `endpoint_fini` FREEUP, error/abort paths, `raft_cb_send_vote_request`, `ha_send_pre_vote_request`).
|
||
- Confirm the memory-order discipline for the refcount vs the existing `atomic_store(&ha->elect_ctx, …, release)` / acquire reads.
|
||
- Validation must run ASAN with **leak detection ENABLED** (success = zero carrier leaks across many rounds) — distinct from the current `detect_leaks=0` suite default.
|
||
|
||
### Effort / Risk (overall)
|
||
- Effort **M** (single-area C change in `ha.c`/`ha.h`), Risk **Medium** (async-teardown UAF is the real hazard; must not regress bug-339/bug-340 failover correctness).
|
||
|
||
**Next step:** `/kiro-spec-requirements ha-elect-ctx-lifecycle` → `/kiro-spec-design` → implement under leak-detection-enabled ASAN.
|
||
|
||
## Design Discovery (2026-07-11)
|
||
|
||
Requirements approved; re-verified the code against the 2026-06-18 sketch above (unchanged in the intervening weeks) and traced every capture/free site precisely:
|
||
|
||
- **Capture sites** (2, exactly as sketched): `raft_cb_send_vote_request` (`ha.c:~191`, `ep->data = vctx` at `~223`), `raft_cb_send_pre_vote_request` (`ha.c:~335`, `ep->data` assignment at its mirror site).
|
||
- **The single true `vctx` free site**: `endpoint_fini` (`src/lib/netio.c:1094`, `FREEUP(ep->data)`), reached via the existing bug-1298 `endpoint_t.refcnt` mechanism from every teardown path (connect-fail, `iomgr_monitor`-fail, the flag-OFF loser-path loop at `ha.c:~1553`, the flag-ON deferred-vtask path `vote_ep_teardown_async`, RDHUP/err-handler). Confirmed roughly a dozen `endpoint_unref`/`free(vctx)` call sites in `ha.c`, but only ~6 are vote-endpoint-related (the rest — `~8256`, `~8310`, `~8330`, `~8337`, `~8432`, `~8447` — are `reconnect_ctx_t`/`sctx` standby-reconnect endpoints, explicitly out of scope, confirmed by reading each in context: they null `ep->data` before unref and free their own `sctx` separately).
|
||
- **The supersession/leak point**: both `ha_start_election_async` (`ha.c:~7153`) and `ha_start_prevote_async` (`ha.c:~6899`, the exact "Do NOT free a prior ctx" comment cited in the 06-18 sketch) `atomic_store_explicit` a new carrier over the old one with no release.
|
||
- **Already-correct paths, confirmed out of scope**: `ha_start_election_async`'s no-local-config bail-out and single-node (`n_peers==1`) fast exits already `free(ctx)` directly and correctly — no vote endpoint is ever created in either case, so the carrier is never shared. These are not part of the leak and this design must not alter their correctness.
|
||
|
||
### Design Decisions (synthesis)
|
||
- **Generalization considered and rejected**: could have generalized to a reusable "refcounted heap object" utility. Rejected — exactly one struct (`ha_elect_ctx_t`) needs this in the codebase today; a generic utility would be speculative abstraction (YAGNI). Instead generalized the *interface* one notch: `endpoint_t.on_fini`, a single generic completion hook mirroring the existing `on_connected` fire-once hook, which naturally supports this consumer and any future one without new machinery.
|
||
- **Build vs. adopt**: the codebase already has a directly analogous, proven mechanism — the bug-1298 `endpoint_t.refcnt` (reference-counted endpoint lifetime, freed at 0). Adopted the *pattern* (atomic refcount + release-to-zero-frees) rather than the mechanism itself, since the two lifetimes are genuinely different objects (the endpoint vs. the carrier a vote endpoint's `ep->data` points into) — conflating them was considered and rejected (Option B/teardown-signal from the 06-18 sketch) because it would require the carrier to know about individual endpoint teardown ordering, whereas a hook fired at each endpoint's own completion is simpler and matches Req 2.4's "every free path" requirement by construction.
|
||
- **Simplification**: settled on Option A (refcount) over Option B (deferred/epoch signal) and Option C (hybrid) from the 06-18 sketch — a plain atomic refcount with a bias reference for the "currently published" slot is sufficient; no epoch/generation counter is needed because the bias reference already disambiguates "still current" from "superseded but draining."
|
||
- **Memory-order note carried to implementation**: the increment on capture can use `memory_order_relaxed` (only atomicity is required, not ordering relative to carrier contents — the carrier's fields are fully populated before the publishing `atomic_store`/`atomic_exchange`, which is the real publish barrier). The supersession release moves the publish site from `atomic_store_explicit` to `atomic_exchange_explicit` to retrieve the outgoing pointer without a second load (avoids a TOCTOU gap between reading the old pointer and overwriting it).
|
||
|
||
Full mechanism, file-by-file plan, and requirements traceability are in `design.md`.
|