diff --git a/alkstore-sqlite/src/substrate/PROVENANCE.md b/alkstore-sqlite/src/substrate/PROVENANCE.md index f4b67c8..939fea5 100644 --- a/alkstore-sqlite/src/substrate/PROVENANCE.md +++ b/alkstore-sqlite/src/substrate/PROVENANCE.md @@ -117,6 +117,7 @@ fixes. Cherry-picks: empty at scaffold, append-only forever. | D-26 | hygiene | `volume_serial_number as u64` → `u64::from(volume_serial_number)` in `VolumeIdentity` (clippy lint conformance, zero behavior change) | watcher.rs (`stat_identity` region) | family clippy `-D warnings` gate | ours | | D-27 | re-derivation | wave-2 review fix — `scheduler_tick`'s fire enqueue resolved the schedule row's relative `expires_s` into the absolute row `expires_at` (`now + s` at the fire transaction, one clock read) instead of passing the relative value through; regression test pins a fired job's `expires_at = fire-instant + s` and its claimability. The pre-fix pass-through would have produced rows expired in 1970 under any `expires_s` schedule | `queue_ops.rs` (`scheduler_tick`, fire-expiry resolution + test) | ADR-020 §2 (relative `expires` → resolved absolute row value, resolved at enqueue against the single clock) — D-12's landed state carried the lineage's `expires_s` field shape without carrying the lineage's enqueue-side resolution; wave-2 review (task review-wave-2) re-derivation spot-check found it | ours | | D-28 | hygiene | wave-2 review fix — the cross-mechanism pressure test's lock-churn branch exercised a per-producer lock name (literal `"p{producer}"` had no interpolation, so all producers contended on one name) with its result swallowed via `.unwrap_or(0)`; now interpolates, releases periodically, propagates errors | `ops.rs` (pressure test, lock branch) | test-quality only (no lineage counterpart, no library code touched); `fork-provenance-and-floor`'s ported-surface pressure intent | ours | +| D-29 | port delta | `SQLITE_OPEN_URI` dropped from `open_conn`'s flags (the watcher's own opens never carried it — the inherited posture was internally inconsistent); the path argument is a plain filesystem path, `?name=value` suffixes are literal filenames, no connection-semantics mutation outside the engine constructor's opts; pinned by test (`open_conn_treats_uri_shaped_path_as_plain_filename`) | `schema.rs` (`open_conn`) | ADR-023 §3 (plain-path open — the URI parameter surface is SQLite's, not this engine's; no consumer-inventory row names URI features) | ours | ### Cherry-picks diff --git a/alkstore-sqlite/src/substrate/schema.rs b/alkstore-sqlite/src/substrate/schema.rs index ae9212f..fa469e7 100644 --- a/alkstore-sqlite/src/substrate/schema.rs +++ b/alkstore-sqlite/src/substrate/schema.rs @@ -309,11 +309,13 @@ pub(crate) fn bootstrap_schema(conn: &Connection) -> Result<(), Error> { } pub(crate) fn open_conn(path: &str, install_notify: bool) -> Result { + // No SQLITE_OPEN_URI (ADR-023 §3 delta): the path argument is a + // plain filesystem path — URI-interpreted `?name=value` suffixes + // would mutate connection semantics outside the engine + // constructor's opts. let conn = Connection::open_with_flags( path, - OpenFlags::SQLITE_OPEN_READ_WRITE - | OpenFlags::SQLITE_OPEN_CREATE - | OpenFlags::SQLITE_OPEN_URI, + OpenFlags::SQLITE_OPEN_READ_WRITE | OpenFlags::SQLITE_OPEN_CREATE, )?; apply_default_pragmas(&conn)?; if install_notify { @@ -554,6 +556,27 @@ mod writer_reader_tests { let _ = std::fs::remove_file(format!("{}-shm", tmp.display())); } + #[test] + fn open_conn_treats_uri_shaped_path_as_plain_filename() { + // ADR-023 §3: no SQLITE_OPEN_URI — a `?name=value` suffix must + // be a literal filename (the file exists with that name), not + // URI interpretation (which for a bare-relative "file"-less + // string with query params would error or mutate semantics). + let dir = std::env::temp_dir().join(format!( + "alkstore-uri-literal-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("plain.db?mode=memory"); + let conn = open_conn(path.to_str().unwrap(), false).unwrap(); + conn.execute("CREATE TABLE t (x)", []).unwrap(); + drop(conn); + let plain = dir.join("plain.db?mode=memory"); + assert!(plain.exists(), "literal-named file must exist on disk"); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn concurrent_opens_never_return_database_is_locked() { const ROUNDS: usize = 8; diff --git a/alkstore/src/payload.rs b/alkstore/src/payload.rs index 4b44022..7157d32 100644 --- a/alkstore/src/payload.rs +++ b/alkstore/src/payload.rs @@ -19,8 +19,15 @@ use crate::Error; /// stream event row are, contract-pinned, the serde_json serialization /// of the `Value` the call carried. Cross-engine byte equality falls /// out — the same `Value` serializes identically on both engines. -pub fn encode_payload(value: &serde_json::Value) -> Vec { - serde_json::to_vec(value).unwrap_or_else(|_| Vec::new()) +/// +/// Fallible per ADR-023 §1: serialization failure is +/// [`Error::Codec`](crate::Error::Codec), typed at this helper — no +/// silent fallback form exists (an empty-byte store would decode to +/// `Null`, silent data corruption). Unreachable for `serde_json::Value` +/// in practice (its variants all serialize), pinned against an +/// encoding posture change. +pub fn encode_payload(value: &serde_json::Value) -> Result, Error> { + serde_json::to_vec(value).map_err(|e| Error::Codec(e.to_string())) } /// Decode stored payload bytes into a concrete target type. diff --git a/alkstore/src/value_type_tests.rs b/alkstore/src/value_type_tests.rs index dc185e0..fcb21c3 100644 --- a/alkstore/src/value_type_tests.rs +++ b/alkstore/src/value_type_tests.rs @@ -86,7 +86,7 @@ fn sample_event(payload: Vec) -> StreamEvent { #[test] fn payload_as_round_trips_json_value_through_job() { let v = json!({"id": 9, "nested": {"ok": [true, null, 1.5]}, "s": "ünï"}); - let bytes = crate::encode_payload(&v); + let bytes = crate::encode_payload(&v).unwrap_or_else(|e| panic!("encode failed: {e}")); assert_eq!(bytes, serde_json::to_vec(&v).unwrap_or_default()); let job = sample_job(bytes); let decoded: Value = job @@ -98,7 +98,8 @@ fn payload_as_round_trips_json_value_through_job() { #[test] fn event_payload_as_round_trips() { let v = json!([1, 2, 3, {"x": "y"}]); - let event = sample_event(crate::encode_payload(&v)); + let bytes = crate::encode_payload(&v).unwrap_or_else(|e| panic!("encode failed: {e}")); + let event = sample_event(bytes); let decoded: Value = event .payload_as() .unwrap_or_else(|e| panic!("decode failed: {e}")); @@ -108,7 +109,8 @@ fn event_payload_as_round_trips() { #[test] fn payload_as_concrete_typed_targets() { let v = json!({"alpha": 1, "beta": 2}); - let job = sample_job(crate::encode_payload(&v)); + let bytes = crate::encode_payload(&v).unwrap_or_else(|e| panic!("encode failed: {e}")); + let job = sample_job(bytes); let decoded: HashMap = job.payload_as().unwrap_or_else(|e| panic!("decode: {e}")); assert_eq!( decoded, @@ -119,7 +121,8 @@ fn payload_as_concrete_typed_targets() { #[test] fn payload_as_undecodable_target_yields_codec() { let v = json!({"name": "x"}); - let job = sample_job(crate::encode_payload(&v)); + let bytes = crate::encode_payload(&v).unwrap_or_else(|e| panic!("encode failed: {e}")); + let job = sample_job(bytes); let r: crate::Result = job.payload_as(); match r { Err(crate::Error::Codec(msg)) => assert!(!msg.is_empty()), @@ -130,6 +133,13 @@ fn payload_as_undecodable_target_yields_codec() { assert!(matches!(r2, Err(crate::Error::Codec(_)))); } +#[test] +fn encode_payload_success_carries_exact_serialization() { + let v = json!({"k": [1, 2, 3]}); + let bytes = crate::encode_payload(&v).unwrap_or_else(|e| panic!("encode failed: {e}")); + assert_eq!(bytes, serde_json::to_vec(&v).unwrap_or_default()); +} + #[test] fn stop_token_cancel_flips_all_clones() { let token = StopToken::new(); diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 53cd5b8..2c32268 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-07 (ADR-022: contract-suite layout resolved) +last_updated: 2026-10-08 (ADR-023: fourth review round resolved — waves-1–2 general-review findings) --- # alkstore — Architecture @@ -57,6 +57,7 @@ pending architecture review and OQ resolution. | [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 | | [022](decisions/022-contract-suite-layout.md) | Contract-suite layout — shared internal suite crate, properties parameterized over a store factory (discharges ADR-017 §4.2's deferral) | Accepted | +| [023](decisions/023-fourth-review-round.md) | Fourth review round — `encode_payload` typed (`Codec`), numeric-argument domains by kind (extents clamp empty, durations reject, boundaries total), plain-path SQLite open (URI flag dropped), watcher cadence 1 ms default + `SqliteOpts` knob | Accepted | ## Open Questions @@ -86,9 +87,11 @@ depth — [ADR-015](decisions/015-streams-depth.md)), ~~OQ-10~~ [ADR-018](decisions/018-provenance-register-and-cherry-picks.md)). 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 +2026-10-06 second review round, the 2026-10-07 third review round, and +the 2026-10-08 fourth review round (the waves-1–2 general review) resolved their findings directly as ADR-019/ADR-020 and -[ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) +[ADR-021](decisions/021-tx-reads-and-value-shape-fixes.md) and +[ADR-023](decisions/023-fourth-review-round.md) respectively, rather than as new OQs. Phase 1 moves to architecture review closure and the implementation-phase gates. diff --git a/docs/architecture/core-contract.md b/docs/architecture/core-contract.md index 08f6b99..9c5ca70 100644 --- a/docs/architecture/core-contract.md +++ b/docs/architecture/core-contract.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-07 (ADR-021 + third-round follow-through: enqueue_tx stamp source, sweep_expired handle form, handle-op/ack_batch/opts-type pins, with_tx signature, payload-limit basis, per-stream offsets) +last_updated: 2026-10-08 (ADR-023 — fourth review round: numeric-argument domains pinned by kind, encode_payload typed, plain-path SQLite open, watcher cadence posture) --- # Core contract @@ -107,11 +107,35 @@ point ([ADR-015](decisions/015-streams-depth.md) §2). Mechanism handles come off the store (or, for transactional variants, off the handle). **Payload encoding** ([ADR-020](decisions/020-enqueue-opt-semantics-and-bridges.md) -§4): the trait crosses `serde_json::Value`; engines serialize it with -serde_json and store exactly those bytes — `payload_as` decodes +§4, signature typed by [ADR-023](decisions/023-fourth-review-round.md) +§1): the trait crosses `serde_json::Value`; engines serialize it with +serde_json via core's `encode_payload` (fallible — `Codec` on +serialization failure, typed at the helper; never a silent fallback +form) and store exactly those bytes — `payload_as` decodes them, `Codec` on failure; round-trip (publish → read → decode → `Value` equality) is what the contract suite pins, on both engines. +**Numeric-argument domains** ([ADR-023](decisions/023-fourth-review-round.md) +§2) — classified by argument kind, uniform on both engines (no engine +may reinterpret a caller-supplied numeric argument via SQL-dialect +semantics; validation happens at the trait-impl entry point, before +the engine's storage layer): + +- **Extents** ("at most this many": `claim_batch(n)`, + `read_since(limit)`, `read_from_consumer(limit)`) — non-positive + values yield the **empty value result** (a value, not an error — + the no-work vocabulary); the SQLite `LIMIT -1` dialect artifact + (a negative extent reaching the whole queue/stream) is dead by + this rule. +- **Durations** (`try_lock(ttl)`, `renew(ttl)`) — non-positive values + are rejections (`Err`, opaque `Database`, detail in the source + chain); an exclusion mechanism must not silently hand back a lease + the caller believes it holds. +- **Boundaries/offsets** (`trim_to(horizon)`, enqueue `delay`/ + `run_at`/`expires`) — total: any `i64` is well-defined (a negative + horizon deletes nothing; a negative delay resolves to a ready + time in the past, i.e. ready now). + ```text store.notify(channel, payload) handle.notify_tx(channel, payload) store.stream(name) -> StreamHandle handle.publish_tx / publish_with_key_tx @@ -258,6 +282,9 @@ business-tx shape). Depth pinned by ([ADR-015](decisions/015-streams-depth.md) §5) stays a valid position marker. - `trim_to(horizon) -> count` — delete events with `offset <= horizon` + (exact-boundary; the horizon is a boundary argument, total per + [ADR-023](decisions/023-fourth-review-round.md) §2 — a negative + horizon deletes nothing, idempotently) ([ADR-015](decisions/015-streams-depth.md) §5; the return is the deleted-row count, [ADR-019](decisions/019-mechanism-handle-surfaces.md)). The stream-side bounded-growth op, consumer-invoked, no engine-default @@ -336,6 +363,10 @@ obligations: ordering: priority DESC, then ready-time, then enqueue order (FIFO under equal priority). Claims return boxed `JobHandle`s — ops + row value (same ADR, §3). + `claim_batch(n)`'s `n` is an extent + ([ADR-023](decisions/023-fourth-review-round.md) §2): `n <= 0` + claims nothing (the empty `Vec`, a value — never an error, never + the whole queue). - Job handle: `ack / retry / fail / heartbeat` on the boxed `JobHandle` a claim returns ([ADR-019](decisions/019-mechanism-handle-surfaces.md) §3 — `job(&self) -> &Job` beside the ops; one-shot `self: Box` @@ -392,7 +423,9 @@ release it — the TTL discipline governs). lock handle. Release on explicit unlock or TTL expiry. Re-acquirable after expiry (POC-pinned on Postgres; on SQLite it rests on the forked substrate's lock machinery — the SQLite-side pin - is in the verification backlog below). + is in the verification backlog below). `ttl <= 0` is a rejection + (duration kind, [ADR-023](decisions/023-fourth-review-round.md) §2) + — opaque `Err`, never a silent immediately-expired lease. - **Guarantee row** ([ADR-008](decisions/008-contract-v1-pinning.md) §7): mutual exclusion bounded by TTL + renewal — after TTL expiry exclusion lapses *silently* (no revocation event); holders must @@ -690,6 +723,31 @@ before the engine specs are called `stable`: 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. +- **Numeric-domain semantics on both engines** + ([ADR-023](decisions/023-fourth-review-round.md) §2) — extents + clamp to empty (`claim_batch(n <= 0)` claims nothing; stream reads + with `limit <= 0` read nothing, never the whole queue/stream), so + the SQLite `LIMIT -1` dialect artifact is dead on both engines; + durations reject (`try_lock`/`renew` with `ttl <= 0` are opaque + `Err`s on both engines); boundaries are total (a negative + `trim_to` horizon deletes nothing, idempotently). +- **`encode_payload` typed failure** + ([ADR-023](decisions/023-fourth-review-round.md) §1) — the helper + returns `Result, Error::Codec>`; enqueue/publish store and + decode the exact serialization of the input `Value` (byte-identical + rows cross-engine, the §4 round-trip unchanged), and the failure + arm is typed at the helper — no silent empty-byte fallback exists. +- **Plain-path SQLite open** ([ADR-023](decisions/023-fourth-review-round.md) + §3) — the SQLite engine's `open` path argument is a plain + filesystem path: a string carrying a `?name=value` suffix is + treated as a filename, no URI interpretation, no connection- + semantics mutation outside the constructor's opts. +- **Watcher cadence default + knob** ([ADR-023](decisions/023-fourth-review-round.md) + §4) — the SQLite store's watcher runs at the inherited 1 ms + default; `SqliteOpts::poll_interval` changes the configured + cadence (config-surface accounting — wall-clock cadence + measurements stay out of the contract suite's deterministic + posture). ## Design Decisions @@ -711,6 +769,7 @@ before the engine specs are called `stable`: | [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 | +| [023](decisions/023-fourth-review-round.md) | Fourth review round (amends 020 §4, 008 §5, 012 §4) | numeric-argument domains by kind (extents clamp empty, durations reject, boundaries total); `encode_payload` typed (`Codec`); plain-path SQLite open (URI flag dropped); watcher cadence 1 ms default + `SqliteOpts` knob | ## Open Questions diff --git a/docs/architecture/decisions/023-fourth-review-round.md b/docs/architecture/decisions/023-fourth-review-round.md new file mode 100644 index 0000000..0c25351 --- /dev/null +++ b/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`** (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` | 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` +(`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. \ No newline at end of file diff --git a/docs/architecture/deployment.md b/docs/architecture/deployment.md index 6fc8c60..ae4bf27 100644 --- a/docs/architecture/deployment.md +++ b/docs/architecture/deployment.md @@ -87,6 +87,7 @@ engine-owned PostgreSQL schema (default `alkstore`, per-engine option) | Engine | Knob | Shape | |---|---|---| | SQLite | `synchronous` | WAL + `NORMAL` shipped ([ADR-003](decisions/003-sqlite-driver.md)); FULL is available consumer-side for stricter durability; commit fsyncs land at WAL checkpoints (the ~1000-commit spike cadence, POC #1) | +| SQLite | `poll_interval` | the watcher's `data_version` poll cadence — 1 ms shipping default ([ADR-023](decisions/023-fourth-review-round.md) §4, the measured-wake-latency posture), carried on `SqliteOpts`; the idle cost is ~1000 poll reads/sec/instance; raising the interval trades wake latency (interval-bound) for idle CPU — the tuning recipe for latency-tolerant deployments | | Postgres | `synchronous_commit` | per-session knob; `on` is ship config (p50 2.40 ms seam); `off` trades max-tail (40.9 ms) for slightly better p50 — measured, honest trade ([ADR-004](decisions/004-postgres-driver.md)); session-level SET mechanics POC-verified | These are engine-configuration concerns, *not* trait surface. What diff --git a/docs/architecture/engine-sqlite.md b/docs/architecture/engine-sqlite.md index c3407a4..01c82e1 100644 --- a/docs/architecture/engine-sqlite.md +++ b/docs/architecture/engine-sqlite.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-07 (ADR-021 — third review round: tx reads, claimed_at, schedule queue validation, drop=rollback, receiver arms) +last_updated: 2026-10-08 (ADR-023 — fourth review round: plain-path open (URI flag dropped), 1 ms watcher default + SqliteOpts knob) --- # SQLite engine @@ -50,13 +50,28 @@ per-contract obligations live in the core spec and are not restated here. fanning out to listeners, overtriggering on purpose (waking all subscribers per poll tick, even when several commits coalesced inside one tick — wake is a hint; consumers re-read indexed state, - [ADR-006](decisions/006-wake-and-delivery-contract.md)). + [ADR-006](decisions/006-wake-and-delivery-contract.md)). The 1 ms + default is the shipping default — the POC's published wake-latency + numbers were measured at it + ([ADR-023](decisions/023-fourth-review-round.md) §4); the cadence + is tunable via `SqliteOpts::poll_interval` + ([ADR-008](decisions/008-contract-v1-pinning.md) §6's config split + — engine opts carry cadence), the documented idle cost and tuning + recipe in [deployment.md](deployment.md). - Each connection runs the substrate's bootstrap at open: pragmas (WAL, `synchronous=NORMAL`, busy timeout), `attach_notify`, the substrate's function attachments and schema bootstrap (the alknet-filesystem POC's wiring shape). Watcher spawn is fallible in the substrate ([ADR-012](decisions/012-forked-substrate-design.md) §4); open fails if the watcher cannot start. +- **The `open` path argument is a plain filesystem path** + ([ADR-023](decisions/023-fourth-review-round.md) §3): the + substrate's `open_conn` drops the inherited `SQLITE_OPEN_URI` flag + (a registered fork delta), so `?name=value` suffixes in the path + are literal filenames — no URI interpretation, no connection- + semantics mutation outside the constructor's opts (`:memory:` works + without URI mode; URI-only features like shared-cache memory DBs + re-enter with a consumer-inventory row). ## Mapping the contract @@ -136,6 +151,7 @@ family is `__alkstore_*` (ADR-010 §8's naming authorization). | [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` | +| [023](decisions/023-fourth-review-round.md) | Fourth review round | `open` path is a plain filesystem path (URI flag dropped — registered fork delta); 1 ms watcher default stands, cadence carried onto `SqliteOpts::poll_interval`; `encode_payload` typed (`Codec`); numeric-argument domains pinned contract-side (extents clamp empty — enforced at this engine's trait-impl entry over the substrate) | ## Open Questions diff --git a/docs/architecture/open-questions.md b/docs/architecture/open-questions.md index f96cd9e..4336774 100644 --- a/docs/architecture/open-questions.md +++ b/docs/architecture/open-questions.md @@ -1,6 +1,6 @@ --- status: draft -last_updated: 2026-10-07 (third review round — findings resolved as ADR-021; question set remains closed) +last_updated: 2026-10-08 (fourth review round — waves-1–2 general-review findings resolved as ADR-023; question set remains closed) --- # alkstore — Open Questions @@ -61,8 +61,22 @@ an ADR-010/ADR-019 `Job`-struct disagreement, an unvalidated 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). + methods on `TxHandle`, `claimed_at` restored, schedule queue-argument + validation, drop = rollback, receiver arms pinned). **Fourth + review round (2026-10-08, the waves-1–2 general review — the + first round after implementation began):** the code review's four + decision-needing findings (`encode_payload`'s empty-Vec failure + arm, `claim_batch(n <= 0)`'s `LIMIT`-dialect semantics, the + inherited `SQLITE_OPEN_URI` flag, the 1 ms watcher-poll default) + were likewise all decidable from decided material — resolved + directly as + [ADR-023](decisions/023-fourth-review-round.md) (the helper + fallible with `Codec`; numeric-argument domains pinned by kind — + extents clamp empty, durations reject, boundaries total — a + core-contract amendment, since engine-side divergence here would + break engine-agnostic composition; plain-path `open`; the 1 ms + default standing with the cadence carried onto `SqliteOpts`). No + new open questions; the register stays closed. Resolved questions stay listed with their resolution; they are not deleted. @@ -537,4 +551,8 @@ narrowed to the pinning work its own record already scoped.)* None. **The open question set is empty** — every OQ above is resolved (2026-10-04 through 2026-10-06, ADR-001 through ADR-018). Phase 1's remaining work is architecture review closure and the implementation -phase's gates; no external arrivals are being waited on. +phase's gates; no external arrivals are being waited on. The review +rounds after closure (2026-10-06 second, 2026-10-07 third, 2026-10-08 +fourth) likewise produced no new OQs — their findings resolved +directly as ADR-019/020, ADR-021, and +[ADR-023](decisions/023-fourth-review-round.md) respectively. diff --git a/docs/reviews/2026-10-08-waves-1-2-general-review.md b/docs/reviews/2026-10-08-waves-1-2-general-review.md index 75b9df8..f575516 100644 --- a/docs/reviews/2026-10-08-waves-1-2-general-review.md +++ b/docs/reviews/2026-10-08-waves-1-2-general-review.md @@ -1,5 +1,17 @@ # General review — waves 1 and 2 +> **Post-review note (2026-10-08):** the four decision-needing +> findings below are resolved as +> [ADR-023](../architecture/decisions/023-fourth-review-round.md) — +> N-2 (`encode_payload` typed, `Codec`), M-2 (numeric-argument domains +> pinned contract-side, extents clamp empty — decided broader than +> this report's engine-layer recommendation: the same dialect hazard +> exists on the stream reads, and domains must be engine-uniform), +> N-4 (URI flag dropped, a registered fork delta), N-6 (1 ms default +> stands, cadence carried onto `SqliteOpts`). The task-carried items +> (lint removal, wave-5 suite adds, wave-6 N-1 review) stand as +> ordered in §7. + - **Reviewer**: opencode (glm-5.3-flash) - **Scope**: general review of everything landed by waves 1–2 (11 tasks, commits `34e0b97..af5b59e`) — security posture, code smells, contract adherence,