# 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-7 item (a wave-6 item when recorded — renumbered by ADR-024; 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).