--- id: review-wave-4-fixes name: Review gate — wave-4 fix batch (review 002 resolutions) status: completed depends_on: [pg-fix-forwarder-reconnect, pg-fix-tx-wake, pg-fix-open-path, pg-fix-stale-unlisten, pg-fix-scheduler-resilience, pg-fix-dedupe-cleanup, pg-fix-docs-alignment] scope: moderate risk: low impact: phase level: review tags: [wave-4-fixes, review] --- ## Description Review the fix batch decomposed from the wave-4 general review (`docs/reviews/002-wave-4-general-review.md`) before wave 5 decomposes. The batch's own gate — distinct from the general review that found the issues: verify each finding's fix is *real* (code-read against the review's failure mechanism, not test-name-trust), and that the fixes didn't damage the postures the wave-4 gate pinned. Check: - **Finding 1 (forwarder reconnect)**: the loop survives repeated failed connects — code-read the restructured reconnect arm (no path back to the `take()`'s `else return`); the failed-connect arm is test-exercised (the seam/injection is real, not a mock that never fails); the fanout-sender release invariant (receivers terminal iff loop gone) holds on every exit path. - **Finding 2 (tx wake)**: `pg_notify` present in `publish_with_key_tx` (contract obligation) and the enqueue twins (parity), commit-atomic inside the caller's tx, empty payload (channel-only wakes); the no-ghost rollback arm pinned; the compose test's determinism evidence (repeated solo runs) recorded. - **Finding 3 (open path)**: `max_size` guard before any round trip; the DSN options append (or documented engine-wins) verified against the actual setter semantics. - **Finding 4 (stale UNLISTEN)**: the generation-drop/reconcile decision unit-pinned across all four command/staleness combinations; the invariant stated in the forwarder docs. - **Finding 5 (scheduler)**: quarantine + retry behavior vs the review's fix shape; the leadership exit-path posture documented; the doc claim corrected. - **Minor notes**: dedupe (one decode owner — grep-auditable), the const, the helper routing, the doc alignments in deployment.md. - **No regressions**: the wave-4 gate's pinned postures re-spot-checked where the fixes touched them — forwarder integrity (the two deadlock pitfalls still structurally excluded), seam integrity, close semantics (the pg arm: open across reconnects, close at shutdown only), the no-replay honesty pin, entry-point validation. - **Records**: `tasks/review-wave-4.md` F-1 note and `docs/plans/implementation.md`'s review-rounds line consistent with what landed. - **Gates**: full workspace build/test/clippy/fmt; the pg suite green against the harness server; the compose test run repeatedly (the determinism claim verified, not asserted). **Wave-5 gate**: per the general review's sequencing, wave 5 decomposes after Findings 1–2's fixes land (the suite's wake-driven rows would otherwise inherit a hang-shaped false failure). This gate confirms both landed; if only they have, say so explicitly — wave 5 may decompose with 3–7 still in flight only if this gate judges the remainder wake-independent (expected: yes — 3–7 touch no wake-driven suite surface). ## Acceptance Criteria - [x] Each finding's fix verified against the review's failure mechanism (code-read + the new tests' actual coverage) - [x] No regressions to the wave-4 gate's pinned postures (re-spot- checked at the touched sites) - [x] Records consistent (F-1 retirement, review-rounds line) - [x] All gates green (incl. against the harness server); compose-test determinism evidence recorded - [x] Findings recorded; wave 5 decomposition disposition stated explicitly ## References - docs/reviews/002-wave-4-general-review.md (the findings this batch resolves) - tasks/review-wave-4.md (the wave-4 gate's pinned postures + F-1's record) - docs/plans/implementation.md (Review gates; Review rounds so far) - tasks/pg-fix-*.md (the batch's seven tasks) ## Notes Review performed 2026-10-10 (opencode, glm-5.3-flash). Gate checklist record — every finding verified against the review's failure mechanism by code-read, not test-name-trust: - **Finding 1 (forwarder reconnect)**: the failure mechanism is gone *by construction*, not just tested — `LoopCtx.connection` is a plain non-`Option` (the `take()` `else { return }` arm is restructured away entirely); the reconnect arm runs `reconnect_with_retry` (sleep-with-shutdown-select → attempt → double, cap 2 s), and its only exits are `Some`'s fresh generation or the shutdown flip — there is no path from a failed connect back into a loop top with an empty slot (forwarder.rs:510-546, 765-801). The failed-connect arm is genuinely test-exercised: the connector is an injectable closure (the server-less paused-time unit pin substitutes an always-failing connector and asserts six attempts across six backoff cycles with the cap reached), and the harness test drives the real seam — config swapped to an unreachable endpoint post-open → backend kill → ~4 failed connect cycles mid-outage (with the honest transient `register` error pinned) → config restored → reconnect-wake + post-recovery NOTIFY delivery + working `listen()`. The fanout invariant (receivers terminal iff the loop is gone) is enforced by the `FanoutRelease` drop guard on every exit path (return, unwind, abort) plus the shutdown take; `subscribe` then fails closed past any loop exit. - **Finding 2 (tx wake)**: `wake_tx` (`SELECT pg_notify($1, '')` — channel-only, ADR-008 §3) rides all three producer sites (`publish_with_key_tx` via which `publish_tx` rides; `enqueue_tx`; `outbox_enqueue_tx`), issued after the INSERT on the caller's tx client — commit-atomic by NOTIFY's native tx delivery, and the no-ghosts arm is native (a rolled-back tx's queued notifies are discarded server-side with the row). `run_once` correctly untouched. No double-wake: the wake sits at the tx *call sites*, not inside the shared `enqueue_row` — the auto-commit twins keep their single statements (verified at queue.rs:486, stream.rs:292 — unchanged). The harness pin covers all four arms (pre-commit 300 ms silence + 100 ms drain windows; deterministic delivery at commit on all three registered channels; drop-rollback silence; row-count truth). Determinism re-verified this session: 20/20 solo runs green. - **Finding 3 (open path)**: the `max_size == 0` guard fires before the config parse and any round trip (`max_size` is `usize` — zero is the whole invalid domain), typed `Database` with the value in the source chain; the harness-unavailable fallback pins it server-less against an unreachable DSN inside a 2 s timeout (a regression re-hangs and fails by timeout). The DSN append is verified against the actual setter semantics: `get_options()` reads the parse-carried value, the engine's `-c synchronous_commit` is appended, postgres order-processes repeated `-c` (engine wins the collision it owns); both `SHOW statement_timeout` (consumer's survives) and `SHOW synchronous_commit` (both knob settings) asserted. - **Finding 4 (stale UNLISTEN)**: shape chosen and recorded — generation-tagging, the pure `reconcile_queued_commands` helper (stale → dropped, current → replayed in queue order) applied at the fresh generation's post-re-issue drain *and* mirrored by the serve loop's recv-arm staleness guard (the one race the drain misses). Replay-proven (neutered helper fails the unit pin). The four command/staleness combinations are unit-pinned, in queue order, with the dropped stale LISTEN's ack asserted as the transient mid-`LISTEN` `Database` error; the stale-LISTEN ack honesty holds end-to-end because `register`'s registry write precedes its generation load, so any pre-bump load's registry entry predates the fresh generation's snapshot read — the re-issue covers it (the ack-time claim in the task's Notes checks out). The invariant is stated in the module doc, the loop doc, and the corrected "replay harmlessly" field comment is gone (grep-clean). - **Finding 5 (scheduler)**: quarantine (log with name + boundary advanced strictly past now via `skip_forward` at minimum interval + the remaining due rows proceed in the still-committing tick tx) is behavioral-pinned against the harness (direct-SQL tamper, the corrupted row never fires and never rewrites its spec, the runner survives to `Ok(())`, the good schedule keeps firing ≥ 2 more fires); the retry policy is unit-pinned (exactly 3 retries beyond the first attempt, 250 ms → 500 ms → 1 s, capped; total added stop latency ≤ 1.75 s) and the retry cannot mask a lost lease (every tick error at the site is `Database`-shaped; the loop-top renew owns loss). Error context rides the source chain via the `ErrorContext` wrapper (schedule name / tick phase + the driver chain preserved). The TTL-lapse exit posture is documented; the false `:46-53` claim is corrected to the honest slice/idle posture. - **Minor notes all landed**: `job_from_row` has one owner (queue.rs — `pub(crate)`; tx.rs imports it, and `get_job_tx` reuses `live_columns()`/`dead_columns()`, SQL strings byte-identical); "one decode owner" doc claims are now true; `sweep_expired`'s doc states the sum + the `in_tx` frame; the closed-store error routes through `database_error`; `bridge_capacity` is `const BRIDGE_CAPACITY`; deployment.md owns the TLS-unavailable statement (the forwarder doc corrected at its site, a `PgOpts` TLS pointer added) and carries the consumer-obligation section; core's `QueueOpts` doc carries the mirror sentence. - **No regressions (re-spot-checked at the touched sites)**: the two POC-pinned deadlock pitfalls remain structurally excluded (the poll task is spawned before any client query *every* generation; the loop owns the `Client` for the generation's lifetime); seam integrity untouched (no `spawn_blocking` anywhere — grep-clean; the dedupe is behaviorally inert: identical SQL strings, error shapes, capacity); the pg close arm holds (open across reconnects, terminal at shutdown only — the release guard + shutdown take; the gap-heal and shutdown-terminal pins stayed green); the no-replay gap-heal pin stayed green; entry-point validation only *gained* a site (the max_size guard). No inline comments beyond load-bearing correctness notes; the new `expect`s are the documented mutex-poisoned-guard posture. - **Records**: `tasks/review-wave-4.md` F-1's retirement (including the correction of the gate's wrong "commit *is* the NOTIFY carrier" conclusion) and `docs/plans/implementation.md`'s review-rounds line are consistent with what landed. **One minor doc-behavior mismatch found by this review — fixed inline (comment-only, reviewed here)**: `outbox_enqueue_tx`'s wake comment claimed "the scheduler's fire-wake targets the same name" — but the scheduler's tick fires issue *no* wake (the re-poll safety net covers their correctness; the wake-latency gap for scheduler fires is the same posture class the review accepted for `run_once`-adjacent paths), and `schedule()` rejects reserved names so it can never target an outbox's derived backing queue (ADR-021 §3). The comment now states the truthful "no other path targets it" claim. **Recorded for later (not defects; no action this gate)**: - The reconnect attempt is awaited outside the retry loop's shutdown-select (the select wraps the sleep, not the in-flight connect), so a connect hanging on a timeout-less DSN delays a shutdown-facing terminal arm — pre-existing posture, unchanged by the fix, bounded by the consumer's `connect_timeout`; noted in case a wave-5/6 task wants the attempt itself raced against the flip. - Scheduler *fire* wakes (the tick's in-tx enqueue) issue no `pg_notify` — SQLite's watcher wakes on those commits, so this is a same-class cross-engine latency-parity gap as the review's Finding 2 twins; correctness is covered by the queues row's pinned re-poll safety net. Not in review 002's fix shape; if wave 5's cross-engine equivalence rows demand the parity, it is a one-call `wake_tx` inside the tick's tx. **[Retired 2026-10-10]** Closed by `tasks/pg-fix-scheduler-fire-wake.md` — the tick now issues the one-call `wake_tx` inside its own `in_tx` frame (one coalesced wake per firing schedule per tick, commit-atomic; the disposition note in `tasks/suite-scheduler-rows.md` and the wave-5 gate's audit row carry the same retirement pointer). ## Summary **Review gate passed: all seven fix tasks verified against the review's failure mechanisms; no postures regressed; one minor doc mismatch found and fixed inline; all gates green; wave 5 may decompose.** Mechanism-verification highlights (full checklist in Notes): - Finding 1's failure mechanism is *structurally* gone (non-`Option` connection slot; the reconnect arm retries with live backoff and has no fall-through exit), and — the part the wave-4 gate's blind spot hid — the failed-connect arm is now genuinely test-exercised: a server-less paused-time unit pin drives an always-failing connector across six backoff cycles (cap reached, shutdown abort-before-any-attempt), and the harness test replays the production shape end-to-end via the `cfg(test)` config seam (unreachable endpoint mid-outage → honest transient `register` errors → config restore → reconnect-wake + delivery + working `listen()`). - Finding 2's `wake_tx` is commit-atomic at all three producer sites, no-ghosts pinned pre-commit and post-rollback, no double-wake on the auto-commit twins, and `tx_publishes_compose_with_the_handle` is deterministic — **re-verified this session: 20/20 solo runs green, zero deadline misses**. - Findings 3–7 landed as pinned (guard-before-round-trip + verified DSN append semantics; the four-combination generation-reconcile unit pin; the behavioral quarantine + retry-policy pins + corrected doc; the one-owner decode + minor cleanups with zero behavior change; the TLS/QueueOpts doc alignments). The wave-4 gate's pinned forwarder/seam/close/no-replay/validation postures re-spot-checked across all touched sites — none moved. Records (`review-wave-4.md` F-1, the review-rounds line) consistent. Gates this session: `cargo build`, `cargo test` (server-less — 25 core + 188 sqlite + 140 pg lib incl. skipped harness rows + 10 + 10 suite + 3 + 9 schema), `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check` — all green; the pg lib suite green against the harness server (121 lib + 10 contract-suite + 9 schema, two consecutive full runs); compose determinism 20/20 solo runs. **Wave-5 disposition: wave 5 decomposes.** Both HIGH fixes (Findings 1–2) landed and are gate-verified; findings 3–7 also landed, so the "3–7 wake-independent" question is moot. The two defense-in-depth observations above ride wave 5/6's natural tasks if anyone picks them up — neither blocks decomposition.