alkstore/tasks/pg-fix-forwarder-reconnect.md

---
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<Output =
  Option<GenerationPair>>` 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<Mutex<Config>>`) 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<Mutex<Option<
  Sender>>>`) 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.