diff --git a/tasks/core-fix-display-log-forging.md b/tasks/core-fix-display-log-forging.md new file mode 100644 index 0000000..9592a0f --- /dev/null +++ b/tasks/core-fix-display-log-forging.md @@ -0,0 +1,59 @@ +--- +id: core-fix-display-log-forging +name: Control-char-safe Display for the consumer-string error variants (review 004 item 14) +status: pending +depends_on: [] +scope: single +risk: low +impact: isolated +level: implementation +tags: [review-004-fixes, core, errors] +--- + +## Description + +The core error `Display` log-forging risk, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 14: `InvalidName`/`ReservedName`/`InvalidSpec` +interpolate raw consumer strings into `Display`; control characters +can forge log lines in naive consumers. Typical-idiom risk, low — but +the fix is small and real. + +Fix: escape/sanitize the interpolated payload in the three variants' +`Display` arms (e.g. `char::escape_debug` or a control-char filter) so +the rendered message cannot carry raw newlines/control bytes from +consumer input. The error taxonomy itself is untouched — variants, +kinds, and match sites stay exactly as pinned (ADR-008 §5); only the +rendered text's hygiene changes. + +While there: grep the `Error` enum's other `Display` arms for the same +consumer-string interpolation class; if any other variant carries it, +include it in the same fix (and say so in Notes). + +## Acceptance Criteria + +- [ ] The three variants' `Display` renders consumer strings + control-char-safe — pinned by test (a name/spec carrying `\n` + and control bytes renders without any raw control byte) +- [ ] The error taxonomy unchanged (variants, kinds, match sites); + existing error/validation tests green unchanged +- [ ] Any additional same-class `Display` arms included (or the + absence recorded in Notes) +- [ ] `cargo test -p alkstore`, clippy `-D warnings`, fmt clean; doc + gate green + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 14 (the finding) +- docs/architecture/decisions/008-*.md §5 (the error taxonomy — the + part that must not change) +- alkstore/src/errors (the `Display` arms) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/core-pin-offset-initial-value.md b/tasks/core-pin-offset-initial-value.md new file mode 100644 index 0000000..fc430f6 --- /dev/null +++ b/tasks/core-pin-offset-initial-value.md @@ -0,0 +1,65 @@ +--- +id: core-pin-offset-initial-value +name: Verify and pin EventReceiver::offset()'s pre-yield value (review 004 item 12) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: project +level: implementation +tags: [review-004-fixes, core, contract, verification] +--- + +## Description + +The named gap from `docs/reviews/004-pre-release-general-review.md` +§"Findings recorded, not fixed" item 12: `EventReceiver::offset()` +pins the *post-yield* value only; the *pre-yield* return (the seeded +checkpoint vs 0) is unpinned in core-contract.md's receiver text. + +Two steps, in order: + +1. **Verify the three engines' pre-yield values empirically** — the + review explicitly did not verify them this session. Probe per + engine: a fresh consumer's `offset()` before any yield, and a + consumer with a saved checkpoint's `offset()` before any yield. + The contract suite is the natural home (a three-way equivalence + leg, stamped per `version_stamp.rs` convention); per-engine tests + are the acceptable alternative if the suite's harness shape fights + the probe — implementer's choice, recorded. +2. **Pin the semantics in the contract text** — core-contract.md's + `EventReceiver::offset()` paragraph gains the pre-yield return + (dated annotation). Doc-only. + +**If the engines diverge**: do NOT paper over with a doc pin — record +the divergence precisely (which engine, which arm) and raise with the +user; a contract-observable divergence is a fix-the-code-or-amend +conversation, not a doc edit. + +## Acceptance Criteria + +- [ ] The three engines' pre-yield `offset()` values verified equal by + live runs (the probe/leg recorded; green on all three columns — + pg against the harness server) +- [ ] core-contract.md's `EventReceiver::offset()` text pins the + pre-yield return with a dated annotation +- [ ] If divergent instead: the divergence recorded and raised with + the user (this AC replaces the doc pin, not supplements it) +- [ ] Doc gate green; any new suite leg green on all three columns + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 12 (the named gap) +- docs/architecture/core-contract.md (the receiver text, the + verification backlog) +- alkstore-contract-suite (the three-column harness; the + version-stamp convention) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/decide-log-facade-adoption.md b/tasks/decide-log-facade-adoption.md new file mode 100644 index 0000000..8b696ff --- /dev/null +++ b/tasks/decide-log-facade-adoption.md @@ -0,0 +1,77 @@ +--- +id: decide-log-facade-adoption +name: Decide the log-facade posture (review 004 item 7, family-level) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: project +level: implementation +tags: [review-004-fixes, cross-cutting, decision] +--- + +## Description + +The family-level decision the review recorded rather than made, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 7: the workspace has no log facade — 17 library-path +`eprintln!` sites carry the engines' diagnostics (verified secret-free: +no DSN/password material ever printed). An operator currently sees +nothing before wake loss in the arms `pg-fix-forwarder-diagnosability` +will fix, and the fix's *output form* depends on this decision — which +is why that task depends on this one. + +Decide, with the user (a family-level call): + +- **Adopt `log`** (facade-only; consumers wire the sink) — the light + shape; check the family repos (alkcall/alktunnels/alksocks/alktty) + for the house posture first and follow it if one exists. +- **Adopt `tracing`** — richer, heavier; only if the family precedent + supports it. +- **Keep `eprintln!`** — document the discipline as the posture of + record (what gets printed, the secret-free rule, the no-facade + rationale). + +Considerations to weigh in the record: core's three-dependency posture +(ADR-020 §4 — a facade dep on `alkstore` is a contract change; an +engines-only adoption is the lighter shape and probably the right +scope); published-crate consumer expectations (a facade that emits +nothing until the consumer installs a sink vs stderr noise they didn't +ask for); the 17 existing sites' disposition under whichever option +lands. + +Output: a decision of record — a new ADR (or an amendment to an +existing one) if adopting; a documented posture in `deployment.md` (or +the engine specs) if declining. Either way, the user's sign-off is +part of the record. + +## Acceptance Criteria + +- [ ] The family-precedent check is done and recorded (what + alkcall/alktunnels/alksocks/alktty do, if anything) +- [ ] The decision of record landed: ADR (adoption) or documented + posture (decline), with the rationale and the core-dep question + answered explicitly +- [ ] The 17 existing `eprintln!` sites' disposition under the + decision is named (migrated / kept / reduced) +- [ ] User sign-off recorded +- [ ] Doc gate green (whatever text landed) + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 7 (the finding, the family-level framing) +- docs/architecture/decisions/020-payload-encoding.md §4 (core's + dependency posture — the constraint on core-side adoption) +- docs/architecture/deployment.md (where a declining posture would be + documented) +- tasks/pg-fix-forwarder-diagnosability.md (the consumer of this + decision) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/docs-pg-identifier-truncation-note.md b/tasks/docs-pg-identifier-truncation-note.md new file mode 100644 index 0000000..1643803 --- /dev/null +++ b/tasks/docs-pg-identifier-truncation-note.md @@ -0,0 +1,58 @@ +--- +id: docs-pg-identifier-truncation-note +name: Document the pg identifier-truncation collision in the release text (review 004 item 13) +status: pending +depends_on: [] +scope: single +risk: trivial +impact: isolated +level: implementation +tags: [review-004-fixes, postgres-engine, docs] +--- + +## Description + +The pg identifier-truncation cross-channel collision, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 13: `LISTEN`/`UNLISTEN` interpolate the channel as a +quoted identifier (truncated at 63 bytes server-side) while +`pg_notify` carries it as a parameter — two distinct names sharing a +63-byte prefix can silently collapse into one wake channel. Names are +unbounded by the validation contract (the no-cap "length-independence" +verdict is deliberate — do not change it; the honest cheap fix is the +doc note the review prescribed). + +Land the note in `deployment.md` (the pg rows / wherever the matrix +discusses naming or limits) and in `engine-postgres.md` if it has a +wake-channel section. The note states the mechanism (63-byte +identifier truncation; the `pg_notify` payload path unaffected) and +the mitigation (keep mechanism names under the PG identifier limit). +The deployment matrix is stable release text — a dated amendment +annotation per the family's convention. + +## Acceptance Criteria + +- [ ] `deployment.md` carries the truncation-collision note with a + dated annotation (mechanism + mitigation, honestly worded) +- [ ] `engine-postgres.md` carries it too if a wake-channel section + exists there (or the absence recorded in Notes) +- [ ] The validation contract's no-cap verdict untouched +- [ ] Doc gate green + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 13 (the finding, the "fold into whichever doc pass + runs next" disposition this task discharges) +- docs/architecture/deployment.md, engine-postgres.md (the landing + sites) +- alkstore-postgres/src/forwarder.rs (the LISTEN/UNLISTEN + interpolation sites, for the note's accuracy) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/mem-doc-growth-outbox-notes.md b/tasks/mem-doc-growth-outbox-notes.md new file mode 100644 index 0000000..bc16e78 --- /dev/null +++ b/tasks/mem-doc-growth-outbox-notes.md @@ -0,0 +1,67 @@ +--- +id: mem-doc-growth-outbox-notes +name: Pin mem's growth postures and outbox semantics in the spec (review 004 items 10+11) +status: pending +depends_on: [] +scope: narrow +risk: trivial +impact: isolated +level: implementation +tags: [review-004-fixes, mem-engine, docs] +--- + +## Description + +Two recorded mem postures get their doc pins, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" items 10 and 11 (the review's "carry into the mem spec's +growth section when next touched" — this is that touch). + +**(a) Growth postures (item 10)** — two honest sentences in +`engine-mem.md`'s growth section: +- The subscribe path drains the full backlog into the receiver's + pending deque before the first yield, and pending grows at publish + rate for a stalled consumer (`stream.rs:314-331` — the documented + "drains to the tail then parks" shape, unbounded on the consumer + side). +- The claim path is an O(total-live-rows) scan with full-row clones + per claim (`queue.rs:122-181`) — blessed economics for the ephemeral + tier. + +**(b) Outbox outcome swallow (item 11)** — `outbox run_once` drops the +ack/retry row-refs' booleans and returns `Ok(true)` even when delivery +outlived the visibility window and nothing landed (`outbox.rs:104-122`). +Inherent to the no-heartbeat outbox design (at-least-once reclaim is +the recovery). Pin the "processed means delivered-or-reclaimable" +semantic with a doc sentence in `engine-mem.md`'s outbox section; add +the `debug_assert!` too if it adds value without noise — the review +offered either, implementer's call (record which and why). + +Doc-only plus at most one `debug_assert!` — no behavioral change. + +## Acceptance Criteria + +- [ ] `engine-mem.md` carries the two growth-posture sentences, + anchored to the code sites +- [ ] The outbox "processed means delivered-or-reclaimable" semantic + is pinned (doc sentence; `debug_assert!` per the recorded call) +- [ ] Doc gate green (`RUSTDOCFLAGS="-D warnings" cargo doc + --no-deps`); mem tests green (the `debug_assert!` arm, if added, + exercised by an existing outbox test) + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" items 10–11 (the findings, the blessed-economics framing) +- docs/architecture/engine-mem.md (the growth section, the outbox + section) +- alkstore-mem/src/stream.rs:314-331, queue.rs:122-181, + outbox.rs:104-122 (the anchored sites) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/normalize-tokio-dev-deps.md b/tasks/normalize-tokio-dev-deps.md new file mode 100644 index 0000000..9a6b396 --- /dev/null +++ b/tasks/normalize-tokio-dev-deps.md @@ -0,0 +1,50 @@ +--- +id: normalize-tokio-dev-deps +name: Normalize the engines' dev-dep tokio feature sets (review 004 item 16) +status: pending +depends_on: [] +scope: single +risk: trivial +impact: isolated +level: implementation +tags: [review-004-fixes, hygiene, metadata] +--- + +## Description + +The cosmetic metadata asymmetry from +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 16: the three engines' dev-dep tokio features disagree +— sqlite `rt-multi-thread+macros+time`, pg `test-util`, mem +`rt+macros`. Harmless (the contract suite carries the test runtime) +but an easy-to-trip asymmetry when copying test code between engines. + +Normalize to one uniform dev-dep feature set across the three engines. +Pick the union that keeps every engine's own tests green (likely +`rt-multi-thread + macros + time + test-util` — verify per engine; +feature unification means the workspace build is unaffected either +way). Dev-deps only: no published-surface change, no runtime-dep +change. + +## Acceptance Criteria + +- [ ] The three engines' `[dev-dependencies]` tokio feature sets are + identical +- [ ] Each engine's own test target green (`cargo test -p` per engine; + the pg lib target's server-less SKIP posture is fine) +- [ ] Workspace gates green (build, clippy `-D warnings`, fmt) + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 16 (the finding) +- alkstore-sqlite/Cargo.toml:28, alkstore-postgres/Cargo.toml:22, + alkstore-mem/Cargo.toml:19 (the three differing sets) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/pg-decide-stamp-arithmetic-side.md b/tasks/pg-decide-stamp-arithmetic-side.md new file mode 100644 index 0000000..528394c --- /dev/null +++ b/tasks/pg-decide-stamp-arithmetic-side.md @@ -0,0 +1,79 @@ +--- +id: pg-decide-stamp-arithmetic-side +name: Decide which side guards pg's server-side stamp arithmetic (review 004 item 5, decision) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [review-004-fixes, postgres-engine, decision, contract] +--- + +## Description + +The decision that must precede the BIGINT-overflow fix, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 5: pg's claim, heartbeat, retry-reschedule, and +retention-sweep statements compute server-side sums over +consumer-supplied i64 values (`queue.rs:386/912/1000/714`, +`lock.rs:91`) — one row stamped with `visibility_timeout_s = i64::MAX` +(or retention `i64::MIN`) makes every claim/heartbeat UPDATE fail with +"bigint out of range", and the failure is *sticky* per row. The +client-side resolution is saturated (`resolution.rs`); the row-stamp +arithmetic is not. + +Decide which side owns the guard: + +- **(a) Clamp/validate at the enqueue seam** — consumer i64s bounded + at entry, matching `resolution.rs`'s client-side saturation posture + and the sibling engines' stamp arithmetic (sqlite/mem saturate per + review 004 inline fix 6 — the engine-uniformity argument). +- **(b) `LEAST`/`GREATEST`-guard the server expressions** — defense at + the arithmetic site; no entry-point change, but the stored stamps + are silently clamped server-side. + +**Contract-adjacent — this is why it's its own task.** The choice +changes which failures are reachable through the public surface, which +touches ADR-023 §2's totality wording. Check the window state first: +as of the decomposition (2026-10-11) no publish tag exists, so the +class-1 window is presumed open — verify, and if open, land the +decision with a dated ADR-023 §2-style annotation (the failure-set +change is class-1-legal pre-publish, the review-004 inline fix 5 +precedent). If the window has closed, raise with the user before +pinning anything. + +The decision must name the guard form precisely: the clamp domain, +which sites it applies to (enqueue vs the stamp sites), and what the +observable behavior of an extreme-stamped row becomes. + +## Acceptance Criteria + +- [ ] The decision of record: which side owns the guard, with the + ADR-023 §2 totality argument and the sibling-engine uniformity + check written down +- [ ] The window state verified and recorded; a dated annotation + landed if class-1, or user sign-off if post-publish +- [ ] The guard form is precise (clamp domain, sites, the + extreme-stamped row's observable semantics) — enough for + `pg-fix-bigint-stamp-overflow` to implement without re-deciding +- [ ] The decision recorded where the family keeps such records (ADR + annotation / task Notes + contract text if the wording needs it) + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 5 (the finding, the which-side question, the + contract-adjacency) +- docs/architecture/decisions/023-fourth-review-round.md §2 (the + totality wording the choice touches) +- alkstore-postgres/src/resolution.rs (the client-side saturation + posture), queue.rs / lock.rs (the stamp sites) + +## 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-bigint-stamp-overflow.md b/tasks/pg-fix-bigint-stamp-overflow.md new file mode 100644 index 0000000..8bedfd7 --- /dev/null +++ b/tasks/pg-fix-bigint-stamp-overflow.md @@ -0,0 +1,66 @@ +--- +id: pg-fix-bigint-stamp-overflow +name: Guard pg's server-side stamp arithmetic against i64 extremes (review 004 item 5, implementation) +status: pending +depends_on: [pg-decide-stamp-arithmetic-side] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [review-004-fixes, postgres-engine, queues] +--- + +## Description + +Implement the guard decided by `pg-decide-stamp-arithmetic-side`, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 5. The sites: the claim statement (`queue.rs:386`), +heartbeat (`:912`), retry-reschedule (`:1000`), retention-sweep +(`:714`), and the lock stamp (`lock.rs:91`) — server-side sums over +consumer-supplied i64s whose overflow fails the UPDATE with "bigint +out of range", sticky per row (one extreme-stamped row poisons every +later claim/heartbeat/sweep that touches it). + +Per the decision of record: clamp/validate at the enqueue seam, or +`LEAST`/`GREATEST`-guard the server expressions — do not re-decide; +if the implementation surfaces a reason the decision can't stand as +written, stop and record the discrepancy instead of deviating. + +**Tests** (against the live harness server): a row stamped with +`visibility_timeout_s = i64::MAX` must not poison claim/heartbeat — +the row is claimable/heartbeatable per the decided semantics and +*other rows are unaffected*; the retention `i64::MIN` arm pinned +similarly. The sticky-failure property is the thing being retired: +before the fix, one extreme row fails every claim UPDATE that sums +against it. + +## Acceptance Criteria + +- [ ] The decided guard lands at the decided sites; no re-deciding + (any discrepancy recorded, not deviated around) +- [ ] An `i64::MAX`-stamped row no longer makes claim/heartbeat + UPDATEs fail — pinned by test against the live server, with the + other-rows-unaffected assertion +- [ ] The retention `i64::MIN` arm pinned (sweep behavior per the + decided semantics) +- [ ] The decision's annotation/contract text updated if the + reachable failure set changed +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 5 (the finding, the sticky-failure mechanism) +- tasks/pg-decide-stamp-arithmetic-side.md (the decision of record + this implements) +- alkstore-postgres/src/queue.rs:386/714/912/1000, lock.rs:91 (the + stamp sites) + +## 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-diagnosability.md b/tasks/pg-fix-forwarder-diagnosability.md new file mode 100644 index 0000000..e2eb58b --- /dev/null +++ b/tasks/pg-fix-forwarder-diagnosability.md @@ -0,0 +1,60 @@ +--- +id: pg-fix-forwarder-diagnosability +name: Surface the forwarder's swallowed connect/poll failures (review 004 item 7 arms) +status: pending +depends_on: [decide-log-facade-adoption, pg-fix-forwarder-hardening] +scope: narrow +risk: low +impact: component +level: implementation +tags: [review-004-fixes, postgres-engine, forwarder] +--- + +## Description + +The two narrow diagnosability arms in the pg forwarder, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 7: the reconnect arm's connect failures are swallowed +with `.ok()` (`forwarder.rs:839` — the review's ref; re-locate) and +the loop's exit discards the poll error (`forwarder.rs:855-869`) — an +operator sees nothing before wake loss. + +Fix: surface both failures. The *output form* follows the +log-facade decision of record (`decide-log-facade-adoption`): through +the adopted facade, or through the documented `eprintln!` discipline +if the decision declined adoption. The error content must stay +secret-free (no DSN/password material — the standing verified +property; re-grep after). + +Sequenced after `pg-fix-forwarder-hardening` deliberately: same file, +and the hardening's restructure may move the reconnect arm and the +loop-exit path this task touches. + +## Acceptance Criteria + +- [ ] Failed reconnect connects are surfaced (not `.ok()`-swallowed) + with enough context to diagnose (error class, attempt/backoff + state — no endpoint credentials) +- [ ] The loop's exit path surfaces the poll error instead of + discarding it +- [ ] The output form matches the log-facade decision of record; the + secret-free property re-verified by grep +- [ ] Existing forwarder tests green unchanged; the hardening task's + new pins green +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 7 (the finding, the swallowed-error sites) +- tasks/decide-log-facade-adoption.md (the output-form decision) +- tasks/pg-fix-forwarder-hardening.md (the same-file sequencing) + +## 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-hardening.md b/tasks/pg-fix-forwarder-hardening.md new file mode 100644 index 0000000..d548d63 --- /dev/null +++ b/tasks/pg-fix-forwarder-hardening.md @@ -0,0 +1,93 @@ +--- +id: pg-fix-forwarder-hardening +name: Forwarder TOCTOU restructure + hang-shaped arms (review 004 items 4+6) +status: pending +depends_on: [] +scope: broad +risk: high +impact: component +level: implementation +tags: [review-004-fixes, postgres-engine, forwarder] +--- + +## Description + +Forwarder surgery that should ride its own session, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" items 4 and 6 (the review bundles them as the +forwarder-hardening session). Line refs are as of the review; the +inline fixes may have shifted them slightly — re-locate on pickup. + +**(a) Register/unregister TOCTOU (item 4).** The registry refcount and +the LISTEN/UNLISTEN command emission are not one critical section +(`forwarder.rs:131-176` vs `438-482`): interleaved register/unregister +across tasks can leave a live refcount with the server-side LISTEN +cleared (both queued commands share a generation, so no stale check +saves it) until the next reconnect re-issue. Fix shape: a +decide-and-queue restructure inside the forwarder — the +register/unregister decision and the derived command enqueue must be +atomic with respect to each other (decide under the registry guard; +queue the command *with* the refcount state it was derived from). + +**(b) Generation-verify/LISTEN failure parks forever (item 6, arm 1).** +A failed generation-verify/LISTEN batch is treated as connection death +and parks on `poll_task.await` forever even though the session may be +alive (`forwarder.rs:698-702/803-808`). + +**(c) Shutdown can leak the listener session (item 6, arm 2).** +`close()`/`Drop` can leak the listener session past shutdown when the +serve loop is awaiting a command on a black-holed connection (the +`run_command` await sits outside the shutdown `select`, `:781`; no +timeout on this connection). + +Treatment family for (b)+(c): timeouts/bounded waits on the listener +path. + +**Preserve while restructuring** (the gate will check these): the two +POC-pinned deadlock pitfalls stay structurally excluded; the reconnect +invariants from `pg-fix-forwarder-reconnect` (the retry core, the +fanout release guard, receivers-terminal-iff-loop-gone) stay green; +the register-failure unregister fix from review 004 (inline fix 3) +stays; the poison-recovery lock idiom from review 004 (inline fix 4) +stays. + +Use the existing test-seam vocabulary (the reconnect-config slot, the +connector injection, the generation tests) for the new pins. + +## Acceptance Criteria + +- [ ] The TOCTOU is closed: register/unregister interleavings across + tasks cannot leave a live refcount with the server-side LISTEN + cleared — pinned by a test that interleaves the two and asserts + the server-side LISTEN state matches the refcount +- [ ] A failed generation-verify/LISTEN batch no longer parks forever: + bounded wait with an honest error or reconnect — pinned by test +- [ ] `close()`/`Drop` reclaims the listener session even when the + serve loop is stuck awaiting on a dead connection — pinned by + test (black-holed connection seam) +- [ ] The preserved postures listed above re-verified green (existing + forwarder pins: reconnect survival, no-replay, shutdown close, + stale-UNLISTEN generation tests, the register-failure + unregister pin) +- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + `-D warnings`, fmt clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" items 4 and 6 (the findings, the fix shapes, the + own-session scoping) +- docs/architecture/decisions/004-postgres-driver.md (the forwarder's + owned failure modes, the two POC-pinned deadlock pitfalls) +- tasks/pg-fix-forwarder-reconnect.md (the retry core, the fanout + guard, the test-seam vocabulary) +- tasks/pg-fix-stale-unlisten.md (the generation/staleness machinery + the TOCTOU restructure must compose with) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/review-004-fixes.md b/tasks/review-004-fixes.md new file mode 100644 index 0000000..91e1605 --- /dev/null +++ b/tasks/review-004-fixes.md @@ -0,0 +1,103 @@ +--- +id: review-004-fixes +name: Review gate — review-004 fix batch (recorded-findings dispositions) +status: pending +depends_on: [sqlite-fix-seam-panic-safety, sqlite-fix-watcher-death-subscribe, sqlite-fix-listener-bridge-alignment, sqlite-fix-scheduler-error-classification, sqlite-substrate-safety-comments, pg-fix-forwarder-hardening, pg-fix-forwarder-diagnosability, decide-log-facade-adoption, pg-decide-stamp-arithmetic-side, pg-fix-bigint-stamp-overflow, mem-doc-growth-outbox-notes, core-pin-offset-initial-value, core-fix-display-log-forging, docs-pg-identifier-truncation-note, normalize-tokio-dev-deps] +scope: broad +risk: low +impact: phase +level: review +tags: [review-004-fixes, review-gate] +--- + +## Description + +The fix batch's own gate (the `review-wave-4-fixes` pattern): review +004's sixteen recorded findings were decomposed into fifteen tasks; +verify each disposition is *real* — code-read against the review's +failure mechanism, not test-name-trust — and that the fixes didn't +damage the postures review 004's inline fixes pinned. + +Check, per finding (the review's §"Findings recorded, not fixed" +numbering): + +1. **Seam panic-safety**: the abandonment plumbing is real (a panic + injected in a seam closure leaves the writer slot grantable and the + reader pool intact — the test injects, it doesn't simulate); the + design of record in Notes matches what landed. +2. **Watcher death + initial-stat**: post-death subscribe fails + closed (not parks); the identity-unknown state survives the first + successful stat; pre-death close-at-death pins green. +3. **Bridge alignment**: the listen bridge is std-thread shaped; the + two bridges' residual difference (if any) named and justified. +4. **Scheduler classification**: the closed-store arm no longer + reports `LeadershipLost`; the window-state/annotation record is + present. +5. **Substrate SAFETY comments**: every flagged block carries a + concrete invariant; the PROVENANCE delta entry registered (the + diff-fidelity posture honored — this is the check that matters + most here). +6. **Forwarder hardening**: the TOCTOU restructure is atomic + decide-and-queue (code-read the guard/command ordering); the two + hang arms bounded; the preserved postures (deadlock pitfalls, + retry core, fanout guard, register-failure unregister, + poison-recovery idiom) re-verified green. +7. **Log facade**: the decision of record exists with user sign-off; + the diagnosability arms surface failures in the decided form; + secret-free grep re-run. +8. **BIGINT stamps**: the decided guard at the decided sites; the + extreme-stamped-row tests run against the live server; no + re-deciding happened (or discrepancies recorded). +9. **Mem doc notes**: the growth/outbox pins present and anchored. +10. **Offset pre-yield pin**: the three-way verification evidence + exists; the contract text pinned (or the divergence raised). +11. **Display forging**: the control-char pin; taxonomy untouched. +12. **Identifier note**: in the release text, dated, no-cap verdict + untouched. +13. **Tokio dev-deps**: the three sets identical; tests green. + +**No regressions**: review 004's inline fixes' pinned postures +re-spot-checked where the batch touched them — the tick's +drop-+-replenish arm, the tx-form negative seed (all three columns), +the register-failure unregister, the poison-recovery idiom, the +encode depth limit (127), the sleep caps, the doc corrections. + +**Records**: review 004's recorded-findings items annotated with +their dispositions/task pointers (the record reads closed); +`docs/plans/implementation.md` gains a review-rounds entry for this +batch; task files audited (all dependencies `completed`, Notes and +Summary filled, no placeholder residue). + +**Gates**: full battery — `cargo build`, `cargo test --workspace` +(server-less), the live pg column vs the harness server (env per +review 003's op note), `cargo clippy --all-targets -- -D warnings`, +`cargo fmt --check`, the fuzz corpus replay, the wasm32 check, and +the doc gate. + +## Acceptance Criteria + +- [ ] Every finding's disposition verified by code-read/live run + against the review's failure mechanism (the per-finding checks + above) +- [ ] No regressions in the inline fixes' pinned postures +- [ ] Records landed (review-004 annotations, implementation.md + review-rounds entry, task-file audit) +- [ ] All gates re-run green this session (incl. live pg column, fuzz + replay, wasm32, doc gate) + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" (the sixteen items this gate disposes) and §"Findings + fixed inline" (the postures re-spot-checked) +- tasks/review-wave-4-fixes.md (the gate pattern) +- docs/plans/implementation.md §Review rounds so far (the record to + extend) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/sqlite-fix-listener-bridge-alignment.md b/tasks/sqlite-fix-listener-bridge-alignment.md new file mode 100644 index 0000000..c30b10c --- /dev/null +++ b/tasks/sqlite-fix-listener-bridge-alignment.md @@ -0,0 +1,58 @@ +--- +id: sqlite-fix-listener-bridge-alignment +name: Align the sqlite listen bridge to the stream bridge's std-thread shape (review 004 item 8) +status: pending +depends_on: [sqlite-fix-watcher-death-subscribe] +scope: narrow +risk: low +impact: component +level: implementation +tags: [review-004-fixes, sqlite-engine, notify] +--- + +## Description + +The SQLite listener-bridge asymmetry, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 8: `listen()` parks one `spawn_blocking` thread per +subscription (`notify.rs:98-104`; tokio's blocking-pool ceiling is +~512) while the stream bridge does the identical job on a dedicated +`std::thread` (`stream.rs:368-379`). Past ~500 concurrent listeners +the *whole engine's* seams queue behind parked bridges. Align the two +bridges: the listen bridge takes the stream bridge's std-thread shape. + +Sequenced after `sqlite-fix-watcher-death-subscribe` deliberately: +both touch the same subscription-path files (`notify.rs`/`stream.rs`), +and the dead-watcher sign that task introduces is part of the bridge's +exit vocabulary — the aligned bridge must surface it the same way the +stream bridge does. + +## Acceptance Criteria + +- [ ] `listen()`'s bridge runs on a dedicated `std::thread` (the + stream bridge's shape); no `spawn_blocking` per subscription +- [ ] The two bridges' shapes are aligned (or the residual difference + is named and justified in Notes) +- [ ] Existing notify/listen tests green unchanged (close semantics, + wake delivery, no-replay); the dead-watcher sign from the + dependency task surfaces identically through the aligned bridge +- [ ] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt + clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 8 (the finding, the ~512 ceiling, the deferred- + alongside-item-2 note) +- alkstore-sqlite/src/notify.rs (the spawn_blocking bridge), + stream.rs (the std-thread bridge — the shape to align to) +- tasks/sqlite-fix-watcher-death-subscribe.md (the same-files + sequencing) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/sqlite-fix-scheduler-error-classification.md b/tasks/sqlite-fix-scheduler-error-classification.md new file mode 100644 index 0000000..87d3aac --- /dev/null +++ b/tasks/sqlite-fix-scheduler-error-classification.md @@ -0,0 +1,65 @@ +--- +id: sqlite-fix-scheduler-error-classification +name: Classify run_schedules' renew-failure arms (review 004 item 9) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [review-004-fixes, sqlite-engine, scheduler] +--- + +## Description + +The SQLite `run_schedules` error flattening, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 9: the renew arm maps *any* renew failure — including +the closed-store `Database` error — to `LeadershipLost` +(`scheduler.rs:216`, `lease.renew(LEADER_TTL_S).await.unwrap_or(false)`), +misdirecting the documented respawn recipe at a dead store (an operator +respawns a leader against a store that is closed). + +Decide the right arm set and implement it: closed-store vs +leadership-lost vs storage failure — what `run_schedules` returns on a +dead store, and what stays `LeadershipLost`. + +**Contract-adjacent — check the window state first.** This is an +error-classification change on a stable surface (what error a public +scheduler entry point returns is contract-observable). As of the +decomposition (2026-10-11) no publish tag exists, so the class-1 +window is presumed open — verify, and if open, land with a dated +ADR-023 §2-style annotation of the classification. If the window has +closed (the user has published), raise with the user before changing +the observable error surface, per AGENTS.md's ADR-wins discipline. + +## Acceptance Criteria + +- [ ] The arm set is decided and recorded in Notes (which error maps + where, citing the contract text that pins `run_schedules`' error + surface); the closed-store arm no longer reports + `LeadershipLost` +- [ ] The window state was verified and recorded; a dated annotation + landed if class-1, or user sign-off if post-publish +- [ ] A test pins the closed-store arm (and the genuine + leadership-lost arm's unchanged disposition) +- [ ] The documented respawn recipe's text (wherever it lives) matches + the new classification +- [ ] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt + clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 9 (the finding, the "record, don't rush" note this + task discharges) +- docs/architecture/core-contract.md (the scheduler error surface) +- alkstore-sqlite/src/scheduler.rs:205-216 (the flattening site) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/sqlite-fix-seam-panic-safety.md b/tasks/sqlite-fix-seam-panic-safety.md new file mode 100644 index 0000000..c9cca39 --- /dev/null +++ b/tasks/sqlite-fix-seam-panic-safety.md @@ -0,0 +1,84 @@ +--- +id: sqlite-fix-seam-panic-safety +name: Panic-safety armor for the sqlite auto-commit seams (review 004 item 1) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [review-004-fixes, sqlite-engine, seam] +--- + +## Description + +Defense-in-depth panic-safety for the SQLite auto-commit seams, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 1. `with_writer`/`with_reader`/`blocking_read` +(`alkstore-sqlite/src/seam.rs`, `substrate/schema.rs`) release their +slot/pool connection only after the closure returns normally. A panic +inside the closure skips the release: the single writer slot wedges +forever (`Writer::acquire` waits on a condvar with no timeout), and an +abandoned reader connection permanently shrinks the pool +(`Readers.outstanding` never decrements on abandonment). The tx seam +already owns this class (`ConnLease`); the auto-commit seams don't. + +No *reachable* panic path today (the library is panic-free by the +standing gates, and these closures take no consumer code), so this is +armor, not exposure — but the fix needs abandonment plumbing on both +`Writer` (drop + path-based replenish) and `Readers` (an outstanding +decrement), which is a deliberate small design task, not an inline +patch. + +**Design first**: decide the abandonment mechanism of record before +touching code, and record it in Notes. The established drop-guard +idioms are the precedents: `ConnLease` (the tx seam's connection +lease), the tick's poisoned-disposition replenish arm (review 004 +inline fix 1), and cross-engine `FanoutRelease` (pg's forwarder guard). +The natural shape is a guard whose `Drop` marks abandonment (poison / +replenish / decrement) — Drop runs on unwind as well as return — but +the design must pin which dispositions each seam's abandonment takes +(writer: drop + replenish per the tick's established arm; reader: +outstanding decrement, connection dropped not released). + +**Testability**: the closures take no consumer code, so the panic must +be injected — a `cfg(test)` panic seam inside a test closure (or a +test-only closure that panics), with the unwind caught, asserting the +slot/pool state after: the writer slot is grantable again and a full +store operation succeeds; the reader pool's outstanding count is +restored and a later reader acquires. + +## Acceptance Criteria + +- [ ] The abandonment design of record is decided and written into + Notes (mechanism per seam, which dispositions, which precedents + it follows) before implementation lands +- [ ] A panic inside the `with_writer` closure leaves the writer slot + grantable after the unwind — pinned by a test with an injected + panic (slot re-acquired, full store operation succeeds after) +- [ ] A panic inside the `with_reader`/`blocking_read` closures leaves + the reader pool's outstanding count restored and a later reader + acquires — pinned by test +- [ ] Normal (non-panic) paths are behaviorally unchanged: the + existing seam/slot/pool tests green unchanged +- [ ] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt + clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 1 (the finding, the fix shape, the "deliberate small + design task" scoping) +- docs/architecture/engine-sqlite.md (connection architecture: writer + slot, reader pool) +- alkstore-sqlite/src/seam.rs, substrate/schema.rs (the three seams) +- The tx seam's `ConnLease` and the tick's poisoned-disposition + replenish arm (review 004 inline fix 1) — the established idioms + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/sqlite-fix-watcher-death-subscribe.md b/tasks/sqlite-fix-watcher-death-subscribe.md new file mode 100644 index 0000000..cfeb265 --- /dev/null +++ b/tasks/sqlite-fix-watcher-death-subscribe.md @@ -0,0 +1,76 @@ +--- +id: sqlite-fix-watcher-death-subscribe +name: Dead-watcher subscribe hang + initial-stat self-kill (review 004 items 2+3) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [review-004-fixes, sqlite-engine, watcher] +--- + +## Description + +Two bundled watcher-state defects in the SQLite engine, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" items 2 and 3 (the review says bundle them — both are +watcher-state changes). + +**(a) Post-watcher-death subscribe hang (item 2).** After the watcher +thread exits (db-file replacement), `subscribe()` still enrolls new +senders with no poller alive (`substrate/watcher.rs:391-396`): a +`listen()` or stream `subscribe()` on the dead store parks forever, +contradicting engine-sqlite.md's "on watcher death every subscriber +closes" posture — which is enforced only for *pre-death* subscribers. +Fix shape: a dead-store/dead-watcher sign that the new subscription +paths consult — a state-machine change across `store.rs`/`notify.rs`/ +`stream.rs`. New subscriptions on a dead store must fail closed (a +typed error, the established failure vocabulary) rather than park. + +**(b) Initial-stat failure self-kill (item 3).** An identity-`stat` +failure at watcher start seeds `(0, 0)` (`watcher.rs:164-170`); the +first *successful* later stat mismatches it and drives the death path, +spuriously closing every subscriber on recovery from a transient stat +hiccup. Rare, but the fix — an identity-unknown state instead of a +false identity — is the same watcher-state family: the watcher treats +identity as unknown until the first successful stat, and that first +successful stat must not trigger the death path. + +**Test shapes**: (a) kill the watcher (the db-file replacement seam or +a `cfg(test)` death injection), then `listen()`/`subscribe()` — assert +the typed failure, not a park; the pre-death subscribers' close-at- +death pins stay green. (b) inject an initial stat failure, let the +first successful stat land, assert the watcher survives and +subscribers stay open. + +## Acceptance Criteria + +- [ ] Subscribing (listen or stream) after watcher death fails closed + with a typed error instead of parking — pinned by test +- [ ] Pre-death subscribers still close at watcher death — the + existing pins green unchanged +- [ ] An initial identity-stat failure no longer seeds a false + identity: the first successful stat does not trigger the death + path and subscribers stay open — pinned by injected-failure test +- [ ] engine-sqlite.md's watcher-death posture text covers the + new-subscriber arm (and the identity-unknown state if it changes + observable behavior) +- [ ] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt + clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" items 2–3 (the findings, the fix shapes, the bundling) +- docs/architecture/engine-sqlite.md (the watcher-death posture) +- alkstore-sqlite/src/substrate/watcher.rs (both sites), + store.rs / notify.rs / stream.rs (the subscription paths) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file diff --git a/tasks/sqlite-substrate-safety-comments.md b/tasks/sqlite-substrate-safety-comments.md new file mode 100644 index 0000000..8fa1c86 --- /dev/null +++ b/tasks/sqlite-substrate-safety-comments.md @@ -0,0 +1,71 @@ +--- +id: sqlite-substrate-safety-comments +name: SAFETY comments on the substrate UDF blocks, riding a PROVENANCE delta (review 004 item 15) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: isolated +level: implementation +tags: [review-004-fixes, sqlite-engine, substrate, provenance] +--- + +## Description + +The eight `unsafe { ctx.get_connection() }` UDF blocks +(`alkstore-sqlite/src/substrate/ops.rs:147-236`) carry no `// SAFETY` +justification comments, per +`docs/reviews/004-pre-release-general-review.md` §"Findings recorded, +not fixed" item 15. The pattern is the standard rusqlite UDF shape and +provably constrained, but AGENTS.md's own carve-out ("inline comments +when a non-obvious safety constraint would otherwise be missed") says +this is where they'd live. + +**The constraint that shapes this task**: the substrate is +lineage-port code under ADR-012 §3's diff-fidelity posture — comments +must ride a PROVENANCE delta. This task *is* the deliberate delta +vehicle: add the comments and register the delta; do not treat this as +a casual edit. + +Each comment must state the *concrete* invariant that makes the access +sound (derive it from the rusqlite UDF contract and the substrate's +actual registration/usage — e.g. the connection the ctx hands out is +confined to the registered UDF call path on the single connection +thread), not a generic "this is safe" hand-wave. + +While there: the review scoped to the eight blocks at ops.rs:147-236, +but the same `unsafe { ctx.get_connection() }` shape also appears at +`schema.rs:104` and `ops.rs:742` — check whether they are the same +class; if so, include them in the same delta (and say so in Notes); +if not, leave them and record why. + +## Acceptance Criteria + +- [ ] Each of the eight UDF blocks (plus any same-class neighbors + included) carries a `// SAFETY` comment stating the concrete + soundness invariant +- [ ] `src/substrate/PROVENANCE.md` carries a dated delta entry + registering the comment-only change (diff-fidelity posture + honored; the entry classifies the delta per the register's + conventions) +- [ ] No behavioral change: the substrate floor tests and the engine's + tests green unchanged +- [ ] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt + clean; workspace gates green server-less + +## References + +- docs/reviews/004-pre-release-general-review.md §"Findings recorded, + not fixed" item 15 (the finding, the PROVENANCE-delta requirement) +- docs/architecture/decisions/012-forked-substrate-design.md §3 (the + diff-fidelity posture) +- alkstore-sqlite/src/substrate/PROVENANCE.md (the delta register) +- alkstore-sqlite/src/substrate/ops.rs:147-236 (the eight blocks) + +## Notes + +> To be filled by implementation agent + +## Summary + +> To be filled on completion \ No newline at end of file