alkstore/docs/architecture/decisions/023-fourth-review-round.md

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 §4, ADR-008 §5, and ADR-012 §4 in place — pre-implementation for the contract pieces (ADR-017 class 1, amend-in-place; no core release exists) and as authorized substrate deltas for the fork pieces (ADR-012 §4's delta discipline, recorded in PROVENANCE.md at adoption per ADR-018).

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<u8> (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 §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<Vec<u8>, Error::Codec>

The helper's signature changes to pub fn encode_payload(value: &serde_json::Value) -> Result<Vec<u8>, Error::Codec>; serialization failure maps to Error::Codec — the taxonomy's pinned variant for payload serialization/deserialization failures (ADR-008 §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	total — any i64 is well-defined (negative horizon deletes nothing; negative delay → ready in the past → ready now); 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.

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 §4 and ADR-018 — 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<Duration> (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 §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 §4 — the payload-encoding bridge §1 completes (signature typed, semantics unchanged).
• ADR-008 — §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 §3/§4 — the fidelity posture that keeps open_conn's delta a register entry, and the delta discipline that authorizes it.
• ADR-018 — the register the URI-flag delta is recorded in at adoption.
• ADR-017 §2 — the class-1 window (pre-release amend-in-place) the contract pieces ride.
• ADR-019 — the handle surfaces whose numeric arguments §2's table classifies.
• ADR-010 §1 — the claims semantics (no-work-is-a-value) that §2's extent rule composes with.