alkstore/tasks/review-wave-4.md

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

id	name	status	depends_on	scope	risk	impact	level	tags
review-wave-4	Review gate — wave 4 (Postgres engine)	completed	pg-engine-integration	moderate	low	phase	review	wave-4 review

Description

Review the Postgres engine before wave 5 decomposes. Primary lens per the plan's review gates: engine-vs-contract conformance — the same discipline as the wave-3 gate, now against the second engine, with the pg-specific machinery added. Wave 5 (the contract suite) pins cross-engine equivalence; this gate checks the pg implementation against the pinned contract text line by line.

Check:

• Contract conformance: every trait method's behavior against core-contract.md's per-mechanism text — validation at entry points (auto-commit AND tx paths), error arms (Database-only receivers, Codec at payload seams, LeadershipLost vs clean stop), value shapes (Job/StreamEvent field-for-field), the no-work-is-a-value vocabulary (None/false/empty-Vec where pinned, never errors).
• ADR-023 follow-through: extent guards at trait-impl entry (claims + all four stream reads); duration guards on both lock call sites; encode_payload consumed fallibly at every call site.
• ADR-016 §5: the 8000-byte check is client-side, typed, before any round trip, on both notify and notify_tx; the limit constant has one owner.
• Forwarder integrity (the pg engine's own high-risk machinery — the analogue of the wave-3 gate's seam-integrity lens): the two POC-pinned deadlock pitfalls structurally excluded (poll loop before first query; Client alive for the listener's lifetime); reconnect backoff + re-LISTEN from the live channel set; the synthetic reconnect-wake on the reserved string broadcast to every subscriber; lag surfaced, not silent; receiver close semantics exactly the pg arm (open across reconnects, close at shutdown only, terminal, never reopens).
• Seam integrity: drop = rollback under panic; failed COMMIT/ROLLBACK arms discard (not re-pool) the object — pool accounting exact under error paths (deployment.md's ADR-021 note); ops-after-consume fail closed; no spawn_blocking anywhere (the natively-async posture — a stray bridge would be a finding).
• Backoff curve + boundary math: the engine-side formulas match ADR-010 §3's pinned range/cap and ADR-009 §4's boundary guarantee exactly (wave 5 pins cross-engine equivalence; this gate checks this implementation against the text).
• Schema posture: one engine-owned schema; payload columns bytea (exact bytes — no JSONB normalization); no per-queue tables; bootstrap idempotent.
• Backlog column: the engine's suite rows present, stamped, green against the pg factory; the PayloadTooLarge pg arm row present.
• Family standards: no panics, no unwrap/expect outside tests, no inline comments (doc comments fine), module-per-file.
• Gates: full workspace build/test/clippy/fmt; the pg suite green against the harness server.

Acceptance Criteria

• Contract conformance spot-checked against the spec text (not just the tests)
• ADR-023's numeric-domain + typed-encoding follow-through verified at their sites
• Forwarder + seam integrity verified (code-read, not test-name-trust)
• Backoff curve + scheduler boundary math match the pinned ADR text
• Backlog column present, stamped, green against the pg factory
• All gates green (incl. against the harness server)
• Findings recorded; wave 5 decomposition may proceed

References

• docs/plans/implementation.md (Review gates)
• docs/architecture/core-contract.md
• docs/architecture/engine-postgres.md
• docs/architecture/decisions/004-postgres-driver.md
• docs/architecture/decisions/016-deployment-honesty.md §5
• docs/architecture/decisions/023-fourth-review-round.md
• tasks/review-wave-3.md (the gate discipline's precedent)

Notes

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 expects 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-7 item (release readiness) — wave 8 since the fuzzing wave's renumber — 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 at gate time — resolved engine-side by the general review (see F-1's updates 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). Update (2026-10-09): root cause found by the wave-4 general review (docs/reviews/002-wave-4-general-review.md, Finding 2) — the tx enqueue/publish paths issue no pg_notify wake at all; the subscriber's pre-commit drain parks on a wake that never comes. Fix is engine-side (one pg_notify in publish_with_key_tx), not the suite-hardening candidates below, which become defense-in-depth. Update 2 (2026-10-09): retired — pg-fix-tx-wake landed the engine-side fix. The tx producer paths (publish_with_key_tx, enqueue_tx, outbox_enqueue_tx) now issue their pg_notify wakes commit-atomically inside the caller's tx (stream name / queue name / derived backing queue as the channel; rollback drops the row with its wake), pinned by tests; tx_publishes_compose_with_the_handle is deterministic (20 consecutive solo runs green). The suite-hardening candidates below remain valid as defense-in-depth but are no longer load-bearing for this flake. Correction to the facts below: the "engine's wake path is honest — the tx INSERT's commit is the NOTIFY carrier" conclusion was wrong (nothing on that path ever sent a NOTIFY); the 64-run probe simply sampled the lucky attach-read/drain-vs-commit ordering every time. 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 7's docs pass notices where the naming live.

Wave 5 decomposition may proceed.

Summary

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. (Superseded 2026-10-09: the wake-path-honesty conclusion was wrong — the general review root-caused the flake engine-side — and pg-fix-tx-wake retired F-1; see F-1's updates in Notes.)

Wave 5 decomposition may proceed.