pg: forwarder TOCTOU closed + hang-shaped arms bounded (review 004 items 4+6)
- (a) the register/unregister decide-and-queue critical sections: the refcount change and the derived LISTEN/UNLISTEN enqueue run under one registry guard (ChannelSet::enqueue_register/enqueue_unregister), so the command queue's order is the registry transitions' linearization — the old thread-preempted decide/send split (no await inside, but mid-unit preemption across workers) could leave a live refcount with the server-side LISTEN cleared until the next reconnect re-issue. The redundant per-registration LISTEN stays as the ack's vehicle, so a listen() that returned still awaits a server-processed LISTEN. - (b) a failed generation-verify no longer parks on the poll task forever: the bounded settle (LISTEN_SETTLE_TIMEOUT=2s, shutdown- selected) reaps a genuinely dead session promptly or aborts the alive-but-unusable connection's reaper at the bound — reconnect either way. - (c) close()/Drop can no longer leak the listener session: every listener-client query (verify batch, replay commands, serve commands) rides bounded_wait (LISTENER_QUERY_TIMEOUT=5s) with the shutdown watch selected alongside — the old unbounded run_command await sat outside the shutdown select; an overrun is the honest death posture (abort + reconnect, the snapshot re-issue recovers). - pins: the interleaved register/unregister server-model equality pin (with a decision-anchored deterministic finale), the bounded-wait + failed-verify-settle paused-time pins, and the black-hole-poxy harness pin (close reclaims the session past a stuck serve arm); the refcount unit pin reworked onto the decide-and-queue API. - replay-proofs (both ways, verified live): the old split shape fails the interleave pin on the review's exact mismatch; the old unbounded serve await fails the black-hole pin (session leaked past close). - the preserved postures re-verified green: reconnect survival, the fanout release guard, receivers-terminal-iff-loop-gone, the stale-UNLISTEN generation machinery, the register-failure unregister discipline, the poison-recovery idiom, and both POC-pinned deadlock-pitfall exclusions. - gates: pg suite with the harness server green (131 lib + 25 contract + 9 schema; lib battery run twice full), workspace build/test/fuzz gates server-less green, clippy -D warnings, fmt, and the rustdoc gate all green; task -> completed
This commit is contained in:
1 parent
c60193ad5c
commit
aa66dd1912
4 files changed
+1225
-190
No files matched your search
@@ -1,7 +1,7 @@
|
||||
---
|
||||
id: pg-fix-forwarder-hardening
|
||||
name: Forwarder TOCTOU restructure + hang-shaped arms (review 004 items 4+6)
|
||||
status: pending
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: broad
|
||||
risk: high
|
||||
@@ -56,20 +56,20 @@ connector injection, the generation tests) for the new pins.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The TOCTOU is closed: register/unregister interleavings across
|
||||
- [x] 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:
|
||||
- [x] 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
|
||||
- [x] `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
|
||||
- [x] 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
|
||||
- [x] `cargo test -p alkstore-postgres` (harness server), clippy
|
||||
`-D warnings`, fmt clean; workspace gates green server-less
|
||||
|
||||
## References
|
||||
@@ -86,8 +86,171 @@ connector injection, the generation tests) for the new pins.
|
||||
|
||||
## Notes
|
||||
|
||||
> To be filled by implementation agent
|
||||
> 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
|
||||
|
||||
> To be filled on completion
|
||||
> 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.
|
||||
Reference in new issue
Block a user