Fourth review round (ADR-023): encode_payload typed (Codec), numeric-argument domains pinned by kind, plain-path SQLite open (URI flag dropped, D-29), watcher cadence posture — waves-1-2 general-review findings

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

@@ -0,0 +1,330 @@
+# 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/2026-10-08-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<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](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<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](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` | 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](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<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](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/2026-10-08-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.