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

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
pg-fix-forwarder-reconnect	Fix forwarder permanent death after one failed reconnect (review 002 Finding 1)	completed		moderate	medium	component	implementation	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 continues — 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.