--- id: pg-fix-forwarder-reconnect name: Fix forwarder permanent death after one failed reconnect (review 002 Finding 1) status: completed depends_on: [] scope: moderate risk: medium impact: component level: implementation tags: [wave-4-fixes, postgres-engine, forwarder] --- ## Description Fix the HIGH finding from the wave-4 general review (`docs/reviews/002-wave-4-general-review.md` §Finding 1, live-proven): the forwarder loop **dies permanently after one failed reconnect connect**. Site: `alkstore-postgres/src/forwarder.rs:498-509` → `:369`. On a failed `reconnect_config.connect(NoTls)` the loop's `Err` arm `continue`s — but the loop top is `let Some(poll_connection) = connection.take() else { return; }` and `connection` is still `None` (the old connection was consumed by `take()` when its generation started). One failed connect ⇒ the loop takes the `else` arm and the forwarder task exits forever. Consequences (all live-proven): every later `listen()` fails with a spurious "mid-reconnect" `Database` for the store's remaining lifetime; wake delivery stops permanently (no re-LISTEN, no reconnect-wake); stream subscribers parked on `wait_wake` stay parked forever (the fanout's senders stay alive — handle-held — so the broadcast never closes and the consumer never even sees a terminal arm), contradicting the streams row's pinned posture that *events never require polling to become visible* (core-contract.md streams section). **Fix shape** (the review's): retry the connect with the live backoff *inside* the reconnect arm — loop `sleep(backoff)` → `connect` until success or shutdown — never falling back into the `take()`'s `else return`. The loop top's `None` arm must become unreachable by construction (or restructured away entirely: e.g. carry the connection slot through the reconnect arm so a failed connect retries in place with the slot still empty but the loop alive). **Hardening (the review's second point)**: make every `forwarder_loop` exit path release the `Forwarder` struct's fanout sender (the shutdown-take machinery already exists — `Forwarder::shutdown` takes it) so *any* loop exit — not just the shutdown flip — surfaces the terminal `Closed`/`None` arm to subscribers instead of a forever-silence. Invariant to establish and pin: **receivers go terminal iff the forwarder loop is gone**. With the retry fix the loop only exits at shutdown, so this is defense-in-depth against the next abnormal-exit bug, not a behavior change. **Testability — the gap that hid the bug**: the gate's reconnect test (`open_tests.rs:244`) kills the backend via `pg_terminate_backend` while the server stays up — the 50 ms reconnect attempt connects successfully every time. The *unreachable-server window* (connect fails) is the one case the machinery exists for and the one case untested. The fix must come with a test that exercises it. Recommended shape: a `cfg(test)` seam on the reconnect config (the loop ctx's `reconnect_config` behind a swappable slot, or the connect step behind an injectable closure) so a test can point reconnects at an unreachable endpoint after `open`, force a reconnect (backend kill), verify the loop survives *repeated* failed connects across several backoff cycles, then swap the real config back and verify full recovery. Alternative: extract the connect step behind a connector trait/closure and unit-test the retry loop with a failing connector — implementer's choice, but the failed-connect arm must be test-exercised, not reasoned-about. ## Acceptance Criteria - [ ] A failed reconnect connect no longer exits the loop: the reconnect arm retries with the live backoff (50 ms → 2 s cap) until success or shutdown — pinned by a test that forces repeated failed connects (unreachable-endpoint seam or connector injection) across multiple backoff cycles - [ ] After the server becomes reachable again: the synthetic reconnect-wake broadcasts, post-reconnect `NOTIFY` delivers on registered channels, and `listen()` succeeds (no spurious "mid-reconnect" `Database`) — recovery pinned by test - [ ] Every `forwarder_loop` exit path releases the fanout sender — an abnormal loop exit surfaces the terminal arm to subscribers (the receivers-terminal-iff-loop-gone invariant; the shutdown case's existing pin stays green) - [ ] The existing backend-kill reconnect tests (`forwarder_reconnects_after_backend_kill_and_delivers`, `receiver_stays_open_across_reconnects_closes_at_shutdown`, the no-replay pin) stay green unchanged - [ ] `cargo test -p alkstore-postgres` (harness server), clippy `-D warnings`, fmt clean; gates green server-less ## References - docs/reviews/002-wave-4-general-review.md §Finding 1 (the finding, the probe, the fix shape) - docs/architecture/decisions/004-postgres-driver.md (forwarder decision, the hand-rolled posture's owned failure modes) - docs/architecture/decisions/006-wake-and-delivery-contract.md (reconnect-recovery semantics) - docs/architecture/core-contract.md (streams: events never require polling to become visible) - tasks/pg-engine-notify-listen.md (the forwarder's task of record; Notes carry the shutdown sender-take decision this hardening builds on) ## Notes > Decisions of record the implementation made that the description > didn't pin: - **The retry core is an extracted helper (`reconnect_with_retry`)** rather than an inline restructure: sleep → shutdown-select → attempt → repeat, generic over an `FnMut() -> Future>` attempt closure. It is only ever exited with a fresh generation pair (`Some`) or at the shutdown flip (`None`) — the loop top's empty-slot arm is restructured away entirely: `LoopCtx.connection` is no longer `Option`; the loop re-binds a plain loop-carried connection from the successful connect at the arm's bottom, so there is no fall-through path left to die on. - **The attempt closure is the connector injection** (the implementer's-choice seam, one mechanism for both shapes): the real closure clones `tokio_postgres::Config` out of a shared `ReconnectConfigSlot` (`Arc>`) and calls `Config::connect(NoTls)`, mapping errors to `None`; the server-less unit tests substitute their own always-failing closures. The slot is also the config-swap seam: `Forwarder::swap_reconnect_config` / `reconnect_config_snapshot`(`#[cfg(test)]`) let a test point reconnects at an unreachable endpoint post-open and restore the real one. The field is `#[cfg(test)]` on `Forwarder` (dead in prod otherwise — the loop reads its own slot clone); the shutdown watch's `changed()` is selected *with* the backoff sleep so a close surfaces the terminal arm mid-outage instead of waiting out a cycle. - **The fanout hardening rides a shared slot + drop guard**: the handle's sender became the shared `FanoutSlot` (`Arc>>`) the loop also receives, and the loop holds a `FanoutRelease` guard whose `Drop` empties the slot — Drop runs on return, unwind, and task abort alike, so *any* loop exit (plus `Forwarder::shutdown`'s take, unchanged) releases the fanout. `subscribe` reads the shared slot — a subscribe arriving past any loop exit fails closed (`None`) rather than parking on a forever-silent broadcast. receivers-terminal-iff-loop-gone is now enforced by construction, not just the shutdown path. - **Backoff reset semantics unchanged**: the cap/growth live in the helper (doubled per attempt, `saturating_mul(2)` → `min(2000)`); the reset to the 50 ms base still fires on the *verified-alive* generation (successful LISTEN/probe), so a connect that succeeds but dies before its LISTEN reconnects at the carried backoff. - **The success path of the retry helper has no server-less unit pin** (a real `(Client, Connection)` pair cannot be fabricated); it is pinned end-to-end by the harness recovery test. The unit pins carry: repeated failed connects across six backoff cycles with the cap, pre-flipped shutdown aborts with zero attempts, and the guard's drop-releases-slot mechanism. - `tokio` gained a `test-util` feature on the crate's `[dev-dependencies]` (feature-unified into test builds only) for the paused-time retry unit test — the gate battery runs deterministic and instant. - The bug was replay-proofed: with the old one-shot `Err → return` shape temporarily reintroduced, the new harness test (`forwarder_survives_failed_reconnects_and_recovers_fully`) fails as expected; with the fix it passes (verified both ways live). ## Summary > What landed, verified how: - **`alkstore-postgres/src/forwarder.rs`**: the forwarder loop restructured — the reconnect arm now carries `reconnect_with_retry` (sleep with live backoff → shutdown-arms → connect attempt, repeating until success or shutdown; failed connects never leave the arm), the loop-carried connection is a plain non-`Option` rebinding (the empty-slot `take() else return` that killed the loop permanently after one failed connect is gone by construction), and the shared `FanoutSlot` + `FanoutRelease` guard make every loop exit path release the fanout sender (receivers-terminal-iff-loop-gone). `LoopCtx`/`Forwarder::spawn` adjusted; the `cfg(test)` reconnect-config seam (swap/snapshot) added; subscribe/shutdown doc comments updated for the invariant. - **`alkstore-postgres/src/store/open_tests.rs`** (4 new tests): `forwarder_survives_failed_reconnects_and_recovers_fully` (harness: config swapped to unreachable → backend kill → repeated failed connects across ~4 backoff cycles mid-outage with the honest transient `register` error pinned → config restored → reconnect-wake + post-recovery NOTIFY delivery + working `listen()`), two server-less paused-time unit pins of the retry core (six failed attempts across six backoff cycles capped at 2 s, shutdown-abort before any attempt), and the fanout release guard's drop mechanism. - **`alkstore-postgres/Cargo.toml`**: `tokio` `test-util` added to `[dev-dependencies]` only. - **Gates**: `cargo test -p alkstore-postgres` with the harness server green (115 lib + 10 contract-suite + 9 schema, run twice full); every pre-existing forwarder/reconnect/no-replay/shutdown pin green unchanged; workspace `cargo build`, `cargo test` (server-less — new tests skip or run server-less cleanly), `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check` all green.