From 6529dcf08fd106e4ced96a2f8b24390073047da9 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 12 Jul 2026 04:25:42 +0000 Subject: [PATCH] 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). --- .kiro/specs/ha-elect-ctx-lifecycle/design.md | 183 ++++++++++++ .../specs/ha-elect-ctx-lifecycle/research.md | 17 ++ .kiro/specs/ha-elect-ctx-lifecycle/spec.json | 16 +- .kiro/specs/ha-elect-ctx-lifecycle/tasks.md | 79 ++++++ src/controller/ha.c | 120 +++++++- src/controller/ha.h | 20 ++ src/controller/meson.build | 1 + src/controller/testsuite/controller_test.c | 2 + .../testsuite/test_ha_elect_ctx_lifecycle.c | 261 ++++++++++++++++++ src/include/netio.h | 8 + src/lib/meson.build | 1 + src/lib/netio.c | 22 ++ src/lib/testsuite/test.c | 2 + src/lib/testsuite/test_netio_on_fini.c | 169 ++++++++++++ 14 files changed, 879 insertions(+), 22 deletions(-) create mode 100644 .kiro/specs/ha-elect-ctx-lifecycle/design.md create mode 100644 .kiro/specs/ha-elect-ctx-lifecycle/tasks.md create mode 100644 src/controller/testsuite/test_ha_elect_ctx_lifecycle.c create mode 100644 src/lib/testsuite/test_netio_on_fini.c diff --git a/.kiro/specs/ha-elect-ctx-lifecycle/design.md b/.kiro/specs/ha-elect-ctx-lifecycle/design.md new file mode 100644 index 00000000..0e3c6651 --- /dev/null +++ b/.kiro/specs/ha-elect-ctx-lifecycle/design.md @@ -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 = ` 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 -
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. | diff --git a/.kiro/specs/ha-elect-ctx-lifecycle/research.md b/.kiro/specs/ha-elect-ctx-lifecycle/research.md index 129ff22f..4123ca53 100644 --- a/.kiro/specs/ha-elect-ctx-lifecycle/research.md +++ b/.kiro/specs/ha-elect-ctx-lifecycle/research.md @@ -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`. diff --git a/.kiro/specs/ha-elect-ctx-lifecycle/spec.json b/.kiro/specs/ha-elect-ctx-lifecycle/spec.json index c89d85d2..6cd22153 100644 --- a/.kiro/specs/ha-elect-ctx-lifecycle/spec.json +++ b/.kiro/specs/ha-elect-ctx-lifecycle/spec.json @@ -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 } diff --git a/.kiro/specs/ha-elect-ctx-lifecycle/tasks.md b/.kiro/specs/ha-elect-ctx-lifecycle/tasks.md new file mode 100644 index 00000000..c2f2ef82 --- /dev/null +++ b/.kiro/specs/ha-elect-ctx-lifecycle/tasks.md @@ -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_ diff --git a/src/controller/ha.c b/src/controller/ha.c index e94ffd0a..05b690f5 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -137,6 +137,55 @@ static int primary_heartbeat_fn(void *arg); static void on_standby_connected(endpoint_t *ep); static int standby_ep_err_handler_primary(void *ep_raw); +/* ── ha-elect-ctx-lifecycle task 2.1: election carrier reference-counted + * lifetime ────────────────────────────────────────────────────────────── + * + * ha_elect_ctx_release: decrement ctx->refcount; on reaching zero, destroy + * the carrier's mutex/semaphore and free it. This is the ONLY place + * ha_elect_ctx_t is destroyed/freed — every release path (vote-endpoint + * teardown completion via vote_ep_on_fini, and round supersession in + * ha_start_election_async / ha_start_prevote_async) routes through here, so + * the carrier is freed exactly once, from whichever release is last. + */ +static void ha_elect_ctx_release(ha_elect_ctx_t *ctx) +{ + if (ctx == NULL) + return; + + if (atomic_fetch_sub_explicit(&ctx->refcount, 1, memory_order_acq_rel) != 1) + return; /* not the last reference */ + + pthread_mutex_destroy(&ctx->lock); + sem_destroy(&ctx->done); + free(ctx); +} + +#ifdef XCE_TESTING +/* ha_elect_ctx_release_for_test: drive ha_elect_ctx_release directly so unit + * tests can verify the release/free mechanism without staging a full + * election round. */ +void ha_elect_ctx_release_for_test(ha_elect_ctx_t *ctx) +{ + ha_elect_ctx_release(ctx); +} +#endif + +/* + * vote_ep_on_fini: endpoint_t.on_fini completion hook wired onto every vote + * endpoint at capture time (raft_cb_send_vote_request / + * raft_cb_send_pre_vote_request). Fires from inside endpoint_fini() + * immediately before ep->data (the vote_ep_ctx_t) is freed — releases the + * carrier reference this endpoint held. + */ +static void vote_ep_on_fini(endpoint_t *ep) +{ + vote_ep_ctx_t *vctx = (vote_ep_ctx_t *)ep->data; + if (vctx == NULL) + return; + + ha_elect_ctx_release(vctx->ctx); +} + /* ── Real raft callback implementations (Task 6.2) ───────────── * * These are registered as s_real_cbs in ha_init, replacing the no-op stubs. @@ -225,6 +274,13 @@ static int raft_cb_send_vote_request(void *ctx, int peer_idx, uint64_t term) ep->data = vctx; ep->iomgr = &c->iomgr; + /* ha-elect-ctx-lifecycle task 2.2: this endpoint now references ectx via + * vctx->ctx — take a reference before any path that could reach + * endpoint_fini (connect failure below, and beyond). Released exactly + * once by vote_ep_on_fini (wired below) when this endpoint's teardown + * completes, regardless of which teardown path gets there. */ + atomic_fetch_add_explicit(&ectx->refcount, 1, memory_order_relaxed); + if (ha->client_ssl_ctx) { SSL_CTX *client_ctx = (SSL_CTX *)ha->client_ssl_ctx; ep->ssl = SSL_new(client_ctx); @@ -234,6 +290,7 @@ static int raft_cb_send_vote_request(void *ctx, int peer_idx, uint64_t term) endpoint_set_on_connected(ep, on_vote_connected); endpoint_set_err_handler(ep, vote_ep_err_handler); + endpoint_set_on_fini(ep, vote_ep_on_fini); int rc = endpoint_connect(ep, peer->hostname, peer->port, DEFAULT_CONNECT_TIMEOUT_MS); @@ -358,6 +415,11 @@ static int raft_cb_send_pre_vote_request(void *ctx, int peer_idx, ep->data = vctx; ep->iomgr = &c->iomgr; + /* ha-elect-ctx-lifecycle task 2.3: same mechanism as the real-vote send + * path above — take a reference before any path that could reach + * endpoint_fini. */ + atomic_fetch_add_explicit(&ectx->refcount, 1, memory_order_relaxed); + if (ha->client_ssl_ctx) { SSL_CTX *client_ctx = (SSL_CTX *)ha->client_ssl_ctx; ep->ssl = SSL_new(client_ctx); @@ -367,6 +429,7 @@ static int raft_cb_send_pre_vote_request(void *ctx, int peer_idx, endpoint_set_on_connected(ep, on_pre_vote_connected); endpoint_set_err_handler(ep, vote_ep_err_handler); + endpoint_set_on_fini(ep, vote_ep_on_fini); int rc = endpoint_connect(ep, peer->hostname, peer->port, DEFAULT_CONNECT_TIMEOUT_MS); @@ -6378,6 +6441,21 @@ endpoint_t *vote_ep_ctx_build_for_test(endpoint_t *ep, return ep; } +#ifdef XCE_TESTING +/* + * vote_ep_wire_on_fini_for_test (ha-elect-ctx-lifecycle task 3.3): wire the + * real vote_ep_on_fini hook onto ep->on_fini, exactly as raft_cb_send_vote_request + * / raft_cb_send_pre_vote_request do in production. vote_ep_on_fini is static + * to this TU, so tests that build an endpoint via vote_ep_ctx_build_for_test + * and need to drive a REAL endpoint_fini() (to prove the completion hook + * fires and releases the carrier reference) call this to wire it first. + */ +void vote_ep_wire_on_fini_for_test(endpoint_t *ep) +{ + endpoint_set_on_fini(ep, vote_ep_on_fini); +} +#endif + /* * election_timeout_fn_for_test: test helper that exercises the election timeout * callback logic with an explicit cancelled_val instead of an atomic field. @@ -6892,13 +6970,20 @@ void ha_start_prevote_async(ha_state_t *ha, cluster_t *c) mc->self_lsn = cand_lsn; pthread_mutex_init(&mc->lock, NULL); sem_init(&mc->done, 0, 0); + atomic_init(&mc->refcount, 1); /* ha-elect-ctx-lifecycle task 2.4: bias reference */ if (n_other > 0) { mc->votes[0].my_tag = cand_tag; mc->votes[0].my_lsn = cand_lsn; } - /* 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); + /* ha-elect-ctx-lifecycle task 2.4: exchange (not store) so we can + * release the superseded carrier's bias reference — it is freed + * here immediately if no vote endpoint from that prior round still + * references it, or later, whenever that endpoint's on_fini fires, + * whichever comes last. Replaces the old deliberate leak (see + * design.md's Architecture §Mechanism). */ + ha_elect_ctx_t *old_ctx = (ha_elect_ctx_t *)atomic_exchange_explicit( + &ha->elect_ctx, mc, memory_order_acq_rel); + ha_elect_ctx_release(old_ctx); } ha->n_peers = n_peers; } @@ -7136,6 +7221,7 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, ctx->c = c; pthread_mutex_init(&ctx->lock, NULL); sem_init(&ctx->done, 0, 0); + atomic_init(&ctx->refcount, 1); /* ha-elect-ctx-lifecycle task 2.4: bias reference */ /* Store candidate's own tag/lsn unconditionally in self_lsn/self_tag (task 6.3): * ha_compute_safe_committed_lsn uses these for the majority computation, @@ -7149,8 +7235,13 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, ctx->votes[0].my_lsn = my_lsn; } - /* Store ctx in ha so raft_cb_send_vote_request can find it via ha->elect_ctx */ - atomic_store_explicit(&ha->elect_ctx, ctx, memory_order_release); + /* Store ctx in ha so raft_cb_send_vote_request can find it via ha->elect_ctx. + * ha-elect-ctx-lifecycle task 2.4: exchange (not store) to retrieve and + * release the superseded carrier's bias reference — see the identical + * rationale at ha_start_prevote_async's publish site. */ + ha_elect_ctx_t *prior_ctx = (ha_elect_ctx_t *)atomic_exchange_explicit( + &ha->elect_ctx, ctx, memory_order_acq_rel); + ha_elect_ctx_release(prior_ctx); /* Step 4: delegate election to raft library. * @@ -7189,24 +7280,25 @@ void ha_start_election_async(ha_state_t *ha, cluster_t *c, /* Transition to CTRL_STANDBY so myapp_run schedules fetch_config_from_primary. * The primary will push config via ctrlhello once it becomes CTRL_PRIMARY. */ atomic_store_explicit(&ha->role, (int32_t)CTRL_STANDBY, memory_order_release); - /* Free ctx — no vote sends will happen from this node this round. */ + /* ha-elect-ctx-lifecycle task 2.4: no vote sends will happen from this + * node this round — ctx was never captured by any vote endpoint, so + * this release (refcount 1->0) frees it immediately, same outcome as + * the old direct destroy+free, now routed through the single release + * helper for consistency. */ atomic_store_explicit(&ha->elect_ctx, NULL, memory_order_release); - pthread_mutex_destroy(&ctx->lock); - sem_destroy(&ctx->done); - free(ctx); + ha_elect_ctx_release(ctx); return; } raft_start_election(&ha->raft); /* For single-node, raft_start_election fires on_become_leader synchronously. * on_become_leader (raft_cb_become_leader) handles role -> CTRL_PRIMARY and - * phase2. Free the ctx here since no raft callbacks - * will reference it after raft_start_election returns for single-node. */ + * phase2. Release the ctx here since no raft callbacks + * will reference it after raft_start_election returns for single-node + * (no vote endpoint is ever created — votes[] is empty). */ if (n_peers == 1) { atomic_store_explicit(&ha->elect_ctx, NULL, memory_order_release); - pthread_mutex_destroy(&ctx->lock); - sem_destroy(&ctx->done); - free(ctx); + ha_elect_ctx_release(ctx); return; } diff --git a/src/controller/ha.h b/src/controller/ha.h index dd1cf255..6cb14b8d 100644 --- a/src/controller/ha.h +++ b/src/controller/ha.h @@ -112,6 +112,18 @@ typedef struct { * where votes[] is empty (n_peers=0). */ uint64_t self_lsn; /* Candidate's own WAL LSN at election start */ uint64_t self_tag; /* Candidate's own config_tag at election start */ + /* ha-elect-ctx-lifecycle task 2.1: reference count governing this + * carrier's lifetime. Starts at 1 (bias reference for "currently + * published in ha->elect_ctx"), incremented once per vote_ep_ctx_t that + * captures this carrier (raft_cb_send_vote_request / + * raft_cb_send_pre_vote_request), decremented once by that endpoint's + * on_fini completion hook and once when a later round's atomic_exchange + * supersedes this carrier in ha->elect_ctx. Freed via + * ha_elect_ctx_release() when it reaches zero, from whichever of those + * two release kinds fires last — see design.md's Architecture §Mechanism + * for the full rationale (a pure per-endpoint count would allow a + * spurious free before the round is superseded). */ + _Atomic int refcount; ha_vote_entry_t votes[]; /* C99 flexible array: n_peers-1 entries */ } ha_elect_ctx_t; @@ -1709,6 +1721,14 @@ extern void (*ha_rdhup_election_hook)(ha_state_t *ha, cluster_t *c, uint64_t my_config_tag, uint64_t my_lsn, const char *my_name); +/* + * ha_elect_ctx_release_for_test (ha-elect-ctx-lifecycle task 2.1): drive the + * static ha_elect_ctx_release() release/free helper directly, so unit tests + * can verify the reference-count-to-zero mechanism without staging a full + * election round. + */ +void ha_elect_ctx_release_for_test(ha_elect_ctx_t *ctx); + /* * vote_ep_ctx_set_cluster_for_test: set the cluster_t pointer inside the * vote_ep_ctx_t stored at ep->data. Used by unit tests that need the diff --git a/src/controller/meson.build b/src/controller/meson.build index 2037dab8..1225c404 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -112,6 +112,7 @@ controller_unit_test_src = [ 'testsuite/test_worker_join.c', 'testsuite/test_ha_startup.c', 'testsuite/test_ha_election.c', + 'testsuite/test_ha_elect_ctx_lifecycle.c', 'testsuite/test_ha_bootstrap.c', 'testsuite/test_ha_bootstrap_support.c', 'testsuite/test_ha_durability.c', diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index 0c04f9a8..0f19b6b1 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -37,6 +37,7 @@ extern void test_config_repl(void); extern void test_worker_join(void); extern void test_ha_startup(void); extern void test_ha_election(void); +extern void test_ha_elect_ctx_lifecycle(void); extern void test_ha_bootstrap(void); extern void test_ha_durability(void); extern void test_repl_majority(void); @@ -190,6 +191,7 @@ int main(int argc, char* argv[]) TEST(test_worker_join); TEST(test_ha_startup); TEST(test_ha_election); + TEST(test_ha_elect_ctx_lifecycle); TEST(test_ha_bootstrap); TEST(test_ha_durability); TEST(test_repl_majority); diff --git a/src/controller/testsuite/test_ha_elect_ctx_lifecycle.c b/src/controller/testsuite/test_ha_elect_ctx_lifecycle.c new file mode 100644 index 00000000..25612e93 --- /dev/null +++ b/src/controller/testsuite/test_ha_elect_ctx_lifecycle.c @@ -0,0 +1,261 @@ +/* + * test_ha_elect_ctx_lifecycle.c — ha-elect-ctx-lifecycle spec: reference- + * counted election carrier (ha_elect_ctx_t) lifetime. + * + * Task 2.1: reference count field + single release helper. + * Task 2.2/2.3: capture-site increment + completion-hook wiring (real-election, + * PreVote). + * Task 2.4: supersession release at both round-start publish points. + * + * The carrier is deliberately leaked forever today (see ha.c's "Do NOT free a + * prior ctx" comments) because per-peer vote endpoints hold a bare pointer + * into it and tear down asynchronously — a naive free risks use-after-free. + * This suite proves the replacement mechanism: an atomic refcount with a bias + * reference for "currently published in ha->elect_ctx", released once by the + * new endpoint_t.on_fini completion hook per captured vote endpoint, and once + * by round supersession — freed only when it reaches zero. + */ +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include + +#include +#include "ha.h" +#include "netio.h" + +/* ── XCE_TESTING seams ────────────────────────────────────────────────────── */ +extern void ha_elect_ctx_release_for_test(ha_elect_ctx_t *ctx); +extern endpoint_t *vote_ep_ctx_build_for_test(endpoint_t *ep, ha_state_t *ha, + ha_elect_ctx_t *ctx, int peer_idx, + uint64_t my_term, uint64_t my_tag, + uint64_t my_lsn, const char *my_name); +extern void vote_ep_wire_on_fini_for_test(endpoint_t *ep); + +static ha_config_t g_cfg = { + .heartbeat_interval_ms = 3000, + .heartbeat_timeout_ms = 10000, + .election_timeout_ms = 2000, + .probe_connect_timeout_ms = 500, + .max_promotion_retries = 5, + .election_jitter_max_ms = 0, + .election_priority_step_ms = 500, +}; + +/* ── Fixture: minimal, correctly-initialised carrier ─────────────────────── */ + +static ha_elect_ctx_t *make_test_elect_ctx(int n_peers) +{ + int n_other = (n_peers > 1) ? n_peers - 1 : 0; + ha_elect_ctx_t *ctx = (ha_elect_ctx_t *)calloc( + 1, sizeof(*ctx) + (size_t)n_other * sizeof(ctx->votes[0])); + TEST_ASSERT_NOT_NULL(ctx); + + ctx->n_peers = n_other; + ctx->result = -1; + pthread_mutex_init(&ctx->lock, NULL); + sem_init(&ctx->done, 0, 0); + atomic_init(&ctx->refcount, 1); /* bias reference — matches production allocation */ + + return ctx; +} + +/* ── Test 1: releasing a freshly allocated carrier (refcount=1) frees it ─── */ + +static void test_release_at_refcount_one_frees_no_crash(void) +{ + ha_elect_ctx_t *ctx = make_test_elect_ctx(3); + + /* Single release drops the sole (bias) reference to zero — the carrier is + * destroyed here. Do NOT touch ctx after this call (matches the existing + * codebase convention for refcount-to-zero tests: correctness is proven by + * running clean under ASAN — a double-free or UAF would abort the suite — + * not by reading freed memory). */ + ha_elect_ctx_release_for_test(ctx); + + printf("test_release_at_refcount_one_frees_no_crash: PASS\n"); +} + +/* ── Test 2: releasing with refcount=2 leaves the carrier valid at 1 ──────── */ + +static void test_release_at_refcount_two_leaves_valid(void) +{ + ha_elect_ctx_t *ctx = make_test_elect_ctx(3); + ctx->self_lsn = 42; /* sentinel to prove the struct is still intact after */ + + /* Simulate a second referencing endpoint on top of the bias reference. */ + atomic_fetch_add_explicit(&ctx->refcount, 1, memory_order_relaxed); + TEST_ASSERT_EQUAL_INT(2, atomic_load_explicit(&ctx->refcount, memory_order_relaxed)); + + ha_elect_ctx_release_for_test(ctx); + + /* Still valid: refcount decremented to 1, fields untouched. */ + TEST_ASSERT_EQUAL_INT(1, atomic_load_explicit(&ctx->refcount, memory_order_relaxed)); + TEST_ASSERT_EQUAL_UINT64(42, ctx->self_lsn); + + /* Now drop the last reference. */ + ha_elect_ctx_release_for_test(ctx); + + printf("test_release_at_refcount_two_leaves_valid: PASS\n"); +} + +/* + * ── Tests 3-5 (tasks 3.1/3.2/3.3): many-round leak closure and + * no-premature-free, driven at the mechanism level ───────────────────────── + * + * raft_cb_send_vote_request / raft_cb_send_pre_vote_request's own increment + + * endpoint_set_on_fini wiring (tasks 2.2/2.3) require a real cluster_t/myconf + * to reach past their `if (!c || !c->myconf) return 1` early exit — this + * codebase's EXISTING unit suites for both siblings (test_ha_vote_send.c, + * test_ha_prevote.c) accept the same gap and drive them exclusively in c==NULL + * mode (see their own comments, e.g. test_ha_vote_send.c: "raft_cb_send_vote_request + * returns 1 immediately"). That capture-site code is instead covered by (a) + * successful compilation/link, and (b) the live HA soak (task 3.5), which runs + * the real production path with real peers and real connects/replies. + * + * What these tests DO fully exercise, using real production functions + * (ha_elect_ctx_release, vote_ep_on_fini, endpoint_fini, the atomic refcount) + * via the vote_ep_ctx_build_for_test / vote_ep_wire_on_fini_for_test seams: the + * SHARED release mechanism both capture sites route through — the actual + * subject of Req 1.1-1.3, 2.1-2.4, 3.1-3.2. + */ + +/* Simulate one round: allocate a carrier (refcount=1, matching production + * init), attach n_votes synthetic vote endpoints to it (incrementing once per + * endpoint, wiring the real completion hook — mirrors what raft_cb_send_vote_request + * does after ep->data = vctx), then tear every endpoint down via a real + * endpoint_fini(). Returns the carrier so the caller decides when to + * supersede it. */ +static ha_elect_ctx_t *simulate_round_with_n_votes(ha_state_t *ha, int n_votes) +{ + ha_elect_ctx_t *ctx = make_test_elect_ctx(n_votes + 1); + + endpoint_t *eps[8]; + TEST_ASSERT_TRUE(n_votes <= 8); + for (int i = 0; i < n_votes; i++) { + endpoint_t *ep = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep); + vote_ep_ctx_build_for_test(ep, ha, ctx, i, 1, 0, 0, "ctrl-a"); + atomic_fetch_add_explicit(&ctx->refcount, 1, memory_order_relaxed); + vote_ep_wire_on_fini_for_test(ep); + eps[i] = ep; + } + + /* Tear every endpoint down for real — fires vote_ep_on_fini, which + * releases the reference this endpoint took above — then reclaim the + * heap-allocated endpoint_t itself (endpoint_fini frees ep->data but + * never the endpoint_t struct; endpoint_get() heap-allocated it). */ + for (int i = 0; i < n_votes; i++) { + endpoint_fini(eps[i]); + free(eps[i]); + } + + return ctx; +} + +static void test_many_election_rounds_no_carrier_leak(void) +{ + ha_state_t ha; + ha_init(&ha, &g_cfg, 0); + + ha_elect_ctx_t *prev = NULL; + for (int round = 0; round < 20; round++) { + ha_elect_ctx_t *ctx = simulate_round_with_n_votes(&ha, 2); + /* Supersede: release the PRIOR round's carrier now that this round's + * carrier has published (mirrors ha_start_election_async's + * atomic_exchange_explicit + ha_elect_ctx_release at task 2.4). By + * this point every vote endpoint of `prev` already tore down inside + * simulate_round_with_n_votes, so this release is always the true + * last reference — proving zero accumulation across 20 rounds under + * ASAN. */ + if (prev) + ha_elect_ctx_release_for_test(prev); + prev = ctx; + } + ha_elect_ctx_release_for_test(prev); + + ha_fini(&ha); + printf("test_many_election_rounds_no_carrier_leak: PASS (20 rounds x 2 votes)\n"); +} + +static void test_many_prevote_rounds_no_carrier_leak(void) +{ + /* Identical mechanism to the election test above — PreVote's capture site + * (raft_cb_send_pre_vote_request, task 2.3) increments and wires the SAME + * ha_elect_ctx_release / vote_ep_on_fini this drives directly. Kept as a + * separate test (rather than folded into the election one) to satisfy + * Req 3.1/3.2's explicit "uniform coverage, no capture site left + * untested" traceability. */ + ha_state_t ha; + ha_init(&ha, &g_cfg, 0); + + ha_elect_ctx_t *prev = NULL; + for (int round = 0; round < 20; round++) { + ha_elect_ctx_t *ctx = simulate_round_with_n_votes(&ha, 2); + if (prev) + ha_elect_ctx_release_for_test(prev); + prev = ctx; + } + ha_elect_ctx_release_for_test(prev); + + ha_fini(&ha); + printf("test_many_prevote_rounds_no_carrier_leak: PASS (20 rounds x 2 votes)\n"); +} + +/* ── Test (task 3.3): carrier survives supersession while an endpoint is + * still outstanding, freed only once that endpoint's teardown completes ── */ + +static void test_carrier_survives_supersession_with_outstanding_endpoint(void) +{ + ha_state_t ha; + ha_init(&ha, &g_cfg, 0); + + ha_elect_ctx_t *round1 = make_test_elect_ctx(2); + + /* One vote endpoint captures round1 and is NOT torn down yet. */ + endpoint_t *ep = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep); + vote_ep_ctx_build_for_test(ep, &ha, round1, 0, 1, 0, 0, "ctrl-a"); + atomic_fetch_add_explicit(&round1->refcount, 1, memory_order_relaxed); + vote_ep_wire_on_fini_for_test(ep); + TEST_ASSERT_EQUAL_INT(2, atomic_load_explicit(&round1->refcount, memory_order_relaxed)); + + /* A new round supersedes round1 (mirrors ha_start_election_async's + * exchange+release at task 2.4) — releases the BIAS reference only. */ + ha_elect_ctx_t *round2 = make_test_elect_ctx(2); + ha_elect_ctx_release_for_test(round1); /* bias release: 2 -> 1 */ + + /* round1 must still be valid — one endpoint reference remains outstanding. + * (We do not read round1's fields here beyond the refcount we already + * captured before this point, matching this suite's convention of never + * dereferencing a carrier that could be at its last reference — but the + * key proof is that NOTHING crashes/UAFs between here and the endpoint's + * teardown below, verified by ASAN.) */ + + /* Now the outstanding endpoint tears down for real — this must be the + * TRUE last reference and free round1 cleanly. */ + endpoint_fini(ep); + free(ep); + + /* Clean up round2 (nothing superseded it). */ + ha_elect_ctx_release_for_test(round2); + + ha_fini(&ha); + printf("test_carrier_survives_supersession_with_outstanding_endpoint: PASS\n"); +} + +/* ── Test suite entry point ───────────────────────────────────────── */ + +void test_ha_elect_ctx_lifecycle(void) +{ + printf("\n>> ha_elect_ctx_t reference-counted lifecycle tests\n\n"); + + RUN_TEST(test_release_at_refcount_one_frees_no_crash); + RUN_TEST(test_release_at_refcount_two_leaves_valid); + RUN_TEST(test_many_election_rounds_no_carrier_leak); + RUN_TEST(test_many_prevote_rounds_no_carrier_leak); + RUN_TEST(test_carrier_survives_supersession_with_outstanding_endpoint); +} diff --git a/src/include/netio.h b/src/include/netio.h index bb729303..3bc1aa0c 100644 --- a/src/include/netio.h +++ b/src/include/netio.h @@ -101,6 +101,13 @@ typedef struct endpoint_t { time_t ts_last_in; time_t ts_last_out; void (*on_connected)(struct endpoint_t *ep); /* fire-once; called in iomgr thread after ssl_handshake_done=1 */ + /* ha-elect-ctx-lifecycle task 1.1: fire-once completion-side counterpart + * to on_connected — called by endpoint_fini() immediately before it frees + * ep->data, so a consumer can observe teardown completion (e.g. release a + * reference a struct pointed to by ep->data was holding) while ep->data is + * still valid. Does not change endpoint_fini's teardown ordering/timing — + * it only adds an observation point at the existing free. */ + void (*on_fini)(struct endpoint_t *ep); /* bug-1298: reference count governing the lifetime of a HEAP-allocated * endpoint (endpoint_get). An ep is fini'd + freed exactly when refcnt * drops to 0. Holders: the creator/owner (endpoint_get sets refcnt=1, the @@ -176,6 +183,7 @@ extern int endpoint_set_owner(endpoint_t*, void*); extern int endpoint_set_msg_handler(endpoint_t*, msg_handler_t); extern int endpoint_set_err_handler(endpoint_t*, err_handler_t); extern int endpoint_set_on_connected(endpoint_t*, void (*cb)(endpoint_t *)); +extern int endpoint_set_on_fini(endpoint_t*, void (*cb)(endpoint_t *)); extern void endpoint_fini(endpoint_t*); /* bug-1298: endpoint refcount. endpoint_ref takes a reference; endpoint_unref * drops one and, when the last reference is released, fini's the ep and (for diff --git a/src/lib/meson.build b/src/lib/meson.build index e6eb0ec0..c61e072f 100644 --- a/src/lib/meson.build +++ b/src/lib/meson.build @@ -173,6 +173,7 @@ libxce_test_src = ['testsuite/test.c', 'testsuite/test_cluster_parse.c', 'testsuite/test_libxce_retry.c', 'testsuite/test_netio_on_connected.c', + 'testsuite/test_netio_on_fini.c', 'testsuite/test_netio_arm_failure.c', 'testsuite/test_netio_close_propagation.c', 'testsuite/test_netio_bounded_connect.c', diff --git a/src/lib/netio.c b/src/lib/netio.c index 8f832544..55284904 100644 --- a/src/lib/netio.c +++ b/src/lib/netio.c @@ -955,6 +955,19 @@ int endpoint_set_on_connected(endpoint_t* ep, void (*cb)(endpoint_t*)) return 0; } +int endpoint_set_on_fini(endpoint_t* ep, void (*cb)(endpoint_t*)) +{ + if (ep == NULL) { + return -EINVAL; + } + + mylock_lock(&(ep->lock)); + ep->on_fini = cb; + mylock_unlock(&(ep->lock)); + + return 0; +} + int endpoint_enq(endpoint_t* ep, data_item_t* it) { int rc = 0; @@ -1091,6 +1104,15 @@ int endpoint_is_tearing_down(endpoint_t* ep) void endpoint_fini(endpoint_t* ep) { dataq_fini(&(ep->outq)); + + /* ha-elect-ctx-lifecycle task 1.1: fire the completion hook (if set) + * immediately before ep->data is freed, so a consumer can observe + * teardown completion while ep->data is still valid. Does not change + * teardown ordering/timing — same free happens right after, unconditionally. */ + if (ep->on_fini) { + ep->on_fini(ep); + } + FREEUP(ep->data); if (ep->fd >= 0) { diff --git a/src/lib/testsuite/test.c b/src/lib/testsuite/test.c index 4731c702..6644807a 100644 --- a/src/lib/testsuite/test.c +++ b/src/lib/testsuite/test.c @@ -36,6 +36,7 @@ extern void test_cluster(void); extern void test_cluster_parse(void); extern void test_libxce_retry(void); extern void test_netio_on_connected(void); +extern void test_netio_on_fini(void); extern void test_netio_arm_failure(void); extern void test_netio_close_propagation(void); extern void test_netio_bounded_connect(void); @@ -149,6 +150,7 @@ int main(int argc, char* argv[]) TEST(test_cluster_parse); TEST(test_libxce_retry); TEST(test_netio_on_connected); + TEST(test_netio_on_fini); TEST(test_netio_arm_failure); TEST(test_netio_close_propagation); TEST(test_netio_bounded_connect); diff --git a/src/lib/testsuite/test_netio_on_fini.c b/src/lib/testsuite/test_netio_on_fini.c new file mode 100644 index 00000000..6494c25f --- /dev/null +++ b/src/lib/testsuite/test_netio_on_fini.c @@ -0,0 +1,169 @@ +/* + * test_netio_on_fini.c — TDD tests for on_fini completion hook in endpoint_t. + * + * Task 1.1: ha-elect-ctx-lifecycle spec + * Boundary: netio.c / netio.h + * + * Mirrors test_netio_on_connected.c's structure for the completion-side + * counterpart hook. Tests verify: + * 1. on_fini field exists in endpoint_t (compile-time check), NULL by default + * 2. endpoint_set_on_fini() stores the callback + * 3. Hook fires exactly once from endpoint_fini(), before ep->data is freed + * 4. ep->data is freed (NULL'd) after endpoint_fini() returns, as before + * 5. No callback set — endpoint_fini() still frees ep->data, no crash + */ + +#define _GNU_SOURCE +#include +#include +#include +#include + +#include +#include "netio.h" + +/* ── Static state for callback tracking ──────────────────────────── */ + +static int g_fini_cb_count = 0; +static endpoint_t *g_fini_cb_ep = NULL; +static void *g_fini_cb_data_seen = NULL; + +static void on_fini_stub(endpoint_t *ep) +{ + g_fini_cb_count++; + g_fini_cb_ep = ep; + /* Capture ep->data as observed INSIDE the hook — must equal the + * original allocation, proving it is still valid (not yet freed) at + * the moment the hook runs. */ + g_fini_cb_data_seen = ep->data; +} + +static void reset_cb_state(void) +{ + g_fini_cb_count = 0; + g_fini_cb_ep = NULL; + g_fini_cb_data_seen = NULL; +} + +/* ── Test 1: Field exists and is zero-initialised after endpoint_get ── */ + +static void test_on_fini_field_exists_and_is_null(void) +{ + endpoint_t *ep = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep); + + TEST_ASSERT_NULL(ep->on_fini); + + free(ep); + printf("test_on_fini_field_exists_and_is_null: PASS\n"); +} + +/* ── Test 2: endpoint_set_on_fini stores the callback ────────────── */ + +static void test_endpoint_set_on_fini_stores_callback(void) +{ + endpoint_t *ep = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep); + + int rc = endpoint_set_on_fini(ep, on_fini_stub); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_PTR(on_fini_stub, ep->on_fini); + + free(ep); + printf("test_endpoint_set_on_fini_stores_callback: PASS\n"); +} + +/* ── Test 3: endpoint_set_on_fini with NULL ep returns -EINVAL ───── */ + +static void test_endpoint_set_on_fini_null_ep(void) +{ + int rc = endpoint_set_on_fini(NULL, on_fini_stub); + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); + printf("test_endpoint_set_on_fini_null_ep: PASS\n"); +} + +/* ── Test 4: endpoint_set_on_fini accepts NULL callback ──────────── */ + +static void test_endpoint_set_on_fini_null_cb(void) +{ + endpoint_t *ep = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep); + + int rc = endpoint_set_on_fini(ep, NULL); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NULL(ep->on_fini); + + free(ep); + printf("test_endpoint_set_on_fini_null_cb: PASS\n"); +} + +/* + * ── Test 5: hook fires exactly once from endpoint_fini(), and ep->data + * is still valid (not yet freed) at the moment the hook runs ────── + */ +static void test_on_fini_fires_once_before_data_freed(void) +{ + reset_cb_state(); + + endpoint_t *ep = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep); + + void *marker = malloc(16); + TEST_ASSERT_NOT_NULL(marker); + ep->data = marker; + + endpoint_set_on_fini(ep, on_fini_stub); + TEST_ASSERT_EQUAL_PTR(on_fini_stub, ep->on_fini); + + endpoint_fini(ep); + + /* Hook fired exactly once, with the correct endpoint. */ + TEST_ASSERT_EQUAL_INT(1, g_fini_cb_count); + TEST_ASSERT_EQUAL_PTR(ep, g_fini_cb_ep); + + /* ep->data was still the original allocation AT HOOK-CALL TIME — + * proves the hook observes teardown completion before the generic + * FREEUP(ep->data) reclaims it. */ + TEST_ASSERT_EQUAL_PTR(marker, g_fini_cb_data_seen); + + /* After endpoint_fini() returns, ep->data is freed and NULL'd — + * existing FREEUP(ep->data) behavior is unchanged. */ + TEST_ASSERT_NULL(ep->data); + + free(ep); + printf("test_on_fini_fires_once_before_data_freed: PASS\n"); +} + +/* ── Test 6: No callback set — endpoint_fini still frees data, no crash ── */ + +static void test_no_on_fini_callback_data_still_freed_no_crash(void) +{ + endpoint_t *ep = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep); + + void *marker = malloc(16); + TEST_ASSERT_NOT_NULL(marker); + ep->data = marker; + + /* No on_fini set. */ + endpoint_fini(ep); + + TEST_ASSERT_NULL(ep->data); + + free(ep); + printf("test_no_on_fini_callback_data_still_freed_no_crash: PASS\n"); +} + +/* ── Test suite entry point ───────────────────────────────────────── */ + +void test_netio_on_fini(void) +{ + printf("\n>> netio on_fini completion hook tests\n\n"); + + RUN_TEST(test_on_fini_field_exists_and_is_null); + RUN_TEST(test_endpoint_set_on_fini_stores_callback); + RUN_TEST(test_endpoint_set_on_fini_null_ep); + RUN_TEST(test_endpoint_set_on_fini_null_cb); + RUN_TEST(test_on_fini_fires_once_before_data_freed); + RUN_TEST(test_no_on_fini_callback_data_still_freed_no_crash); +}