feat: the embedding API facade — Connection, Statement, Rows, Transaction (spec 013, all 8 requirements) - #705
Open
dpsiderius wants to merge 17 commits into
Open
dpsiderius wants to merge 17 commits into
dpsiderius wants to merge 17 commits into
Conversation
… the opcode (#692) Nothing in `src/vdbe/` reported how many rows an `INSERT`/`UPDATE`/ `DELETE` changed. Spec 013 calls this the one item on its list a consumer cannot work around: `execute_transaction_step` returns rows and the autocommit flag, so a caller cannot tell an `UPDATE` that matched from one that did not, and that is the distinction every optimistic-concurrency scheme is built on. SQE swaps a table's metadata pointer with a conditional `UPDATE` and treats zero rows affected as a lost race; without the count that becomes SELECT-then-UPDATE, sound only while the consumer guarantees a single writer. The obvious implementation is wrong, and measurably so. Counting in the `Insert`/`Delete` handlers reports, per changed row: INSERT Insert -> 1 DELETE Delete -> 1 UPDATE, single-pass Delete + Insert -> 2 UPDATE, two-pass range-seek ephemeral Insert + Delete + Insert -> 3 The two-pass plan is #666/#675's range-seek path, which stashes matched rowids in an ephemeral b-tree using the same `Opcode::Insert`. So one `UPDATE` reports 2 or 3 depending on which plan the optimizer picked, and neither is 1. Index maintenance is the same shape: a write next to a row that is not a row change. So codegen marks the one mutation that counts, with `OPFLAG_NCHANGE` (`0x01`) on `P5` — stock SQLite's flag, same bit, same job. `P5` was unread by both opcodes, so nothing had to move, and no new opcode means the frozen-set ADRs (0015/0018/0020) stay closed. An `UPDATE` flags its `Insert` and not the paired `Delete`: one changed row, counted once. `StepOutcome::changes` is `Option<u64>`, and the two cases are not the same answer. `Some(0)` is "this was a DML statement and it changed nothing" — the lost race. `None` is "not that kind of statement", so a connection tracking `sqlite3_changes()` leaves its stored count alone after a `SELECT`. `Program::counts_changes()` is the discriminator and is deliberately *static*: an `UPDATE` whose `WHERE` matches nothing never executes its flagged `Insert` but must still report `Some(0)`. `execute_transaction_step` is now a wrapper over `execute_transaction_step_counted`, per ADR-0040's pattern — one loop, the old signature expressed in terms of the new one, so they cannot drift and its ten existing call sites are untouched. ADR-0042 records all of it, including why `u64` instead of `Option<u64>` would be a bug rather than a simplification. Both wrong designs are mutation-checked, not just argued: - counting unconditionally in the handlers fails `update_of_one_row_reports_one_under_both_plans`, `index_maintenance_does_not_count` and `conditional_update_reports_match`; - additionally flagging `UPDATE`'s `Delete` fails the same three, and fails the oracle diff with 4 against the oracle's 2. Verified: 1569 unit tests (1562 baseline + 7) and 381 corpus (380 + 1) pass, clippy/fmt/mod-files clean, assurance 86/86 and 276/276 with no dead links. `tests/corpus/changes_oracle_test.rs` diffs a thirteen-statement sequence against the pinned 3.53.4 oracle's own `changes()`, covering both `UPDATE` plans, a miss, a partial `DELETE` and a full one. Not included: `Connection::changes` and the cross-statement retention rule. A `Vm` lives for one statement so it cannot own that rule; it is spec 013/Req 1's surface and belongs to the facade ticket, where it is one line — store on `Some`, ignore `None`. One note for whoever merges second: `Execution` (#683) should grow a `changes()` the same way, which is a three-line addition once both are on `main`. The requirement IDs cited here live in #678, not yet on `main`. Refs: 013/Req-1, #692, #678, #683 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#691's streaming `Execution` landed on `main` and moved the run loop out of `fn run` and into `Execution::next_row`, which is where this branch was reading `vm.changes` from. Resolved as #692's own issue text called for ("the `Option<u64>` on the execution entry points, plus `Execution` once #683 lands"): `Execution` grows a `changes()` accessor beside `autocommit()`, and `run` — now a wrapper over `next_row` — carries the count out as its third tuple element. `StepOutcome` and `execute_transaction_step_counted` are unchanged. Also in this merge: - `tests/unit/vdbe_changes_test.rs::streaming_execution_reports_the_rows_it_changed` covers the new accessor. It pins absolute counts rather than streaming-vs-batch parity: `run` reads the count through this very accessor, so a parity test stays green when the accessor is stubbed to zero. Mutation-checked both ways. - `Execution::changes`'s doc says "so far" and means it, since a caller that abandons a stream sees a partial count. It does not claim `RETURNING` as the motivating case — that is a V9 rule the tokenizer knows and the parser does not. `Cargo.toml` and `.openspec/adr/index.md` were additive conflicts (both sides appended an entry); both entries kept. Gates on the merged tree: 1576 passed / 0 failed, corpus 388, `make lint` clean, assurance 86/86 and 276/276 with no dead links introduced — the 24 it reports are #693's spec-013 `(planned)` links, identical on `main`.
…brary (#695) `Connection::prepare(sql)` cannot be written today. `compile_statement` handles write and DDL statements only — hand it a `SELECT` and it answers `Unrecognized("SELECT")`, which I hit for real while writing #692's tests. A `SELECT` needs its CTEs and views expanded, its FROM tables resolved and `sqlite_stat1` stats loaded first, and the pipeline that does all of that lived in `src/bin/sqlite-rs/query.rs` — inside the executable, where no library consumer can reach it. That split is a CLI implementation detail, not something a consumer should have to know about. Stock SQLite has exactly one `sqlite3_prepare_v2()`: hand it any statement, get a handle back. Spec 013's `Connection::prepare` has to behave the same way, so the missing half moves into `src/codegen/prepare.rs` where both callers can use it. Not a rewrite. `compile_select_program` and `SelectOutcome` are the CLI's own code relocated, and the 127-line local copy is deleted rather than left to drift — `query.rs` and `repl.rs` now call the library's. `derive_headers` comes along as `result_column_names`, so `Row`'s by-name access and the CLI's column headers will agree by construction instead of by coincidence. One thing changed rather than moved: every error was `.map_err(|e| e.to_string())`, flattening a structured `CodegenError` into a message. That is fine when the next step is printing to a terminal and wrong for a library, where a caller needs to match on what went wrong. Errors are now `PrepareError`, which wraps `CodegenError` and adds the one case that is about the request rather than the SQL: asking for `EXPLAIN QUERY PLAN` on a FROM-less `SELECT`, which is not a `CodegenError::NoFromClause` because `SELECT 1` compiles fine, it just has no access path to explain. `Display` reproduces the old strings exactly, so CLI output is unchanged. `from_less_schema()` stays private. The CLI inlined that 12-field literal; a public `TableSchema::none()` would be new API surface this lift does not need. The CLI behaving identically is the claim that matters here, since this is a refactor of the path every `sqlite-rs query` and REPL statement takes. Evidence: 1575 unit, 388 corpus (which includes the CLI e2e suite) and 15 sqllogictest all pass unchanged, `make lint` clean both clippy passes, and four queries plus `EXPLAIN QUERY PLAN` hand-checked byte-identical against the pinned 3.53.4 oracle — including the FROM-less EQP error, whose message survives the `String` -> `PrepareError` change intact. No `Connection` or `Statement` yet; this is the groundwork they need. PRAGMA dispatch is deliberately not lifted: spec 013's non-goals give the PRAGMA catalogue to V7, and SQE's statement list contains none. Refs: 013/Req-3, #695, #678 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… it falsified (#695) The lift commit had no tests of its own. Every existing exercise of `compile_select_program` went through the `sqlite-rs` binary, so the newly-public library surface had zero direct coverage and a regression in it would only have surfaced as a CLI failure. Two files fix that. `tests/unit/prepare_test.rs` (10 tests) pins the dispatch and the error type: one case per `compile_select_program` arm (FROM-less, single-table, joined, compound), `eqp_mode` returning rows rather than a program, `PrepareError::EqpWithoutFrom` including its message being byte-identical to the string the CLI printed before the lift, an unknown table arriving as a matchable `PrepareError::Codegen` rather than a message to parse, and `result_column_names`' positional fallback for joins and compounds — asserted rather than left implicit, because `Row::get_by_name` will be built on it and `column1` is a real answer a consumer can receive. `tests/corpus/prepare_oracle_test.rs` pins the answers. Eleven queries compiled through the library and diffed against the pinned 3.53.4 oracle, on a fixture the oracle itself created — the adoption direction that matters. Shapes include SQE's own: `UNION`, `UNION ALL`, `LIMIT 1` existence probes that hit and miss, a join, aggregates. I had checked four queries by hand while doing the lift, which is worth nothing once the terminal closes; a refactor of the path every `sqlite-rs query` takes deserves a check that runs in CI. Mutation-verified that it really talks to the oracle: perturbing integer rendering fails it. `SelectOutcome` gains `derive(Debug)`. It is public and a consumer matches on it, both payloads already derive it, and without it `unwrap`/`expect_err` on a `Result<SelectOutcome, _>` will not compile — which is a papercut for every caller, not just these tests. Also corrected: `tests/performance/v6.rs`'s comment explaining why that bench duplicates the compile pipeline said the real function "is `pub(crate)` to the binary crate and not callable from an external test/bench crate". The lift made that false. The comment now says so and points at the duplication as removable — deliberately not removed here, since replacing it changes the path this bench measures and that belongs in a change whose bench numbers are the point. One test failure worth recording: `result_column_names_honours_aliases` was written as `SELECT a AS first, b AS second` and failed to parse. Not a bad test — `FIRST` is one of 89 keywords we reserve that SQLite treats as an identifier (`parse.y:272`'s `%fallback ID`). Filed as #696, which also blocks SQE outright: its `iceberg_namespace_properties` has a `key` column, and we cannot create or read that table at all. The test here uses non-keyword aliases and the keyword case is pinned in #696 rather than smuggled in. Verified: 1572 unit (1562 + 10), 388 corpus (387 + 1), 15 sqllogictest, `make lint` clean both passes, `make check-mod-files`, assurance 86/86 and 276/276 with no dead links. Refs: 013/Req-3, #695, #696 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eholder counting (013/Req 1, Req 3) Spec 013's `Connection` needs three things from the engine that it cannot synthesise, and this is all of them bar the ones #692 already landed. **`last_insert_rowid`, flagged by codegen** (`OPFLAG_LASTROWID`, 0x20 — same bit as stock SQLite, `sqliteInt.h:4068` at the pinned 3.53.4). A row inserted into a table with a surrogate key is unaddressable until the caller learns its rowid, so this is not a convenience. Two properties of the upstream handler are reproduced deliberately, and neither is guessable from the name: * It nests *inside* `OPFLAG_NCHANGE`. `vdbe.c:5803` updates `db->lastRowid` within the `if( p5 & OPFLAG_NCHANGE )` arm, and `vdbe.c:5800` asserts the implication. A mutation that is not a counted row change must not move the rowid either. * An `UPDATE` does not set it. `insert.c:2834` reads `pik_flags |= (update_flags ? update_flags : OPFLAG_LASTROWID)` — the flag is the *else* branch. An UPDATE emits an `Insert`, so hooking the opcode instead of the flag would report the updated row's rowid and destroy the value the caller was about to use. The hook is on `Insert`, not `NewRowid`, because for `INSERT INTO t(id, ...)` over an `INTEGER PRIMARY KEY` the rowid comes from the bound value and `NewRowid` never executes — which is exactly the shape a consumer with its own keys uses. **`Program::param_count()`**, derived rather than stored, for the same reason #692 gave for `counts_changes()`: a stored field can disagree with the instructions it describes, and `Program::new` is public. It reads the ceiling off the emitted `Variable` instructions, matching `sqlite3_bind_parameter_count`'s "largest index, not number of distinct" (`expr.c:1331` for bare `?`, `expr.c:1356` for `?nnn`). **Named parameters are now refused** rather than compiled. `:name`/ `@name`/`$name` parse but were never wired to an index, and compiled to a fresh NULL-reading register — turning `WHERE x = :name` into `WHERE x = NULL`, which matches no row and raises no error. A silent wrong answer is the worst failure mode for a consumer binding by name, so this is a deliberate behaviour change. The indices still don't exist; the refusal is honest until they do. `StepOutcome` gains `last_insert_rowid: Option<i64>`, `None` meaning "leave the connection's stored value alone" — the same retention rule as `changes`, and an `Option` rather than a sentinel because rowid 0 is legal. Tests ----- `tests/corpus/last_insert_rowid_oracle_test.rs` diffs the *retained* value against the pinned 3.53.4 after all 14 statements of a sequence, in one oracle invocation (the value is connection-scoped, so a fresh connection per statement would reset it and make retention untestable). Comparing only the inserting statements would pass even if `UPDATE` cleared the value — the bug the flag exists to prevent. Confirmed discriminating: with `OPFLAG_LASTROWID` wrongly added to `update.rs`, `UPDATE ... WHERE a = 1` drags the value to 1 where the oracle holds at 2. `tests/unit/vdbe_last_insert_rowid_test.rs` (8 tests) pins the mechanism, including the explicit-`INTEGER PRIMARY KEY` case, a deliberately *decreasing* rowid so a stale-value bug cannot hide behind a monotonic sequence, and two structural assertions: an INSERT flags exactly one instruction, and no program may ever carry the rowid flag without the change flag. `tests/unit/param_binding_test.rs` (9 tests) covers the counting rules and every named-parameter form, in reads and writes. Gates: make test (1603 passed), make test-corpus (390 passed), make lint (both clippy passes + fmt), check-mod-files, check-assurance 100%/100%. Spend: within estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e3 (013/Req 2)
Nothing in the tree verified this before, and the gap was structural rather
than an oversight. Both bootstrap-adjacent suites seed fixtures the same
way — `tests/tiers/tier2.rs`'s `seed_db` and
`tests/corpus/cli_write_test.rs`'s `seed_db` both read:
if let Some(oracle) = pinned_oracle() { /* oracle builds the file */ }
else { /* our CLI builds the file */ }
They *prefer* the oracle and fall back to our own creation path only when
no oracle is installed. So with an oracle present it builds the file and
our path is never exercised; with it absent our path runs and there is no
oracle left to check the result. The two never run together, so "a file we
create is a valid SQLite database" was never actually asserted.
Spec 013 makes it load-bearing: `Connection::open` creates the database if
it does not exist, and a consumer's first file is one nothing else has
ever touched.
Four tests, all against the pinned 3.53.4:
* `new_empty_page1` at all eight supported page sizes — integrity_check
is `ok`, the page size round-trips, the schema is empty, and the oracle
can then grow the file. 65536 is the interesting one: it cannot be
stored literally in the 16-bit page-size field and is encoded as `1`,
with the cell-content-area offset wrapping to 0, so it is the only
branch in the function.
* A file created and written entirely by our path (DDL, two index kinds,
inserts, an update, a delete) is `ok` to the oracle, and the oracle
reads identical schema/rows/index lookups from it and from a file it
built itself from the same statements.
* A byte-level header comparison — the class of bug integrity_check
cannot see, since it validates b-tree structure rather than every
header byte. Exactly three ranges differ and all three are fields this
crate does not model, so the divergence set is asserted exhaustively:
bytes 0..24 and 28..92 must match the oracle's, and 24..28, 92..96,
96..100 must be zero. The test also asserts the oracle's own copies of
those fields are non-zero, so it cannot pass by comparing two sets of
zeroes.
* Writing to an oracle-created file preserves those unmodelled fields
rather than zeroing them — which a writer that re-serialised the header
from its own struct would do.
One finding recorded rather than fixed: we never increment the file change
counter (offset 24). It stays self-consistent with version-valid-for
(offset 92), so the cached page count remains trusted and integrity_check
passes — which is exactly why no existing test caught it. It is a real
interop limitation, though: another SQLite connection already holding a
cached image of the file has no way to learn our writes happened. A fresh
connection reads from disk and sees them, which is why it is latent. Not a
malformation, and out of scope here; the header test states it so a fix
shows up as a failure there.
Tests only — no src/ changes.
Gates: make test-corpus (394 passed), make lint (both clippy passes + fmt).
Spend: within estimate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ad (013/Reqs 1, 2, 4) The facade spec 013 specifies, in the shape ADR-0041 decided. `Connection` with `open`/`open_with`/`open_in_memory`, `execute`/`execute_with`/ `execute_batch`, `changes`/`last_insert_rowid`, and a flat `Error` carrying SQLite result codes. Why a worker thread, precisely ------------------------------ The engine's page-source graph is `Rc`/`RefCell` by decision (ADR-0013, ADR-0017: a read path that pays no atomic refcount) and `Rc` is not `Send`. No wrapper fixes that — `Mutex<T>` is `Send` only when `T: Send`, so wrapping the pager graph in a lock changes nothing at the type level. That leaves an `Arc` refactor of `Pager`/`PageSource`, rejected on read-path cost, or a thread that owns the engine. One further constraint makes the thread the only *expressible* design rather than merely the preferred one, and it is worth recording because neither ADR-0041 nor Jacob's proposal knew it: `make check-mvl-limit` forbids named lifetime parameters in `src/`, so no type in `src/api.rs` may hold an `Execution` — it borrows its `Program`. The execution has to live inside a single worker stack frame, which is exactly what this gives it. (`src/codegen/stmt/insert.rs::ColumnSource` already carries a comment making the same trade for the same reason.) `Connection` is `Arc<Shared>`, so cloning is cheap and every clone addresses one worker. The request channel is a `SyncSender`, not a `Sender`: `Sender<T>` is `Send` but not `Sync`, and Requirement 4 needs both. One bug found and fixed while building it, worth naming because it is invisible in review: `Drop for Shared` originally joined the worker while still holding the only sender. The worker's loop ends when `recv` fails, which needs every sender gone — so the drop waited for the thread and the thread waited for the drop. The channel is now closed explicitly first, and `worker_thread_joins_on_drop` would hang rather than fail if that regressed, which the test says out loud. Read-only mode -------------- Enforced per statement, not by the pager, and recorded as an ADR-0004 divergence: `Pager::open` calls `open_write` unconditionally, there is no read-only pager, and the read-only page source that does exist bypasses `Pager` and so merges no WAL frames — using it would silently serve stale data. The guard keys on `OpenWrite` and the DDL opcodes, *not* on `Insert`/`Delete`. Those also target ephemeral cursors: measured on this tree, `SELECT s.a FROM (SELECT a FROM t LIMIT 5) AS s` compiles to Insert x1 / OpenWrite x0 — a pure read that emits an `Insert`. A guard keyed on `Insert` would refuse it. The `LIMIT` matters too: without it the subquery is flattened and no ephemeral write is emitted, so the test that covers this says why the LIMIT must not be tidied away. The error type -------------- Flat, not wrapping: every payload is a `String`, an `i32` or a `Copy` enum. It has to be unconditionally `Send + Sync + 'static` because every error travels back over a channel, and it derives `PartialEq` so tests assert the error rather than substring-matching a message. The price is no `source()` chain, which is small — the engine's error enums barely implement it. `sqlite_code()` returns the *primary* code and `extended_sqlite_code()` the extended one, mirroring `sqlite3_errcode()`/`sqlite3_extended_errcode()`. That answers the open question in the plan (primary 19 or extended 2067?) by offering both under the names SQLite already uses, rather than picking one. Codes verified against `sqlite3.h` at the pinned 3.53.4. Also here: `From` conversions for `Value` (`i64`, `i32`, `bool`, `f64`, `&str`, `String`, `&[u8]`, `Vec<u8>`, `Option<T>`), so binding a parameter does not require the caller to name `Arc`. Requirement 6 asks that a consumer never reach into the engine, and `Value::Text(Arc<str>)` is a storage detail. Catalog invalidation after DDL is correctness, not caching: a program addresses tables by root page, so compiling against a stale catalog after a DROP/CREATE could read a recycled page. Same conservative rule the CLI already uses. Tests ----- 28 new tests. `tests/unit/api_connection_test.rs` (8), `api_changes_test.rs` (9) and `api_threading_test.rs` (5) use the scenario names spec 013 cites. `tests/corpus/api_oracle_test.rs` (4) diffs against the pinned 3.53.4: rows-affected counts for a twelve-statement sequence, the resulting file read back through the oracle, and parameterised writes covering all five storage classes with `typeof()` agreeing. That corpus file imports only `sqlite_rs::api` and `sqlite_rs::record::Value` — no `pager`, `vdbe`, `codegen` or `dump` — so it compiling is itself Requirement 6's no-escape-hatch claim for the write path. `handle_is_send_sync` is a `const` assertion, not a runtime check: Requirement 4 is a type-level claim. It also runs 8 threads x 25 statements through one cloned handle, and `worker_thread_joins_on_drop` does 200 sequential open/write/drop cycles on one path — a drop that returned before the join would meet its predecessor's file lock. No new dependencies: `std::sync::mpsc` only. Gates: make test (1625 passed), make test-corpus (398 passed), make lint (both clippy passes + fmt), check-mod-files, check-assurance 100%/100%. No named lifetimes, no `dyn`, no `unsafe` in src/api.rs. Spend: within estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lue (013/Reqs 7, 3) Rows arrive from the worker in batches, so peak memory is bounded by the batch size and the pager's page cache rather than by the result. Spike 014 (#682) measured the alternative at 137.7 MB and 5.36 ms to first row for 1,000,000 rows, against 8.68 MB and 44.7 us streamed. Built on `Execution::next_row`, inside one worker stack frame — the only place it can live, since `Execution` borrows its `Program` and no type in `src/api.rs` may carry a lifetime. Two findings worth recording, both from tests that failed first -------------------------------------------------------------- **ORDER BY is a blocking operator, and the first draft of `partial_read_is_bounded` measured it.** It used `SELECT a FROM t ORDER BY a` and failed at 33x on a 50x larger table — correctly. A sort with no usable index has to consume every row before emitting the first, so time-to-first-row is genuinely linear there and no amount of streaming changes it; stock SQLite sorts the same way. The streaming property has to be measured on a plan that can emit as it scans. The test now uses a bare scan and says why in full, and `blocking_plans_are_linear_by_nature` pins the contrast so the limit is recorded rather than quietly avoided. **An unread `Rows` could park the worker, and my own test deadlocked on it.** `joins_and_compounds_report_positional_column_names` held one result alive while issuing a second query; the worker was blocked sending the first result's `Done` into a full one-slot channel, so it never returned to serve. Two changes came out of that: * the result channel now holds two batches, not one (`CHUNK_SLOTS`), so any result that fits in a single batch completes and frees the worker whether or not the caller ever reads it. That covers the accident that is easy to have — `let rows = conn.query(..)` and then forget it — and `an_unread_small_result_does_not_block_the_next_statement` would hang rather than fail if it regressed. * a larger unread result still parks the worker until the handle is read or dropped. That is inherent to one worker streaming one execution, so it is documented on `Rows` with the concrete failing snippet, and `dropping_a_large_unread_result_releases_the_connection` pins that dropping is sufficient. Design points ------------- `Rows` is deliberately not an `Iterator`: collapsing `Result<Option<Row>, Error>` into `Option<Result<Row, Error>>` to fit the trait makes "ended" and "failed" the same shape at the call site. ADR-0040 rejected an `Iterator` impl on `Execution` for this reason. Only a stream that runs to completion updates the connection's counters and autocommit flag; an abandoned one leaves them untouched, since a partial count is not a count. `FromValue` conversions are the ones `sqlite3_column_*` performs without reinterpreting storage. An INTEGER widens to `f64`; a REAL does *not* narrow to `i64`, because truncating silently is how a rowid becomes wrong. `NULL` into a non-`Option` type is a `TypeMismatch` rather than a default. By-name access reports real names only for a single-table `SELECT`: a join or a compound gets `column1`, `column2`, … because that is all `result_column_names` derives (`src/codegen/prepare.rs:181`). `joins_and_compounds_report_positional_column_names` asserts exactly that, so the limit is covered rather than discovered by a consumer. Tests ----- 15 unit tests in `tests/unit/api_streaming_test.rs`, using Requirement 7's own scenario names, plus `queried_rows_match_the_oracle` in the corpus suite: eight queries over a 200-row table — larger than one batch — compared row by row against the pinned 3.53.4. Confirmed discriminating. With the final partial batch dropped in `drain` (the classic streaming bug), the oracle test fails on the first query and 10 of the 15 unit tests fail. Gates: make test (1640 passed), make test-corpus (399 passed), make lint (both clippy passes + fmt), check-mod-files, check-assurance 100%/100%. Spend: within estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…13/Req 5)
`Transaction` (deferred/immediate/exclusive) that rolls back on drop,
`Connection::pragma`, `Connection::set_busy_timeout`, and a stated
durability contract. `Transaction` derefs to its `Connection`, so every
`execute`/`query` method runs inside the transaction.
The point of the type is the drop: a `?` returning early out of a function
holding a transaction cannot leave it open, which for a consumer storing
pointers to data it cannot otherwise find is the difference between a
retryable failure and a half-applied catalog.
Busy retry, and why it is scoped the way it is
----------------------------------------------
The timeout retries only while the connection is in autocommit. There the
statement *is* the transaction, so `Pager::rollback` followed by re-running
it is a faithful retry of the whole unit. Inside an explicit transaction it
would not be: the statement's mutations sit in the same pending set as
every earlier statement's, so re-running one would double-apply it. A
`Busy` there is the transaction's to retry, which is what stock SQLite does
with `SQLITE_BUSY` at `COMMIT`.
The rollback before each retry is load-bearing, not hygiene.
`Pager::flush` documents that a contended escalation surfaces
`VfsError::Locked` "before any byte of this transaction is journaled or
written" and leaves `dirty` intact (`src/pager.rs:524`) — so without the
rollback, a retried INSERT would land once per attempt.
`a_retried_statement_succeeds_exactly_once` is the test for exactly that.
Backoff ladder mirrors `sqliteDefaultBusyCallback`. Default timeout is
zero, matching SQLite.
`Error::Busy` is now matched **structurally** rather than on message text
(`ExecError::FlushFailed(PagerError::Vfs(VfsError::Locked { .. }))`).
Requirement 5 makes busy a distinct, retryable variant, so classifying it
on a substring was exactly the wrong trade: a reworded `Display` would have
silently turned every busy error permanent with no test failing.
A silent data-loss bug found, recorded as a ratchet
--------------------------------------------------
**Two connections on one file in the same process do not lock against each
other, and a write that reports success can be silently discarded.**
Measured on this tree: connection A takes `BEGIN IMMEDIATE` and inserts
row 2; connection B's insert of row 3 returns `Ok(1)`; A commits; the file
then holds `[1, 2]`. Row 3 is gone, and `PRAGMA integrity_check` says `ok`,
so nothing flags it.
The cause is POSIX, not this crate's logic: `fcntl` locks are scoped to
`(process, inode)`, which `src/vfs/lock.rs:96` documents and
`check_reserved_lock` states outright ("whether some *other* process
currently holds a write lock"). Stock SQLite closes it with `unixInodeInfo`
in `os_unix.c` — a process-global registry keyed by `(device, inode)` with
its own mutex and lock counts, so two connections in one process serialize
like two processes. There is no equivalent here.
Pre-existing in the engine, but this facade makes it far easier to reach:
Requirement 4 exists so a *pool* can hold a handle, and opening the same
path twice is the obvious thing to do. Recorded as an `#[ignore]`d ratchet
(`in_process_connections_lock_against_each_other`) written to assert the
correct behaviour, so it passes unchanged once a registry lands. Verified
to fail when run: "a write that reported success was silently discarded".
Needs its own ticket.
Tests
-----
`tests/unit/api_durability_test.rs` (2 + the ratchet) and
`tests/unit/api_transaction_test.rs` (10) cover the surface. The
busy-contention tests moved to
`tests/corpus/api_durability_oracle_test.rs` (4), because they need a
*second process* to hold the lock — the pinned `sqlite3`, which also makes
the claim stronger: the lock protocol is honoured against stock SQLite, not
just against ourselves.
`commit_survives_hard_kill` re-executes this test binary as a child, which
commits under `synchronous = FULL`, reports via a marker file, and blocks;
the parent SIGKILLs it and checks the rows survive and the oracle finds the
file well-formed. Its doc states what that does *not* prove: SIGKILL leaves
the kernel page cache intact, so it cannot distinguish FULL from OFF. Only
a power cut or a crash-injecting VFS does, and that regime is
`crash_torture_test.rs`.
Gates: make test (1652 passed, 10 ignored — 1 new ratchet), make
test-corpus (403 passed), make lint (both clippy passes + fmt).
Spend: within estimate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…3/Req 3, Req 8) `Connection::prepare` returns a `Statement` that owns its compiled program on the worker and finalizes it on drop. `execute`/`query`/`query_all`/ `query_row` take the parameters; `param_count` and `column_names` are available before it runs. Requirement 3 is explicit that the value is not speed — a dozen statements at commit frequency saves nothing measurable by compiling once. It is that a handle owning its parameter slots refuses a wrong argument count instead of writing a valid row pointing at the wrong table. So the arity check runs on every execution, not just at prepare. Kept statements take the *same* path as ad-hoc ones: `run_compiled` and `check_runnable` are shared, because a second copy of the mode and arity checks is how a prepared statement ends up honouring a different contract from `execute`. Schema refresh (Req 8) ---------------------- A program addresses tables by root page, and a `DROP` can return that page to the freelist for a later `CREATE` to reuse — so a statement compiled before a schema change and run after it could read a page that now belongs to a different table. The engine carries a schema generation, bumped whenever the catalog is invalidated; a statement compiled against an older one is recompiled on next use, which is what `sqlite3_prepare_v2` does on `SQLITE_SCHEMA`. If it no longer compiles at all (its table was dropped) the failure is reported and the handle stays valid, so the error is repeatable rather than one-shot. `Statement::reprepare_count` exposes how often that happened. Not test scaffolding: it is SQLite's own `SQLITE_STMTSTATUS_REPREPARE`, documented as "the number of times that the prepared statement has been automatically regenerated due to schema changes" (`sqlite3.h:9274`, pinned 3.53.4). It is also what makes Requirement 3's "compilation happened once" an observable claim rather than an assertion about internals. Tests, and one claim I had to move to make it real -------------------------------------------------- 12 unit tests in `tests/unit/api_statement_test.rs`, using Requirement 3's scenario names. Confirmed discriminating: with the refresh disabled, 3 of them fail. The load-bearing Req 8 claim — that a write prepared before an index existed still maintains that index — turned out *not* to be provable in a unit test, and I had claimed it was. A stale program inserts the table row and skips the index, but a freshly-compiled read may table-scan and find the row anyway, so the assertion passed under the mutant. Measured, not assumed. It now lives where it can be checked: `tests/corpus/api_oracle_test.rs::a_prepared_write_after_create_index_keeps_the_file_valid` inserts through a statement prepared before two indexes existed and has the pinned sqlite3 run `PRAGMA integrity_check` — which is precisely what detects a row present in the table with no matching index entry. That test does fail under the mutant. #685 is the precedent: this class of bug was invisible until the oracle was asked. The unit test's comment now says what it does and does not show, and points at the corpus test. Gates: make test (1664 passed, 10 ignored), make test-corpus (404 passed), make lint (both clippy passes + fmt), check-mod-files, check-assurance 100%/100%. Spend: within estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…olicy (013/Req 6) `src/lib.rs` now says which module is the API and which is the engine, and `CHANGELOG.md` carries the policy: `sqlite_rs::api` is supported and its breaking changes are called out under **Changed**; every other public module changes whenever the implementation needs it to, at any version. Before this, `CHANGELOG.md` said only "Pre-1.0: minor bumps may break the public API" over an export list containing the whole engine, so a consumer wiring `dump::open` to `execute_transaction_step` was building on items carrying no promise while having no way to know it. *SQE* confines every `sqlite_rs::` reference to one module for exactly that reason. `Value` is re-exported from `api`, so binding a parameter or reading a column no longer means naming `sqlite_rs::record`. The test with teeth ------------------- `tests/unit/api_surface_test.rs::facade_is_sufficient_alone` runs the whole workload — create, schema, prepare, bind, transaction, streamed read, by-name access, the optimistic-concurrency swap, a constraint error's result code — importing nothing but `sqlite_rs::api`. That it compiles is the assertion. On its own that would rot: a future capability gap "fixed" by importing `sqlite_rs::pager` would keep it passing while Requirement 6 became silently false. So `this_file_names_no_engine_module` reads this file's own source and fails if any of the fourteen engine modules is named in it. Comment lines are stripped first, because the prose deliberately names them to say what must not appear — an earlier version of the test failed on its own explanation. Verified to bite: adding `use sqlite_rs::pager::Pager` fails it. Examples -------- `query.rs`, `crud.rs` and `read_database.rs` rewritten against the facade; all four examples still run. `crud.rs` no longer copies `examples/fixtures/ empty.db` first — its doc comment used to read "This crate has no API to create a brand-new database file from nothing", which stopped being true this session. `examples/README.md` said the crate "exposes its parser/ codegen/VM pipeline directly rather than an ergonomic Connection/prepare/ bind wrapper"; also no longer true. `wal_mode.rs` stays engine-level, and the README now says why: explicit checkpointing is a `pager` operation the facade does not expose. One gap found and one closed ---------------------------- Writing `read_database.rs` — whose whole job is "lists its tables" — surfaced that **`sqlite_master` is not queryable through `SELECT` at all**: `resolve_from_table_schema` does not resolve it, so `SELECT name FROM sqlite_master` fails to compile. Introspection is plan.md's V7 and spec 013's non-goals hand the PRAGMA catalogue there, so this is recorded rather than fixed. `Connection::table_names` closes the consumer-facing half without touching codegen: it reads the catalog the worker has already decoded. Requirement 6 asks that the facade cover what the engine offers a consumer, and `schema::read_schema` could always enumerate tables — just not from here. Gates: make test (1666 passed, 10 ignored), make test-corpus (404 passed), make lint (both clippy passes + fmt), all four examples run. Spend: within estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-> 0.2.0)
Amended in place rather than superseded: two specs describing `Connection`
would double-count in `tools/assurance.py` and collide on domain. Status
stays `draft`.
The dashboard moves, which is the point
---------------------------------------
main this branch
Requirements 86 94 (planned excluded: 10 -> 3)
Scenarios 276 299
Completeness 100% 100%
Coverage 100% 100%
Dead links 0 0
Spec 013 was 7/7 `(planned)` and therefore excluded from all scoring; it is
now 0/8, and the three remaining planned requirements repo-wide belong to
specs 002, 005 and 010.
What changed beyond flipping the markers
----------------------------------------
**Req 1** restated around the retention rule, which is the half with the
surprising semantics and the half that belongs to the connection.
`last_insert_rowid` specified alongside it, with a scenario. Deleted "The
only capability gap here", false once #694 landed.
**Req 2** gained `open_in_memory`, a page-size scenario, and a read-only
scenario. Read-only is now specified as *statement-level* enforcement, an
ADR-0004 divergence: `Pager::open` calls `open_write` unconditionally
(`src/pager.rs:419`), there is no read-only pager, and `VfsPageSource`
bypasses `Pager` so it merges no WAL frames. Also records that creation was
never verified against the oracle before now, and why (both `seed_db`
helpers prefer the oracle and fall back to our path only when it is
absent, so the two never ran together).
**Req 3** gained `execute_batch`, `Error::ParamCount`,
`Error::MultipleStatements`, and an explicit scoping of by-name column
access to single-table `SELECT`s — `result_column_names` returns
`column1, column2…` for joins and compounds
(`src/codegen/prepare.rs:181`), and this spec's own acceptance list
contains a `UNION`.
**Req 4** replaced the unfalsifiable "all engine access happened on the
connection's thread" with what is actually checkable, and replaced "the
thread count is unchanged" with the structural argument (drop joins, so a
worker that failed to terminate hangs the drop rather than leaking). Added
the no-named-lifetimes constraint as the reason the worker thread is the
only *expressible* design.
It also had to be corrected on a claim it made: "Coordination between
connections in one process is spec 007's file locks, as between processes."
It is not. POSIX `fcntl` locks are `(process, inode)`-scoped, so two
connections in one process do not exclude each other, and a write that
reports success can be silently discarded. Measured, and now written down
with `unixInodeInfo` named as the mechanism stock SQLite uses.
**Req 5** `::ApiError` -> `::Error`; added the `sqlite_code()`/
`extended_sqlite_code()` MUST (both, under the names SQLite uses, rather
than picking one); scoped the busy timeout to autocommit and said why;
noted `Connection::pragma` cannot serve the nine introspection pragmas
(ADR-0029, they live in the binary) and that `sqlite_master` is not
queryable through `SELECT` at all. The hard-kill scenario now states what
SIGKILL does *not* prove.
**Req 6** split acceptance into Part A (on the tree) and Part B (*SQE*'s
literal DDL, a ratchet on #687 and #697), and named the three documented
header fields exhaustively instead of gesturing at them.
**Req 7** `Statement::next_row` -> `Rows::next_row`; the memory scenario
now names **both** constants; and it records that the plan must be
non-blocking, because an `ORDER BY` with no usable index is a blocking
operator and a first draft of the test measured one and failed at 33x,
correctly.
**Req 8 (new)** — a prepared statement must not run against a changed
catalog. Not a caching concern: a program addresses tables by root page,
and `DROP` returns that page to the freelist for a later `CREATE`.
Every `Tests:` link points at a test that exists, with the symbol present —
zero dead links, verified by `tools/assurance.py`.
Gates: check-assurance PASS (100%/100%), no dead links.
Spend: within estimate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four decisions in the facade close alternatives defensible enough to record,
and CLAUDE.md's convention is that such a decision gets an ADR in the same
PR. ADR-0041 settled where the API lives; it said nothing about what a
failure looks like coming out of it.
* The error type is **flat** — every payload a String, an i32 or a Copy
enum. It has to be, because every failure crosses a channel from the
worker, so it must be unconditionally Send + Sync + 'static; an error
holding an Rc or borrowing engine state could not be returned at all.
Rejected: the idiomatic wrapping error with `source()`, which cannot
derive PartialEq and would need sixteen engine enums made Send + Sync
to satisfy a facade.
* **Both** result codes are exposed, under SQLite's own names —
`sqlite_code()` primary, `extended_sqlite_code()` extended. Rejected:
picking one, which was the open question in the plan and would have
made the other unreachable.
* **Busy is classified structurally**, never on message text, because a
reworded Display would otherwise turn every retryable error permanent
with no test failing.
* **Busy retry is autocommit-only and rolls back first.** Rejected:
retrying inside a transaction, which is unsound — it double-applies,
since `Pager::flush` leaves the dirty set intact by design.
* **A stale prepared statement recompiles** rather than failing, which is
what `prepare_v2` does on SQLITE_SCHEMA. Rejected: failing, which
pushes a retry loop onto every caller.
Spec 013's "Decisions and rejected alternatives" line now cites both ADRs.
Spend: within estimate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven items, all found by checking what shipped against what the consumer actually asked for rather than against our own spec. The substantive one is Requirement 4's concurrency scenario. It asked for eight threads doing a hundred inserts each on a file, with the oracle confirming the result is well-formed. What existed was four threads doing one insert apiece, in memory, with no integrity check — which proves the handle compiles and does not crash, and barely queues two requests against each other. It is now 800 inserts through one handle, asserting the total, each thread's share individually, and survival across a reopen; the oracle half lives in `tests/corpus/api_oracle_test.rs` per the test-layout convention, against an indexed table so `integrity_check` has an index to validate. That matters because #685 was a class of write bug our own reads could not see. `parse_error` in the dispatcher was `format!("{other:?}")`, so an embedding consumer's error text contained Rust struct syntax — `Unsupported { message: "...", span: Span { line: 1, .. } }`. The parse outcome already carries a message and a position; it now reads as prose. A `PRAGMA` through `prepare` now names plan.md V7 and `Connection::pragma` instead of handing back a bare parser message about a pragma name. That was #695's one open acceptance criterion. The setting form still compiles, which is why the message has to be specific rather than "PRAGMA is unsupported". Two behaviours were already correct but untested, and the untested half of each is the one that would cost a consumer real data: refusing a file that is not a database must leave its bytes alone (`Pager::open` calls `Vfs::open_write` before the header is ever parsed, so nothing structural stops a future change from truncating it), and `ReadOnly` on a missing path must create nothing. Journal mode is recorded in the spec as a deliberate scoping rather than left as an omission: the consumer's proposal wanted it among the open options, and it is set with `pragma` instead because the mode is persistent state in the file's header, not a property of one handle's session. Five of the eight `tests/unit/api_*.rs` files imported `Value` from `sqlite_rs::record` while the commit next door declared `sqlite_rs::api` the supported surface. They now import it from `api`, which re-exports it. CHANGELOG gains the facade entry it should have had, under an `[Unreleased]` heading — the versioning policy above it governs which minor version a completed phase ships as, not where a change waits beforehand. 1670 tests (was 1666), corpus 405 (was 404), lint clean on both passes, assurance still 100%/100% with no dead links. Spend: small, on top of the facade's estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Requirement 4's serialization is per *statement* — the worker runs one
request at a time — and a `Transaction` is several. `Connection` is `Clone`
precisely so a pool or several tasks can hold it, so per-statement
serialization left three interleavings reachable, reported by the first
consumer to run two concurrent catalog commits:
1. another task's `BEGIN` inside an open transaction — refused, loud;
2. another task's statements committed with a transaction they know
nothing about;
3. an autocommit write from a third task **rolled back** with someone
else's transaction, having already returned `Ok(1)`.
The third decides it. That is a write which reported success being silently
discarded — the same failure class as two connections on one file not
locking against each other, except reachable through a single `Connection`,
which is the object this API tells consumers to share. A documented caveat
cannot make a lost write visible, and the advice it would give ("hold your
own mutex") is code every consumer would then write identically.
`transaction_with` claims a slot on `Shared` before issuing `BEGIN` — before,
not after, because a statement reaching the worker in between would land
inside the transaction. Commit, rollback and `Drop` release it. The gate sits
in `Connection::send`, which every request already funnels through.
Three arms, and the middle one is load-bearing: the handle that *is* the
transaction proceeds; the **thread that opened it** proceeds, because holding
a `Transaction` and still using the original handle is what a single-threaded
caller has always done and what SQLite does, and blocking there would be a
deadlock against oneself rather than exclusion; everyone else waits. That arm
is why `the_same_thread_may_still_use_the_connection_directly` hangs rather
than fails if it is removed, which the test says out loud.
Same-thread re-entry asking for a second transaction is
`Error::TransactionActive`, not a wait: a nesting bug, with no `SAVEPOINT` to
make nesting legitimate, and waiting would hide it as a hang.
A `MutexGuard` in `Transaction` was the obvious shape and is not
expressible — it carries a lifetime and `check-mvl-limit` forbids those in
`src/`, the same constraint that made the worker thread the only expressible
design. Hence a slot and a condvar. ADR-0045 records that, and why a raw
`execute("BEGIN")` stays unguarded: nothing would release a slot it claimed.
Five tests, including the silent case end to end — the other thread's write
must wait, succeed, and survive a rollback that discards only its own row.
Under a mutant that disables the gate, exactly the two cross-thread tests
fail and the other thirteen pass.
1675 tests (was 1670), corpus 405, lint clean on both passes, assurance
still 100%/100%.
Spend: small-medium, on top of the facade's estimate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 11, 2026
Open
This was referenced Sep 11, 2026
dpsiderius
added a commit
that referenced
this pull request
Sep 11, 2026
…706) The Consequences section described the WalReadLock in-process residual as benign-in-the-safe-direction ("a checkpoint bounds itself more conservatively than necessary"). That's backwards: active_reader_marks probes occupancy with a non-blocking F_WRLCK, which POSIX never conflicts with a lock this same process already holds, so a same-process reader's mark is invisible to it; checkpoint.rs then folds the resulting empty mark list into .unwrap_or(total_frames), letting a checkpoint backfill the whole WAL rather than bounding conservatively. Also corrected the "dormant" framing: the checkpoint path (Pager::set_journal_mode -> switch_wal_to_journal -> checkpoint_passive) is reachable today through PRAGMA journal_mode round-tripping (verified against the built CLI); what's still missing for harm is a second in-process reader, which needs the unmerged embedding API (#705). No other file cites ADR-0047 yet (only CHANGELOG.md's own "See ADR-0047" note and the ADR index), so this stays an in-place edit per CLAUDE.md's uncited-ADR carve-out rather than a superseding ADR. spend: small, doc-only — a few file reads, a CLI build/repro to verify reachability, and the rewrite itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #695.
What this is
src/api.rs— the supported embedding surface. A consumer opens aConnection, runsexecute/query, gets typed rows, usesprepareforrepeated statements and
transactionfor atomicity.ConnectionisArc<Shared>,Clone, andSend + Sync, so it can live in a pool.Public surface:
Connection,Statement,Rows,Row,FromValue,Transaction,TransactionBehavior,OpenMode,Error, and a re-exportof
Value. Nothing else.src/lib.rsnow says so out loud:apiis thesupported surface, the other 14 modules are the engine and carry no
stability promise.
CHANGELOG.mdgains the matching policy section.[dependencies]stays empty —std::sync::mpsconly.Scope: this PR also lands #695's three non-goals
#695 deferred Requirements 4, 5 and 6 to follow-up tickets that were never
filed. They are here instead, because the worker thread (Req 4) is not a
wrapper you can retrofit — it determines whether any type may hold a
lifetime, which changes
RowsandStatementfundamentally. Splitting itwould have meant writing
Rowstwice.So: all 8 spec-013 requirements, in 9 reviewable commits. Spec 013 goes
0.1.0 → 0.2.0 with every requirement de-
(planned), plus a new Requirement8 (a prepared statement must not run against a changed catalog).
The three design points worth a reviewer's time
The worker thread is the only expressible design, not the preferred one.
Mutex<T>: SendrequiresT: Sendand the pager graph isRc, so noamount of locking makes it cross a thread. And
make check-mvl-limitforbids named lifetime parameters in
src/, so no API type can hold anExecution<'p>as a field. TheExecutiontherefore lives entirely insideone worker stack frame and rows come back over a bounded channel in chunks
of 64. ADR-0041 chose this; this PR adds the constraint that makes it the
only option.
Rowsstreams, and the read-only discriminator is keyed onOpenWrite.Not on
Insert/Delete: a materialized FROM-subquery emitsOpcode::Insertinside a plain
SELECT(src/codegen/subquery/from_clause.rs:296), sokeying on those would refuse read queries in
ReadOnlymode. Measured:SELECT s.a FROM (SELECT a FROM t LIMIT 5) AS semits Insert x1,OpenWrite x0.
The error type is flat, not wrapping. Every payload is
String/i32/aCopyenum, soErrorderivesPartialEqand is unconditionallySend + Sync + 'static— which it must be to travel back over the channel.Price: no
source()chain. ADR-0043 records the decision.sqlite_code()and
extended_sqlite_code()are both provided, with the primary derived asthe low byte, which is how
sqlite3.hdefines the relationship.Findings this work produced — please read this section
1. Two
Connections on one file in the same process do not lock againsteach other, and a write can be silently lost. Measured: A takes
BEGIN IMMEDIATEand inserts row 2; B's insert of row 3 returnsOk(1);A commits; the file holds
[1, 2]andPRAGMA integrity_checksaysok.Row 3 is gone with no error anywhere.
The cause is POSIX:
fcntllocks are(process, inode)-scoped, whichsrc/vfs/lock.rs:96already documents andcheck_reserved_lockstatesoutright. Stock SQLite closes this with
unixInodeInfoinos_unix.c;there is no equivalent here.
This is a pre-existing engine defect, not introduced by this PR — but
this PR makes it reachable, because Requirement 4 exists so that a pool can
hold a handle. It is recorded as an
#[ignore]d ratchet,in_process_connections_lock_against_each_other, verified to fail with "awrite that reported success was silently discarded". It needs its own
ticket and a
unixInodeInfo-equivalent registry.Note this answers the question #491 was closed without answering: "Multiple
Pagerinstances opened against the same file within one process (embeddingscenario, or a test harness) — plausible exposure". The answer is yes.
Consequence for #621: the SQE pool must be capped at one connection per file
until this is fixed.
Cross-process locking is correct — verified against the pinned sqlite3 — so
every busy-timeout test in this PR runs against a real second process rather
than a second in-process handle.
2. The file change counter (header offset 24) is never incremented. It
is preserved rather than zeroed, and stays consistent with offset 92, which
is why
integrity_checkpasses and nothing caught it. Another SQLiteconnection holding a cached image cannot learn our writes happened. Filed as
#710.
3.
sqlite_masteris not queryable throughSELECT.resolve_from_table_schemadoes not resolve it, soSELECT name FROM sqlite_masterfails to compile. Worked around withConnection::table_names()reading the decoded catalog, so consumers are not blocked; the SELECT-path
gap is #707.
Follow-up commits on this PR
Two commits after the original nine, both from running the consumer's real
statements and auditing what shipped against what they asked for:
fix: close the gaps an audit of the consumer's spec proposal found—seven items. Requirement 4's concurrency scenario asked for eight threads
doing a hundred inserts each on a file with the oracle confirming the result;
it was four threads doing one insert apiece, in memory, with no integrity
check. It is now 800 inserts, asserting the total, each thread's share, and
survival across a reopen, with the oracle half in
tests/corpus/api_oracle_test.rsagainst an indexed table.parse_errorinthe dispatcher was
format!("{other:?}"), so consumer-visible error textcontained Rust struct syntax. A
PRAGMAthroughpreparenow names plan.mdV7 and
Connection::pragma— that was #695's one open acceptancecriterion. Refusing a foreign file leaving its bytes untouched, and
ReadOnlyon a missing path creating nothing, were both correct and bothuntested.
journal_modeis recorded as a deliberate scoping rather than anomission. Five
api_*test files importedValuefromsqlite_rs::recordnext to a commit declaring
apithe supported surface.feat: a transaction holds the connection; other threads wait(ADR-0045) — the substantive one. Requirement 4's serialization is per
statement; a
Transactionis several, andConnectionisCloneso a poolcan hold it. That left an autocommit write from one task being rolled back
with another task's transaction, after returning
Ok(1)— a write thatreported success and vanished.
transaction_withnow claims a slot beforeissuing
BEGIN, and commit/rollback/Droprelease it. The thread that openedthe transaction still passes the gate, because blocking there is a deadlock
against oneself rather than exclusion; my first cut had exactly that bug, and
the test that catches it hangs rather than fails, which the test says out loud.
A
MutexGuardfield was not expressible — it carries a lifetime andcheck-mvl-limitforbids those insrc/, the same constraint behind theworker thread.
Every finding in the section above now has a ticket: #706 (in-process
locking), #707 (
sqlite_master), #710 (change counter), plus #708(
rowid), #709 (by-name columns on joins) and #711 (the consumerworkload through
Connection).Verification
Tests were used to find real bugs, not to decorate the diff. Two of my own
tests were wrong first and both taught something:
partial_read_is_boundedinitially measuredORDER BYand failed at 33x— correctly. A sort with no usable index is a blocking operator, so
time-to-first-row is genuinely linear; stock SQLite behaves the same.
blocking_plans_are_linear_by_naturenow pins that contrast rather thanthe failure being merely avoided.
Rowswhile issuing a secondquery. That was a real defect: with a single channel slot, a small unread
result parks the worker forever. Fixed with two slots, so a single-batch
result completes and frees the worker even if never read.
There was also a genuine deadlock in
Drop for Shared— it joined theworker while still holding the only sender, so the worker's
recv()neverreturned
Err. Fixed by taking the sender before the join;worker_thread_joins_on_dropwould hang rather than fail if that regressed,which the test says out loud.
Mutation-tested to prove the tests discriminate: adding
LASTROWIDtoupdate.rsfails the oracle test; dropping the final partial batch failsthe oracle test and 10 of 15 unit tests; disabling schema refresh fails 3
unit tests and the oracle index test; importing an engine module in the
surface test fails the self-check.
One claim I had to walk back: a unit test does not prove a prepared
write maintains a newly-created index — the read table-scans, so the
assertion passes even under the mutant. The real claim now lives in
tests/corpus/api_oracle_test.rs::a_prepared_write_after_create_index_keeps_the_file_valid,which has the pinned sqlite3 run
PRAGMA integrity_checkand does failunder the mutant. The unit test's comment now says what it does and does not
show.
Gates, from a detached clean worktree at the commit (not the working
tree):
make test1675 passed / 0 failed / 10 ignored,make test-corpus405 passed / 0 failed,
make lintexit 0 on both clippy passes,make check-assurancePASS,make check-mod-filesclean,make check-grammar-driftno drift,make version-pinall sites agree on3.53.4, all four examples run.
Of the 10 ignored: 9 are pre-existing tier-3 stubs, 1 is the new locking
ratchet.
make check-mvl-limitpassed in CI (insideLint & supply chain), alongwith
check-denyandcheck-audit. It could not run locally —cargo-mvl-limitis not installed — so CI is the evidence.Assurance moved, which #695 requires: 86 -> 94 scored requirements
(planned 10 -> 3), 276 -> 299 scenarios, both 100%, zero dead links.
Merge order
Stacks on two unmerged PRs and must merge after both:
#704 (the SQE consumer corpus family) is independent and can merge
any time.
Until those land, this PR's diff includes theirs. Both are merged into this
branch as explicit bookkeeping merges (
c189894,6fdd0e6), so the facade'sown 9 commits are reviewable on their own.
spend: roughly 3x #695's
mediumestimate, because this PR also lands thethree requirements #695 listed as non-goals (Reqs 4, 5, 6) rather than
deferring them, and because three engine findings above were investigated to
the point of a reproducing test.
Refs: 013/Req-1, 013/Req-2, 013/Req-3, 013/Req-4, 013/Req-5, 013/Req-6,
013/Req-7, 013/Req-8, #682, #692, #695, #491, #621
🤖 Generated with Claude Code