From 08dc1bf01111c92fe5bad29b0cce0e13c82ce3a3 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Wed, 7 Oct 2026 06:25:39 +0000 Subject: [PATCH] =?UTF-8?q?ADR-021:=20third=20review=20round=20=E2=80=94?= =?UTF-8?q?=20tx-read=20methods,=20Job.claimed=5Fat,=20schedule()=20queue?= =?UTF-8?q?=20validation,=20drop=3Drollback,=20receiver=20arms?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/architecture/README.md | 13 +- docs/architecture/core-contract.md | 76 +++- .../decisions/007-transactional-seam.md | 7 + .../decisions/008-contract-v1-pinning.md | 6 +- .../decisions/009-scheduler-collapse.md | 8 +- .../decisions/010-queue-semantics-depth.md | 15 +- .../decisions/014-outbox-tx-enqueue.md | 7 +- .../019-mechanism-handle-surfaces.md | 26 +- .../021-tx-reads-and-value-shape-fixes.md | 331 ++++++++++++++++++ docs/architecture/deployment.md | 3 +- docs/architecture/engine-postgres.md | 3 +- docs/architecture/engine-sqlite.md | 3 +- docs/architecture/open-questions.md | 13 +- docs/architecture/overview.md | 3 +- docs/architecture/queues.md | 14 +- 15 files changed, 499 insertions(+), 29 deletions(-) create mode 100644 docs/architecture/decisions/021-tx-reads-and-value-shape-fixes.md diff --git a/docs/architecture/README.md b/docs/architecture/README.md index e1eea5f..fb2f0ee 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (ADR-019/020 — second review round resolved) +last_updated: 2026-10-07 (ADR-021 — third review round resolved) --- # alkstore — Architecture @@ -55,6 +55,7 @@ pending architecture review and OQ resolution. | [018](decisions/018-provenance-register-and-cherry-picks.md) | Substrate provenance register and cherry-pick procedure — `PROVENANCE.md` in-tree, per-delta category-tagged entries, five-step adoption discipline | Accepted | | [019](decisions/019-mechanism-handle-surfaces.md) | Mechanism-handle surfaces — handle traits (`Queue`/`StreamHandle`/`Outbox`/`Lock`/`JobHandle`), `Job`/`Schedule` shapes, `worker_id` identity, core `StopToken` | Accepted | | [020](decisions/020-enqueue-opt-semantics-and-bridges.md) | Enqueue-option semantics — delay/run_at precedence, relative `expires`, scheduler stamp source, serde_json payload encoding | Accepted | +| [021](decisions/021-tx-reads-and-value-shape-fixes.md) | Third review round — tx-read methods on `TxHandle`, `Job.claimed_at`, `schedule()` queue-argument validation, drop = rollback, receiver close/error arms | Accepted | ## Open Questions @@ -83,10 +84,12 @@ depth — [ADR-015](decisions/015-streams-depth.md)), ~~OQ-10~~ (fork follow-through — provenance register + cherry-pick procedure, [ADR-018](decisions/018-provenance-register-and-cherry-picks.md)). -No deferred OQs. With the question set closed (and the 2026-10-06 -second review round resolving its findings directly as ADR-019/ADR-020 -rather than as new OQs), Phase 1 moves to architecture review closure -and the implementation-phase gates. +No deferred OQs. The question set closed with OQ-11 (2026-10-06); the +2026-10-06 second review round and the 2026-10-07 third review round +resolved their findings directly as ADR-019/ADR-020 and +[ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) +respectively, rather than as new OQs. Phase 1 moves to architecture +review closure and the implementation-phase gates. ## Document Lifecycle diff --git a/docs/architecture/core-contract.md b/docs/architecture/core-contract.md index 175093f..971c787 100644 --- a/docs/architecture/core-contract.md +++ b/docs/architecture/core-contract.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (ADR-019/020 — handle surfaces, enqueue semantics, payload bridge) +last_updated: 2026-10-07 (ADR-021 — third review round: tx reads, claimed_at, schedule queue validation, drop=rollback, receiver arms) --- # Core contract @@ -72,11 +72,29 @@ trait TxHandle { publish_with_key_tx(stream, key, payload) -> offset notify_tx(channel, payload) save_offset_tx(stream, consumer, offset) + get_job_tx(queue, job_id) -> Option // ADR-021 + get_offset_tx(stream, consumer) -> i64 // ADR-021 + read_since_tx(stream, offset, limit) -> Vec // ADR-021 + read_from_consumer_tx(stream, consumer, limit) -> Vec // ADR-021 outbox_enqueue_tx(outbox, opts, payload) -> job_id commit(self: Box) -> Result<()> // or rollback } ``` +The tx read methods (`get_job_tx` / `get_offset_tx` / `read_since_tx` +/ `read_from_consumer_tx`, +[ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §1) make +read-your-own-writes possible inside the business transaction — they +take the mechanism name (mirroring `enqueue_tx`/`publish_tx`'s +name-taking shape), read the caller's own transaction's rows, and are +pure reads (no ops ride them). Reads and saves are tx-shaped; +claim/maintenance ops never are (no `claim_tx` — a claim's visibility +deadline must not be tied to a business transaction's lifetime; +deliberate absence, not an omission). **Dropping a handle without +`commit`/`rollback` rolls it back** ([ADR-021](decisions/021-tx-reads- +and-value-shape-fixes.md) §4) — the no-ghosts property holds through +RAII paths, not only the explicit ones. + `outbox_enqueue_tx` takes the **outbox name** and derives the backing queue engine-side (the reserved prefix makes the derived name unreachable by `enqueue_tx` by design — @@ -223,7 +241,11 @@ business-tx shape). Depth pinned by (`EventReceiver::save_offset(&mut self)`, which saves the last-yielded event's offset through the same op, [ADR-019](decisions/019-mechanism-handle-surfaces.md) §6) - interleave freely and cannot regress one another. A saved offset + interleave freely and cannot regress one another. Both save + forms' `Err` arm carries `Database` only + ([ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §5) — + regression refusal is a silent no-op, so a save result errs only + on storage failure. A saved offset below the trim horizon ([ADR-015](decisions/015-streams-depth.md) §5) stays a valid position marker. @@ -241,6 +263,16 @@ business-tx shape). Depth pinned by consumption: attach, read to current tail, resume after restart from the stored offset; explicit `save_offset` on the receiver (shape in [ADR-008](decisions/008-contract-v1-pinning.md) §8). + **Error and close arms pinned** + ([ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §5): + `recv()`'s `Err` arm carries `Database` only (payloads cross raw; + `Codec` lives at `payload_as`; `Closed` is the `None` arm); + close is terminal and its *cause* is the documented engine + asymmetry — SQLite's receiver closes on watcher death (POC-pinned), + the pg receiver stays open across forwarder reconnects (reconnect + wakes arrive on it) and closes only at engine shutdown. A closed + receiver never reopens; recovery is a fresh `subscribe(consumer)` + resuming from the saved offset. Consumption is wake-driven with table re-read — the same mechanism split as queues (durable row, LISTEN/watcher wake, [ADR-006](decisions/006-wake-and-delivery-contract.md)); the @@ -375,7 +407,15 @@ Collapsed into queues per [ADR-009](decisions/009-scheduler-collapse.md): runner (`StopToken` is a core-crate cloneable cancel handle — [ADR-019](decisions/019-mechanism-handle-surfaces.md) §4; the leader-elected leadership lock is the reserved name -`__alkstore_scheduler`). Schedules never fire +`__alkstore_scheduler`). Both name-bearing arguments validate at the +entry point: the schedule *name* like a queue name (non-empty, +reserved-prefix rejected — +[ADR-009](decisions/009-scheduler-collapse.md) §1) and, since +[ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §3, the +**queue argument** identically (`InvalidName`/`ReservedName`) — a +schedule cannot target a reserved name, so the outbox backing queue +stays reachable only through the outbox surface +([ADR-014](decisions/014-outbox-tx-enqueue.md)). Schedules never fire without a runner (no ambient timers). v1 spec grammar: `@every ` only (`s|m|h|d`); cron strings are rejected (`InvalidSpec`) pending a consumer-inventory row naming wall-clock @@ -570,6 +610,30 @@ before the engine specs are called `stable`: §1/§2/§3) — delay-over-run_at precedence, relative-expires resolution, and the scheduler-fired jobs' derived-default stamps (`get_job`-visible via the row) are identical on both engines. +- **In-tx read-your-own-writes on both engines** + ([ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §1) — + reads issued on the tx handle see the transaction's own writes + (`get_job_tx` a row the same tx enqueued; `read_since_tx` an event + the same tx published); another engine connection sees none of it + pre-commit; post-rollback the reads' subjects are gone (no-ghosts + for reads). +- **Drop = rollback on both engines** + ([ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §4) — + a handle dropped before commit leaves no job/event/notify/offset + residue (no-ghosts via RAII); afterward the SQLite writer slot is + reusable and the pg client re-poolable. +- **`schedule()` queue-argument validation on both engines** + ([ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §3) — + reserved-prefix and empty queue arguments rejected at registration + with `ReservedName`/`InvalidName`; stored schedules fire only into + non-reserved queue names. +- **Receiver error/close arms on both engines** + ([ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §5) — + `recv()`'s `Err` carries `Database` only; `save_offset`'s `Err` + carries `Database` only; regression saves are silent no-ops; + SQLite's receiver closes on watcher death (terminal `None`); the + pg receiver stays open across forwarder reconnects (reconnect-wakes + arrive; no close) and closes only at engine shutdown. ## Design Decisions @@ -590,6 +654,7 @@ before the engine specs are called `stable`: | [017](decisions/017-contract-versioning.md) | Contract versioning (governs this surface's changes) | core crate's semver *is* the contract version; four change classes; amend-in-place ends at first release; pairing = manifest pin + version-stamped contract suite + docs | | [019](decisions/019-mechanism-handle-surfaces.md) | Mechanism handles (amends 008 §1/§2/§8) | handle traits pinned (`Queue`/`StreamHandle`/`Outbox`/`Lock`/`JobHandle`); `worker_id` claimant identity; `Job`/`Schedule` struct shapes; core-owned `StopToken`; save-offset composition | | [020](decisions/020-enqueue-opt-semantics-and-bridges.md) | Enqueue options + bridges (amends 008 §1/§2, 009 §3) | `delay` wins over `run_at`; `expires` = relative seconds; scheduler stamps from derived queue defaults; serde_json byte-level payload encoding | +| [021](decisions/021-tx-reads-and-value-shape-fixes.md) | Third review round (amends 008 §2/§8, 019 §1/§3, 009 §1) | tx-side read methods on `TxHandle` (read-your-own-writes); `Job.claimed_at` restored; `schedule()` queue argument validated; drop = rollback; receiver close/error arms pinned | ## Open Questions @@ -619,3 +684,8 @@ its findings resolved directly as surfaces, value shapes, stop token) and [ADR-020](decisions/020-enqueue-opt-semantics-and-bridges.md) (enqueue-option semantics, scheduler stamp source, payload encoding). +The third review round (2026-10-07, pre-decomposition) likewise found +no new open questions — its five findings resolved directly as +[ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) (tx-side +reads, `Job.claimed_at`, `schedule()` queue-validation, drop = +rollback, receiver close/error arms). diff --git a/docs/architecture/decisions/007-transactional-seam.md b/docs/architecture/decisions/007-transactional-seam.md index 0206b45..2627221 100644 --- a/docs/architecture/decisions/007-transactional-seam.md +++ b/docs/architecture/decisions/007-transactional-seam.md @@ -35,6 +35,13 @@ handle.commit() / handle.rollback() `*_tx` operation lands in *the caller's transaction*; commit/rollback are explicit and owned by the caller. Non-`_tx` operations are the auto-commit convenience counterparts, each atomic alone. + *(Drop disposition pinned 2026-10-07 by + [ADR-021](021-tx-reads-and-value-shape-fixes.md) §4: a handle + dropped without `commit`/`rollback` rolls back — the no-ghosts + property holds through RAII paths, not only the explicit ones; + SQLite's writer-slot lease releases with the rollback, the pg + pooled client rolls back and re-pools; honker's documented + `Transaction` `Drop` behavior is the inherited precedent.)* - **Postgres** ([ADR-004]): the handle holds the pooled connection object directly (tokio-postgres `Client` is `Send + Sync` — verified by a compile-time probe). Straight `.await`s, no bridge, no hop diff --git a/docs/architecture/decisions/008-contract-v1-pinning.md b/docs/architecture/decisions/008-contract-v1-pinning.md index df21397..88ab136 100644 --- a/docs/architecture/decisions/008-contract-v1-pinning.md +++ b/docs/architecture/decisions/008-contract-v1-pinning.md @@ -132,9 +132,13 @@ trait TxHandle { publish_with_key_tx(stream, key, payload) -> offset // ADR-015 notify_tx(channel, payload) save_offset_tx(stream, consumer, offset) + get_job_tx(queue, job_id) -> Option // ADR-021 + get_offset_tx(stream, consumer) -> i64 // ADR-021 + read_since_tx(stream, offset, limit) -> Vec // ADR-021 + read_from_consumer_tx(stream, consumer, limit) -> Vec // ADR-021 outbox_enqueue_tx(outbox, opts, payload) -> job_id // ADR-014 commit(self: Box) -> Result<()> // or rollback -} +} // ADR-021: drop = rollback ``` *(Sketches elide `Result<>` wrappers on `*_tx` returns for brevity — diff --git a/docs/architecture/decisions/009-scheduler-collapse.md b/docs/architecture/decisions/009-scheduler-collapse.md index 82ea529..2933cd0 100644 --- a/docs/architecture/decisions/009-scheduler-collapse.md +++ b/docs/architecture/decisions/009-scheduler-collapse.md @@ -74,7 +74,13 @@ store.run_schedules(stop) -> Result<()> // runs until `stop` [ADR-008](008-contract-v1-pinning.md) §4 sense (their consumers-side obligation extends the entry-point list: a reserved-prefix schedule name could collide with the machinery's derived names). No charset - rule beyond that in v1. + rule beyond that in v1. *(The **queue argument** of `schedule()` + gets the same directly-supplied-queue-name validation, added + 2026-10-07 by [ADR-021](021-tx-reads-and-value-shape-fixes.md) §3 — + without it, a schedule row could store a reserved name as its fire + target and every boundary fire would enqueue into it, a third write + path into the outbox's backing queue contradicting + [ADR-014](014-outbox-tx-enqueue.md)'s stated guarantee.)* - `run_schedules` is the tick: acquire the leadership lock, loop {renew lock — on loss, *return before ticking*; fire every due boundary; sleep until the next due boundary or `stop`}. Losing diff --git a/docs/architecture/decisions/010-queue-semantics-depth.md b/docs/architecture/decisions/010-queue-semantics-depth.md index 1485f43..ca19090 100644 --- a/docs/architecture/decisions/010-queue-semantics-depth.md +++ b/docs/architecture/decisions/010-queue-semantics-depth.md @@ -51,11 +51,16 @@ Contract job states: **`pending` → `processing` → `dead`** (+ absence: - **`get_job` sees dead jobs.** The returned job carries state, payload, attempts, priority, and the timestamps `created_at`, `run_at`, `claimed_at`, `expires_at` — plus, dead-only, `last_error` - and `died_at` (no job = `None`). This deliberately *narrows* honker's - surface (honker's `get_job` reads only live rows — post-mortem - diagnosis was SQL-only); diagnosis-by-API is the honest fix, and it - is what makes move-to-dead (vs flag-in-place) inspectable without - raw SQL. + and `died_at` (no job = `None`). *(Struct disposition 2026-10-07 + per [ADR-021](021-tx-reads-and-value-shape-fixes.md) §2: + `claimed_at: Option` — None pre-claim, set on every claim — + now stands in [ADR-019](019-mechanism-handle-surfaces.md) §3's + pinned struct, which is the field list of record; ADR-019's + original omission is corrected there.)* This deliberately *narrows* + honker's surface (honker's `get_job` reads only live rows — + post-mortem diagnosis was SQL-only); diagnosis-by-API is the honest + fix, and it is what makes move-to-dead (vs flag-in-place) + inspectable without raw SQL. - **`cancel` is unconditional** (honker parity): it deletes the row in either `pending` or `processing`, regardless of which worker holds it. Not an interrupt — the holder's next ack/heartbeat returns diff --git a/docs/architecture/decisions/014-outbox-tx-enqueue.md b/docs/architecture/decisions/014-outbox-tx-enqueue.md index e196055..a0810cc 100644 --- a/docs/architecture/decisions/014-outbox-tx-enqueue.md +++ b/docs/architecture/decisions/014-outbox-tx-enqueue.md @@ -179,7 +179,12 @@ smallest surface that restores the property. documentation must state plainly: **there is no way to enqueue into the backing queue except through the outbox surface** (auto-commit `outbox.enqueue` or tx `outbox_enqueue_tx`) — itself the guarantee - that the derivation cannot be collided with. + that the derivation cannot be collided with. *(Guarantee completed + 2026-10-07 by [ADR-021](021-tx-reads-and-value-shape-fixes.md) §3: + `schedule()`'s queue argument now rejects the reserved prefix too — + without that pin, a schedule row storing `__alkstore_outbox:{name}` + as its fire target would have been a third write path into the + backing queue.)* ## References diff --git a/docs/architecture/decisions/019-mechanism-handle-surfaces.md b/docs/architecture/decisions/019-mechanism-handle-surfaces.md index 95c91f8..82a445f 100644 --- a/docs/architecture/decisions/019-mechanism-handle-surfaces.md +++ b/docs/architecture/decisions/019-mechanism-handle-surfaces.md @@ -114,10 +114,16 @@ trait Lock { carries; a `Queue` handle without `claim_*` would be a construction ceremony with no behavior. Honker's `Job`-ops-on-claimed-work shape and `Lock::release(self)` RAII shape are likewise inherited. -- `get_job`, `save_offset`, `get_offset`, `read_*` remain available +- `get_job`, `save_offset`, `get_offset`, `read_since`, + `read_from_consumer` remain available from *inside* a transaction through the `TxHandle`'s `*_tx` methods - — the handle forms are the auto-commit convenience counterparts - ([ADR-007](007-transactional-seam.md)). + (`get_job_tx`, `get_offset_tx`, `read_since_tx`, + `read_from_consumer_tx` — + [ADR-021](021-tx-reads-and-value-shape-fixes.md) §1) — the handle + forms are the auto-commit convenience counterparts + ([ADR-007](007-transactional-seam.md)); claim/maintenance ops are + deliberately not tx-shaped (no `claim_tx` — a claim's visibility + deadline must not be tied to a business transaction's lifetime). - The **reserved-prefix / name validation rules apply to the constructors** (`queue`/`stream`/`outbox`/`try_lock`) exactly as pinned by [ADR-008](008-contract-v1-pinning.md) §4 — nothing new; @@ -160,6 +166,9 @@ struct Job { attempts: i64, // every claim counts max_attempts: i64, // the row's stamp worker_id: Option, // claimant, None pre-claim + claimed_at: Option, // unix seconds at the most + // recent claim; None pre-claim + // (added by ADR-021 §2) claim_expires_at: Option, // deadline, None pre-claim created_at: i64, // unix seconds expires_at: Option, // job-level expiry, None = @@ -191,10 +200,19 @@ trait JobHandle { §2 — `processing` + unexpired caller deadline) governs a `JobHandle`'s ops; a get_job row yields no handle, so no ghost ops exist on it. Ops take `self: Box` mirroring `TxHandle`'s - commit shape (consumed handle, one-shot discipline). + commit shape (consumed handle, one-shot discipline); `heartbeat` is + the deliberate counter-case — `self, extend: i64`, repeatable, + because renewal is inherently repeated within one claim (the same + one-shot/repeatable split the receiver shapes carry; do not + "fix" the asymmetry at implementation). `err: Option`: `None` = the engine's documented-default string (taxonomy-consistent: these strings are row content, `get_job` data — not matched error variants). + *(Struct corrected 2026-10-07 by + [ADR-021](021-tx-reads-and-value-shape-fixes.md) §2: `claimed_at` + was missing from the original pinning — + [ADR-010](010-queue-semantics-depth.md) §1's `get_job` list carried + it and the fork schema adds the column — restored above.)* - `Schedule` (the `schedule()` read-back value): `Schedule { name: String, spec: String, queue: String, opts: ScheduleOpts }`. diff --git a/docs/architecture/decisions/021-tx-reads-and-value-shape-fixes.md b/docs/architecture/decisions/021-tx-reads-and-value-shape-fixes.md new file mode 100644 index 0000000..20ab91d --- /dev/null +++ b/docs/architecture/decisions/021-tx-reads-and-value-shape-fixes.md @@ -0,0 +1,331 @@ +# ADR-021: Third review round — tx-side reads, `Job.claimed_at`, `schedule()` queue validation, tx-handle drop, receiver close/error arms + +## Status + +Accepted (2026-10-07, Phase 1 — third architecture review round, +pre-decomposition; amends [ADR-008](008-contract-v1-pinning.md) §2/§8, +[ADR-019](019-mechanism-handle-surfaces.md) §1/§3/§6, and +[ADR-009](009-scheduler-collapse.md) §1 in place, pre-implementation +([ADR-017](017-contract-versioning.md) class 1 — no release exists; +the amend-in-place frame is still open). Also records two factual +corrections to accepted decisions found by the review — ADR-019 §1's +tx-methods claim and ADR-019 §3's `Job` field list — corrected inline +there, per this repo's convention that wrong statements in decisions +are fixed where they stand, not left for readers to discover.) + +## Context + +The third review round (the pre-decomposition sweep after the second, +which produced ADR-019/020) checked the spec set — the seven +architecture documents and all twenty ADRs — for cross-document +consistency and implementation-blocking ambiguity. Five findings were +decidable from decided material; none opens a new design space: + +1. **ADR-019 §1's sentence about tx-side reads was factually + incorrect.** It states: "`get_job`, `save_offset`, `get_offset`, + `read_*` remain available from *inside* a transaction through the + `TxHandle`'s `*_tx` methods." The pinned `TxHandle` trait + ([ADR-008](008-contract-v1-pinning.md) §2, extended by + [ADR-014](014-outbox-tx-enqueue.md)/[ADR-015](015-streams-depth.md)) + carries only write-and-save ops (`enqueue_tx`, `publish_tx`, + `publish_with_key_tx`, `notify_tx`, `save_offset_tx`, + `outbox_enqueue_tx`) plus `commit`/`rollback` — there is no + `get_job_tx`, `get_offset_tx`, `read_since_tx`, or + `read_from_consumer_tx`. The *capability* the sentence claims is + real and load-bearing: without tx-shaped reads, read-your-own- + writes inside a business transaction is impossible on both engines + (SQLite's reader pool cannot see the writer connection's + uncommitted rows; a pg pool checkout may land on another + connection without seeing the tx client's uncommitted writes). + Honker provides the capability only through its raw + `Transaction::query_row`/`conn()` escape hatch — the seam + [ADR-014](014-outbox-tx-enqueue.md) §3 deliberately rejected + (downcast or core→engine dependency). So the inheritance claims + named the capability without a mechanism: the review caught the + claim outrunning the pinned surface. + +2. **`Job.claimed_at` — ADR-010 §1 and ADR-019 §3 disagree.** + [ADR-010](010-queue-semantics-depth.md) §1 pins `get_job`'s + timestamps as "`created_at`, `run_at`, `claimed_at`, `expires_at`"; + ADR-019 §3's pinned `Job` struct omits `claimed_at`. ADR-019 is + the struct of record and its list is the narrower, later pinning — + but the fork's re-derivation adds the `claimed_at` column + ([ADR-012](012-forked-substrate-design.md) §5), `claim_expires_at` + (the field ADR-019 *did* carry) is meaningless without it, and + `claimed_at` is the dual-execution window's diagnostic field (which + worker's claim deadline was running when a job died). The omission + was an error in ADR-019's field list, not a narrowing ADR-010 + needed annotated. + +3. **`schedule()`'s queue argument is an unvalidated write path into + reserved names.** Every directly-supplied name of every kind + rejects the reserved prefix ([ADR-008](008-contract-v1-pinning.md) + §4), and [ADR-014](014-outbox-tx-enqueue.md) rests its outbox + guarantee on that: "there is no way to enqueue into the backing + queue except through the outbox surface." But + `schedule(name, spec, queue, payload, opts)` takes a queue name + whose validation is unpinned ([ADR-009](009-scheduler-collapse.md) + §1 validates the *schedule* name), and each boundary fire enqueues + engine-side into whatever name was stored — + `schedule("x", "@every 60s", "__alkstore_outbox:foo", …)` is a + third write path into the outbox's backing queue, contradicting + the guarantee as stated. + +4. **The drop-of-un-committed-handle case is unpinned.** A + `Box` that is neither `commit`ed nor `rollback`ed — + a dropped handle, an error path that exits early, a panic across + the box — has no stated fate. This is observable behavior with + direct interaction with the no-ghosts property + ([ADR-007](007-transactional-seam.md)), and on the pg side it + determines whether a pooled client leaks. + +5. **The `EventReceiver` close/error arms are unpinned.** Its shape + is pinned ([ADR-008](008-contract-v1-pinning.md) §8: + `recv() -> Option>`, `save_offset(&mut self) + -> Result<()>`) but the v1 error taxonomy + ([ADR-008](008-contract-v1-pinning.md) §5) never says which + variants the `Err` arms can carry, what closes a pg-side receiver, + and ADR-019 §6's save-offset bullet says "returns `Result<()>` — + refusal (regression) is a no-op, not caller-actionable" without + saying what the `Err` arm ever is. + +All five resolve from the starting artifact, the pinned posture, and +the existing POC ground — the same "decidable from decided material" +basis the second round's findings were resolved on. + +## Decision + +### 1. Tx-side reads: four methods join the `TxHandle` trait + +ADR-019 §1's sentence is *corrected rather than retracted* — the +capability stays, the mechanism is now pinned. The trait gains: + +```text +trait TxHandle { + enqueue_tx(queue, opts, payload) -> job_id + publish_tx(stream, payload) -> offset + publish_with_key_tx(stream, key, payload) -> offset + notify_tx(channel, payload) + save_offset_tx(stream, consumer, offset) + get_job_tx(queue, job_id) -> Option // added + get_offset_tx(stream, consumer) -> i64 // added + read_since_tx(stream, offset, limit) -> Vec // added + read_from_consumer_tx(stream, consumer, limit) -> Vec // added + outbox_enqueue_tx(outbox, opts, payload) -> job_id + commit(self: Box) -> Result<()> +} +``` + +- The tx read forms carry the **same shapes as the auto-commit + forms** (`Queue::get_job`, `StreamHandle::read_since` / + `read_from_consumer` / `get_offset`, + [ADR-019](019-mechanism-handle-surfaces.md) §1) — pure reads, no + ops boxed off them. They take the mechanism **name** as their first + parameter (not a handle), mirroring `save_offset_tx` / + `enqueue_tx` / `publish_tx`'s name-taking shape. +- The property pinned: **in-tx reads read the caller's own + transaction** — a `get_job_tx`/`read_since_tx` issued on a handle + sees exactly the rows that handle's transaction has written (the + pg POC's read-your-writes probe, in-tx direction); after rollback + those reads' subjects no longer exist (no-ghosts, uniformly, for + reads as for writes). +- **The line stays where it was**: reads and saves are tx-shaped; + claim/maintenance ops are not. There is no `claim_tx` — claiming + inside the caller's business transaction would tie a visibility + deadline's lease to a business commit the caller controls, tangling + the at-least-once machinery with an unrelated lifetime. Absence + here is deliberate design, recorded so an implementer does not + "fix" it. +- Correction posture for the record: ADR-019 §1's original sentence + asserted these methods as already-pinned surface; the assertion was + wrong (the trait it cites never carried them, and honker's escape + hatch was rejected in [ADR-014](014-outbox-tx-enqueue.md) §3). The + sentence stands corrected in place there, citing this ADR. + +### 2. `Job` gains `claimed_at: Option` — None pre-claim + +Corrected inline in [ADR-019](019-mechanism-handle-surfaces.md) §3's +pinned struct (the field list there restored to what +[ADR-010](010-queue-semantics-depth.md) §1 promised and the fork's +schema carries — [ADR-012](012-forked-substrate-design.md) §5): + +- `claimed_at: Option` — unix seconds at the row's most recent + claim, `None` before any claim. Set on every successful claim + (fresh or reclaim — a reclaim updates it), which is what makes it + the dual-execution window's diagnostic (which worker's claim + deadline was live when a job died) and the pairing field for + `claim_expires_at` (meaningless without it). +- The pairing closes a real decomposition hazard: ADR-010 §1's + text and ADR-019 §3's struct were both "pinned" descriptions of + one struct, disagreeing; tasks written off either text would have + built different value types. Both now say the same thing; dead-row + visibility (`get_job` on dead jobs, "dead rows included") carries + the field with the rest of the stamps. + +### 3. `schedule()`'s queue argument rejects the reserved prefix + +The queue argument gets exactly the directly-supplied-queue-name +validation ([ADR-008](008-contract-v1-pinning.md) §4): non-empty else +`InvalidName`, reserved-prefix else `ReservedName`, rejected at the +entry point (registration), no engine round trip. + +- This closes the third write path the review found: a boundary fire + can no longer enqueue into `__alkstore_outbox:*` because the only + way to store that name in a schedule row is through the (now + rejecting) entry point. [ADR-014](014-outbox-tx-enqueue.md)'s + stated guarantee — the backing queue is reachable only through the + outbox surface — becomes true of the scheduler's fires as well; + annotated there. +- Unschedule/registration semantics untouched + ([ADR-009](009-scheduler-collapse.md) §1); the schedule-name + validation pinned there is unchanged, this pins its **queue + argument**. + +### 4. Dropping a `TxHandle` without commit or rollback rolls it back + +Pinned: **drop = rollback.** A `Box` dropped without +`commit` (or `rollback`) executes rollback on its way down; the +no-ghosts property holds through Rust's RAII paths, not only through +the explicit ones. + +- SQLite: the writer-slot lease releases with `ROLLBACK` before the + slot frees ([ADR-003](003-sqlite-driver.md)'s lease mechanics — a + dropped handle cannot park the writer forever). +- Postgres: the held pooled client's transaction rolls back; the + client is usable and returns to the pool on the drop path (no + leaked budget line; the `max_size + 1` accounting in + [deployment.md](../deployment.md) stays accurate under error + paths). +- Precedent is the starting artifact's documented `Drop` behavior — + honker-rs' `Transaction` rolls back on drop without commit/rollback + (`/workspace/honker` `packages/honker-rs/src/lib.rs` @ `f4e53c6`, + "on drop without either, the transaction rolls back") — inherited + where every other tx-seam behavior was inherited. +- The other three candidates were weighed and rejected: *drop = + commit* (silently landing business writes on an error path — + contradicts no-ghosts and every durable-transaction convention), + *drop = leak* (pooled-client exhaustion on error paths; not honest + RAII), *drop = panic* (panics in library code are banned family- + wide). The rejected-with-reasons list is the record's value here — + an implementer improvising a `Drop` impl would otherwise pick + exactly one of them. + +### 5. `EventReceiver` close/error arms pinned + +The pinned shape's unpinned arms: + +- **`recv()`'s `Err` arm carries `Database` only** — the one variant + a stream read can produce. Payloads cross the trait as raw bytes; + `payload_as` (where `Codec` lives) is a decode-side convenience + on the yielded `StreamEvent`, so no `Codec` exists at recv; + `Closed` is the `None` arm by construction + ([ADR-008](008-contract-v1-pinning.md) §3's close semantics, §5's + taxonomy); no third variant can occur. +- **Close is terminal; what causes it is the documented engine + asymmetry**: SQLite's receiver closes on watcher death (POC-pinned, + the `WatcherDeathGuard` surface); the pg engine's forwarder + reconnects indefinitely (the reconnect-wake is its recovery + channel — [ADR-004](004-postgres-driver.md)), so its receivers + close only at engine shutdown. Same shape as the wake-coalescing + asymmetry: one call site (`recv() -> None`), documented occurrence + difference, `Closed`/`None` uniform. A closed receiver never + reopens; the recovery recipe is a fresh `subscribe(consumer)`, + which resumes from the saved checkpoint — the mechanism replay + exists for. +- **`save_offset`'s `Err` arm carries `Database` only** — the + monotone rule makes regression refusal a silent no-op (pinned + semantics, [ADR-019](019-mechanism-handle-surfaces.md) §6), so the + `Result` exists for storage failures; closes that ADR's dangling + return type. A receiver asked to save before any event has been + yielded performs a no-op save (nothing to checkpoint is not an + error state; the stored checkpoint is untouched — `get_offset`'s + absent-consumer = 0 rule, + [ADR-019](019-mechanism-handle-surfaces.md) §1, already defines + the pre-save state). + +## Consequences + +**Positive** + +- The contract surface's last known claim-vs-text mismatch closes: + ADR-019's sentence, the `Job` struct, and the pinned trait text + now say the same thing. +- In-tx read-your-own-writes becomes a real capability (the outbox/ + streams consumer reading back what it just wrote in the business + transaction) instead of an ADR promise with no method behind it. +- The drop path is pinned before implementation — the honest-RAII + answer is also the no-ghosts answer and the honker-inherited answer, + so nothing was traded away. +- The outbox guarantee closes its last hole; the schedule surface + cannot reach reserved names. +- Receiver error matching is now writable engine-agnostically: match + `Database` (and nothing else) in the `Err` arm, `None` as terminal. + +**Negative** + +- Four more `TxHandle` methods for both engines to implement — the + class-2-locked trait cost every tx method carries, priced in + [ADR-017](017-contract-versioning.md) §5; they ride `read_*` / + `get_*` SQL shapes both engines already own, so implementation is + composition, not new machinery. +- `Job`'s field list widens again (one field) — non_exhaustive-governed + growth from here, but the pinned initial text grows. +- Drop = rollback makes forgotten handles silently undo work whose + side effects may have already run inside the transaction — the + same honesty every `Drop`-rollback transaction API carries; the + loud alternative (drop = commit) is the dangerous one and was + rejected for it. +- Contract-suite surface grows: the new backlog rows below. + +## Verification backlog additions + +- **In-tx read-your-own-writes on both engines** (§1) — reads issued + on the tx handle see the transaction's own writes (`get_job_tx` a + row the same tx enqueued; `read_since_tx` an event the same tx + published); another engine connection sees none of it pre-commit; + post-rollback the reads' subjects are gone (no-ghosts for reads). +- **Drop = rollback on both engines** (§4) — a handle dropped before + commit leaves no job/event/notify/offset residue (no-ghosts via + RAII); the SQLite writer slot is reusable and the pg client + re-poolable afterward. +- **`schedule()` queue-argument validation** (§3) — reserved-prefix + and empty queue names rejected with `ReservedName`/`InvalidName` + at registration, identically on both engines; the stored schedule + fires into only non-reserved queue names. +- **Receiver error arms** (§5) — `recv()`'s `Err` carries `Database` + only; `save_offset`'s `Err` carries `Database` only; regression + saves are silent no-ops; SQLite receiver closes on watcher death + (`None`, terminal); pg receiver stays open across forwarder + reconnects (reconnect-wakes arrive; no close) — close pinned to + shutdown only. + +## References + +- [ADR-019](019-mechanism-handle-surfaces.md) — §1's corrected + sentence (the origin of finding 1), §3's corrected `Job` struct + (finding 2), §1's `get_offset` absent-consumer rule and §6's + monotone-save rule (§5 of this ADR builds on both). +- [ADR-008](008-contract-v1-pinning.md) — §2 (the trait shape this + ADR's §1 extends), §4 (the reserved-prefix rule §3 extends to the + schedule queue argument), §5 (the taxonomy the `Err` arms are + pinned against), §8 (`EventReceiver`'s shape whose arms §5 pins). +- [ADR-010](010-queue-semantics-depth.md) §1 — the `get_job` + timestamp list whose `claimed_at` §2 restores into the struct. +- [ADR-014](014-outbox-tx-enqueue.md) — §3 (the raw-`Transaction` + rejection that made finding 1 a real hole, and the guarantee §3 + of this ADR completes), §1 (the name-taking tx-method shape the + read forms mirror). +- [ADR-009](009-scheduler-collapse.md) §1 — the `schedule()` entry + point whose queue argument §3 validates. +- [ADR-007](007-transactional-seam.md) — the seam the drop pin and + the tx reads ride; the no-ghosts property they extend. +- [ADR-003](003-sqlite-driver.md) — the writer-slot lease the drop + pin releases; [ADR-004](004-postgres-driver.md) — the pooled + client and forwarder the close asymmetry and drop pin ride. +- Honker-rs `Transaction` (`/workspace/honker` + `packages/honker-rs/src/lib.rs` @ `f4e53c6`) — the raw-escape + capability (reads without a tx method, §1's capability evidence) + and the documented drop-rolls-back `Drop` behavior (§4's + precedent), read and respectively re-mechanized / inherited. +- [core-contract.md](../core-contract.md) — the spec this ADR's + §1–§5 pin into place. \ No newline at end of file diff --git a/docs/architecture/deployment.md b/docs/architecture/deployment.md index ec3d8e9..6fc8c60 100644 --- a/docs/architecture/deployment.md +++ b/docs/architecture/deployment.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (ADR-019/020 — handle surfaces, enqueue semantics, payload bridge) +last_updated: 2026-10-07 (ADR-021 — third review round: drop=rollback keeps pg pool accounting exact under error paths) --- # Deployment @@ -129,6 +129,7 @@ From both POCs (single-box, relative shapes are the deliverable — | [006](decisions/006-wake-and-delivery-contract.md) | Wake contract | where capability differences may surface | | [008](decisions/008-contract-v1-pinning.md) | Contract v1 | constructor/options in engine crates; no capability surface in v1 (OQ-08; resolved by [ADR-016](decisions/016-deployment-honesty.md)) | | [016](decisions/016-deployment-honesty.md) | Deployment honesty | no runtime capability surface — compile-time identity + this matrix; `PayloadTooLarge` is the one runtime asymmetry carriage | +| [021](decisions/021-tx-reads-and-value-shape-fixes.md) | Third review round | drop = rollback keeps the connection budgets exact under error paths (SQLite lease releases; pg client re-pools) | ## Open Questions diff --git a/docs/architecture/engine-postgres.md b/docs/architecture/engine-postgres.md index bb9c92d..adc4e0a 100644 --- a/docs/architecture/engine-postgres.md +++ b/docs/architecture/engine-postgres.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (ADR-019/020 — handle surfaces, enqueue semantics, payload bridge) +last_updated: 2026-10-07 (ADR-021 — third review round: tx reads, claimed_at, schedule queue validation, drop=rollback, receiver arms) --- # Postgres engine @@ -115,6 +115,7 @@ the record in [ADR-003](decisions/003-sqlite-driver.md). | [017](decisions/017-contract-versioning.md) | Contract versioning | engine pins core `alkstore = "1.y"` in its manifest; changes touching contract semantics classified under its four-class taxonomy | | [019](decisions/019-mechanism-handle-surfaces.md) | Handle surfaces | boxed handle traits (`Queue`/`StreamHandle`/`Outbox`/`Lock`/`JobHandle`) implemented over the pooled/claimed rows; `worker_id` stamps the claimant column | | [020](decisions/020-enqueue-opt-semantics-and-bridges.md) | Enqueue options | delay-over-`run_at` resolution in the engine's enqueue SQL; scheduler fires stamp from derived defaults; serde_json byte encoding (the equivalence suite's byte rows) | +| [021](decisions/021-tx-reads-and-value-shape-fixes.md) | Third review round | tx-read methods on the held pooled client (read-your-own-writes, POC-verified); `Job.claimed_at`; `schedule()` queue argument validated; drop = rollback re-pools the client; receiver stays open across reconnects, closes at shutdown only | ## Open Questions diff --git a/docs/architecture/engine-sqlite.md b/docs/architecture/engine-sqlite.md index 9cb1971..c3407a4 100644 --- a/docs/architecture/engine-sqlite.md +++ b/docs/architecture/engine-sqlite.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (ADR-019/020 — handle surfaces, enqueue semantics) +last_updated: 2026-10-07 (ADR-021 — third review round: tx reads, claimed_at, schedule queue validation, drop=rollback, receiver arms) --- # SQLite engine @@ -135,6 +135,7 @@ family is `__alkstore_*` (ADR-010 §8's naming authorization). | [018](decisions/018-provenance-register-and-cherry-picks.md) | Substrate provenance | `src/substrate/PROVENANCE.md` (identity, delta register — cherry-picks as tagged entries); cherry-picks recorded at adoption, non-versioning | | [019](decisions/019-mechanism-handle-surfaces.md) | Handle surfaces | boxed handle traits (`Queue`/`StreamHandle`/`Outbox`/`Lock`/`JobHandle`); `worker_id` stamps the row's claimant column; `StopToken` core-owned | | [020](decisions/020-enqueue-opt-semantics-and-bridges.md) | Enqueue options | delay-over-`run_at` in the substrate's re-derived enqueue; scheduler fires stamp from derived defaults; serde_json byte encoding | +| [021](decisions/021-tx-reads-and-value-shape-fixes.md) | Third review round | tx-read methods route through the writer-slot lease; `Job.claimed_at` (the fork's claimed_at column, surfaced in `Job`); `schedule()` queue argument validated; drop = rollback releases the lease with `ROLLBACK` | ## Open Questions diff --git a/docs/architecture/open-questions.md b/docs/architecture/open-questions.md index 5ec3c04..74abcd3 100644 --- a/docs/architecture/open-questions.md +++ b/docs/architecture/open-questions.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (OQ-11 resolved — Phase 1 question set closed) +last_updated: 2026-10-07 (third review round — findings resolved as ADR-021; question set remains closed) --- # alkstore — Open Questions @@ -53,7 +53,16 @@ window ends at first release) and (`delay`/`run_at` precedence, relative `expires`, scheduler-fired jobs' stamp source, the serde_json payload encoding bridge), plus mechanical fixes (ADR index staleness in overview/engine-postgres, -stale wording, framing drift). +stale wording, framing drift). **Third review round (2026-10-07, +pre-decomposition):** likewise found no *new* open questions — five +findings (a factually-incorrect ADR-019 §1 claim about tx-side reads, +an ADR-010/ADR-019 `Job`-struct disagreement, an unvalidated +`schedule()` write path into reserved names, the unpinned tx-handle +drop disposition, unpinned `EventReceiver` error/close arms), all +decidable from decided material, resolved directly as +[ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) (tx-read +methods on `TxHandle`, `claimed_at` restored, schedule queue-argument +validation, drop = rollback, receiver arms pinned). Resolved questions stay listed with their resolution; they are not deleted. diff --git a/docs/architecture/overview.md b/docs/architecture/overview.md index bb36c0e..d9794a0 100644 --- a/docs/architecture/overview.md +++ b/docs/architecture/overview.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (ADR-019/020 — handle surfaces, enqueue semantics, payload bridge) +last_updated: 2026-10-07 (ADR-021 — third review round: tx reads, value-shape fixes, schedule queue validation, drop=rollback, receiver arms) --- # alkstore — Overview @@ -84,6 +84,7 @@ Per [ADR-002](decisions/002-feature-scope.md): | [018](decisions/018-provenance-register-and-cherry-picks.md) | Substrate provenance register and cherry-pick procedure (`PROVENANCE.md` in-tree, per-delta entries) | Accepted | | [019](decisions/019-mechanism-handle-surfaces.md) | Mechanism-handle surfaces (handle traits, `Job`/`Schedule` shapes, `worker_id`, `StopToken`) | Accepted | | [020](decisions/020-enqueue-opt-semantics-and-bridges.md) | Enqueue-option semantics (delay/run_at, expires, scheduler stamp source, payload encoding) | Accepted | +| [021](decisions/021-tx-reads-and-value-shape-fixes.md) | Third review round (tx-read methods on `TxHandle`, `Job.claimed_at`, `schedule()` queue-argument validation, drop = rollback, receiver close/error arms) | Accepted | ## Non-goals diff --git a/docs/architecture/queues.md b/docs/architecture/queues.md index 2a30f11..c71995d 100644 --- a/docs/architecture/queues.md +++ b/docs/architecture/queues.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-06 (ADR-019/020 — handle surfaces, enqueue semantics) +last_updated: 2026-10-07 (ADR-021 — third review round: tx reads, claimed_at, schedule queue validation) --- # Queues, scheduler, outbox — semantics depth @@ -43,7 +43,10 @@ made under; the ADRs carry the WHY. relative-`expires` pinned by [ADR-020](decisions/020-enqueue-opt-semantics-and-bridges.md) §1/§2); `ack / retry / fail / heartbeat` on the boxed `JobHandle` a claim - returns ([ADR-019](decisions/019-mechanism-handle-surfaces.md)). + returns ([ADR-019](decisions/019-mechanism-handle-surfaces.md)); + reads (`get_job`, and tx-side `get_job_tx` — + [ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §1) are + name-taking pure reads, tx-shaped on the handle. - **Cut-flag context**: result storage is [ADR-002](decisions/002-feature-scope.md)'s cut-flag row; [ADR-010](decisions/010-queue-semantics-depth.md) §7 reconsidered it @@ -184,7 +187,11 @@ Collapsed into queues: no `Scheduler` handle, no schedule objects. - Surface: `schedule(name, spec, queue, payload, opts)` (upsert by name), `unschedule(name) -> bool`, `run_schedules(stop) -> Result<()>`. - Update = re-register; pause = unregister + re-register. + Update = re-register; pause = unregister + re-register. Both + name-bearing arguments validate at the entry point — the schedule + name and (since + [ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) §3) the + queue argument, identically (non-empty, reserved-prefix rejected). - **Spec grammar v1: `@every ` only** (`s|m|h|d`). Cron strings rejected with `InvalidSpec { spec }` (grammar rationale in the ADR; extension path: a consumer-inventory row first). @@ -284,6 +291,7 @@ Collapsed into queues: no `Scheduler` handle, no schedule objects. | [014](decisions/014-outbox-tx-enqueue.md) | Outbox tx enqueue | `outbox_enqueue_tx` on `TxHandle`; derived backing queue reached only through the outbox surface | | [019](decisions/019-mechanism-handle-surfaces.md) | Handle surfaces | queue ops on the `Queue` handle, `JobHandle` from claims, `worker_id` identity, `Job`/`Schedule` shapes, core `StopToken` | | [020](decisions/020-enqueue-opt-semantics-and-bridges.md) | Enqueue options | delay-over-run_at; relative `expires`; scheduler fires stamp from derived queue defaults | +| [021](decisions/021-tx-reads-and-value-shape-fixes.md) | Third review round | tx-read methods on `TxHandle` (`get_job_tx` etc.); `Job.claimed_at` restored; `schedule()` queue argument rejects the reserved prefix (backing queue reachable only through the outbox surface) | ## Open Questions