271 lines
15 KiB
Markdown
271 lines
15 KiB
Markdown
---
|
||
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. |