diff --git a/docs/plans/implementation.md b/docs/plans/implementation.md index 9660b3e..8e30463 100644 --- a/docs/plans/implementation.md +++ b/docs/plans/implementation.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-09 (wave-4 general review written — 2 live-proven bugs incl. F-1's root cause; fixes pending; reviews renumbered 001/002 per the alk* numbering pattern) +last_updated: 2026-10-09 (wave-4 fix batch decomposed from the general review — 7 fix tasks + review-wave-4-fixes gate; wave 5 decomposition gates on the two HIGH fixes landing) --- # alkstore — Implementation plan @@ -257,4 +257,18 @@ decomposes. Specific gates: - **Wave 4 decomposition** (2026-10-08) — shaped as wave 3's structural twin; wave-3 outcomes absorbed (constructors exist; arithmetic re-owned per engine; per-engine `@every` parser; - SQLite-scoped deferred notes stay put). See the Wave 4 section. \ No newline at end of file + SQLite-scoped deferred notes stay put). See the Wave 4 section. +- **Wave-4 fix-batch decomposition** (2026-10-09) — the general + review's findings decomposed into seven `pg-fix-*` tasks + a + `review-wave-4-fixes` gate: Finding 1 → `pg-fix-forwarder-reconnect` + (with the failed-connect test seam the gate's blind spot demands), + Finding 2 → `pg-fix-tx-wake` (retires F-1's engine arm), Finding 3 + + the DSN-options note → `pg-fix-open-path`, Finding 4 → + `pg-fix-stale-unlisten`, Finding 5 + the scheduler doc fix → + `pg-fix-scheduler-resilience`, the decode-duplication smell + small + cleanups → `pg-fix-dedupe-cleanup`, the TLS/QueueOpts doc + alignments → `pg-fix-docs-alignment`. Sequencing per the review's + recommendation: **wave 5 decomposes after the two HIGH fixes land** + (the suite's wake-driven rows would otherwise inherit a hang-shaped + false failure); 3–7 may trail into wave 5's window if the + fix-batch gate judges them wake-independent. \ No newline at end of file diff --git a/tasks/pg-fix-dedupe-cleanup.md b/tasks/pg-fix-dedupe-cleanup.md new file mode 100644 index 0000000..546a875 --- /dev/null +++ b/tasks/pg-fix-dedupe-cleanup.md @@ -0,0 +1,83 @@ +--- +id: pg-fix-dedupe-cleanup +name: Decode dedupe + small cleanups in queue/tx/notify (review 002 minor notes) +status: pending +depends_on: [pg-fix-tx-wake] +scope: narrow +risk: low +impact: component +level: implementation +tags: [wave-4-fixes, postgres-engine, cleanup] +--- + +## Description + +Batch the wave-4 general review's minor notes that share files and are +too small for standalone tasks +(`docs/reviews/002-wave-4-general-review.md` §Minor notes + §Finding +6's queue bullet): + +- **`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 `Job` 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`). The + doc comments must again tell the truth (one decode owner). +- **`queue.rs:71` doc fix (Finding 6)**: `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.) Correct both claims. +- **`notify.rs:106-110`**: the closed-store error is built inline + (`Error::database(std::io::Error::other(...))`) instead of the + shared `database_error` helper — one duplicate of the one message. + Route it through the helper. +- **`bridge_capacity()`** (`notify.rs:137-139`) is a function + returning a constant — a `const` says it plainly. Convert (keep the + doc comment's rationale on the const). + +No behavior changes anywhere in this task — it is dedupe + doc +truth-telling. All existing tests must pass unchanged; where a test +asserts on the moved-count semantics, it already pins the sum (the +doc was the only liar). + +**Depends on `pg-fix-tx-wake`**: that task edits `tx.rs`'s +publish/enqueue paths; this task restructures `tx.rs`'s decode side — +landing after avoids touching the same file regions blind. + +## Acceptance Criteria + +- [ ] `job_from_row` has one owner (`queue.rs`); `tx.rs` imports it; + both files' "one decode owner" doc claims are true +- [ ] `get_job_tx`'s dead/live column lists reuse `queue.rs`'s + `live_columns()`/`dead_columns()` (no inlined duplicates) +- [ ] `sweep_expired`'s doc states the moved + retention-deleted sum + and the `in_tx` frame (matching the SQLite twin's semantics) +- [ ] `notify.rs`'s closed-store error routes through + `database_error`; `bridge_capacity` is a `const` +- [ ] Zero behavior change: the full pg lib suite green against the + harness server, unchanged assertions +- [ ] `cargo clippy --all-targets -- -D warnings`, fmt clean; gates + green server-less + +## References + +- docs/reviews/002-wave-4-general-review.md §Minor notes + §Finding 6 + (the queue bullet) +- docs/architecture/core-contract.md (the pinned `Job` shape — why one + decode owner matters) +- tasks/pg-engine-queues.md, tasks/pg-engine-seam-tx.md (the tasks of + record for the touched files) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/pg-fix-docs-alignment.md b/tasks/pg-fix-docs-alignment.md new file mode 100644 index 0000000..bc3732a --- /dev/null +++ b/tasks/pg-fix-docs-alignment.md @@ -0,0 +1,82 @@ +--- +id: pg-fix-docs-alignment +name: Doc alignment — TLS posture, QueueOpts consumer obligations (review 002 Finding 6 remainder) +status: pending +depends_on: [pg-fix-forwarder-reconnect] +scope: single +risk: trivial +impact: isolated +level: implementation +tags: [wave-4-fixes, postgres-engine, docs] +--- + +## Description + +The doc-alignment remainder of the wave-4 general review +(`docs/reviews/002-wave-4-general-review.md` §Finding 6 + §Minor +notes) — the items that live in docs rather than the code files the +other fix tasks own. (The scheduler and `sweep_expired` doc fixes are +NOT here — they ride `pg-fix-scheduler-resilience` and +`pg-fix-dedupe-cleanup`, their files' tasks.) + +- **TLS posture honesty**: every connection (pooled `store.rs:161`, + 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. Fix both ends: + correct the forwarder doc to match the code, and **`deployment.md` + owns the statement** (v1 ships `NoTls` on all connection paths; + TLS is a post-v1 deployment concern — the deployment matrix's + honesty posture, ADR-016's spirit). Check the crate-level docs and + `PgOpts` docs for the same claim while there. +- **QueueOpts numeric consumer-obligation note**: 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) prevents surprise. Home: `deployment.md` + (the budget/consumer-obligation section), with a mirror sentence in + `queue.rs`'s or `opts.rs`'s doc where the opts are documented. +- **Light mismatch sweep**: re-read the pg engine's doc comments + touched by the fix batch (forwarder, tx, scheduler, store) for any + *new* doc-behavior mismatch the fixes introduced — the fix tasks + correct their own sites, this is the cross-file consistency pass. + +Doc-only task: no code behavior changes; gates must stay green. + +**Depends on `pg-fix-forwarder-reconnect`** only because the forwarder +doc being corrected is in the file that task restructures — land after +it and correct against the settled code. + +## Acceptance Criteria + +- [ ] `forwarder.rs`'s sslmode claim matches the code (`NoTls` + hardwired); no other doc in the crate repeats the claim +- [ ] `deployment.md` states the v1 TLS-unavailable posture on all + connection paths +- [ ] `deployment.md` (plus the opts' doc home) carries the + QueueOpts numeric consumer-obligation note +- [ ] Cross-file doc sweep over the fix batch's touched files: no + doc-behavior mismatch remains (grep-auditable claims spot-checked) +- [ ] `cargo build`, clippy `-D warnings`, fmt clean (doc-only change; + tests unaffected) + +## References + +- docs/reviews/002-wave-4-general-review.md §Finding 6 + §Minor notes + (TLS bullet, QueueOpts bullet) +- docs/architecture/deployment.md (the budget/consumer-obligation + home; the visibility-budgeting note's precedent shape) +- docs/architecture/decisions/016-deployment-honesty.md (the honesty + posture) +- docs/architecture/decisions/023-fourth-review-round.md (the domain + table — why QueueOpts numerics are consumer-obligation, not defect) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/pg-fix-forwarder-reconnect.md b/tasks/pg-fix-forwarder-reconnect.md new file mode 100644 index 0000000..2904585 --- /dev/null +++ b/tasks/pg-fix-forwarder-reconnect.md @@ -0,0 +1,110 @@ +--- +id: pg-fix-forwarder-reconnect +name: Fix forwarder permanent death after one failed reconnect (review 002 Finding 1) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [wave-4-fixes, postgres-engine, forwarder] +--- + +## Description + +Fix the HIGH finding from the wave-4 general review +(`docs/reviews/002-wave-4-general-review.md` §Finding 1, live-proven): +the forwarder loop **dies permanently after one failed reconnect +connect**. Site: `alkstore-postgres/src/forwarder.rs:498-509` → +`: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 forever. Consequences +(all live-proven): every later `listen()` fails with a spurious +"mid-reconnect" `Database` for the store's remaining lifetime; wake +delivery stops permanently (no re-LISTEN, no reconnect-wake); stream +subscribers parked on `wait_wake` stay parked forever (the fanout's +senders stay alive — handle-held — so the broadcast never closes and +the consumer never even sees a terminal arm), contradicting the +streams row's pinned posture that *events never require polling to +become visible* (core-contract.md streams section). + +**Fix shape** (the review's): 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`. The loop top's `None` arm must become unreachable by +construction (or restructured away entirely: e.g. carry the connection +slot through the reconnect arm so a failed connect retries in place +with the slot still empty but the loop alive). + +**Hardening (the review's second point)**: make every `forwarder_loop` +exit path release the `Forwarder` struct's fanout sender (the +shutdown-take machinery already exists — `Forwarder::shutdown` takes +it) so *any* loop exit — not just the shutdown flip — surfaces the +terminal `Closed`/`None` arm to subscribers instead of a +forever-silence. Invariant to establish and pin: **receivers go +terminal iff the forwarder loop is gone**. With the retry fix the loop +only exits at shutdown, so this is defense-in-depth against the next +abnormal-exit bug, not a behavior change. + +**Testability — the gap that hid the bug**: the gate's 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 *unreachable-server window* (connect +fails) is the one case the machinery exists for and the one case +untested. The fix must come with a test that exercises it. Recommended +shape: a `cfg(test)` seam on the reconnect config (the loop ctx's +`reconnect_config` behind a swappable slot, or the connect step behind +an injectable closure) so a test can point reconnects at an +unreachable endpoint after `open`, force a reconnect (backend kill), +verify the loop survives *repeated* failed connects across several +backoff cycles, then swap the real config back and verify full +recovery. Alternative: extract the connect step behind a connector +trait/closure and unit-test the retry loop with a failing connector — +implementer's choice, but the failed-connect arm must be +test-exercised, not reasoned-about. + +## Acceptance Criteria + +- [ ] A failed reconnect connect no longer exits the loop: the + reconnect arm retries with the live backoff (50 ms → 2 s cap) + until success or shutdown — pinned by a test that forces + repeated failed connects (unreachable-endpoint seam or connector + injection) across multiple backoff cycles +- [ ] After the server becomes reachable again: the synthetic + reconnect-wake broadcasts, post-reconnect `NOTIFY` delivers on + registered channels, and `listen()` succeeds (no spurious + "mid-reconnect" `Database`) — recovery pinned by test +- [ ] Every `forwarder_loop` exit path releases the fanout sender — + an abnormal loop exit surfaces the terminal arm to subscribers + (the receivers-terminal-iff-loop-gone invariant; the shutdown + case's existing pin stays green) +- [ ] The existing backend-kill reconnect tests + (`forwarder_reconnects_after_backend_kill_and_delivers`, + `receiver_stays_open_across_reconnects_closes_at_shutdown`, the + no-replay pin) stay green unchanged +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; gates green server-less + +## References + +- docs/reviews/002-wave-4-general-review.md §Finding 1 (the finding, + the probe, the fix shape) +- docs/architecture/decisions/004-postgres-driver.md (forwarder + decision, the hand-rolled posture's owned failure modes) +- docs/architecture/decisions/006-wake-and-delivery-contract.md + (reconnect-recovery semantics) +- docs/architecture/core-contract.md (streams: events never require + polling to become visible) +- tasks/pg-engine-notify-listen.md (the forwarder's task of record; + Notes carry the shutdown sender-take decision this hardening builds on) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/pg-fix-open-path.md b/tasks/pg-fix-open-path.md new file mode 100644 index 0000000..3df0607 --- /dev/null +++ b/tasks/pg-fix-open-path.md @@ -0,0 +1,84 @@ +--- +id: pg-fix-open-path +name: Fix `max_size: 0` open hang + DSN options override (review 002 Finding 3 + options note) +status: pending +depends_on: [] +scope: single +risk: low +impact: component +level: implementation +tags: [wave-4-fixes, postgres-engine, open] +--- + +## Description + +Two open-path fixes in `alkstore-postgres/src/store.rs` +(`open_store`), from the wave-4 general review: + +**Finding 3 (MEDIUM, live-proven)**: `PgOpts { max_size: 0 }` hangs +`open` forever. 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, so a `max_size: 0` pool 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 (the review's): validate `max_size > 0` in `open_store` — +typed `Error::Database` **before any round trip** (the engine-wide +validate-at-entry posture; the source-chain detail should say what was +rejected, e.g. "max_size must be positive"). The alternative shape +(configure deadpool timeouts so the hang surfaces as an error) is +weaker — a timeout-configured hang still burns the consumer's time; +prefer the guard. Do both only if trivial. + +**Minor note (same file, same surface)**: `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. Preferred fix: **append** to the DSN's existing options +string (tokio-postgres `Config::get_options()` reads what the parse +carried) so the engine's `synchronous_commit` SET rides alongside +whatever the consumer configured. If appending proves impractical, +document at the site and in the crate docs that engine-configured +options win — but append is the honest default (the consumer's DSN +options were there first). + +Tests (against the harness server): `max_size: 0` fails typed and +*promptly* (assert the error; the test's own structure bounds the +hang — a regression re-hangs and fails the test by timeout); a DSN +carrying pre-existing `options=` plus `open` succeeds and +`synchronous_commit` is still observable via `SHOW` on both settings +(the append path doesn't break the engine's SET); the existing +opts-flow tests stay green. + +## Acceptance Criteria + +- [ ] `max_size: 0` fails `open` with a typed `Database` error before + any round trip — pinned by test (prompt failure, no hang) +- [ ] The consumer DSN's `options=` survive: appended, not replaced — + pinned by test (DSN with options + engine SET coexist, `SHOW` + observable) +- [ ] If append proved impractical instead: the engine-wins semantics + documented at the site and in crate docs (record the decision in + Notes) +- [ ] Existing open/opts tests stay green +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; gates green server-less + +## References + +- docs/reviews/002-wave-4-general-review.md §Finding 3 + §Minor notes + (the options bullet) +- docs/architecture/engine-postgres.md (Connection architecture) +- docs/architecture/decisions/008-contract-v1-pinning.md §6 (config + split) +- tasks/pg-engine-open-opts.md (the open task of record; Notes carry + the connect-options SET mechanics this task extends) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/pg-fix-scheduler-resilience.md b/tasks/pg-fix-scheduler-resilience.md new file mode 100644 index 0000000..572bdd8 --- /dev/null +++ b/tasks/pg-fix-scheduler-resilience.md @@ -0,0 +1,112 @@ +--- +id: pg-fix-scheduler-resilience +name: Scheduler runner resilience — quarantine bad rows, retry transient errors (review 002 Finding 5) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [wave-4-fixes, postgres-engine, scheduler] +--- + +## Description + +Fix the LOW/MEDIUM finding from the wave-4 general review +(`docs/reviews/002-wave-4-general-review.md` §Finding 5): one bad row +or one transient error permanently kills `run_schedules`. Sites: +`alkstore-postgres/src/scheduler.rs:403` +(`parse_every_interval(&row.spec)?`), `:429` (fire +`enqueue_row(...)?`), `:519-526` (the `in_tx` tick `?`) — all +propagating straight out of the leader loop. The `@every` grammar is +re-parsed from stored text per tick (unvalidated-on-read), so a +tampered or future-foreign spec row makes the whole runner exit with +`InvalidSpec`; 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. + +**Fix shape** (the review's, made concrete): + +- **Bad-row quarantine, not fatality**: a stored spec that fails + re-parse is *quarantined* — log (`eprintln!`, the engine's + diagnostic posture) with the schedule name, advance the row's + `next_fire_at` past now (the tick's skip-forward helper — the row + must not spin), and continue with the remaining due rows. The tick + tx still commits (the quarantine advance is a normal write). + Re-validate-on-read at *registration* is unnecessary (specs are + pre-validated at `schedule()`); quarantine is only the tampered/ + foreign-row arm. +- **Transient-error retry**: a tick that fails with a pool/database + error retries N times (pin N — 3 is the natural floor) with a short + backoff before giving up and returning `Err` from the loop (the + consumer's respawn recipe remains the last resort, but a single + hiccup no longer kills the runner). Leadership renewal continues to + own the loss decision — a retry must not mask a lost lease (the + renew check at the loop top already runs before each tick). +- **Error context**: the exiting error's source chain carries the + schedule name / tick phase (via the `database_error` message shape + — `Database` Display is opaque; the chain is the carrier). +- **Exit-path leadership honesty**: on the `Err` exit path the + leadership lock is not released — the TTL lapse covers it. Say so + in the module docs (the review's fine print); opportunistically + releasing on the error exit is acceptable if it stays simple, but + the documented TTL-lapse posture is the bar. +- **Doc-comment fix (Finding 6's scheduler bullet, same file)**: + `scheduler.rs:46-53` claims "a newly-registered schedule is noticed + by the re-read no later than the next slice (1 s)" — false: the 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 `IDLE_SLEEP_S` (60 s) late; an active runner waits for the + current soonest deadline. Correct the comment to the honest posture + (the 60 s idle-tick floor is already acknowledged two paragraphs + down as acceptable — the doc comment above it contradicts it). + Implementing the per-slice soonest re-check is *optional* (a + behavior improvement, not required by this task) — if done, the + corrected comment describes it. + +Tests: a tampered spec row is quarantined (the runner survives, other +schedules still fire, the bad row's boundary advanced — behavioral, +against the harness server, tampering via a direct SQL UPDATE); the +retry policy pinned (unit-level pin of the retry count/backoff shape +is the bar; a behavioral transient-error injection is welcome if a +deterministic arrangement exists); the doc-comment claim matches the +code. + +## Acceptance Criteria + +- [ ] A tampered/foreign spec row is quarantined (logged with name, + boundary advanced past now, other schedules still fire, runner + survives) — pinned by behavioral test +- [ ] A transient tick error is retried N times (pinned N) with + backoff before the loop returns `Err` — policy pinned by test +- [ ] The exiting error's chain carries schedule-name/tick-phase + context +- [ ] The error-exit leadership posture (TTL lapse covers the + unreleased lock) stated in the module docs +- [ ] The `scheduler.rs:46-53` doc claim corrected to the honest + slice/idle posture +- [ ] Existing scheduler tests stay green (clean stop, + `LeadershipLost`, catch-up cap, boundary math) +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; gates green server-less + +## References + +- docs/reviews/002-wave-4-general-review.md §Finding 5 + §Finding 6 + (the scheduler doc bullet) +- docs/architecture/decisions/009-scheduler-boundaries.md (the + boundary guarantee the quarantine advance must preserve) +- docs/architecture/decisions/010-queue-stamps-and-backoff.md §3 +- tasks/pg-engine-scheduler-outbox.md (the scheduler's task of record) +- alkstore-sqlite/src/scheduler.rs (the structural twin — check its + error posture for parity notes) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/pg-fix-stale-unlisten.md b/tasks/pg-fix-stale-unlisten.md new file mode 100644 index 0000000..8153fc3 --- /dev/null +++ b/tasks/pg-fix-stale-unlisten.md @@ -0,0 +1,100 @@ +--- +id: pg-fix-stale-unlisten +name: Fix stale queued UNLISTEN cancelling a re-issued LISTEN after reconnect (review 002 Finding 4) +status: pending +depends_on: [pg-fix-forwarder-reconnect] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [wave-4-fixes, postgres-engine, forwarder] +--- + +## Description + +Fix the LOW/MEDIUM finding from the wave-4 general review +(`docs/reviews/002-wave-4-general-review.md` §Finding 4): a stale +queued UNLISTEN can cancel a re-issued LISTEN after reconnect. Sites: +`alkstore-postgres/src/forwarder.rs:294-300` (unregister best-effort), +the snapshot re-issue `: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** (the review's two candidates — pick one, record in +Notes): + +1. **Generation-tag the commands**: tag each `ListenCommand` with the + connection generation it was queued under; on reconnect, drop + queued commands that predate the current generation. Rationale: + stale LISTENs are redundant (the snapshot re-issue covers them — + registry-write-first ordering means the registry is the source of + truth); stale UNLISTENs are the hazard. Commands queued *after* + the reconnect began (new registrations with acks) must still + process — the generation tag discriminates. +2. **Reconcile UNLISTENs against the registry snapshot** instead of + replaying them verbatim: an UNLISTEN only issues when the registry + currently shows zero subscribers for that channel. + +Either way the invariant to establish: **after a reconnect, the +server-side LISTEN set matches the registry snapshot and nothing +queued from a dead generation can undo it.** + +**Testing**: the full race is timing-narrow, so pin the decision +logic directly — extract the drop/reconcile decision as a pure helper +(commands + current generation + registry snapshot → the commands to +issue) and unit-test it server-less: a stale-generation UNLISTEN is +dropped; a current-generation UNLISTEN issues; a stale LISTEN is +dropped (snapshot covers it); a current-generation LISTEN issues. A +behavioral pin through the live forwarder is welcome if a +deterministic arrangement exists, but the unit pin is the acceptance +bar. Also correct the "replay harmlessly" doc comment at the replay +site to state the actual posture. + +**Depends on `pg-fix-forwarder-reconnect`**: both tasks restructure +the same loop (the reconnect arm and the command replay); the +reconnect fix lands first so this task builds on the settled loop +shape. + +## Acceptance Criteria + +- [ ] A stale queued UNLISTEN can no longer unlisten a channel the + registry believes is listening — the generation-drop (or + reconcile) decision pinned by server-less unit tests covering + all four command/staleness combinations +- [ ] Current-generation commands (acked registrations included) + still process normally after a reconnect — existing + register/ack tests stay green +- [ ] The "replay harmlessly" doc comment corrected to the actual + posture +- [ ] The invariant stated in the forwarder docs (post-reconnect + server LISTEN set = registry snapshot) +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; gates green server-less + +## References + +- docs/reviews/002-wave-4-general-review.md §Finding 4 (the sequence, + both fix shapes) +- docs/architecture/decisions/004-postgres-driver.md (forwarder + decision) +- tasks/pg-engine-notify-listen.md (the forwarder's task of record — + registry-write-first ordering, the acked register) +- tasks/pg-fix-forwarder-reconnect.md (lands first; the loop shape + this builds on) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/pg-fix-tx-wake.md b/tasks/pg-fix-tx-wake.md new file mode 100644 index 0000000..3150323 --- /dev/null +++ b/tasks/pg-fix-tx-wake.md @@ -0,0 +1,105 @@ +--- +id: pg-fix-tx-wake +name: Fix tx enqueue/publish paths issuing no pg_notify wake (review 002 Finding 2 — F-1's root cause) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [wave-4-fixes, postgres-engine, tx, wake] +--- + +## Description + +Fix the HIGH finding from the wave-4 general review +(`docs/reviews/002-wave-4-general-review.md` §Finding 2, live-proven): +none of the tx-side enqueue/publish paths run `pg_notify` in the +caller's transaction. Sites: `alkstore-postgres/src/tx.rs:290-329` +(`publish_tx`/`publish_with_key_tx`), `tx.rs:272-288` (`enqueue_tx`), +`tx.rs:545-562` (`outbox_enqueue_tx`). `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. A +*committed* `publish_tx`/`enqueue_tx` never wakes a registered +listener (deterministic silence, live-proven), while the auto-commit +`publish`/`Queue::enqueue` wake immediately. + +**This is F-1's root cause, engine-side** — `tasks/review-wave-4.md` +F-1's "root cause unresolved" and the gate's "the tx INSERT's commit +*is* the NOTIFY carrier" conclusion were both wrong. Mechanism: the +subscriber bridge attach-read + drain race the commit; when both beat +the commit the bridge drains empty and parks on `wait_wake` — and no +wake ever comes from a tx-committed publish, so the 15 s deadline +exhausts (~2% under the 8-thread suite). + +**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 dropping the event row *with its wake*. 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 shape** (the review's): + +- `publish_with_key_tx`: after the INSERT, one + `SELECT pg_notify($1, '')` inside the caller's tx — the channel is + the stream name (the mechanism-name-is-the-channel realization, + already pinned in `stream.rs`'s docs). Empty payload: the wake + carries `Wake { channel }` only, never payloads (ADR-008 §3). This + is the **contract obligation** — the load-bearing fix. +- `enqueue_tx` and `outbox_enqueue_tx`: add the same wake (channel = + queue name; the outbox's derived `__alkstore_outbox:{name}`) for + **posture parity** with SQLite's watcher — a weaker obligation (the + queues row's pinned re-poll safety net covers correctness; the wake + is latency posture), but the asymmetry is free to close and the + review recommends it. +- `run_once` is a pull op, consumer-driven — no wake needed there + (do not add one). + +**Record updates**: this fix retires F-1's engine arm. Update +`tasks/review-wave-4.md`'s F-1 note (engine arm retired by this fix; +the wave-5 suite-hardening candidates — parked-recv deadline, lag +diagnostics — remain valid as defense-in-depth but are no longer +load-bearing for this flake) and `docs/plans/implementation.md`'s +review-rounds line for the general review. + +## Acceptance Criteria + +- [ ] `publish_with_key_tx` issues the `pg_notify` wake commit-atomically + (channel = stream name, empty payload) — pinned by test: a + registered listener receives the wake at/after the caller's + commit, deterministically +- [ ] The no-ghosts arm pinned: a *rolled-back* `publish_tx` produces + no wake (the wake is commit-atomic, not statement-atomic) +- [ ] `enqueue_tx` and `outbox_enqueue_tx` issue their wakes (channel = + queue name / derived backing queue) — pinned by test +- [ ] `tx_publishes_compose_with_the_handle` is deterministic: repeated + solo runs (≥ 20) green with zero deadline misses +- [ ] Auto-commit `publish`/`enqueue` behavior unchanged (still wake; + no double-wake regression pinned where the suite observes it) +- [ ] `tasks/review-wave-4.md` F-1 note + `docs/plans/implementation.md` + review-rounds line updated to record the retirement +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; gates green server-less + +## References + +- docs/reviews/002-wave-4-general-review.md §Finding 2 (the finding, + the probe, the fix shape, the F-1 mechanism) +- docs/architecture/engine-postgres.md (pg_notify as the streams wake + trigger) +- docs/architecture/core-contract.md (streams visibility; tx/no-ghosts + rows) +- docs/architecture/decisions/008-contract-v1-pinning.md §3 (Wake + carries channel only) +- tasks/review-wave-4.md (F-1's record — updated by this task) +- alkstore-postgres/src/tx.rs (`notify_tx` — the proven commit-atomic + mechanics these paths adopt) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/review-wave-4-fixes.md b/tasks/review-wave-4-fixes.md new file mode 100644 index 0000000..f0e2830 --- /dev/null +++ b/tasks/review-wave-4-fixes.md @@ -0,0 +1,93 @@ +--- +id: review-wave-4-fixes +name: Review gate — wave-4 fix batch (review 002 resolutions) +status: pending +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 +impact: phase +level: review +tags: [wave-4-fixes, review] +--- + +## Description + +Review the fix batch decomposed from the wave-4 general review +(`docs/reviews/002-wave-4-general-review.md`) before wave 5 +decomposes. The batch's own gate — distinct from the general review +that found the issues: verify each finding's fix is *real* (code-read +against the review's failure mechanism, not test-name-trust), and +that the fixes didn't damage the postures the wave-4 gate pinned. + +Check: + +- **Finding 1 (forwarder reconnect)**: the loop survives repeated + failed connects — code-read the restructured reconnect arm (no path + back to the `take()`'s `else return`); the failed-connect arm is + test-exercised (the seam/injection is real, not a mock that never + fails); the fanout-sender release invariant (receivers terminal iff + loop gone) holds on every exit path. +- **Finding 2 (tx wake)**: `pg_notify` present in + `publish_with_key_tx` (contract obligation) and the enqueue twins + (parity), commit-atomic inside the caller's tx, empty payload + (channel-only wakes); the no-ghost rollback arm pinned; the compose + test's determinism evidence (repeated solo runs) recorded. +- **Finding 3 (open path)**: `max_size` guard before any round trip; + the DSN options append (or documented engine-wins) verified against + the actual setter semantics. +- **Finding 4 (stale UNLISTEN)**: the generation-drop/reconcile + decision unit-pinned across all four command/staleness + combinations; the invariant stated in the forwarder docs. +- **Finding 5 (scheduler)**: quarantine + retry behavior vs the + review's fix shape; the leadership exit-path posture documented; the + doc claim corrected. +- **Minor notes**: dedupe (one decode owner — grep-auditable), the + const, the helper routing, the doc alignments in deployment.md. +- **No regressions**: the wave-4 gate's pinned postures re-spot-checked + where the fixes touched them — forwarder integrity (the two + deadlock pitfalls still structurally excluded), seam integrity, + close semantics (the pg arm: open across reconnects, close at + shutdown only), the no-replay honesty pin, entry-point validation. +- **Records**: `tasks/review-wave-4.md` F-1 note and + `docs/plans/implementation.md`'s review-rounds line consistent with + what landed. +- **Gates**: full workspace build/test/clippy/fmt; the pg suite green + against the harness server; the compose test run repeatedly (the + determinism claim verified, not asserted). + +**Wave-5 gate**: per the general review's sequencing, wave 5 +decomposes after Findings 1–2's fixes land (the suite's wake-driven +rows would otherwise inherit a hang-shaped false failure). This gate +confirms both landed; if only they have, say so explicitly — wave 5 +may decompose with 3–7 still in flight only if this gate judges the +remainder wake-independent (expected: yes — 3–7 touch no wake-driven +suite surface). + +## Acceptance Criteria + +- [ ] 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- + checked at the touched sites) +- [ ] Records consistent (F-1 retirement, review-rounds line) +- [ ] All gates green (incl. against the harness server); compose-test + determinism evidence recorded +- [ ] Findings recorded; wave 5 decomposition disposition stated + explicitly + +## References + +- docs/reviews/002-wave-4-general-review.md (the findings this batch + resolves) +- tasks/review-wave-4.md (the wave-4 gate's pinned postures + F-1's + record) +- docs/plans/implementation.md (Review gates; Review rounds so far) +- tasks/pg-fix-*.md (the batch's seven tasks) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file