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

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

id	name	status	depends_on	scope	risk	impact	level	tags
pg-fix-forwarder-hardening	Forwarder TOCTOU restructure + hang-shaped arms (review 004 items 4+6)	completed		broad	high	component	implementation	review-004-fixes postgres-engine forwarder

Description

Forwarder surgery that should ride its own session, per docs/reviews/004-pre-release-general-review.md §"Findings recorded, not fixed" items 4 and 6 (the review bundles them as the forwarder-hardening session). Line refs are as of the review; the inline fixes may have shifted them slightly — re-locate on pickup.

(a) Register/unregister TOCTOU (item 4). The registry refcount and the LISTEN/UNLISTEN command emission are not one critical section (forwarder.rs:131-176 vs 438-482): interleaved register/unregister across tasks can leave a live refcount with the server-side LISTEN cleared (both queued commands share a generation, so no stale check saves it) until the next reconnect re-issue. Fix shape: a decide-and-queue restructure inside the forwarder — the register/unregister decision and the derived command enqueue must be atomic with respect to each other (decide under the registry guard; queue the command with the refcount state it was derived from).

(b) Generation-verify/LISTEN failure parks forever (item 6, arm 1). A failed generation-verify/LISTEN batch is treated as connection death and parks on poll_task.await forever even though the session may be alive (forwarder.rs:698-702/803-808).

(c) Shutdown can leak the listener session (item 6, arm 2). close()/Drop can leak the listener session past shutdown when the serve loop is awaiting a command on a black-holed connection (the run_command await sits outside the shutdown select, :781; no timeout on this connection).

Treatment family for (b)+(c): timeouts/bounded waits on the listener path.

Preserve while restructuring (the gate will check these): the two POC-pinned deadlock pitfalls stay structurally excluded; the reconnect invariants from pg-fix-forwarder-reconnect (the retry core, the fanout release guard, receivers-terminal-iff-loop-gone) stay green; the register-failure unregister fix from review 004 (inline fix 3) stays; the poison-recovery lock idiom from review 004 (inline fix 4) stays.

Use the existing test-seam vocabulary (the reconnect-config slot, the connector injection, the generation tests) for the new pins.

Acceptance Criteria

• The TOCTOU is closed: register/unregister interleavings across tasks cannot leave a live refcount with the server-side LISTEN cleared — pinned by a test that interleaves the two and asserts the server-side LISTEN state matches the refcount
• A failed generation-verify/LISTEN batch no longer parks forever: bounded wait with an honest error or reconnect — pinned by test
• close()/Drop reclaims the listener session even when the serve loop is stuck awaiting on a dead connection — pinned by test (black-holed connection seam)
• The preserved postures listed above re-verified green (existing forwarder pins: reconnect survival, no-replay, shutdown close, stale-UNLISTEN generation tests, the register-failure unregister pin)
• cargo test -p alkstore-postgres (harness server), clippy -D warnings, fmt clean; workspace gates green server-less

References

• docs/reviews/004-pre-release-general-review.md §"Findings recorded, not fixed" items 4 and 6 (the findings, the fix shapes, the own-session scoping)
• docs/architecture/decisions/004-postgres-driver.md (the forwarder's owned failure modes, the two POC-pinned deadlock pitfalls)
• tasks/pg-fix-forwarder-reconnect.md (the retry core, the fanout guard, the test-seam vocabulary)
• tasks/pg-fix-stale-unlisten.md (the generation/staleness machinery the TOCTOU restructure must compose with)

Notes

Decisions of record the implementation made that the description didn't pin:

• The real interleaving window was thread preemption, not a yield. Locating the TOCTOU on pickup: the old register (add → connected check → send) and unregister (remove → send) each had no await between their decide and their send, so the split is not an interleave point under tokio's cooperative scheduling within one thread — but the multi-thread runtime's worker threads preempt mid-unit, so thread-A's decide can land, thread-B's whole unit runs (decide + send), and thread-A resumes and sends its stale-contrary command. That is the review item 4's shape, and it quiesces persistently: registry count 1 with the queue [L, U] (UNLISTEN replayed after the live registration's LISTEN). The fix makes decide+send one critical section under the registry guard, so the queue order is the registry transitions' linearization.
• Every registration keeps its own (redundant) LISTEN + ack. A shape that skipped the redundant LISTEN for shared (non-new) refcounts was considered and rejected: it would let a later listen() return Ok while the first registration's LISTEN still sat queued — breaking the "a listen() that returned sees every later commit / starts from now" contract for that caller. The redundant per-registration LISTEN is the ack's vehicle; semantics preserved exactly (documented in enqueue_register).
• The decide-and-queue lives on ChannelSet (enqueue_register / enqueue_unregister), not on Forwarder: the registry guard hosts the critical section; the connected read and the generation-tag load ride the same section; the ack receiver is returned so the caller awaits outside the guard (never a std lock across an await). The old standalone add_subscriber/remove_subscriber were folded into the under-guard decisions and deleted — the refcount unit pin (channel_set_refcounts_last_subscriber_rule) was reworked to drive the decide-and-queue API (now it also pins that each registration derives its own LISTEN, and the mid-reconnect Err-with-entry-stays posture).
• One bounded-wait primitive for the whole treatment family (bounded_wait: output / bound expiry / shutdown flip, biased toward shutdown): the verify batch, the replay commands, the serve commands, and the failed-verify settle all ride it. Constants: LISTENER_QUERY_TIMEOUT (5 s) and LISTEN_SETTLE_TIMEOUT (2 s) — generous against a merely-slow server (an overrun re-issues from the snapshot on reconnect; nothing is lost either way; the settle sits far above real death-detection latency, which surfaces in poll_message at once).
• Death postures, two shapes. (1) A verify overrun/failure runs the bounded settle: the poll task (the reaper) gets LISTEN_SETTLE_TIMEOUT; an unsettled reaper is aborted — the connection closes with it (an aborted settle's socket close absorbs the reaper's job). (2) A serve/replay command overrun is treated as honest connection death directly (abort + fall to the reconnect), not via a second settle wait — the unresponsive connection is already proven; the reaper is aborted immediately and the reconnect arm proceeds. Every bounded await selects the shutdown watch alongside (biased), so close() surfaces through any stuck listener-path await — the review item 6 arm 2's hostage shape is closed at all of its await sites (the review cited only the serve arm's run_command; the verify batch and the generation-top replay had the same shape and got the same treatment family).
• first_listen flip moved to the verify's success (before the drain/replay): a generation that dies mid-replay keeps the wake for the next generation's top (a replay-death generation never broadcasts its own wake). The existing exactly-one-wake pin (LISTEN-window death) stays green — re-derived against the new order.
• (c)'s live harness shape is a TCP black-hole proxy seam (the task's "black-holed connection seam"): a local pass-through TCP proxy in front of the harness server (the store's DSN points at it); engagement eats server→client bytes (reads hang forever, both TCP ends up — the one failure shape backend kills cannot reach: alive, attached, unresponsive). An eaten-bytes counter pins that the serve loop is demonstrably parked inside the UNLISTEN await before close(), and the reclaim is measured server-side by the store's exact application_name vanishing from pg_stat_activity (the proxy propagates the client's close to the backend). No fabricatable alternatives exist for the connection types (the Client/Connection pair cannot be faked), and OS-level freezes (SIGSTOP) cannot reach dockerized harness backends — the network layer is the testable seam. LISTENER_QUERY_TIMEOUT, LISTEN_SETTLE_TIMEOUT, bounded_wait, and BoundedOutcome are pub(crate).
• (b)'s harness-level live arrangement is not separately added — the stale-unlisten precedent's bar applies ("the unit pin is the acceptance bar"): the failed-verify settle is pinned server-less in paused time (a wedged reaper bounded + aborted + cancelled, a promptly-dying reaper reaped, a close surfacing through the settle promptly), and the genuinely-dead-verify path through the real loop stays covered by the existing LISTEN-window-death harness test (forwarder_recovers_from_connection_death_during_the_listen_window), which reaps under the bound and recovers unchanged.
• The stale-generation machinery (tags, counter bump, the generation-top drain, the recv-arm staleness guard), the retry core, the fanout release guard, the register-failure unregister discipline, and the poison-recovery lock idiom are all preserved — re-verified green (existing pins' full battery in Gates).
• Replay-proofs (both ways, verified live): (1) with the old decide/send split temporarily reintroduced in enqueue_unregister/enqueue_register (a widened post-decision window), the interleave pin fails on exactly the review's assert (registry/model divergence — a live refcount at a cleared server-side LISTEN); with the fix restored it passes instantly and deterministically (the finale anchors on the observable decision — the entry vanishing — so a commit-time run never flirts with the timing). (2) with the serve arm's bounded await temporarily reverted to the old unbounded run_command await, the black-hole harness test fails on "close must reclaim the listener session…" (the session leaked past close, as reviewed); with the fix restored it reclaims (2.4 s wall, well inside the 5 s command bound + the 15 s observation window).

Summary

What landed, verified how:

• alkstore-postgres/src/forwarder.rs: (a) the TOCTOU — ChannelSet gained the decide-and-queue critical sections (enqueue_register: refcount + connected-check + acked LISTEN enqueue under one registry guard; enqueue_unregister: last-subscriber decision + UNLISTEN enqueue under one guard); Forwarder::register/unregister ride them (the ack await is outside the guard); the standalone refcount helpers folded away; the module/method docs state the invariant (the queue's order is the registry transitions' linearization; a live refcount can never sit against a cleared server-side LISTEN). (b) the failed-verify settle — the old unbounded poll_task.await is replaced by the bounded settle (LISTEN_SETTLE_TIMEOUT=2 s, shutdown-selected): reaped fast when the session is genuinely dead, aborted at the bound (the connection closes with the reaper) when it is alive-but-unusable, then the reconnect either way. (c) the close-time leak — a new bounded_wait primitive bounds every listener-client query (5 s) with the shutdown watch selected alongside: the verify batch, the generation-top replay commands, and the serve loop's LISTEN/UNLISTEN commands; an overrun is the honest death posture (abort + reconnect; the snapshot re-issue recovers, the stalled LISTEN's caller sees the transient error family), so close()/Drop can no longer leak the listener session through a stuck await.
• Pins added (open_tests.rs, 4 new; notify_tests.rs, 1 reworked): interleaved_register_unregister_keep_server_listen_set_matching_registry (server-less: a server-model task re-plays the command queue in FIFO order onto a LISTEN model, two interleaving tasks hammer register/unregister on two channels, a per-channel deterministic finale races a last-subscriber drop against one re-register with the decision anchored on the registry snapshot — the final registry/model equality is the pin); bounded_wait_done_overran_and_shutdown_arms and failed_verify_settle_bounds_aborts_and_surfaces_shutdown (paused-time, the settle/query bounds' arms incl. prompt close); close_reclaims_the_listener_session_through_a_black_holed_connection (harness, the TCP black-hole proxy seam: eaten-response proof the serve arm is parked, close reclaims inside a bounded window, the session stays reclaimed, post-close ops fail closed, subscribe past the loop exit fails closed); channel_set_refcounts_last_subscriber_rule reworked onto the decide-and-queue API (last-subscriber rule, redundant-LISTEN ack vehicle, past-zero no-op, mid-reconnect Err + entry-stays + nothing enqueued).
• Gates: cargo test -p alkstore_postgres with the harness server green (131 lib + 25 contract-suite + 9 schema; lib battery run twice full), every pre-existing forwarder/reconnect/no-replay/ shutdown/stale-UNLISTEN/LISTEN-window-death pin green unchanged; workspace cargo build, server-less cargo test --workspace, cargo clippy --all-targets -- -D warnings (workspace), cargo fmt --check — all green; the doc gate (RUSTDOCFLAGS="-D warnings" cargo doc --no-deps) green.