# ADR-023: Fourth review round — `encode_payload` failure arm, numeric-argument domains, `SQLITE_OPEN_URI`, watcher poll cadence ## Status Accepted (2026-10-08, Phase 1 — fourth review round, following the 2026-10-08 waves-1–2 general review (`docs/reviews/001-waves-1-2-general-review.md`). Amends [ADR-020](020-enqueue-opt-semantics-and-bridges.md) §4, [ADR-008](008-contract-v1-pinning.md) §5, and [ADR-012](012-forked-substrate-design.md) §4 in place — pre-implementation for the contract pieces ([ADR-017](017-contract-versioning.md) class 1, amend-in-place; no core release exists) and as authorized substrate deltas for the fork pieces ([ADR-012](012-forked-substrate-design.md) §4's delta discipline, recorded in `PROVENANCE.md` at adoption per [ADR-018](018-provenance-register-and-cherry-picks.md)). ## Context The general review of waves 1–2 (the first review round after implementation began) found four decisions needing a call before or at wave 3's engine wiring, none decidable by one-liner: 1. **`encode_payload` maps serialization failure to empty `Vec`** (N-2 in the review). The helper is `pub` and re-exported from the core crate's lib root — it is contract surface, not an internal helper — and the engines' enqueue/publish paths call it to produce the stored bytes ([ADR-020](020-enqueue-opt-semantics-and-bridges.md) §4). In practice `serde_json::to_vec` on a `&Value` cannot fail (`serde_json::Number` cannot hold NaN/∞, so every `Value` serializes), so the arm is unreachable — but if it ever fired, it would silently store empty bytes, which decode to `Null`: silent data corruption. The current signature bakes that disposition in with no way for an engine call site to know. 2. **`claim_batch(n <= 0)` claims the entire queue or is a silent no-op** (M-2 in the review). SQLite's `LIMIT` dialect is the substrate's contract: `LIMIT -1` = no limit, `LIMIT 0` = empty. The fork correctly passes `n` through (contract-blind), but the *trait method's* semantics are the crate family's to define, and as inherited they are a dialect artifact: `n = -1` hands out every claimable row in the queue to one worker. And the same hazard is a **class**, not a lone call site: the stream reads (`read_since`/`read_from_consumer`) pass `limit` into a `LIMIT` clause the same way — `limit = -1` reads the entire stream into a `Vec`, the memory-shaped version of the same hole. Extending to the trait surface, every numeric scalar argument's out-of-domain behavior is currently decided by whichever SQL dialect the engine's substrate speaks: `ttl <= 0` on `try_lock`/`renew` errors substrate-side (an opaque `Database`), extents clamp or explode by dialect, and the remaining numerics are well-defined by accident of their SQL shape. No domain rule exists; both engines must decide identically or engine-agnostic consumer code breaks. 3. **`open_conn` passes `SQLITE_OPEN_URI`** (N-4 in the review) — inherited from upstream (honker uses it for `file:` paths), which makes the path argument URI-interpretable: a `?name=value` suffix mutates connection semantics (locking modes, cache modes) without the constructor's knowledge. The watcher's own open path does *not* pass the flag — the inherited code is internally inconsistent, evidence nothing depends on it. The engine's `open` constructor is wave-3 territory, so the posture must be decided now or wired silently. 4. **The watcher's default poll interval is 1 ms** (N-6 in the review) — the lineage default, inherited verbatim (register D-20). At idle this is ~1000 `PRAGMA data_version` reads/sec per instance. It measures as shipped: deployment.md's wake-latency numbers (p50 ~1.1–2.2 ms) were measured at this cadence. Deciding whether 1 ms is the shipping default or a dev default is a config posture call, not a defect. A fifth shape-review consideration bounds the set: whether a *new* error variant is needed anywhere. The answer below is no (ADR-008 §5's act-differently rule holds throughout). ## Decision ### 1. `encode_payload` becomes fallible: `Result, Error::Codec>` The helper's signature changes to `pub fn encode_payload(value: &serde_json::Value) -> Result, Error::Codec>`; serialization failure maps to `Error::Codec` — the taxonomy's pinned variant for payload serialization/deserialization failures ([ADR-008](008-contract-v1-pinning.md) §5) — no variant growth. The unreachable arm stops encoding a silent-corruption disposition and becomes a typed failure at the right seam; the engines' error plumbing absorbs it (both engines' enqueue/publish paths already return `Result`, so the signature change composes with existing wiring). Rejected: **infallible via `debug_assert`/unreachable** — the assert compiles out in release, leaving the same hole the old arm covered, without a panic's loudness (panics are banned outside tests), so it buys nothing over a typed error; **leave infallible with the empty-Vec arm** — the silent-decode-to-`Null` disposition is the one outcome that cannot be defended, and wave 3 would build on it engines-side. Versioning: core is pre-release — ADR-017 §2 class 1 (amend-in-place), the window this change rides. No post-release consumer exists to break; the contract suite pins the new shape (backlog row below). The semantic content of §4 is unchanged: still *serialize with serde_json and store exactly those bytes* — the stored form, the cross-engine byte-equality pin, and the round-trip property all hold; only the failure disposition of the one helper is now typed. ### 2. Numeric-argument domain rule — by argument kind The trait surface's numeric scalar arguments get a shared domain rule, classified by argument **kind**, not call site: | Kind | Arguments | Out-of-domain (non-positive) rule | |---|---|---| | **Extent** ("at most this many") | `claim_batch(n)`, `read_since(limit)`, `read_from_consumer(limit)` | `n <= 0` → **empty value result** (claims nothing / reads nothing), not an error | | **Duration** (seconds of lease/TTL semantics) | `try_lock(ttl)`, `renew(ttl)` | `ttl <= 0` → **`Err`, opaque** (engine detail in the source chain, `Database`) — the substrate's existing guard (`ttl_s <= 0`), now pinned contract-side | | **Boundary / offset** (`<=` predicates, absolute or relative instants) | `trim_to(horizon)`, enqueue `delay`/`run_at`/`expires`; `heartbeat(extend)`, `save_offset(offset)` *(annotated 2026-10-11)* | total — any `i64` is well-defined (negative horizon deletes nothing; negative delay → ready in the past → ready now; negative extension → the renewed deadline `now + extend` is in the past, voiding the claim through the validity predicate, never an error; negative offset → stored, reads resume forward); no clamping, no rejection | The rule the table pins: **extents clamp to empty (no-work is a value, the pinned claims vocabulary), durations error, boundaries and offsets are total.** Each disposition answers to a decided principle: - **Extents.** "Claim up to `n` rows" with `n = 0` is vacuously zero rows — the contract text already promises the empty `Vec` is ordinary. Minting an error variant for a caller-configurable extent's degenerate value fails ADR-008 §5's act-differently test: empty is a *result* the caller already handles (the no-work arm), indistinguishable from an empty-but-honest poll. The `LIMIT -1`-claims-everything artifact dies with the clamp, engine- side at the trait-impl boundary — no dialect may reinterpret an extent argument. The silent-stall risk of a misconfigured `n` is real but bounded: `n` is a per-call constant (typically a loop bound from config), the misconfiguration is a caller-code bug like any other, and an error variant would not surface it louder in the loop's first iteration than the stuck-empty behavior would. The no-work-is-a-value posture composes with the whole claims surface. - **Durations.** The opposite disposition, and correctly so: a non-positive `ttl` on an exclusion mechanism must not silently hand back a lock the caller believes it holds. The substrate's existing guard is the disposition, not a new one — pinned here so the pg engine (no inherited guard) implements the same rule, and so engine-agnostic code sees a uniform shape. No `InvalidArgument` variant is minted: the caller's remedy is the same regardless — fix the constant — so per ADR-008 §5 the failure stays in the opaque fallback, detail in the source chain. - **Boundaries/offsets.** The remaining numerics are total by accident of their SQL today; the table makes it rule, not accident — an implementer reading the table has nothing to improvise. *(Annotation, 2026-10-11 — the pre-release class-1 close-out pass ([ADR-017](017-contract-versioning.md) §2), disposing the wave-6 review gate's recorded under-classifications. Two arguments join the boundary/offset row, and deliberately not the duration row: `heartbeat(extend)`'s seconds-shaped argument is not lease/TTL — `extend` is the new full deadline in seconds from now, so a non-positive value is not a handed-back lease the caller believes it holds (the duration rule's rejection rationale never applies); it is a relative instant like `delay`, and the three engines' uniform arithmetic (`saturating_add` / plain SQL integer add — code of record, mem `queue.rs`, substrate `queue_ops.rs`, pg `queue.rs`) is total over non-positive values: a negative extension shortens the deadline, a past one is voided by the uniform validity predicate on the *next* op, and the heartbeat itself reports the successful renewal (`true`) exactly when the row was processing and unexpired at call time. And `save_offset`'s offset is literally an offset: a negative is stored as the checkpoint, reads resume forward past it, and saves at-or-below the stored checkpoint are the [ADR-019](019-mechanism-handle-surfaces.md) §6 monotone no-ops — rejections were never the behavior, so pinning the duration row's shape here would have contradicted all three engines.) *(Annotation, 2026-10-11 — the pg stamp-arithmetic guard decision ([ADR-017](017-contract-versioning.md) §2 class 1, the window verified open — no tag exists, no `cargo publish` has occurred; the review-004 item-5 pre-fix precedent). Review 004's post-round finding: pg's server-side stamp arithmetic — the claim's `$2 + visibility_timeout_s` (claim deadline), the heartbeat's `$3 + extend`, the retry-reschedule's `$2 + delay_s`, the retention sweep's `$2 − dead_letter_retention_s`, and the lock stamp/renew's `$3 + ttl` (`lock.rs:92/217`) — overflow-failed on plain bigint ("bigint out of range") and the failure is *sticky per row*, one extreme-stamped row poisoning every later claim/heartbeat/sweep that sums against it. The side question is here because the boundary row's totality lives on the *argument* values these sums consume, and the two candidate guards differ in what they touch: an enqueue-seam clamp/validate owns the guard at the entry of the consumer-supplied i64s; a server-expression guard clamps the arithmetic *result*. **Decided: the server expression owns it — the clamp domain is the sum's result, no input is clamped or validated at any seam.** The enqueue seam cannot own the guard: four of the six arguments are per-call (`heartbeat(extend)`, the `retry(err, Some(d))` override, `try_lock`/`renew(ttl)` ×2) and have no enqueue to clamp at; and where an enqueue does exist, a *result* saturation is what the pinned posture already computes, not a new input rule — enqueue's own stamps resolve in Rust `saturating_add` (`resolution.rs`, both engines), the boundary row pins argument totality ("any i64 is well-defined… no clamping, no rejection" — a caller's `heartbeat(i64::MAX)` or `retry(err, Some(i64::MAX))` is a value, not a violation), and `QueueOpts` stamps enter rows unvalidated verbatim by the recorded review-002 disposition, so validating them at entry would be a *posture change*, not a guard. The guard form, precise: **every server-side stamp expression saturates its result to the i64 domain** — `LEAST(GREATEST(, -9223372036854775808), 9223372036854775807)::bigint`, subtraction sites analogously — verified green against a Postgres 16 harness: an overflowing sum clamps at either edge, a representable result rides untouched (`1700000000 + i64::MIN` stays exact, matching Rust's `saturating_add`), `now − i64::MIN` clamps to `i64::MAX` (matching mem's retention `saturating_sub`). The extreme-stamped row's observable semantics, per site: a row stamped `visibility_timeout_s = i64::MAX` claims to the `i64::MAX` deadline (the claim never lapses through expiry; effectively-never, the same shape as sqlite's REAL-approximation and mem's saturation); a row stamped `dead_letter_retention_s = i64::MIN` sweeps every dead row (`sat(now − MIN) = i64::MAX`); `heartbeat(extend)` extremes renew to the saturated edge (a past saturated deadline voids through the validity predicate on the next op, the annotation above's negative semantics preserved); `retry(err, Some(i64::MAX))` reschedules to `i64::MAX` (ready ~never, the caller's value honored to the edge); `ttl = i64::MAX` stamps an `i64::MAX` expiry. No contract text changes: the table pins these dispositions already — what the annotation completes is the *arithmetic form*, correcting the "plain SQL integer add" parenthetical above: on pg the pinned saturating arithmetic is realized as the server-side guard, not a plain add, and the "bigint out of range" failure leaves the reachable surface (no opaque-Err arm covers arithmetic overflow; the duration row's rejection stays `ttl <= 0` only). Sites of record: pg `queue.rs:386/912/1000/715`, `lock.rs:92/217`; enqueue itself stays Rust-resolved (no SQL-side sum at enqueue). The implementation rides `tasks/pg-fix-bigint-stamp-overflow.md` — `cargo test -p alkstore-postgres` live-server legs pin the sticky-poison retirement.)* This is a **core-contract amendment** (doc text on `claim_batch`, both `read` forms, `try_lock`/`renew`, and a shared domain-rule paragraph), not an engine-layer choice: if the two engines resolved extent clamping differently (one errors, one clamps), engine-agnostic consumers could not compose. Domains are semantics, and semantics live in the contract. The engine-layer validation point (trait-impl entry, before substrate) is *where* the rule is enforced — the review's correct instinct — but *what* is enforced is the contract's. ### 3. `SQLITE_OPEN_URI` dropped — the engine `open` path is a plain filesystem path The substrate's `open_conn` drops the `SQLITE_OPEN_URI` flag; the path argument to the engine's `open(path, opts)` is a plain filesystem path (`:memory:` included, which works without URI mode — verified; what is lost is only `file:` URI syntax: shared-cache in-memory DBs and locking/cache query-parameter mutation of the connection outside the constructor's knowledge). Rationale: - A constructor argument silently interpreted as a URI — where a `?nolock=1` suffix mutates locking semantics — is an inherited dishonest surface of exactly the class this fork exists to shed (the URI parameter surface is SQLite's, not `alkstore-sqlite`'s, and would ride consumer-influenced path strings into connection-semantics mutation the engine cannot review). - The engine's posture is file-backed single-host with plain paths; the flag has no consumer-inventory row naming URI features (the row-first gate: a shared-cache/in-memory-URI use case re-enters with a row; the flag is cheaply restored — engine-crate scope, non-contract). - The internal inconsistency is the tell: the watcher's reconnect opens never carried the flag, so URI and non-URI opens already mix within the inherited machinery for any `file:`-shaped path. Nothing inherits URI semantics coherently even today. - This is an **owned fork delta** (an `ours`-lineage change to inherited code, not a cherry-pick); it is recorded as a `port delta` register entry at adoption per [ADR-012](012-forked-substrate-design.md) §4 and [ADR-018](018-provenance-register-and-cherry-picks.md) — the delta discipline's regular operation, no separate record surface. - The upstream-removal comparison: upstream (honker) uses the flag for `file:`-URI shared-cache memory databases in its embedding harness. If a consumer needs `:memory:` shared across connections, that is the same re-entry path (an inventory row; possibly a named engine-opts surface), not a silent flag. Rejected: **keep + document that path is URI-interpreted** — honest, but it leaves the attack surface (consumer-influenced strings reaching connection-semantics mutation) and the mixed watcher/main flag posture in place for zero named need; the documented form is strictly weaker than removal while the surface has no users. ### 4. Watcher poll cadence — 1 ms stays the shipping default; the knob is carried onto `SqliteOpts` The inherited default (1 ms, register D-20) stays: **the 1 ms default stands as the shipping default**, and the engine `open` constructor carries it onto `SqliteOpts` as `poll_interval: Option` (`None` = the 1 ms default), with deployment.md carrying the documented facts (the idle `data_version` read rate) and the tuning recipe (raise the interval for latency-tolerant deployments). Rationale: - The POC's published wake-latency numbers (p50 ~1.1–2.2 ms, deployment.md) were measured at 1 ms — the default *is* the advertised behavior. Changing it would silently un-advertise the measured facts. - The knob already exists substrate-side (`WatcherConfig:: with_poll_interval`, zero-interval rejected); carrying it onto `SqliteOpts` is wiring, not machinery. - The cost (~1000 reads/sec idle) is documented, not hidden — this is the family's deployment-asserts-truth posture; idle CPU is a tuning concern for latency-tolerant deployments, and the engine opts surface is the pinned home for cadence ([ADR-008](008-contract-v1-pinning.md) §6 — engine-configuration concerns live in the engine's option structs). Rejected: **raising the shipping default to ~50 ms** — it would change the measured-and-advertised wake latency fivefold to save an idle cost the docs already surface, for a trade no consumer row names; **hiding the cadence** (no knob) — dishonest about the cost and removes the tuning lever the config surface exists for. ## Consequences **Positive** - The silent-corruption arm (empty-Vec-on-failure) is gone before wave 3 consumes the helper; both engines now *cannot* silently store undecodable payloads. - The `LIMIT`-dialect artifact dies before wiring: no engine may reinterpret an extent argument, and both engines implement the same domain table — engine-agnostic code composes by construction. - The class is decided once, not per call site: future numeric arguments land in a row of the table, an implementer has nothing to improvise. - The URI surface is gone before a constructor exists to pass consumer-influenced strings through it; the fork shed another inherited surface with a named rationale. - The measured wake-latency facts stay advertised and true; the tuning lever is real and documented. - No new error variants — ADR-008 §5's taxonomy holds throughout (the domain rule's error case reuses the opaque fallback; the encode case reuses `Codec`). **Negative** - `encode_payload`'s call sites and tests gain `?`/`.map_err` at the seam — five call sites today, both engines' enqueue/publish paths wave 3 onward. The fallible shape is the honest cost of the typed seam. - `claim_batch`/`read_*`'s `n <= 0` empty-value semantics are observable but oblique (an out-of-domain extent looks like an honest empty result); the contract suite pins both, so a review round sees the rule rather than rediscovering it. - The URI surface is not *gone* from SQLite itself — a consumer could still pass a `file:` string to the engine, whose behavior is now plain-path (SQLite treats it as a filename); the docs should note the shape so surprises route to the documented posture, not SQLite URI folklore. - Contract-suite surface grows: the new backlog rows below. ## Verification backlog additions - **Extent-clamp semantics on both engines** (§2) — `claim_batch(n = 0)` claims nothing (empty `Vec`, no error); `read_since`/`read_from_consumer` with `limit <= 0` read nothing (empty `Vec`); `claim_batch(n < 0)` never claims more rows than a positive-n claim on the same queue state would (the `LIMIT -1` dialect artifact is dead on both engines). - **Duration-refusal on both engines** (§2) — `try_lock(name, owner, ttl <= 0)` and `renew(ttl <= 0)` are rejections (opaque `Err` shape, `Database`) on both engines, with the substrate guard's message preserved in the source chain where the substrate carries it. - **`encode_payload` typed-failure round-trip** (§1) — enqueue/publish store and decode the exact serialization of the input `Value`; the helper's failure arm is typed (`Codec`), not silent. - **Plain-path open on the SQLite engine** (§3) — a path string carrying a `?name=value` suffix is treated as a filename (file created with that literal name) unless URI mode is explicitly enabled — pinning that no consumer path re-enables URI interpretation silently; the watcher's non-URI opens are unchanged shape. - **Poll-cadence default and knob** (§4) — the SQLite store's watcher runs at 1 ms with `SqliteOpts` default; `poll_interval` set on the opts changes the observed cadence (sleep accounting in the watcher's config surface, not wall-clock timing in the suite — cadence measurements stay out of the contract suite's deterministic posture). ## References - `docs/reviews/001-waves-1-2-general-review.md` — the review report that found all four (N-2, M-2, N-4, N-6; M-2's engine-layer resolution point recommended there is adopted *where*, while this ADR decides *what*, contract-side). - [ADR-020](020-enqueue-opt-semantics-and-bridges.md) §4 — the payload-encoding bridge §1 completes (signature typed, semantics unchanged). - [ADR-008](008-contract-v1-pinning.md) — §5 (the taxonomy rule every disposition answers to: `Codec` reuse, opaque-fallback durations, no variant growth), §6 (the config split §4's `SqliteOpts` knob rides). - [ADR-012](012-forked-substrate-design.md) §3/§4 — the fidelity posture that keeps `open_conn`'s delta a register entry, and the delta discipline that authorizes it. - [ADR-018](018-provenance-register-and-cherry-picks.md) — the register the URI-flag delta is recorded in at adoption. - [ADR-017](017-contract-versioning.md) §2 — the class-1 window (pre-release amend-in-place) the contract pieces ride. - [ADR-019](019-mechanism-handle-surfaces.md) — the handle surfaces whose numeric arguments §2's table classifies. - [ADR-010](010-queue-semantics-depth.md) §1 — the claims semantics (no-work-is-a-value) that §2's extent rule composes with.