Wave-4 general review (002): two live-proven bugs — the forwarder's permanent death after one failed reconnect (the failure arm the gate's test never covered) and the tx enqueue/publish paths' missing pg_notify wake (F-1's root cause, engine-side) — plus a max_size:0 open-hang, two narrow robustness gaps, doc mismatches, and the decode-duplication smell; reviews renumbered 001/002 per the alk* numbering pattern (references updated)

docs/reviews/002-wave-4-general-review.md

@@ -0,0 +1,266 @@
+# General review — wave 4 (Postgres engine)
+
+- **Reviewer**: opencode (glm-5.3-flash)
+- **Scope**: general review of everything landed by wave 4 (13
+  machinery files in `alkstore-postgres/src`, ~6.6k lib lines; test
+  modules read selectively). Distinct from the wave-4 review gate
+  (`tasks/review-wave-4.md`, engine-vs-contract conformance — 0
+  findings): this review looks for security/performance issues, code
+  smells, and the classic correctness defects a wave of implementation
+  can carry, plus live-probe verification against the harness server.
+- **Input**: full read of the 13 machinery files; core-contract.md,
+  engine-postgres.md, ADR-004/005/006/007/008/009/010/015/016/019/020/
+  021/023; `tasks/review-wave-4.md` (F-1..F-4 known). Three suspicions
+  were live-verified with throwaway probe tests run against the
+  harness server (probes removed after; the tree is clean).
+- **Gates at review time**: `cargo clippy --all-targets -D warnings`
+  + `cargo fmt --check` re-run green after probe cleanup.
+
+## Verdict
+
+**Two real consumer-facing bugs found, both live-proven; one of them
+is the F-1 flake's root cause (engine code, not test-infra).** The
+contract-conformance story the gate told holds — but the gate's two
+"engine code is honest" conclusions were both reached through the same
+blind spot (the reconnect path and the tx wake path are never
+exercised under failure), and both hid production-facing defects.
+Findings 1–3 come with concrete failure probes; 4–5 are reasoned
+narrow races/robustness gaps; the rest are doc mismatches and smells.
+
+**Harness-server note (honesty record)**: during probe verification,
+`docker stop -t 0` on the original `pglo-poc` container *removed* it
+(it had been created with `--rm`). The container was recreated on
+`postgres:16-alpine` bound to the original anonymous data volume
+(verified intact — the `blobs` db answers on :15432), now **without**
+`--rm`, so future stops are safe.
+
+---
+
+## Finding 1 — HIGH: the forwarder dies permanently after one failed
+ reconnect attempt (live-proven)
+
+**Site**: `forwarder.rs:498-509` → `forwarder.rs:369`.
+
+On a failed `reconnect_config.connect(NoTls)` the loop's `Err` arm
+`continue`s — but the loop top is
+`let Some(poll_connection) = connection.take() else { return; }`, and
+`connection` is still `None` (the old connection was consumed by
+`take()` when its generation started). One failed connect ⇒ the loop
+takes the `else` arm and **the forwarder task exits permanently**. The
+in-line comment says "Still unreachable — back off again and retry";
+there is no retry. The `connected` flag stays `false`, so:
+
+- every subsequent `listen()` fails with a spurious "mid-reconnect"
+  `Database` for the store's remaining lifetime (transient posture
+  implies retry recovers it — it doesn't; retry recovers nothing
+  without a reconnect);
+- wake delivery stops permanently — registered channels never
+  re-LISTEN, no synthetic reconnect-wake ever fires;
+- stream subscribers already parked on `wait_wake` stay parked
+  **forever** (the fanout's senders are alive — handle-held + the dead
+  loop's clone leaked in the task — so the broadcast never closes and
+  the consumer never even sees a terminal arm). That contradicts the
+  streams row's pinned posture: *events never require polling to
+  become visible* (core-contract.md streams section).
+
+**Probe (removed)**: opened a store, registered a channel via
+`forwarder.register`, verified pre-outage delivery worked, SIGKILLed
+the server for ~1.2 s, brought it back — `NOTIFY` on the registered
+channel did not deliver for 20 s of continuous retry, no
+reconnect-wake, no recovery. The registry entry, the honest transient
+`register` errors, all the machinery around the bug is correct; the
+loop just dies.
+
+**Why the wave-4 gate missed it**: the reconnect test
+(`open_tests.rs:244`) kills the backend via `pg_terminate_backend`
+while the server stays up — the 50 ms reconnect attempt connects
+successfully every time. The reconnect machinery was only ever
+exercised with a live server; the *unreachable-server window* is the
+one case the machinery exists for, and the one case untested.
+
+**Fix shape**: retry the connect with the live backoff *inside* the
+reconnect arm (loop `sleep(backoff)` → `connect` until success or
+shutdown), never falling back into the `take()`'s `else return`. Also
+consider releasing the dead loop's fanout clone so an abnormal loop
+exit still surfaces the terminal `Closed` arm to subscribers instead
+of a forever-silence.
+
+## Finding 2 — HIGH (and F-1's root cause): the tx paths issue no wake
+ (live-proven)
+
+**Site**: `tx.rs:290-329` (`publish_tx`/`publish_with_key_tx`),
+`tx.rs:272-288` (`enqueue_tx`), `tx.rs:545-562` (`outbox_enqueue_tx`).
+
+None of the tx-side enqueue/publish paths run `pg_notify` in the
+caller's transaction. `notify_tx` proves `pg_notify` inside a tx is
+native commit-atomic (delivers at the caller's commit) — the exact
+mechanism these paths have available and don't use.
+
+**Probe (removed)**: with a registered LISTEN on the stream/queue
+channels (and verified no unrelated traffic): auto-commit
+`publish` and `Queue::enqueue` wake immediately (fanout receives the
+channel name); a *committed* `publish_tx`/`enqueue_tx` never wakes —
+3 s windows, dead silence, deterministic across runs.
+
+**This is the F-1 flake's root cause, engine-side** — `tasks/
+review-wave-4.md` F-1's "root cause unresolved" and the review gate's
+"the tx INSERT's commit *is* the NOTIFY carrier" conclusion were both
+wrong (nothing on that path sends a NOTIFY). Mechanism of the flake
+(`stream_tests.rs:1022`): the subscriber bridge attach-read + drain
+race the commit. If the drain query *starts* after the commit it sees
+the row (test passes — the ~98% case, since the drain is two pool
+round trips behind a start-vs-commit lottery); if both bridge reads
+beat the commit (~2% under the 8-thread suite; reproduced ~1/5 solo),
+the bridge drains empty and parks on `wait_wake` — and **no wake will
+ever come** from a tx-committed publish, so `must_recv_event`'s 15 s
+deadline exhausts. All the gate's evidence (the 64-run standalone
+probe, the "delivery ≤ 210 ms" honesty) simply sampled the lucky
+ordering; the parked state is genuine and unbounded.
+
+**Conformance angle**: engine-postgres.md's mapping names `pg_notify`
+as the streams wake trigger; core-contract.md pins "events never
+require polling to become visible" and the tx/no-ghosts rows pin
+rollback drops the event row *with its wake*. The missing wake is a
+real gap the engine produces (SQLite's watcher fires on any committed
+write, so its tx path wakes — the asymmetry would also have failed
+wave 5's cross-engine equivalence rows). Fix: one
+`SELECT pg_notify($1, '')` (the stream's channel = the
+mechanism-name-is-the-channel realization) inside
+`publish_with_key_tx`. `enqueue_tx`/`outbox_enqueue_tx` are
+contract-covered by the queues row's pinned re-poll safety net, so
+their wakes are posture-parity (SQLite's watcher fires there too);
+worth adding for symmetry, weaker obligation. Note `run_once` is a
+pull op consumer-driven — no wake needed there.
+
+**Fixing the wake properly retires F-1's engine arm**; the wave-5
+suite-hardening candidates in `tasks/review-wave-4.md` (parked-recv
+deadline, lag diagnostics) remain valid as defense-in-depth but
+become no longer load-bearing for this flake.
+
+## Finding 3 — MEDIUM: `PgOpts { max_size: 0 }` hangs `open` forever
+ (live-proven)
+
+**Site**: `store.rs:162-165` + the bootstrap `pool.get()` at
+`store.rs:177`; deadpool (0.13/0.14) does not validate `max_size` and
+defaults to no timeouts.
+
+A pool with `max_size: 0` can never grant a checkout: `open` blocks
+forever at the bootstrap checkout (no `BuildError`, no default pool
+timeouts) instead of failing typed. Probe (removed): `open` with
+`max_size: 0` timed out at 5 s, never returned.
+
+**Fix shape**: validate `max_size > 0` in `open_store` (typed
+`Database` before any round trip), or configure deadpool timeouts so a
+deadlock-class hang surfaces as an error.
+
+## Finding 4 — LOW/MEDIUM: a stale queued UNLISTEN can cancel a
+ re-issued LISTEN after reconnect
+
+**Site**: `forwarder.rs:294-300` (unregister best-effort), the
+snapshot re-issue `forwarder.rs:380-389` + command replay
+`:429-470`.
+
+Sequence: last subscriber drops → `UNLISTEN` queued to the dead/dying
+connection → connection dies before the command is processed → the
+channel is re-registered by someone else → on reconnect the snapshot
+re-issues `LISTEN` (registry says live) — then the **stale queued
+UNLISTEN replays after it**, silently unlistening a channel the
+registry believes is listening. Wakes for that channel are lost until
+the next reconnect or a fresh registration. The "queued commands
+replay harmlessly across reconnects" comment holds for stale LISTENs
+(re-issued from the snapshot anyway) — not for stale UNLISTENs. Very
+narrow window; silent effect. Fix shape: tag commands with the
+connection generation and drop commands that predate the current one,
+or have the loop reconcile UNLISTENs against the registry snapshot
+instead of replaying them verbatim.
+
+## Finding 5 — LOW/MEDIUM: one bad row or one transient error
+ permanently kills `run_schedules`
+
+**Site**: `scheduler.rs:403` (`parse_every_interval(&row.spec)?`),
+`scheduler.rs:429` (fire `enqueue_row(...)?`), `:519-526` (the
+`in_tx` tick `?`), all propagating straight out of the leader loop.
+
+`tick`'s errors escape the loop (the `@every` grammar is re-parsed
+from stored text per tick, unvalidated-on-read; a tampered or
+future-foreign spec row makes the whole runner exit with
+`InvalidSpec`), and one transient pool/database error does the same.
+The consumer's respawn recipe makes this survivable but brittle —
+`schedules never fire without a runner`, and the runner dies on any
+hiccup rather than skipping/retrying. The exiting error also carries
+no schedule-name context. Consider: re-validate on read is
+unnecessary (stored specs are pre-validated), but a bad row should be
+*quarantined* (log + advance its boundary) rather than fatal, and
+transient errors retried N times before giving up. Fine print: on
+that exit path the leadership lock isn't released — the TTL lapse
+covers it, but say so.
+
+## Finding 6 — doc-behavior mismatches (cheap fixes)
+
+- `scheduler.rs:46-53` claims "a newly-registered schedule is noticed
+  by the re-read no later than the next slice (1 s)". False:
+  `keep_awake_renewing` takes a fixed `soonest` and its 1 s slices
+  only renew the lease and check the stop token — the soonest is
+  never re-read mid-sleep. An idle runner notices a new schedule up to
+  60 s late (`IDLE_SLEEP_S`); an active runner waits until the
+  current soonest deadline. Either implement the claim (re-check the
+  soonest per slice) or correct the comment (the 60 s idle-tick
+  posture is already acknowledged two paragraphs down as the
+  acceptable floor; the doc comment above it contradicts it).
+- `queue.rs:71` says `sweep_expired` "returns the moved count" — it
+  returns moved + retention-deleted (`queue.rs:720`). The SQLite twin
+  returns the same sum (substrate `queue_ops.rs:232-243`), so the
+  engines agree; only the doc is wrong. ("Single-statement atomic" is
+  also inaccurate — it's the `in_tx` multi-statement frame.)
+
+## Minor notes (no action forced)
+
+- **`job_from_row` is byte-for-byte duplicated** (`queue.rs:411` and
+  `tx.rs:626`; only `pub(crate)` differs), and `get_job_tx` inlines
+  the 18-column dead/live select column lists (`tx.rs:420-428,
+  438-442`) duplicating `queue.rs`'s `live_columns()`/`dead_columns()`.
+  Both files claim "one decode owner" in their doc comments; both are
+  wrong. The shape is contract-pinned, so two owners is exactly where
+  a shape edit goes wrong later. Dedupe into one module (queue.rs is
+  the natural home; tx.rs imports it — as `tx.rs` already imports
+  `stream_events_from_rows` from `stream.rs`).
+- `notify.rs:106-110` builds the closed-store error inline
+  (`Error::database(std::io::Error::other(...))`) instead of the
+  shared `database_error` helper — one duplicate of the one message.
+- `store.rs:152-156`: `pool_cfg.options(...)` **replaces** any
+  `options=` present in the consumer's DSN (tokio-postgres's setter
+  semantics) — a silent config override; append (or document that
+  engine-configured options win).
+- TLS posture: every connection (pooled `store.rs:161` and listener
+  `store.rs:196`) is hardwired `NoTls`, but `forwarder.rs:53-57`'s
+  doc says the pooled path "rides the consumer's Config sslmode" —
+  misleading; a `sslmode=require` DSN fails at connect. v1 TLS is
+  effectively unavailable; deployment.md should own that statement
+  (and the forwarder doc corrected to match the code).
+- `bridge_capacity()` (`notify.rs:137-139`) is a function returning a
+  constant — a `const` says it plainly.
+- Queue/`QueueOpts` numeric values are trusted unvalidated (negative
+  `max_attempts`/visibility/retention stamp into rows verbatim; zero
+  `max_attempts` self-dead-letters into churn). The ADR-023 domain
+  table pins extents/durations/boundaries only, so this is not a
+  defect — but a one-sentence doc note (consumer-obligation, like the
+  visibility-budgeting one) would prevent surprise.
+- `eprintln!` diagnostics at the swallowed-wake/lag sites match the
+  SQLite twin's posture; the logging story is a wave-6 item (already
+  recorded by the wave-4 gate).
+- F-2..F-4 from `tasks/review-wave-4.md` stand as recorded there.
+
+## Recommended sequencing
+
+1. Fix Finding 1 (forwarder reconnect retry) — includes extending the
+   reconnect test with an unreachable-server phase (the missing
+   failure arm the gate's test never covered).
+2. Fix Finding 2 (`pg_notify` in `publish_with_key_tx`, optionally
+   the enqueue tx twins) — this closes F-1's engine arm; the
+   existing `tx_publishes_compose_with_the_handle` becomes
+   deterministic as a side effect.
+3. Fix Finding 3 (max_size guard).
+4. Findings 4–6 in whatever batch wave 5's proximity allows (4 and 5
+   are robustness-shaped; 6 is doc).
+5. Wave 5 decomposes after 1–2 land (the suite's wake-driven rows
+   would otherwise inherit a hang-shaped false failure).