From 9a00fb8a7e56fa996a017fb36e5a8a6341ddd923 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Sat, 10 Oct 2026 05:18:27 +0000 Subject: [PATCH] =?UTF-8?q?Review=20gate=20=E2=80=94=20wave-4=20fix=20batc?= =?UTF-8?q?h=20passed:=20all=20seven=20pg-fix=20resolutions=20verified=20a?= =?UTF-8?q?gainst=20review=20002's=20failure=20mechanisms=20(code-read=20+?= =?UTF-8?q?=20live=20gates),=20one=20minor=20doc=20mismatch=20fixed=20inli?= =?UTF-8?q?ne=20(outbox=20wake=20comment's=20'scheduler's=20fire-wake'=20c?= =?UTF-8?q?laim=20=E2=80=94=20the=20tick=20fires=20issue=20no=20wake=20and?= =?UTF-8?q?=20schedule()=20can=20never=20target=20a=20reserved=20backing?= =?UTF-8?q?=20queue);=20no=20pinned=20posture=20regressed;=20pg=20suite=20?= =?UTF-8?q?green=20vs=20harness=20twice=20+=20compose=20determinism=2020/2?= =?UTF-8?q?0=20re-verified;=20wave=205=20may=20decompose=20(task=20review-?= =?UTF-8?q?wave-4-fixes)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- alkstore-postgres/src/tx.rs | 6 +- tasks/review-wave-4-fixes.md | 189 +++++++++++++++++++++++++++++++++-- 2 files changed, 185 insertions(+), 10 deletions(-) diff --git a/alkstore-postgres/src/tx.rs b/alkstore-postgres/src/tx.rs index 9780587..6de0334 100644 --- a/alkstore-postgres/src/tx.rs +++ b/alkstore-postgres/src/tx.rs @@ -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) }) diff --git a/tasks/review-wave-4-fixes.md b/tasks/review-wave-4-fixes.md index f0e2830..22f3529 100644 --- a/tasks/review-wave-4-fixes.md +++ b/tasks/review-wave-4-fixes.md @@ -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 \ No newline at end of file +**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. \ No newline at end of file