decompose review-004 recorded findings: 15 disposition tasks + the review-004-fixes gate (sqlite seam panic-safety, watcher-death subscribe + initial-stat, bridge alignment, scheduler classification, substrate SAFETY delta; pg forwarder hardening + diagnosability, BIGINT stamp decision + fix, log-facade decision; core offset pin + Display forging, mem doc notes, pg identifier note, tokio dev-dep normalization) — taskgraph validate green (95)
This commit is contained in:
glm-5.3-flash committed
1 parent
029f885aa1
commit
9bbe645558
16 files changed
+1131
No files matched your search
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
Reference in new issue
Block a user