feat(ha): reference-counted ha_elect_ctx_t lifecycle (ha-elect-ctx-lifecycle)
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).
This commit is contained in:
parent
58e047f533
commit
6529dcf08f
14 changed files with 879 additions and 22 deletions
183
.kiro/specs/ha-elect-ctx-lifecycle/design.md
Normal file
183
.kiro/specs/ha-elect-ctx-lifecycle/design.md
Normal file
|
|
@ -0,0 +1,183 @@
|
|||
# Design Document: ha-elect-ctx-lifecycle
|
||||
|
||||
## Overview
|
||||
|
||||
**Purpose**: Give the HA election carrier (`ha_elect_ctx_t`) a real, safe lifetime instead of the current deliberate per-round leak, without touching election/PreVote/failover semantics.
|
||||
|
||||
**Users**: Controller maintainers (leak is invisible to operators today — ASAN's `detect_leaks=0` default hides it — but it accumulates one carrier per failover/election/PreVote round for the life of the process).
|
||||
|
||||
**Impact**: Adds a reference count to `ha_elect_ctx_t` and one new generic completion hook to `endpoint_t` (`on_fini`, mirroring the existing `on_connected` fire-once hook). No change to when elections start, how votes are tallied, or how endpoints are torn down — this design only observes the moment a vote endpoint's teardown actually completes and releases the carrier reference that endpoint held.
|
||||
|
||||
### Goals
|
||||
- Carrier is freed exactly once, exactly when no vote endpoint can still reference it (Req 1).
|
||||
- Freeing can never race a live endpoint's read of the carrier (Req 2).
|
||||
- One mechanism, applied identically to the real-election path (`raft_cb_send_vote_request` / `ha_start_election_async`) and the PreVote path (`raft_cb_send_pre_vote_request` / `ha_start_prevote_async`) (Req 3).
|
||||
- Zero observable change to vote tallying, freshness gates, or failover timing (Req 4).
|
||||
|
||||
### Non-Goals
|
||||
- Changing `endpoint_fini`, `vote_ep_teardown_async`, or any teardown *ordering/timing* — this design adds an observation point, not a new teardown path.
|
||||
- Changing the PreVote protocol, `ha_elect_ctx_t`'s field contents, or vote-tally logic.
|
||||
- A general-purpose object-lifetime framework. The mechanism here is specific to this one carrier; it is not offered as reusable infrastructure for other structs.
|
||||
|
||||
## Boundary Commitments
|
||||
|
||||
### This Spec Owns
|
||||
- The reference count field on `ha_elect_ctx_t` and its increment/decrement discipline.
|
||||
- The two capture sites that must increment (`raft_cb_send_vote_request`, `raft_cb_send_pre_vote_request`).
|
||||
- The one release path that must decrement (vote-endpoint fini completion) and the one release path that must decrement on round supersession (`ha_start_election_async`, `ha_start_prevote_async` overwriting `ha->elect_ctx`).
|
||||
- The new `endpoint_t.on_fini` hook's existence and its firing point inside `endpoint_fini` (one field, one conditional call — not a new subsystem).
|
||||
|
||||
### Out of Boundary
|
||||
- The PreVote protocol itself (`ha-prevote`, shipped).
|
||||
- Any change to election, vote-granting, or failover semantics.
|
||||
- `endpoint_fini`'s existing teardown mechanics (`FREEUP(ep->data)`, socket/SSL/packet cleanup) — untouched, just preceded by one optional hook call.
|
||||
- The async endpoint-teardown *ordering* itself (`vote_ep_teardown_async`, `endpoint_mark_tearing_down`, the bug-1197 deferred-vtask path) — this design fires after that ordering completes, whatever path got there.
|
||||
- The bug-1298 endpoint `refcnt` mechanism — a different lifetime (the `endpoint_t` itself) from the one this spec adds (the `ha_elect_ctx_t` carrier a vote endpoint's `ep->data` points into). The two refcounts are independent and must not be conflated.
|
||||
|
||||
### Allowed Dependencies
|
||||
- `endpoint_t` (`src/include/netio.h`, `src/lib/netio.c`) — may add one field + one call site, may not change existing field semantics.
|
||||
- `ha_elect_ctx_t`, `vote_ep_ctx_t` (`src/controller/ha.h`, `src/controller/ha.c`) — internal to `ha.c`/`ha.h`, this spec's primary work area.
|
||||
- Existing atomics discipline on `ha->elect_ctx` (`atomic_store_explicit` / `atomic_load_explicit`, `memory_order_release` / `memory_order_acquire`) — extended (one site moves to `atomic_exchange_explicit` to retrieve the superseded pointer), not replaced.
|
||||
|
||||
### Revalidation Triggers
|
||||
- Any new site that assigns `ep->data = <vote_ep_ctx_t*>` and wires up a vote endpoint outside the two existing capture sites (would need the same increment + hook wiring).
|
||||
- Any change to `endpoint_fini`'s call sequence that could reorder `FREEUP(ep->data)` relative to the new `on_fini` hook.
|
||||
- Any future feature that gives `endpoint_t.on_fini` a second consumer (must confirm the two consumers cannot conflict on the same endpoint).
|
||||
- Any change to the single-node / no-local-config fast-exit paths in `ha_start_election_async` that currently `free(ctx)` directly without ever creating a vote endpoint (they must keep doing so — see Architecture).
|
||||
|
||||
## Architecture
|
||||
|
||||
### Existing Architecture Analysis
|
||||
|
||||
Two sibling capture functions allocate a fresh `ha_elect_ctx_t` each round and publish it via `atomic_store_explicit(&ha->elect_ctx, ..., memory_order_release)`:
|
||||
|
||||
- `ha_start_election_async` (real election) has **two fast-exit paths that already free the ctx correctly**: no-local-config bail-out and the single-node case (`n_peers == 1`, `raft_start_election` resolves synchronously via `on_become_leader`). Neither path ever creates a vote endpoint, so the ctx is never shared — a direct `free(ctx)` is correct today and stays correct; this design does not touch them.
|
||||
- The **multi-node path** is where the leak lives: `raft_start_election` fires `on_send_vote_request` per peer, each call routing through `raft_cb_send_vote_request`, which `calloc`s a `vote_ep_ctx_t` (`vctx`), sets `vctx->ctx = ectx`, attaches it as `ep->data`, and hands the endpoint to iomgr. `ha_start_prevote_async` → `raft_cb_send_pre_vote_request` is the structurally identical PreVote sibling.
|
||||
- `vctx` is freed exactly once, generically, inside `endpoint_fini` (`src/lib/netio.c:1094`, `FREEUP(ep->data)`) — reached via `endpoint_unref` (bug-1298 refcount hitting 0) on every teardown path: connect failure, `iomgr_monitor` failure, the flag-OFF loser-path loop (`ha.c:~1553`), the flag-ON deferred-vtask path (`vote_ep_teardown_async` → `vote_ep_teardown_vtask_entry`), and RDHUP/err-handler paths. This is deliberately the **single** place `vctx` memory is reclaimed; the design must not duplicate that free, only observe it.
|
||||
- The carrier itself is currently **never** freed for the multi-node case: the next round's `atomic_store_explicit(&ha->elect_ctx, mc, ...)` (both the election and PreVote sites) silently discards the previous pointer, with a comment explicitly documenting the leak as deliberate (UAF avoidance).
|
||||
|
||||
### Mechanism
|
||||
|
||||
Reference-count the carrier. One bias reference represents "this is (or was, pending drain) the published `ha->elect_ctx` value"; one reference per live `vote_ep_ctx_t` that captured it.
|
||||
|
||||
```
|
||||
ha_elect_ctx_t.refcount (new, _Atomic int, starts at 1)
|
||||
|
||||
+1 at capture: raft_cb_send_vote_request / raft_cb_send_pre_vote_request,
|
||||
at the point ep->data = vctx is committed (i.e. only for vctx instances
|
||||
that actually reach a live endpoint — the pre-endpoint_get() alloc-fail
|
||||
bailouts never increment, so they need no matching decrement)
|
||||
|
||||
-1 at vote-endpoint completion: new endpoint_t.on_fini(ep) hook, fired by
|
||||
endpoint_fini() immediately before FREEUP(ep->data), for every teardown
|
||||
path (connect-fail, iomgr_monitor-fail, loser-path unref, deferred
|
||||
vtask teardown, RDHUP/err-handler) — all of them funnel through
|
||||
endpoint_fini exactly once (bug-1298 invariant), so this is the single
|
||||
correct observation point regardless of which path got there
|
||||
|
||||
-1 at supersession: ha_start_election_async / ha_start_prevote_async, at
|
||||
the atomic_store_explicit(&ha->elect_ctx, new_ctx, ...) site — changed
|
||||
to atomic_exchange_explicit to retrieve the outgoing pointer, then
|
||||
release it (decrement; free at 0) instead of discarding it
|
||||
|
||||
free when refcount reaches 0 (pthread_mutex_destroy + sem_destroy + free),
|
||||
regardless of which of the two release kinds above is the last one to fire
|
||||
```
|
||||
|
||||
Why a bias reference is required (not pure per-endpoint counting): between `atomic_store_explicit(&ha->elect_ctx, ctx, ...)` and the first `raft_cb_send_vote_request` callback actually running, the carrier has zero vote-endpoint references yet but is already live and readable (`ha.c:123-124`, `234-235`, `903-904` read `ha->elect_ctx` without assuming any endpoint exists — e.g. single-vote-outstanding or mid-fan-out windows). A pure "count of attached endpoints" would allow a spurious free at refcount 0 before the round is superseded. The bias reference removes that window: the carrier is only reclaimed once it is *both* superseded *and* drained of endpoint references, in whichever order those two events occur.
|
||||
|
||||
Why a generic `endpoint_t.on_fini` hook (not per-call-site decrements at every `endpoint_unref`/teardown call): a `vctx` is only *actually freed* inside `endpoint_fini`, but `endpoint_unref` (which may or may not trigger `endpoint_fini` synchronously, depending on whether a pool vtask still holds a ref) is called from roughly a dozen sites in `ha.c` across connect-failure, iomgr-failure, the loser-path loop, and the bug-1197 deferred-teardown-vtask path. Decrementing at each call site would require knowing, at every one of those dozen sites, whether this call is the one that actually drops the count to zero — which duplicates `endpoint_fini`'s own refcount logic and is exactly the kind of coupling Req 2.3/2.4 (uniform coverage of every free path, "including error and abort paths") is trying to make robust against drift. A single hook fired at the one true free point is correct by construction for every current and future teardown path, mirroring the existing `on_connected` fire-once hook already on `endpoint_t`.
|
||||
|
||||
### Architecture Pattern & Boundary Map
|
||||
|
||||
```mermaid
|
||||
sequenceDiagram
|
||||
participant ES as ElectionStart (election or PreVote)
|
||||
participant CB as SendVoteRequest callback
|
||||
participant EP as VoteEndpoint
|
||||
participant FI as endpoint_fini
|
||||
|
||||
ES->>ES: allocate carrier, refcount = 1
|
||||
ES->>ES: exchange published carrier for new one, get old carrier
|
||||
ES->>ES: release old carrier (decrement, free if zero)
|
||||
ES->>CB: start election, fan out per peer
|
||||
CB->>CB: record carrier on vctx, increment refcount
|
||||
CB->>EP: attach vctx as endpoint data, set on_fini hook
|
||||
Note over EP: connect, vote RPC, reply, or error -<br/>any teardown path funnels here
|
||||
EP->>FI: endpoint_fini runs once refcount reaches zero
|
||||
FI->>FI: call on_fini hook if set (decrement carrier refcount, free if zero)
|
||||
FI->>FI: free endpoint data (existing behavior, unchanged)
|
||||
```
|
||||
|
||||
**Selected pattern**: reference counting with a bias reference, observed via a generic completion hook — same shape as the existing bug-1298 `endpoint_t.refcnt` mechanism, applied one level up (the carrier a vote endpoint points into, not the endpoint itself).
|
||||
**Domain boundaries**: carrier lifetime lives entirely in `ha.c`/`ha.h` (internal type, per research.md); the only cross-boundary touch is the one generic hook field added to `endpoint_t`.
|
||||
**Existing patterns preserved**: `endpoint_fini` remains the single `vctx` free site; `atomic_store_explicit(..., memory_order_release)` / `atomic_load_explicit(..., memory_order_acquire)` discipline on `ha->elect_ctx` is preserved (the supersession site becomes `atomic_exchange_explicit`, same memory order semantics, just also yielding the old value instead of discarding it).
|
||||
**New components rationale**: `on_fini` hook — needed because no existing hook fires at endpoint-teardown completion; `refcount` field — needed because the leak is a multi-referrer lifetime problem, not a single-owner one.
|
||||
**Steering compliance**: no thread-role change — the hook fires on whatever thread already calls `endpoint_fini` today (iomgr thread or pool worker, per bug-1197's deferred-teardown-vtask path); the decrement/free is O(1) non-blocking (atomic decrement + conditional `free()`), so it introduces no new blocking work on any thread class.
|
||||
|
||||
## File Structure Plan
|
||||
|
||||
### Modified Files
|
||||
- `src/include/netio.h` — add `void (*on_fini)(struct endpoint_t *ep);` field to `endpoint_t`, adjacent to the existing `on_connected` field, with a doc comment cross-referencing it as the completion-side counterpart.
|
||||
- `src/lib/netio.c` — in `endpoint_fini`, add `if (ep->data && ep->on_fini) ep->on_fini(ep);` immediately before the existing `FREEUP(ep->data)` line. No other change to the function.
|
||||
- `src/controller/ha.h` — add `_Atomic int refcount;` to `ha_elect_ctx_t` (next to the existing `done_posted` guard field, same struct region).
|
||||
- `src/controller/ha.c`:
|
||||
- New static helper `ha_elect_ctx_release(ha_elect_ctx_t *ctx)` — decrement; on reaching 0, `pthread_mutex_destroy` + `sem_destroy` + `free`. Single choke point for both release kinds (endpoint completion, supersession).
|
||||
- New static hook function `vote_ep_on_fini(endpoint_t *ep)` — reads `vctx = ep->data`, calls `ha_elect_ctx_release(vctx->ctx)`. Wired as `ep->on_fini` at both capture sites, same place `endpoint_set_err_handler` is already called.
|
||||
- `raft_cb_send_vote_request`: initialize `ectx->refcount` is NOT done here (ctx already exists, allocated earlier) — this site only increments (`atomic_fetch_add_explicit(&ectx->refcount, 1, memory_order_relaxed)`) at the point `ep->data = vctx` is committed, and sets `ep->on_fini`.
|
||||
- `raft_cb_send_pre_vote_request`: identical increment + hook wiring at its `ep->data = vctx` site.
|
||||
- `ha_start_election_async`: at carrier allocation, initialize `ctx->refcount = 1`. At the multi-node publish site, change `atomic_store_explicit(&ha->elect_ctx, ctx, memory_order_release)` to `atomic_exchange_explicit(&ha->elect_ctx, ctx, memory_order_acq_rel)` capturing the return as `old_ctx`; if `old_ctx` non-NULL, call `ha_elect_ctx_release(old_ctx)`. The two existing fast-exit `free(ctx)` paths (no-local-config, single-node) are unchanged — they never share the ctx, so a direct free at refcount 1→destroy is still correct (or, for consistency and to avoid a second free idiom, route them through `ha_elect_ctx_release(ctx)` too — same result, one less code path to reason about).
|
||||
- `ha_start_prevote_async`: identical `refcount = 1` init at allocation; identical `atomic_exchange_explicit` + `ha_elect_ctx_release(old_ctx)` at its publish site (the "Do NOT free a prior ctx" comment site, `ha.c:~6899`), comment updated to describe the new mechanism instead of documenting the leak as deliberate.
|
||||
|
||||
### No New Files
|
||||
This is a localized extension to two existing files (`ha.c`/`ha.h`) plus a minimal, generic addition to the shared endpoint abstraction (`netio.h`/`netio.c`). No new translation units.
|
||||
|
||||
## System Flows
|
||||
|
||||
See the sequence diagram in Architecture above — it is the complete flow for both the real-election and PreVote paths (structurally identical, differing only in which capture function runs).
|
||||
|
||||
State transitions for `ha_elect_ctx_t.refcount` (per instance, monotonically decreasing after peak):
|
||||
|
||||
```mermaid
|
||||
stateDiagram-v2
|
||||
[*] --> Live: calloc, refcount=1 (bias)
|
||||
Live --> Live: vote-ep capture (+1) — 0..n_peers-1 times
|
||||
Live --> Live: vote-ep endpoint_fini completes (-1) — any order, any thread
|
||||
Live --> Superseded: next round's atomic_exchange (-1, bias released)
|
||||
Live --> Freed: refcount reaches 0 while still current (all endpoints drained before next round starts)
|
||||
Superseded --> Freed: last outstanding vote-ep drains (-1 reaches 0)
|
||||
Freed --> [*]
|
||||
```
|
||||
|
||||
## Requirements Traceability
|
||||
|
||||
| Requirement | Summary | Components | Interfaces | Flows |
|
||||
|-------------|---------|------------|------------|-------|
|
||||
| 1.1 | Reclaim once all referencing vote-eps freed | `ha_elect_ctx_release`, `refcount` field | — | refcount state diagram |
|
||||
| 1.2 | No bounded wait/timeout — driven by actual release | `vote_ep_on_fini` hook fired from `endpoint_fini` | `endpoint_t.on_fini` | sequence diagram |
|
||||
| 1.3 | Zero ASAN leak over many rounds under leak detection | full mechanism (all sites below) | — | validation: `--detect_leaks=1` soak, new test |
|
||||
| 2.1 | Never free while a vctx holds a reference | refcount discipline (+1 capture / -1 completion) | — | sequence diagram |
|
||||
| 2.2 | Carrier still valid at `endpoint_fini` read time | hook fires from inside `endpoint_fini`, before `FREEUP(ep->data)` — vctx itself (thus `vctx->ctx`) still valid at hook-call time | `endpoint_t.on_fini` firing order | sequence diagram |
|
||||
| 2.3 | Same mechanism at both capture sites | `raft_cb_send_vote_request`, `raft_cb_send_pre_vote_request` both increment + wire hook | — | Modified Files |
|
||||
| 2.4 | Same mechanism on every vctx free path, incl. error/abort | single hook fires from the one true `vctx` free site (`endpoint_fini`) regardless of which teardown path reached it | `endpoint_t.on_fini` | Architecture §Mechanism rationale |
|
||||
| 3.1 | Same lifetime mechanism for election and PreVote | identical `refcount`/`ha_elect_ctx_release`/`vote_ep_on_fini` reused by both paths | — | Modified Files |
|
||||
| 3.2 | Superseded PreVote round's carrier reclaimable like election's | `ha_start_prevote_async`'s publish site uses the same `atomic_exchange_explicit` + release pattern | — | state diagram |
|
||||
| 4.1 | No change to vote-tally/quorum/freshness behavior | mechanism touches only allocation/free bookkeeping, never `votes[]` contents or tally logic | — | — |
|
||||
| 4.2 | No change to election/PreVote/failover timing or outcome | hook/refcount work is O(1) non-blocking, off the liveness-critical send/reply path | — | Architecture §Steering compliance |
|
||||
| 4.3 | Full HA soak shows no new loss/divergence/stall | validation plan (Testing Strategy) | — | — |
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
- **New unit test — leak closure**: run N consecutive multi-node election (and separately, PreVote) rounds in the existing controller test harness under ASAN with leak detection **enabled** (distinct from the suite's current `detect_leaks=0` default, per research.md) and assert zero `ha_elect_ctx_t` bytes leaked. This is Req 1.3's direct acceptance test.
|
||||
- **New unit test — no premature free**: construct a round with an in-flight vote endpoint (captured, not yet torn down) and drive a supersession (start a new round); assert the carrier is still valid (not freed) while the outstanding endpoint's teardown has not yet completed, and becomes freed only after its `on_fini` fires. Directly tests Req 2.1/2.2.
|
||||
- **New unit test — both paths covered**: parametrize (or duplicate) the above two tests for `ha_start_prevote_async` / `raft_cb_send_pre_vote_request`, confirming Req 3.1/3.2 — no path left on the old leak-forever behavior.
|
||||
- **Regression — existing election/PreVote/failover suites**: run unchanged; must stay green with zero behavioral diff (Req 4.1/4.2). Any assertion change here would itself indicate a semantic regression.
|
||||
- **Live — full HA soak**: `/xce-test-ha` sequential and chaotic regression post-merge (Req 4.3), watching specifically for job loss, divergence, or a liveness stall newly attributable to this change (bug-339/bug-340 vote-freshness behavior must be bit-for-bit unaffected, since the fix touches allocation/free timing, not vote-freshness computation).
|
||||
|
||||
## Risks
|
||||
|
||||
| Risk | Mitigation |
|
||||
|------|------------|
|
||||
| A future second consumer of `endpoint_t.on_fini` on the same endpoint could silently override this one | Not applicable today (vote endpoints have exactly one hook consumer); flagged as a Revalidation Trigger above so a future feature adding a second hook consumer on a vote endpoint is forced to notice the conflict. |
|
||||
| Off-by-one in the increment/decrement pairing (e.g. incrementing at `vctx->ctx = ectx` instead of at `ep->data = vctx`) could reintroduce either a leak or a premature free | Increment is placed at the exact point the reference becomes "live" (attached to an endpoint that will reach `endpoint_fini`); the two pre-`endpoint_get()` alloc-fail bailouts never increment, matching that they never attach `ep->data`. Verified by the "no premature free" unit test above. |
|
||||
| Relaxed memory order on the increment (`memory_order_relaxed`) racing the release of the bias reference | The increment happens on the same thread that is about to hand the endpoint to iomgr for connect — happens-before is already established by the existing `ep->data = vctx` write and subsequent `endpoint_connect`/`iomgr_monitor` calls, which themselves use acquire/release on the endpoint's own `refcnt`. The carrier's `refcount` only needs atomicity (concurrent increments from a fan-out of N peers, concurrent decrements from N `endpoint_fini` calls on different threads), not ordering relative to carrier *contents* — the carrier's field contents are fully populated before the round's `atomic_store`/`atomic_exchange` publish (`memory_order_release`), which is the actual publish barrier readers rely on. |
|
||||
|
|
@ -37,3 +37,20 @@ The deliberate-leak pattern the brief describes is **still present and unfixed**
|
|||
- 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`.
|
||||
|
|
|
|||
|
|
@ -1,22 +1,22 @@
|
|||
{
|
||||
"feature_name": "ha-elect-ctx-lifecycle",
|
||||
"created_at": "2026-06-16T03:10:00Z",
|
||||
"updated_at": "2026-07-09T17:15:00Z",
|
||||
"updated_at": "2026-07-11T05:30:00Z",
|
||||
"language": "en",
|
||||
"phase": "requirements-generated",
|
||||
"phase": "tasks-generated",
|
||||
"approvals": {
|
||||
"requirements": {
|
||||
"generated": true,
|
||||
"approved": false
|
||||
"approved": true
|
||||
},
|
||||
"design": {
|
||||
"generated": false,
|
||||
"approved": false
|
||||
"generated": true,
|
||||
"approved": true
|
||||
},
|
||||
"tasks": {
|
||||
"generated": false,
|
||||
"approved": false
|
||||
"generated": true,
|
||||
"approved": true
|
||||
}
|
||||
},
|
||||
"ready_for_implementation": false
|
||||
"ready_for_implementation": true
|
||||
}
|
||||
|
|
|
|||
79
.kiro/specs/ha-elect-ctx-lifecycle/tasks.md
Normal file
79
.kiro/specs/ha-elect-ctx-lifecycle/tasks.md
Normal file
|
|
@ -0,0 +1,79 @@
|
|||
# Implementation Plan: ha-elect-ctx-lifecycle
|
||||
|
||||
- [x] 1. Foundation: endpoint teardown-completion hook
|
||||
- [x] 1.1 Add a generic completion hook to the endpoint abstraction, fired exactly once when an endpoint's teardown reaches its final free
|
||||
- Add an `on_fini` function pointer field to `endpoint_t` (src/include/netio.h), mirroring the existing `on_connected` fire-once hook
|
||||
- In `endpoint_fini` (src/lib/netio.c), call the hook (if set) immediately before the existing `FREEUP(ep->data)` line — no other change to the function's sequence
|
||||
- New unit test: set `on_fini` on a test endpoint, drive it through teardown, assert the hook fires exactly once and that `ep->data` is still valid (non-freed) at the moment the hook runs
|
||||
- Observable: the new unit test passes; every other existing endpoint (which never sets `on_fini`) tears down with zero behavioral change (hook is a no-op NULL check)
|
||||
- _Requirements: 2.2_
|
||||
|
||||
- [x] 2. Core: election carrier reference-counted lifecycle
|
||||
- [x] 2.1 Add a reference count to the election carrier and a single release helper
|
||||
- Add an `_Atomic int refcount` field to `ha_elect_ctx_t` (src/controller/ha.h)
|
||||
- Add a static helper in ha.c that decrements the refcount and, on reaching zero, destroys the mutex/semaphore and frees the carrier
|
||||
- New unit test: call the helper once on a freshly allocated carrier (refcount=1) and assert it is freed; call it once on a carrier with refcount=2 and assert it is still valid with refcount=1
|
||||
- Observable: the helper is the only place `ha_elect_ctx_t` is destroyed/freed in ha.c
|
||||
- _Requirements: 1.1_
|
||||
- _Boundary: ElectionCarrierLifecycle (ha.c/ha.h)_
|
||||
|
||||
- [x] 2.2 (P) Increment the carrier refcount and wire the completion hook at the real-election vote-request capture site
|
||||
- In `raft_cb_send_vote_request`, at the point `ep->data = vctx` is committed (only for vctx instances that reach a live endpoint), atomically increment the carrier's refcount and set `ep->on_fini` to a new hook function that releases the carrier via the 2.1 helper
|
||||
- The two pre-`endpoint_get()` alloc-fail bailouts in this function do not increment (they never attach `ep->data`) and need no matching release
|
||||
- Observable: after a multi-node election round runs to completion and every vote endpoint's teardown finishes, the carrier's refcount reflects only the still-outstanding supersession bias reference (no endpoint-attributable count remains)
|
||||
- _Requirements: 1.1, 2.1, 2.3, 2.4_
|
||||
- _Boundary: raft_cb_send_vote_request_
|
||||
- _Depends: 1.1, 2.1_
|
||||
|
||||
- [x] 2.3 (P) Increment the carrier refcount and wire the completion hook at the PreVote vote-request capture site
|
||||
- Same mechanism as 2.2, applied to `raft_cb_send_pre_vote_request`
|
||||
- Observable: identical post-round refcount behavior as 2.2, but for PreVote rounds
|
||||
- _Requirements: 1.1, 2.1, 2.3, 2.4, 3.1_
|
||||
- _Boundary: raft_cb_send_pre_vote_request_
|
||||
- _Depends: 1.1, 2.1_
|
||||
|
||||
- [x] 2.4 Release the superseded carrier when a new election or PreVote round publishes its own
|
||||
- In `ha_start_election_async` and `ha_start_prevote_async`, change the carrier-publish assignment from `atomic_store_explicit` to `atomic_exchange_explicit` to retrieve the outgoing carrier pointer, then release it via the 2.1 helper if non-NULL
|
||||
- Route the two existing no-vote-endpoint fast-exit paths in `ha_start_election_async` (no-local-config bail-out, single-node case) through the same 2.1 helper instead of a direct `free()`, for one consistent destruction path
|
||||
- Update the "Do NOT free a prior ctx" comment at the PreVote publish site to describe the new mechanism instead of documenting the leak as deliberate
|
||||
- Observable: starting a second round while the first round's carrier already has zero endpoint references frees the first round's carrier immediately at supersession; starting a second round while the first still has an outstanding vote endpoint leaves the first carrier alive until that endpoint's teardown completes
|
||||
- _Requirements: 1.1, 1.2, 3.1, 3.2_
|
||||
- _Boundary: ElectionCarrierLifecycle (ha.c publish sites)_
|
||||
- _Depends: 2.1, 2.2, 2.3_
|
||||
|
||||
- [x] 3. Validation: leak closure and premature-free safety
|
||||
- [x] 3.1 (P) Prove zero carrier leaks across many consecutive real-election rounds under leak-detection-enabled ASAN
|
||||
- New unit test drives N consecutive multi-node election rounds with ASAN leak detection enabled (distinct from the suite's current `detect_leaks=0` default)
|
||||
- Temporarily revert the 2.2/2.4 changes to confirm this test fails (leak reported) pre-fix, then restore and confirm it passes (RED→GREEN)
|
||||
- Observable: zero `ha_elect_ctx_t` bytes reported leaked across the run
|
||||
- _Requirements: 1.3_
|
||||
- _Boundary: election test coverage_
|
||||
- _Depends: 2.2, 2.4_
|
||||
|
||||
- [x] 3.2 (P) Prove zero carrier leaks across many consecutive PreVote rounds under leak-detection-enabled ASAN
|
||||
- Same shape as 3.1, driving PreVote rounds via `ha_start_prevote_async` instead
|
||||
- Observable: zero `ha_elect_ctx_t` bytes reported leaked across the run
|
||||
- _Requirements: 1.3, 3.1, 3.2_
|
||||
- _Boundary: PreVote test coverage_
|
||||
- _Depends: 2.3, 2.4_
|
||||
|
||||
- [x] 3.3 (P) Prove the carrier is never freed while an outstanding vote endpoint still references it across a round supersession
|
||||
- Construct a round with one captured, not-yet-torn-down vote endpoint, then start a new round (triggering supersession) before that endpoint's teardown completes
|
||||
- Assert the original carrier is still valid (not freed) immediately after supersession, and becomes freed only after the outstanding endpoint's `on_fini` fires
|
||||
- Covers both the real-election and PreVote capture paths (parametrized or duplicated per design's Testing Strategy)
|
||||
- Observable: a naive "free on supersession" implementation would fail this test (UAF or premature free caught by ASAN); the refcounted implementation passes it
|
||||
- _Requirements: 2.1, 2.2, 2.3, 3.1, 3.2_
|
||||
- _Boundary: no-premature-free test coverage_
|
||||
- _Depends: 2.2, 2.3, 2.4_
|
||||
|
||||
- [x] 3.4 Run the full existing controller unit suite and confirm no behavioral change
|
||||
- Run the complete `controller_unit_test` suite; assertions in pre-existing election, PreVote, and failover test files must be byte-for-byte unchanged
|
||||
- Observable: full suite green, zero modified assertions in pre-existing election/prevote/failover tests
|
||||
- _Requirements: 4.1, 4.2_
|
||||
- _Depends: 3.1, 3.2, 3.3_
|
||||
|
||||
- [x] 3.5 Run a full HA sequential and chaotic regression soak and confirm no new job loss, divergence, or liveness stall
|
||||
- Run `/xce-test-ha` sequential and chaotic regression against a live 3-node cluster
|
||||
- Observable: zero committed job loss, zero divergence, no liveness stall attributable to this change; bug-339/bug-340 vote-freshness behavior unaffected
|
||||
- _Requirements: 4.3_
|
||||
- _Depends: 3.4_
|
||||
Loading…
Add table
Add a link
Reference in a new issue