Review gate — wave-4 fix batch passed: all seven pg-fix resolutions verified against review 002's failure mechanisms (code-read + live gates), one minor doc mismatch fixed inline (outbox wake comment's 'scheduler's fire-wake' claim — the tick fires issue no wake and schedule() can never target a reserved backing queue); no pinned posture regressed; pg suite green vs harness twice + compose determinism 20/20 re-verified; wave 5 may decompose (task review-wave-4-fixes)

2026-10-10 05:18:27 +00:00
1 parent 49face898d
commit 9a00fb8a7e
@@ -607,8 +607,10 @@ impl TxHandle for PgTxHandle {
                .enqueue_row(client, &queue, bytes, &opts, outbox_default_stamps())
                .await?;
            // The outbox's posture-parity twin of `enqueue_tx`'s wake:
-            // the derived backing-queue name is the channel (the
-            // scheduler's fire-wake targets the same name).
+            // the derived backing-queue name is the channel. No other
+            // path targets it — `schedule()` rejects reserved names, so
+            // the outbox's backing queue stays out of the scheduler's
+            // reach (ADR-021 §3).
            wake_tx(client, &queue).await;
            Ok(id)
        })
@@ -1,7 +1,7 @@
 ---
 id: review-wave-4-fixes
 name: Review gate — wave-4 fix batch (review 002 resolutions)
-status: pending
+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
@@ -65,14 +65,14 @@ suite surface).

 ## Acceptance Criteria

- [ ] Each finding's fix verified against the review's failure
+- [x] Each finding's fix verified against the review's failure
      mechanism (code-read + the new tests' actual coverage)
- [ ] No regressions to the wave-4 gate's pinned postures (re-spot-
+- [x] No regressions to the wave-4 gate's pinned postures (re-spot-
      checked at the touched sites)
- [ ] Records consistent (F-1 retirement, review-rounds line)
- [ ] All gates green (incl. against the harness server); compose-test
+- [x] Records consistent (F-1 retirement, review-rounds line)
+- [x] All gates green (incl. against the harness server); compose-test
      determinism evidence recorded
- [ ] Findings recorded; wave 5 decomposition disposition stated
+- [x] Findings recorded; wave 5 decomposition disposition stated
      explicitly

 ## References
@@ -86,8 +86,181 @@ suite surface).

 ## Notes

-> To be filled by implementation agent
+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.

 ## Summary

-> To be filled on completion
+**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.