Wave-4 review gate: conformance code-read clean (0 findings) — forwarder/seam integrity, ADR-023/016 follow-through, backoff + boundary math vs ADR text, schema posture, 10-row backlog column green against the harness server; the flagged tx-compose flake reproduced twice, root cause unresolved, recorded as F-1 for wave 5's suite hardening + three no-action notes (task review-wave-4)

tasks/review-wave-4.md

@@ -1,7 +1,7 @@
 ---
 id: review-wave-4
 name: Review gate — wave 4 (Postgres engine)
-status: pending
+status: completed
 depends_on: [pg-engine-integration]
 scope: moderate
 risk: low
@@ -87,8 +87,251 @@ Check:
 
 ## Notes
 
-> To be filled by implementation agent
+Review performed 2026-10-08 (opencode, glm-5.3-flash) — full code-read of
+`alkstore-postgres/src` (all 13 machinery files + 9 test modules, ~4.4k
+lib lines) against core-contract.md, engine-postgres.md, ADR-004/007/
+008/009/010/015/016/019/020/021. **Zero conformance findings needing a
+code change.** Two flake investigations and four recorded observations
+(all documentation/coverage-shaped, deferred to wave 5's natural homes)
+below.
+
+Gate checklist record (code-read against the spec text, per the
+acceptance criteria — not test-name-trust):
+
+- **Contract conformance**: entry-point validation on every name-bearing
+  method, auto-commit AND tx paths, before any round trip
+  (`store.rs` wiring + all eleven `*_tx` in tx.rs; `InvalidName` for
+  empty/whitespace-only, `ReservedName` for prefixed, via core's
+  `validate_shared_name`/`validate_local_name`; stream-consumer/
+  owner/worker ids as local names). `Database`-only receivers
+  (`recv`/`save_offset` Err arms — `database_only` remap in stream.rs);
+  `Codec` at every payload/decode seam (encode + row decodes, no silent
+  fallback); `LeadershipLost` vs clean stop arms (acquire-refusal and
+  renewal-loss both return before any tick); `Closed` on consumed tx
+  shells and pool-checkout paths. Value shapes decoded through core's
+  `#[doc(hidden)]` constructors (18-column `Job` shape, `StreamEvent`
+  field-for-field, dead-visible `get_job` carrying `last_error`/
+  `died_at`). No-work vocabulary held (`Ok(None)`/`Ok(false)`/empty
+  `Vec` where pinned). Queue scoping (`WHERE queue = $2`) verified on
+  `get_job_tx`/`get_job`/`ack_batch`/`cancel`; the live-then-dead
+  select order matches the SQLite arm.
+- **ADR-023 follow-through**: extent guards at trait-impl entry on
+  `claim_batch` (`n <= 0` → empty `Vec`), both auto-commit stream
+  reads, both tx stream reads, and `EventReceiver::read_since` — pg's
+  negative-`LIMIT`-is-a-server-error artifact unreachable. Duration
+  guards on both lock call sites (`try_lock`, `Lock::renew` —
+  `ttl <= 0` → opaque `Database`, detail in the source chain).
+  Boundary args total (negative `trim_to` deletes nothing, unguarded —
+  verified `id <= -7` matches no rows). `encode_payload` consumed
+  fallibly at all 8 call sites through the one `encode_payload_bytes`
+  alias; no silent fallback.
+- **ADR-016 §5**: the 8000-byte check is client-side, typed
+  (`PayloadTooLarge { limit: NOTIFY_PAYLOAD_LIMIT }`), before any round
+  trip, on **both** `notify` and `notify_tx`; one owner of the limit
+  constant (`tx::NOTIFY_PAYLOAD_LIMIT`); predicate `len >= limit` (the
+  NUL-inclusive wire budget, probe-pinned in tx_tests); measured on the
+  serde_json serialization (the stored-bytes form).
+- **Forwarder integrity**: pitfall 1 structurally excluded (the poll
+  task owns the `Connection` and is spawned before any client query
+  every generation; the command select keeps co-residency permanent);
+  pitfall 2 excluded (the loop owns the `Client` for the generation's
+  lifetime; drop only at shutdown or post-session-death). Reconnect:
+  50 ms → 2 s cap exponential, re-LISTEN from the live channel-set
+  snapshot (registry-write-first ordering so a registration is never
+  lost), synthetic reconnect-wake on the reserved string broadcast to
+  every subscriber (verified: it rides the raw fanout, and the
+  wake-per-receiver bridge passes it through with the channel filter's
+  explicit reserved-string arm). No-replay honesty pinned (the gap
+  test asserts the committed-during-gap notify is *not* delivered).
+  Lag surfaced not silent (`Lagged(n)` logged-and-skipped at both the
+  bridge loop and the stream subscriber). Receiver close semantics
+  exactly the pg arm: the broadcast subscription + bridge outlive
+  connection generations (close is shutdown-only — the fanout sender
+  take in `Forwarder::shutdown` plus the loop's clone at exit); a
+  backend kill delivers the reconnect-wake, not `None`; terminal; the
+  pinned tests assert never-reopens.
+- **Seam integrity**: drop = rollback via the captured
+  `Handle::spawn` detached teardown (no current-context resolve —
+  panic-safe; the runtime-gone arm closes the session server-side and
+  the server aborts the open tx, so no ghost survives it); failed
+  COMMIT/ROLLBACK/BEGIN arms discard via `Object::take` (never
+  re-pool an unknowable-state client) — pool accounting exact under
+  error paths (the deferred-FK COMMIT-failure test pins it);
+  ops-after-consume fail closed (`Error::Closed`); `with_tx` derives
+  from the trait's provided method over `begin_tx`/commit/drop — no
+  engine override to audit. `grep spawn_blocking` over
+  `alkstore-postgres/src` matches only doc text; every op is a straight
+  `.await` through the held object (the natively-async posture holds —
+  no stray bridge). The one sync-signature bridge
+  (`EventReceiver::save_offset` → `drive_sync` on the dedicated idle
+  wake runtime) is documented at the site with its probed-arms
+  rationale — the honest sync→async seam, not a `spawn_blocking`
+  re-entrancy trick, and it carries no engine state.
+- **Backoff curve + boundary math**: `backoff_delay_s` is
+  line-identical reasoning to the text — equal-jitter
+  `[base·2^(a−1)/2, base·2^(a−1)]` integerized inclusive, 3600 s cap,
+  attempt index = the post-claim row count, exponent clamp at 30
+  (saturating, never overflowing); statistical range pins at 400 draws
+  per tuple + the 1800..=3600 cap band + end-to-end row assertions.
+  Scheduler: `@every` parser matches the pinned grammar (`@every
+  <n><unit>`, s|m|h|d, positive n, checked_mul — cron/floats/zero/
+  unknown-units reject `InvalidSpec` pre-storage); first-fire =
+  `now + interval` (strictly after now); the one-firer-per-boundary
+  machinery is exactly ADR-009 §4's two layers — (a) leadership via
+  the engine's own lock machinery on `__alkstore_scheduler` (per-
+  instance owner token; loss returns `Err(LeadershipLost)` before any
+  tick, both at acquire and mid-run), (b) the row-locked `SELECT …
+  FOR UPDATE` tick in one pool tx with fire-enqueues + boundary
+  advance + soonest read committing together (crash mid-tick
+  refires); 64-cap catch-up with skip-forward strictly past now
+  (`(gap/interval + 1)·interval` in 128-bit, clamped at `i64::MAX`).
+  Resolved-stamps sets (plain 300/3/5/none, outbox 60/5/5, one
+  `max_attempts` override, delay-over-`run_at`, relative-`expires`,
+  one clock read per enqueue) match ADR-010 §3a / ADR-020 line for
+  line; the outbox derivation `__alkstore_outbox:{name}` is reserved
+  and unreachable by `enqueue_tx`/`queue()`.
+- **Seam error mappings**: all four helper shapes map into the opaque
+  `Database` with the source chain preserved; no engine-specific
+  variants minted anywhere.
+- **Schema posture**: one engine-owned schema (default `alkstore`,
+  `PgOpts`-parametrized), `bytea` payload columns asserted per-table
+  (schema_tests), bigserial offsets monotone-never-renumbered (gap
+  test), no per-queue tables (queues are rows), idempotent bootstrap
+  (triple-run converges), schema-prefixed index names (the co-tenant
+  silent-skip hazard documented and answered), identifiers quoted
+  through the one `quote_identifier` boundary (schema name is the only
+  consumer-supplied interpolation; injection-safe).
+- **Backlog column**: 10 rows green against the pg factory (twice this
+  session): the exemplar name-validation row, the three ADR-023 rows
+  verified as the same suite functions the SQLite column runs (not
+  rewritten — `extent_clamp_semantics`,
+  `duration_refusal_on_non_positive_ttl`,
+  `payload_round_trip_stores_exact_encoding`),
+  `payload_too_large_produced_on_pg` (the pg arm of the asymmetry row,
+  stamped ADR-008 §5/ADR-016 §5/ADR-020 §4 — the SQLite task's
+  deferred adoption discharged), drop-rollback, in-tx-RYOW,
+  enqueue-opts-resolution, receiver-close-and-save-arms, plus the
+  factory-shape row. All carry `Contract stamp:` doc stamps per the
+  ADR-022 convention (`grep`-audited).
+- **Family standards**: no panics in library code (the only `expect`s
+  outside `#[cfg(test)]` are the five Mutex-poisoned guards in
+  forwarder.rs and the wake-runtime build — documented, unwrap-free
+  elsewhere; one wake-runtime `expect` is a fixed-shape builder that
+  cannot fail, module-doc'd). No inline comments beyond the
+  correctness-constraint sites AGENTS.md's exception names (wake
+  postures, unknowable-state arms, the loss-decision owner,
+  predicate conjuncts — all non-obvious-behavior notes). Module-per-
+  file holds; `#[non_exhaustive]` policy respected (none on `PgOpts`);
+  core has no driver deps; `eprintln!` diagnostics at the five
+  swallowed-wake/lag sites match the SQLite twin's posture (the
+  logging story is a wave-6 item, not a wave-4 defect).
+- **Gates**: `cargo build`, `cargo test` server-less (25 core + 188
+  sqlite + 111 pg-skip + 10 + 10 suite + 3 + 9 harness), clippy
+  `-D warnings`, fmt — all green; the pg lib suite 111/111 green
+  against the harness server (two consecutive full runs, 2×47 s, plus
+  stream-module and suite-target re-runs).
+
+**Flake investigation** (the two task agents' `tx_publishes_compose_with_the_handle`
+report — see Summary below for the disposition): reproduced once in 53
+solo runs and once in the first full-suite run of this session; wake
+path code-read clean; the stand-alone binary probe that mirrors the
+test's beats (64 runs) never missed (delivery latency ~20–210 ms).
+Root cause unresolved — recorded for wave 5 (see below).
+
+Deferred (documented, not fixed — each needs its own small task or a
+wave-5/6 decision; none is a consumer-visible defect):
+
+- **F-1 — the `tx_publishes_compose_with_the_handle` flake is
+  open** (contradicting the streams/locks agents' "unrelated, wait for
+  wave 5" read: reproduced twice this session — once in a
+  first-of-session full lib run, once in a solo run; ~2/106 runs
+  total, all at the same site). Failure signature: `must_recv_event`'s
+  15 s deadline exhausts — the bridge's re-drain never fires after a
+  committed `publish_tx`. Facts established: the engine's wake path is
+  honest (the tx INSERT's commit *is* the NOTIFY carrier — verified by
+  the 64-run probe mirroring the test's beats, which never missed and
+  never even saw >210 ms; and by 6 whole-stream-module suite runs and
+  30 full-suite 8-thread runs, all clean); the delivery pipeline
+  introduces no artificial delay (no debounce/sleep — wake → re-drain
+  straight-line); the two test beats (the post-commit `try_recv`
+  drains and the two 200 ms sleeps) are all before the wait and only
+  make the test *late-poll*, not miss; and the deadline miss cannot be
+  a slow delivery (a 210 ms probe ceiling vs a 15 s deadline). The
+  residual suspicion therefore sits in the wake *subscription* path —
+  a `Lagged`/command-race interaction between the fresh receiver's
+  bridge and the forwarder under load — but no mechanism was proved,
+  and nothing in the code contradicts the contract. Recorded for wave
+  5's suite hardening (or a wave-5-adjacent test-infra task): the
+  candidate dispositions are (a) harden `must_recv_event`'s deadline
+  (a parked `recv().await` with a timeout wrapper instead of the
+  20 ms polling loop — the deadline assert then still bounds the
+  property), (b) a `LOG_LAGGED`-style diagnostic counter on the bridge
+  to discriminate "wake never arrived" from "wake arrived, re-drain
+  missed", and (c) leave the skip-posture deadline as-is and re-triage
+  on recurrence. Not decomposed now: the flake is test-infra, not
+  engine code, and (b)+(c) are the cheap first steps.
+- **F-2 — `bridge_loop`'s filter drops non-subscriber channel
+  notifications *before* the reserved-string check is reachable in the
+  `&&`'s false path** — cosmetic only: the code is correct (the
+  reserved-wake arm matches), the observation is that the condition's
+  order matters and is load-bearing; a `//` note at the site would
+  help future readers. Not fixed: AGENTS.md's comment policy says
+  no inline comments unless load-bearing — this one arguably is, and
+  the site already carries a doc-comment explaining the mapping; left
+  as-is to avoid comment churn.
+- **F-3 — `spawn_bridge`'s `spawned.is_finished() → abort()` guard**
+  (stream.rs:599): detects a bridge task that finished before the
+  receiver was even boxed (runtime-gone class) but the boxed receiver
+  would surface after the bridge died — honest, but it's a
+  probabilistic check (a task could finish between the check and the
+  return, an unavoidable race the guard narrows). The task's failure
+  arm (the mpsc disconnect) still delivers the terminal `None` — no
+  lie is surfaced either way. Noted for the wave-5 close-arm rows'
+  fidelity; no action.
+- **F-4 — wake-channel naming is now realized in code but no doc
+  section states it for streams**: the plan's Wave 4 section (lines
+  174–182) records the decision, and `stream.rs`/`queue.rs` doc
+  comments name it ("the mechanism-name-is-the-channel realization"),
+  but `core-contract.md`'s streams/notify sections don't mention the
+  channel naming (they don't need to — it's engine-internal), while
+  the reserved reconnect string *is* a contract string. No action —
+  recorded here so wave 6's docs pass notices where the naming live.
+
+Wave 5 decomposition may proceed.
 
 ## Summary
 
-> To be filled on completion
+**Review gate passed: 0 conformance findings; 1 open flake recorded
+(F-1) + 3 documentation/coverage-shaped notes (F-2..F-4, no action);
+all gates green.**
+
+The gate's primary lens (engine-vs-contract conformance, line-by-line
+against core-contract.md and the ADRs) came back clean across every
+checklist item: entry-point validation (both paths), error arms, value
+shapes, no-work vocabulary, the ADR-023 guards at their pinned sites,
+the ADR-016 §5 client-side typed 8000-byte check with one limit owner,
+the forwarder's structural exclusions + reconnect + re-LISTEN +
+reserved-string reconnect-wake + no-replay honesty + the pg close arm,
+seam integrity incl. the unknowable-state discard arms and pool
+accounting, the natively-async posture (no `spawn_blocking` anywhere),
+the backoff curve and scheduler boundary math matching the ADR text
+exactly, the one-schema/`bytea`/no-per-queue-tables posture, the
+10-row backlog column (stamped, green against the pg factory, twice),
+and family standards.
+
+Gates at review time: `cargo build` / `cargo test` /
+`cargo clippy --all-targets -- -D warnings` / `cargo fmt --check` all
+green; the pg lib suite 111/111 green against the harness server
+(2×47 s full runs + stream-module and suite-target re-runs).
+
+The one thing this gate cannot close: the flagged
+`tx_publishes_compose_with_the_handle` flake was **reproduced twice**
+(~2/106 runs) and root-caused to neither engine code nor the test's
+beat ordering — the engine's wake path is verified honest (64-run
+stand-alone probe, whole-module and 30× full-suite runs clean); the
+miss is a still-unexplained wake-subscription interaction under load.
+Recorded as F-1 for wave 5's suite hardening (candidate dispositions
+in Notes); the task agents' "wait for wave 5 if it recurs" posture is
+adopted — it recurred, wave 5 gets the record.
+
+Wave 5 decomposition may proceed.