alkstore/tasks/review-wave-4-fixes.md

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

id	name	status	depends_on	scope	risk	impact	level	tags
review-wave-4-fixes	Review gate — wave-4 fix batch (review 002 resolutions)	completed	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	moderate	low	phase	review	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

• 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- checked at the touched sites)
• Records consistent (F-1 retirement, review-rounds line)
• All gates green (incl. against the harness server); compose-test determinism evidence recorded
• 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 expects 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.