From 60b22bfe6827186edf8a92aaf1134de146e37e84 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 1 Jul 2026 18:25:29 -0700 Subject: [PATCH] docs: record the 2026-07-02 deep review and triage findings into ISSUES.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the fresh-eyes review report (docs/reviews/review-20260702-001902.md) and triages its verified findings as I143–I160 under a new "Deep review 2026-07-02" section in ISSUES.md. - 18 findings confirmed by adversarial verification (0 refuted): 4 BUG, 13 DESIGN, 1 SMELL. Prior-review delta: 0 regressions, 71 resolved. - I143–I146 (the BUGs) are marked FIXED (PR #89). - I147–I160 (DESIGN/SMELL) recorded OPEN for triage — corrupt-page hardening asymmetry, KDF-param DoS, per-page replay, the encrypted-minor write-gate off-by-series, Python finished-transaction aliasing, and the encrypted on-disk-spillway test gap are the notable ones. Docs only; no code change. --- ISSUES.md | 141 +++++++++ docs/reviews/review-20260702-001902.md | 399 +++++++++++++++++++++++++ 2 files changed, 540 insertions(+) create mode 100644 docs/reviews/review-20260702-001902.md diff --git a/ISSUES.md b/ISSUES.md index 854b628..b300407 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -1725,3 +1725,144 @@ Source: **[encryption 2026-06-29]** — deferred work captured while implementin Bulk DEK rotation is only needed when the DEK itself is believed compromised (e.g., a process memory dump exposed the in-session DEK). Credential rotation — the far more common operational need (password change, key rollover, adding a second credential) — is already available and O(1). Because no production databases exist today and the DEK is not separately distributed, the risk of DEK compromise is low; the heavy whole-file cost makes this a poor default rotation path. **Direction of fix (when needed):** Implement a `rekey(old_key, new_key)` or `rekey_dek(key)` API that (1) generates a fresh DEK, (2) reads, decrypts, re-encrypts, and writes back every non-superblock page in a single pass using the existing stride-aware `PageIo`, (3) re-wraps the new DEK into all currently-active key slots, and (4) commits an updated superblock. The operation must be crash-safe: either complete or leave the original file intact. A copy-then-atomic-rename strategy (write the new file alongside, then rename) is the simplest crash-safe approach for an embedded store; an in-place two-pass strategy is also possible but more complex. Reuse `PageCipher::seal` / `PageCipher::open` and `CryptoHeader` from the existing crypto layer. + +--- + +## Deep review 2026-07-02 + +Source: **[deepdive 2026-07-02]** — fresh-eyes Rust review at `581be36`; full report at `docs/reviews/review-20260702-001902.md`. Every BUG/DESIGN finding here was confirmed by an independent adversarial verification pass (0 refuted). The four BUGs (I143–I146) are fixed in PR #89; the DESIGN/SMELL items are recorded for triage. Delta vs the prior two reviews: **0 regressions, 71 findings verified resolved.** + +#### I143. TOCTOU create-vs-open race: create decision made before the flock [deepdive 2026-07-02] — **P1** ✅ FIXED 2026-07-02 (PR #89) +**Where:** `src/lib.rs:366` + +**Problem:** Chisel::open decides create-vs-open BEFORE the flock is taken: `file_exists` is computed at lines 366-369, the exclusive lock is only acquired inside `PageIo::open` at line 375, and the stale boolean then selects `TransactionManager::create_new` at line 389. The doc comment (lines 339-341) claims the lock is acquired 'before any parsing... rather than racing on the superblock', but the create/open decision itself races. Cross-process TOCTOU with silent data loss: process B stats a nonexistent/zero-length file; process A concurrently creates the DB, commits data, and closes (releasing the flock); B then acquires the lock and runs create_new on the now-populated file, writing a fresh superblock over A's committed database. Multi-process exclusion is an explicit part of the public contract (flock, LockFailed), so this is exactly the race the lock exists to prevent. + +**Direction of fix:** Re-check the file length after the flock is held (e.g. have PageIo::open return whether it created the file, or re-stat via the locked fd) and choose create_new vs open_existing from that post-lock observation. + +**Fixed (2026-07-02, PR #89):** see the commit; a regression test is included where feasible (I144 forges the on-disk stride and asserts a typed error, not a panic). + +#### I144. Unvalidated crypto-header `stride` → div-by-zero panic / huge-alloc DoS on open [deepdive 2026-07-02] — **P1** ✅ FIXED 2026-07-02 (PR #89) +**Where:** `src/transaction/recovery.rs:258` + +**Problem:** open_existing takes the crypto-header's stride field verbatim (`sb.encryption.map(|h| h.stride as usize)`) and calls `cache.io_mut().set_stride(stride)` with no validation. `PageIo::set_stride` (src/page_io.rs:226) computes `len / stride as u64`. The header comment in src/superblock/crypto_header.rs:11 claims stride is "validated by the engine"; no such check exists anywhere (only the two hardcoded ENC_PAGE_SIZE fallback sites are safe). stride is a plaintext field protected only by the forgeable XXH3 page checksum — the codebase itself established this trust boundary when it bounds-checked ct_len in decrypt_body for exactly this reason. A forged stride of 0 is a guaranteed division-by-zero panic at open (DoS, violates the poison-not-panic error model); a huge stride drives multi-GiB read-buffer allocations. Also a first-class comment-vs-code mismatch. + +**Direction of fix:** Before set_stride, require header.stride == ENC_PAGE_SIZE as u32 (the only value ever written) and treat a mismatch like an unsupported format / corrupt slot; that also makes the crypto_header.rs comment true. + +**Fixed (2026-07-02, PR #89):** see the commit; a regression test is included where feasible (I144 forges the on-disk stride and asserts a typed error, not a panic). + +#### I145. `reclaim_freemap_orphans` not poison-wrapped — fatal mid-sweep leaves a usable, indeterminate manager [deepdive 2026-07-02] — **P2** ✅ FIXED 2026-07-02 (PR #89) +**Where:** `src/transaction/freemap.rs:663` + +**Problem:** Minor imprecision only: the failure requires the fatal error to strike mid-sweep (reachable_pages, a live-page cache.get, or mark_free_growing); the CorruptPage/ChecksumMismatch skip on dead pages at freemap.rs:457-458 is intentionally non-poisoning and not part of the bug. The BUG severity and substance are correct as stated. A fatal `IoError`/`CorruptPage` from `reachable_pages`, `cache.get`, or `mark_free_committed_path` during the sweep returns to the caller with the manager NOT poisoned. Worse, `mark_free_committed_path` writes the partially-advanced tree root back into `current_roots` even on error (put_tree on the error path, freemap.rs:291-293), so the un-poisoned manager holds a freemap tree in an indeterminate mid-mutation state that a subsequent `commit()` will happily make durable — exactly the class of state the I1 poison model exists to fence off. + +**Direction of fix:** Wrap the sweep at the TransactionManager boundary the same way as everything else: `let result = self.freemap.reclaim_orphans(...); self.poison_on_fatal(result)` inside `reclaim_freemap_orphans` (freemap.rs:663-673). + +**Fixed (2026-07-02, PR #89):** see the commit; a regression test is included where feasible (I144 forges the on-disk stride and asserts a typed error, not a panic). + +#### I146. README says the newer-minor write-refusal gate is unwired; it shipped as I29 [deepdive 2026-07-02] — **P2** ✅ FIXED 2026-07-02 (PR #89) +**Where:** `README.md:363` + +**Problem:** README.md:363 falsely states the MINOR write-refusal gate is "not yet wired up"; it shipped as the I29 write-gate in src/transaction/recovery.rs:409-411 (force_read_only on file-MINOR > binary-MINOR, mutations return ReadOnlyMode), consistent with THEORY.md:132 (repo root). Only the README sentence needs updating. This is the user-facing compatibility contract, in a README refreshed three commits ago (#86). A user reading README concludes that opening a newer-minor file risks clobbering unknown fields on write; in reality mutations return ReadOnlyMode. The two design docs disagree with each other on a safety guarantee. + +**Direction of fix:** Update README's format-compatibility section to say the read-only write-gate is implemented (matching THEORY.md:132 and recovery.rs:409); drop the "lands with the first post-1.0 minor bump" sentence. + +**Fixed (2026-07-02, PR #89):** see the commit; a regression test is included where feasible (I144 forges the on-disk stride and asserts a typed error, not a panic). + +#### I147. HandleTable::insert grow loop unbounded for handle==u64::MAX (latent; unreachable via monotonic handles) [deepdive 2026-07-02] — **P3** 🔶 OPEN +**Where:** `src/handle_table.rs:253` + +**Problem:** HandleTable::insert's grow loop (src/handle_table.rs:253) is unbounded: for handle == u64::MAX, capacity() saturates to u64::MAX at depth 6 and the loop allocates one page per iteration forever, and delete (line 289) reports u64::MAX absent at saturated capacity while find_leaf (lines 694-697) explicitly supports it. Real code-level defect and internally inconsistent with the module's own MAX_DEPTH comment and read path, but unreachable through the engine (handles are monotonic from 1), so latent — fix by bounding the loop with `&& self.depth < MAX_DEPTH` as FreeMapTree::mark_free_growing does. Runaway loop with unbounded page allocation on a key the module's own comments and read path explicitly claim to support. Practically unreachable through the engine today (handles are monotonic from `next_handle` starting at 1, so u64::MAX needs ~1.8e19 allocations), but it is a live in-crate API and the code around it (find_leaf's u64::MAX carve-out, the MAX_DEPTH comment) asserts the case is handled when it is not. + +**Direction of fix:** Mirror freemap_tree: bound the loop (`while handle >= self.capacity() && self.depth < MAX_DEPTH`) and/or make `grow` a no-op at MAX_DEPTH; give `delete` the same `cap != u64::MAX` exemption `find_leaf` has. + +#### I148. `free_subtree` / membership descent has no positional type check — corrupt-but-checksummed page amplifies corruption [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `src/membership_index.rs:84` + +**Problem:** As stated, with one softening: the remove()-path scenario where leaf values (1) are freed as page ids usually aborts earlier with a misleading ChecksumMismatch (inner.delete/any_present descend the same bogus depth before free_subtree runs), so the sharpest unmitigated instance is a corrupt interior child pointer at the depth-1 boundary of free_subtree, which is pushed into `freed` without any read or validation. Inconsistent hardening across the crate's three radixes against the exact threat model the repo tests everywhere (checksum-valid, structurally wrong pages): freemap_tree, overflow, and data_page fail closed with typed CorruptPage; handle_table and membership_index fail open. For `free_subtree` the failure mode is corruption amplification — bogus ids (potentially live data pages or even superblock slot 1) enter `txn_freed_pages` and get marked reusable at commit. + +**Direction of fix:** Add the freemap_tree-style positional type check (`buf[0]` vs expected MembershipLeaf/MembershipInterior/HandleTable+FLAG) on every descent read in find_leaf/insert_recursive/delete_recursive/iter/free_subtree, returning CorruptPage on mismatch. + +#### I149. `total_length` trusted unbounded before `Vec::with_capacity` — corrupt chain can OOM (contradicts I14) [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `src/overflow.rs:183` + +**Problem:** As stated, with one precision: the failing page cannot arise from any legitimate Chisel write or from the stale-handle scenario (a genuinely Overflow-typed page always carries a truthfully-written total_length, replicated per page). Reaching the panic requires on-disk corruption/tampering that also passes the XXH3 checksum. Within the module's own I14 standard — corrupt-but-checksummed chains surface as typed CorruptPage, never panic/abort — the gap is real: total_length is the one header field read() trusts unbounded before allocating, and a cheap bound (next_page_id * OVERFLOW_PAYLOAD, the same universe bound reclaim_orphans uses) would close it. The module's own hardening standard (I14: corrupt-but-checksummed chains must surface as typed CorruptPage, never a panic/abort) is contradicted for the one field it trusts most. A well-formed chain can never carry more bytes than the file holds, so a cheap plausibility bound exists and is already used elsewhere (reclaim_orphans uses cache.next_page_id()). + +**Direction of fix:** Bound total_length against a file-derived maximum (cache.next_page_id() * OVERFLOW_PAYLOAD → CorruptPage if exceeded), or clamp the initial with_capacity and let Vec grow as pages are validated. + +#### I150. Attacker-controlled Argon2 params consumed before the AEAD tag rejects — KDF-param OOM/DoS [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `src/superblock/crypto_header.rs:209` + +**Problem:** As stated, except the twin lives at src/transaction/recovery.rs:625-653 (derive_kek call at line 637), not src/superblock/recovery.rs. Everything else — params-before-auth ordering, u32::MAX m_cost accepted by argon2 0.5.3, re-stampable XXH3 checksum — checks out. Fix is a small read-time ceiling on the slot's Argon2 params (or clamp in derive_kek) before deriving. An attacker who edits a slot's m_cost/t_cost/p_cost and re-stamps the non-cryptographic XXH3 checksum turns every subsequent open into an OOM/allocation-abort (or minutes of grinding). Unauthenticated-KDF-param DoS is inherent to the construction, but an unbounded cost parameter converts "corrupt file fails to open" into "process dies". + +**Direction of fix:** Clamp attacker-controlled params with sanity ceilings before deriving (e.g. m_cost <= a few GiB, t_cost/p_cost small maxima); reject out-of-range slots the same way as an unknown kdf_id (skip/continue), so a tampered slot degrades to InvalidEncryptionKey. + +#### I151. Pack cursor not gated on packing_enabled; rollback restores a live cursor under an active savepoint → in-place write below the watermark [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `src/transaction/packing.rs:119` + +**Problem:** `SlotPacker::insert` consults `self.insert_cursor` unconditionally — the cursor branch is not gated on `packing_enabled`. `rollback_to_inner` (savepoints.rs:98-102) restores the packer snapshot taken at savepoint creation, which was captured BEFORE `clear_cursor()` ran (savepoints.rs:32-33), so rolling back to the first savepoint of a transaction that had packed inserts restores a live `Some(P)` cursor while that savepoint is still on the stack. This contradicts the module's stated invariant (packing.rs:27-28: "Packing is disabled entirely when savepoints are active") and the savepoint design premise that no in-place mutation happens below the savepoint watermark. After the restore, the next insert writes a slot into pre-watermark page P in place; a second `rollback_to` to the same savepoint truncates only >= watermark, so P keeps the physically-written slot forever — each rollback_to+insert cycle leaks one dead slot into a page that then gets committed (rolled-back value bytes become durable). No API-visible wrong reads, but the rollback mechanism's "nothing below the watermark changed" premise is silently violated. + +**Direction of fix:** Gate the cursor branch on `packing_enabled` (`if packing_enabled { if let Some(cursor) = self.insert_cursor { ... } }`) — one guard at the single decision point, which also makes the restore harmless — or have `restore()` force the cursor to None when a savepoint remains active. + +#### I152. I29 write-gate compares file MINOR against the plaintext constant unconditionally — a future (2,1) file opens writable [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `src/transaction/recovery.rs:409` + +**Problem:** The I29 write-gate compares the file's MINOR against the plaintext constant `page::FORMAT_MINOR_VERSION` (= 1) unconditionally, but src/page.rs:120-123 declares that the encrypted MAJOR=2 series "carries its own minor series, independent of plaintext" starting at FORMAT_MINOR_VERSION_ENCRYPTED = 0. A future encrypted file at (2,1) opened by today's binary (whose encrypted-series minor is 0) evaluates `1 > 1 == false` and stays WRITABLE — exactly the clobber-newer-fields hazard the gate exists to prevent. Latent today (no (2,≥1) files exist), but the gate silently fails for the second major series the moment its minor is first bumped. + +**Direction of fix:** Select the comparison constant by major series: gate against FORMAT_MINOR_VERSION_ENCRYPTED when the file's MAJOR is 2, FORMAT_MINOR_VERSION when 1. + +#### I153. Per-page temporal replay: page AAD is page_id only, so stale-but-valid pages can be spliced into a current DB [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `src/crypto/mod.rs:321` + +**Problem:** Accurate as written; one refinement: 're-authenticates forever' holds until a (currently deferred) full DEK rotation/re-encryption — KEK rotation does not invalidate old sealed images. An attacker with file access can splice stale versions of individual pages into a current database, producing a mixed state that never existed at any commit (stale freemap or tree pages that pass AEAD and then corrupt structure silently). The design spec's documented non-goal covers only substitution of a "wholly older, validly-signed database image"; per-page temporal splicing is strictly stronger and is not covered by the spec's "cryptographic tamper-detection" / anti-relocation claims. + +**Direction of fix:** At minimum, extend the spec §9 boundary to name per-page replay explicitly. If it should be defended, bind pages to a commit epoch in the AAD (costs rewriting reachable pages on epoch bump) or hash-chain page tags into the sealed superblock body. + +#### I154. Blanket CryptoError→InvalidEncryptionKey conflates bad Argon2Params on create/add_key with a wrong key [deepdive 2026-07-02] — **P3** 🔶 OPEN +**Where:** `src/error.rs:397` + +**Problem:** Accurate as stated. Minor precision note: the add_key path (keys.rs:151) reaches InvalidEncryptionKey via an explicit `.map_err`, not the blanket From impl, but the effect (BadKeyLength/Kdf on the new credential reported as "does not match any key slot") is identical. Creating a brand-new encrypted DB with bad Argon2Params yields 'key does not match any key slot' — nonsensical on create where no slots exist yet; the same conflation hits add_key's wrap of the NEW credential (transaction/keys.rs:151 map_err → InvalidEncryptionKey). The comment above the From impl claims a CryptoError here 'is always a key-or-KDF problem on intact on-disk data', which is true, but collapsing 'your params are invalid' into 'your key is wrong' sends users debugging the wrong thing. + +**Direction of fix:** Validate `argon2_params` (and raw-key length) in open()/the setter alongside the existing superblock_count check, or map Kdf/BadKeyLength to a distinct operational variant (e.g. InvalidKeyParameters) instead of the blanket InvalidEncryptionKey. + +#### I155. Finished PyTransaction data ops don't check the `finished` guard — a stray op aliases into the next transaction [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `python/src/transaction.rs:135` + +**Problem:** PyTransaction's data operations (allocate, read, update, delete, delete_many, allocate_tagged, set_root_name, savepoint, etc., lines 135–227) do not check the `finished` guard; only commit()/rollback()/__exit__ do. A finished Transaction object still forwards every operation to the db. After `t1 = db.transaction(); t1.commit(); t2 = db.transaction()`, a stray `t1.allocate(b"x")` silently writes into t2's transaction — the exact 'called the wrong object' bug class that the I22/I24 AlreadyFinishedError guard was added to surface, applied to commit/rollback but not to the data ops. If no new transaction is active the engine at least raises NoActiveTransactionError, but the aliasing case is silent data misattribution. + +**Direction of fix:** Check `finished` at the top of each forwarding method and raise AlreadyFinishedError, consistent with the commit/rollback policy. + +#### I156. GIL held across commit fsyncs and Argon2id passphrase rotate_key — freezes all Python threads for the KDF/fsync duration [deepdive 2026-07-02] — **P3** 🔶 OPEN +**Where:** `python/src/db.rs:652` + +**Problem:** Per-op engine calls (including commit's 3 fsyncs, defrag, and add_key/rotate_key/remove_key) hold the GIL for their full duration; only open() detaches (db.rs:276). With passphrase keys, rotate_key runs Argon2id (19 MiB, t=2) once or twice under the GIL — roughly 100-300 ms, not seconds — freezing all other Python threads. The GIL-hold is an explicitly documented design tradeoff (db.rs:634-651 names the consequence and both fix paths), but that comment predates the encryption work (2026-06-20 vs PR #85), so the memory-hard KDF case was never weighed; the comment should at least be updated to name key ops alongside commit/defrag. A passphrase rotate_key or a large commit freezes every other Python thread in the process for the full KDF/fsync duration — seconds-scale for Argon2id. The encryption work postdates the 'per-op calls hold the GIL' comment, which was reasoned about quick engine ops, not a deliberately slow memory-hard KDF. + +**Direction of fix:** Move the mutex acquisition inside py.detach (detach → lock → engine call → unlock → reattach); the closure only needs Chisel: Send, which open() already proves. Do NOT detach while holding the guard — that inverts the GIL/Mutex order and can deadlock. At minimum, detach around the three key-management methods. + +#### I157. redb-strict skips F_FULLFSYNC on macOS — the PR-8 fairness fix was applied to SQLite but not redb [deepdive 2026-07-02] — **P3** 🔶 OPEN +**Where:** `bench/src/redb_engine.rs:93` + +**Problem:** DurabilityMode::Strict maps to redb Durability::Immediate, which on macOS commits via File::sync_data() (redb-2.6.3 unix.rs backend); Rust std's sync_data on Darwin issues fcntl(F_BARRIERFSYNC) (and even a plain fdatasync would not flush the disk write cache). Meanwhile chisel-strict commits via sync_all/F_FULLFSYNC (src/page_io.rs:445-448,480) and sqlite-strict is deliberately handicapped to parity with PRAGMA fullfsync=ON (bench/src/sqlite_engine.rs:50-62), whose comment explicitly names the ~3-orders-of-magnitude macOS artifact this causes. The PR-8 fairness fix was applied to SQLite but not redb, so on macOS (the dev machine this repo runs benches on) redb-strict skips the F_FULLFSYNC cost that both other strict engines pay. Cross-engine strict comparisons on macOS systematically flatter redb — exactly the measurement artifact the sqlite comment says the harness exists to prevent. + +**Direction of fix:** redb exposes no full-fsync knob, so either document the asymmetry in the DurabilityMode::Strict doc and the summary renderer (footnote redb-strict on macOS), or treat macOS redb-strict numbers as non-comparable and rely on Linux CI for cross-engine strict columns. + +#### I158. freemap_churn claims aux-metric persistence that never happens — reclamation regressions are invisible [deepdive 2026-07-02] — **P3** 🔶 OPEN +**Where:** `bench/benches/freemap_churn.rs:193` + +**Problem:** The comments at freemap_churn.rs lines 14, 126, and 193 claim the churn metrics are persisted to bench/results/aux_metrics.jsonl via AuxMetricsWriter, but this bench never uses that writer (it exists in bench/src/runner.rs and is used only by micro_grid.rs); the pages_allocated and file-size deltas are computed in the timed closures and discarded via black_box, so only wall-clock timing is reported. The bench's two stated purposes — trend-tracking the flat-high-water property and reclamation pages_allocated — are unfulfilled: only wall-clock timing reaches Criterion's output. A freemap reclamation regression (file growing per commit) would be invisible unless it also changed timing. The comments tell a reader the safety net exists when it does not. + +**Direction of fix:** Either wire an AuxMetricsWriter (as micro_grid does) to emit the deltas per case, or delete the three aux-file claims and state that only timing is tracked. + +#### I159. No test combines encryption with the on-disk (Path) spillway sidecar — the 'no plaintext hits disk' promise is only unit-tested in-memory [deepdive 2026-07-02] — **P2** 🔶 OPEN +**Where:** `tests/spillway_integration.rs:22` + +**Problem:** No test anywhere combines encryption with the production on-disk spillway sidecar. Chisel::open always uses SpillwayLocation::Path (src/lib.rs:381), but every encrypted-spillway test (src/page_cache.rs:1985 fresh_encrypted_cache_with_spillway) hardcodes SpillwayLocation::InMemory, and tests/spillway_integration.rs never sets encryption_key (grep confirms zero 'encryption' hits in the spillway integration tests). The spillway sidecar is a second file that receives page payloads mid-transaction. The at-rest-encryption promise ('no plaintext hits disk') is exactly about this path, and the only coverage is unit-level against an in-memory buffer. A regression that routed plaintext bytes into the Path-backed sidecar (e.g. a drain refactor picking the wrong payload_size branch) would pass the entire suite. + +**Direction of fix:** Add one integration test: open an encrypted DB with a ~4-page cache, allocate sentinel-patterned values until pages spill, scan the sidecar file for the sentinel bytes (must be absent), then commit + reopen + read back. + +#### I160. RadixU64::insert(u64::MAX) infinite grow loop + delete lacks find_leaf's cap guard (latent; twin of I147) [deepdive 2026-07-02] — **P3** 🔶 OPEN +**Where:** `src/membership_index.rs:243` + +**Problem:** Latent, unreachable-from-public-API inconsistency in the pub(crate) RadixU64: insert(key = u64::MAX) infinite-loops in the grow() while-loop (capacity saturates at u64::MAX for depth >= 6, and grow has no MAX_DEPTH cap), and delete lacks find_leaf's cap != u64::MAX exemption (benign today since u64::MAX can never be inserted, but contradicts find_leaf's documented u64::MAX support). Production keys are u32 tags and validated engine-minted handles, so no external input reaches these paths; fix is a one-line MAX_DEPTH cap / error in the grow loop plus mirroring find_leaf's guard in delete. Same runaway grow-and-allocate loop; same internal contradiction with find_leaf's explicit u64::MAX support and the MAX_DEPTH comment (lines 22-27, "forces one final grow to depth 6"). Unreachable in practice via engine-assigned handles / u32 tags, but latent for any future in-crate caller of the generic radix. + +**Direction of fix:** Add `&& self.depth < MAX_DEPTH` to the grow loop (or a MAX_DEPTH no-op in `grow`, matching FreeMapTree::grow), and align `delete`'s capacity guard with `find_leaf`'s saturated-capacity exemption. + diff --git a/docs/reviews/review-20260702-001902.md b/docs/reviews/review-20260702-001902.md new file mode 100644 index 0000000..4d06fd0 --- /dev/null +++ b/docs/reviews/review-20260702-001902.md @@ -0,0 +1,399 @@ +# Code Review — Chisel +Date: 2026-07-02T00:19:02Z +Reviewer: Claude Code (fresh-eyes pass) +Commit: 581be36ca50c07df6ec38370e02e8e5554360ed8 +Prior review: review-20260622-054729.md + +Method: 8 category reviewers (read-only) over the full engine + Python binding + bench harness; every BUG/DESIGN finding put through an independent adversarial verifier before inclusion. 18 findings confirmed, 0 refuted; 45 SMELL/NIT reported without individual verification. Prior-review delta verified against current code. + +## Executive summary + +1. **TOCTOU create-vs-open race** (`src/lib.rs:366`, BUG) — the create-vs-open decision is made from an unlocked `path.exists()` stat before `flock` is acquired, so two processes racing `open()` can silently overwrite a just-committed database with a fresh superblock. +2. **Unvalidated `stride` from the crypto-header panics on open** (`src/transaction/recovery.rs:258`, BUG/security) — a forged `stride=0` (protected only by a re-stampable XXH3 checksum) is fed straight into `set_stride`, giving a division-by-zero panic that violates the poison-not-panic model; a huge value drives multi-GiB allocations. +3. **`reclaim_freemap_orphans` is not poison-wrapped** (`src/transaction/freemap.rs:663`, BUG) — a fatal error mid-sweep returns to the caller *without* poisoning, and the partially-advanced freemap tree is written back into `current_roots`, leaving an un-poisoned manager holding an indeterminate freemap. +4. **README contradicts the code on a safety contract** (`README.md:363`, BUG/docs) — the README says the newer-minor write-refusal gate is 'not yet wired up'; it shipped (I29, `recovery.rs:409`) and THEORY.md agrees — the two design docs disagree on a durability guarantee in a doc refreshed three commits ago. +5. **Corrupt-but-checksummed pages fail *open* in two of five radixes** (`membership_index.rs`, `overflow.rs:183`, `handle_table.rs:253`, DESIGN) — freemap/data-page/overflow descent fails closed with typed `CorruptPage`, but membership `free_subtree` and the handle-table/RadixU64 grow loops trust structurally-wrong pages (corruption amplification; unbounded grow on `u64::MAX`). + +## Delta from prior review + +Prior reviews read: `review-20260622-054729.md`, `review-20260621-185541.md`. **71 findings verified resolved, 0 regressions.** The two most recent reviews were acted on almost in full. + +- **Regressions:** none. +- **Resolved (71):** the entire 0622 and 0621 finding sets — depth-0 aliasing guards, SpillwayFull data-loss restore, multi-page freemap, overflow type-confusion checks, the `transaction.rs` god-module decomposition, `Handle`/`Tag(NonZeroU32)` newtypes, `is_fatal` exhaustiveness test, PyO3 clippy in CI, and more. (Full list retained in the workflow record.) +- **Unchanged (1):** [0621 read_u64_le helper (I135)]: still open — no helper exists; u64::from_le_bytes(...unwrap()) still repeated (30x superblock/mod.rs, 9x handle_table.rs); ISSUES.md I135 carries no fixed/deferred marker +- **Declined / deferred — not re-filed** (per the rules of engagement): + - [0622 transaction.rs StagingTxn extraction]: I141 explicitly DEFERRED 2026-06-22 — shared prepare/install vocabulary spans staging.rs AND mutate.rs; do not re-file from the SMELL alone + - [0621 MSRV job lib-only, no --tests]: I110 declined-with-rationale — '--tests was tried and reverted' (proptest→getrandom needs edition2024/1.85) (.github/workflows/ci.yml:112-118 comment) + - [0622/0621 wheel gate runs release-only, debug_assertions compiled out]: declined — documented as 'a known, accepted coverage boundary' in wheels.yml:17-22; debug invariants covered by ci.yml's debug test job + - [0622 python CI builds maturin --release, never exercises engine debug asserts]: declined — same documented accepted-boundary comment (.github/workflows/ci.yml:161-166) + - [0621 commit the ADR as docs/ADR.md]: declined — I129 decision keeps the ADR MCP-only in .codebase-memory/ + - [0621 read() -> Vec per-read copy (F2)]: deliberately deferred perf-backlog item, carried unchanged across reviews; read.rs:142 still to_vec() + - [0622 per-variant Rust-side to_py_err fatal-arm coverage]: narrowed by choice — one end-to-end fatal test landed; full per-variant Rust-side test 'tracked separately' per the test-file comment + +## Findings by category + +### correctness + +**`BUG`** — `src/lib.rs:366` +*What:* Chisel::open decides create-vs-open BEFORE the flock is taken: `file_exists` is computed at lines 366-369, the exclusive lock is only acquired inside `PageIo::open` at line 375, and the stale boolean then selects `TransactionManager::create_new` at line 389. The doc comment (lines 339-341) claims the lock is acquired 'before any parsing... rather than racing on the superblock', but the create/open decision itself races. +*Why:* Cross-process TOCTOU with silent data loss: process B stats a nonexistent/zero-length file; process A concurrently creates the DB, commits data, and closes (releasing the flock); B then acquires the lock and runs create_new on the now-populated file, writing a fresh superblock over A's committed database. Multi-process exclusion is an explicit part of the public contract (flock, LockFailed), so this is exactly the race the lock exists to prevent. +*Fix:* Re-check the file length after the flock is held (e.g. have PageIo::open return whether it created the file, or re-stat via the locked fd) and choose create_new vs open_existing from that post-lock observation. + +**`DESIGN`** — `src/transaction/packing.rs:119` +*What:* `SlotPacker::insert` consults `self.insert_cursor` unconditionally — the cursor branch is not gated on `packing_enabled`. `rollback_to_inner` (savepoints.rs:98-102) restores the packer snapshot taken at savepoint creation, which was captured BEFORE `clear_cursor()` ran (savepoints.rs:32-33), so rolling back to the first savepoint of a transaction that had packed inserts restores a live `Some(P)` cursor while that savepoint is still on the stack. +*Why:* This contradicts the module's stated invariant (packing.rs:27-28: "Packing is disabled entirely when savepoints are active") and the savepoint design premise that no in-place mutation happens below the savepoint watermark. After the restore, the next insert writes a slot into pre-watermark page P in place; a second `rollback_to` to the same savepoint truncates only >= watermark, so P keeps the physically-written slot forever — each rollback_to+insert cycle leaks one dead slot into a page that then gets committed (rolled-back value bytes become durable). No API-visible wrong reads, but the rollback mechanism's "nothing below the watermark changed" premise is silently violated. +*Fix:* Gate the cursor branch on `packing_enabled` (`if packing_enabled { if let Some(cursor) = self.insert_cursor { ... } }`) — one guard at the single decision point, which also makes the restore harmless — or have `restore()` force the cursor to None when a savepoint remains active. + +**`DESIGN`** — `src/handle_table.rs:253` +*What:* HandleTable::insert's grow loop (src/handle_table.rs:253) is unbounded: for handle == u64::MAX, capacity() saturates to u64::MAX at depth 6 and the loop allocates one page per iteration forever, and delete (line 289) reports u64::MAX absent at saturated capacity while find_leaf (lines 694-697) explicitly supports it. Real code-level defect and internally inconsistent with the module's own MAX_DEPTH comment and read path, but unreachable through the engine (handles are monotonic from 1), so latent — fix by bounding the loop with `&& self.depth < MAX_DEPTH` as FreeMapTree::mark_free_growing does. +*Why:* Runaway loop with unbounded page allocation on a key the module's own comments and read path explicitly claim to support. Practically unreachable through the engine today (handles are monotonic from `next_handle` starting at 1, so u64::MAX needs ~1.8e19 allocations), but it is a live in-crate API and the code around it (find_leaf's u64::MAX carve-out, the MAX_DEPTH comment) asserts the case is handled when it is not. +*Fix:* Mirror freemap_tree: bound the loop (`while handle >= self.capacity() && self.depth < MAX_DEPTH`) and/or make `grow` a no-op at MAX_DEPTH; give `delete` the same `cap != u64::MAX` exemption `find_leaf` has. + +**`DESIGN`** — `bench/src/redb_engine.rs:93` +*What:* DurabilityMode::Strict maps to redb Durability::Immediate, which on macOS commits via File::sync_data() (redb-2.6.3 unix.rs backend); Rust std's sync_data on Darwin issues fcntl(F_BARRIERFSYNC) (and even a plain fdatasync would not flush the disk write cache). Meanwhile chisel-strict commits via sync_all/F_FULLFSYNC (src/page_io.rs:445-448,480) and sqlite-strict is deliberately handicapped to parity with PRAGMA fullfsync=ON (bench/src/sqlite_engine.rs:50-62), whose comment explicitly names the ~3-orders-of-magnitude macOS artifact this causes. +*Why:* The PR-8 fairness fix was applied to SQLite but not redb, so on macOS (the dev machine this repo runs benches on) redb-strict skips the F_FULLFSYNC cost that both other strict engines pay. Cross-engine strict comparisons on macOS systematically flatter redb — exactly the measurement artifact the sqlite comment says the harness exists to prevent. +*Fix:* redb exposes no full-fsync knob, so either document the asymmetry in the DurabilityMode::Strict doc and the summary renderer (footnote redb-strict on macOS), or treat macOS redb-strict numbers as non-comparable and rely on Linux CI for cross-engine strict columns. + +**`DESIGN`** — `src/transaction/recovery.rs:409` +*What:* The I29 write-gate compares the file's MINOR against the plaintext constant `page::FORMAT_MINOR_VERSION` (= 1) unconditionally, but src/page.rs:120-123 declares that the encrypted MAJOR=2 series "carries its own minor series, independent of plaintext" starting at FORMAT_MINOR_VERSION_ENCRYPTED = 0. +*Why:* A future encrypted file at (2,1) opened by today's binary (whose encrypted-series minor is 0) evaluates `1 > 1 == false` and stays WRITABLE — exactly the clobber-newer-fields hazard the gate exists to prevent. Latent today (no (2,≥1) files exist), but the gate silently fails for the second major series the moment its minor is first bumped. +*Fix:* Select the comparison constant by major series: gate against FORMAT_MINOR_VERSION_ENCRYPTED when the file's MAJOR is 2, FORMAT_MINOR_VERSION when 1. + +**`SMELL`** — `src/membership_index.rs:243` +*What:* Latent, unreachable-from-public-API inconsistency in the pub(crate) RadixU64: insert(key = u64::MAX) infinite-loops in the grow() while-loop (capacity saturates at u64::MAX for depth >= 6, and grow has no MAX_DEPTH cap), and delete lacks find_leaf's cap != u64::MAX exemption (benign today since u64::MAX can never be inserted, but contradicts find_leaf's documented u64::MAX support). Production keys are u32 tags and validated engine-minted handles, so no external input reaches these paths; fix is a one-line MAX_DEPTH cap / error in the grow loop plus mirroring find_leaf's guard in delete. +*Why:* Same runaway grow-and-allocate loop; same internal contradiction with find_leaf's explicit u64::MAX support and the MAX_DEPTH comment (lines 22-27, "forces one final grow to depth 6"). Unreachable in practice via engine-assigned handles / u32 tags, but latent for any future in-crate caller of the generic radix. +*Fix:* Add `&& self.depth < MAX_DEPTH` to the grow loop (or a MAX_DEPTH no-op in `grow`, matching FreeMapTree::grow), and align `delete`'s capacity guard with `find_leaf`'s saturated-capacity exemption. + +**`SMELL`** — `bench/benches/micro_grid.rs:268` +*What:* The read-cold aux calibration reuses capture_aux_metrics_snapshot_restore with ops_per_tx=1, which routes the single Read through drive_workload_with_tx_granularity — wrapping it in engine.begin()/commit() (bench/src/runner.rs:296-302). The timed cold-read routine (run_cold_read_cell, lines 146-154) performs a bare apply_op with no transaction. Chisel's commit has no empty-transaction early-out (src/transaction/lifecycle.rs:178-222 always runs the 3-fsync run_commit). +*Why:* The aux_metrics.jsonl row for read-cold reports fsync_calls ≈ 3 plus any commit-path page allocations that the measured cold read never performs, so the Chisel-internals appendix table misattributes commit-protocol activity to a read-only row. +*Fix:* Give the cold-read row a calibration path that mirrors the timed path (open, one bare apply_op, capture counters) instead of reusing the tx-wrapping snapshot-restore capture. + +**`SMELL`** — `bench/benches/micro_grid.rs:349` +*What:* bench_row_delete_n_per_tx clamps workload_count = ops_per_tx.min(prepop_count) (25 at the 1MB size for a 1000-per-tx row) while the group keeps Throughput::Elements(ops_per_tx); the comment (lines 349-354) claims this preserves 'cross-row comparability'. +*Why:* Criterion divides measured time by Elements, so reporting 1000 elements when only 25 deletes ran would make per-op delete latency ~40x too fast — destroying exactly the comparability the comment claims. Latent today only because delete-1000pertx is not registered in micro_grid(); the moment that row is re-enabled (see the stale-skip finding) the lie goes live. +*Fix:* Set the group throughput to Throughput::Elements(workload_count as u64) per cell (or skip clamped cells) and correct the comment. + +### error-handling + +**`BUG`** — `src/transaction/freemap.rs:663` +*What:* Minor imprecision only: the failure requires the fatal error to strike mid-sweep (reachable_pages, a live-page cache.get, or mark_free_growing); the CorruptPage/ChecksumMismatch skip on dead pages at freemap.rs:457-458 is intentionally non-poisoning and not part of the bug. The BUG severity and substance are correct as stated. +*Why:* A fatal `IoError`/`CorruptPage` from `reachable_pages`, `cache.get`, or `mark_free_committed_path` during the sweep returns to the caller with the manager NOT poisoned. Worse, `mark_free_committed_path` writes the partially-advanced tree root back into `current_roots` even on error (put_tree on the error path, freemap.rs:291-293), so the un-poisoned manager holds a freemap tree in an indeterminate mid-mutation state that a subsequent `commit()` will happily make durable — exactly the class of state the I1 poison model exists to fence off. +*Fix:* Wrap the sweep at the TransactionManager boundary the same way as everything else: `let result = self.freemap.reclaim_orphans(...); self.poison_on_fatal(result)` inside `reclaim_freemap_orphans` (freemap.rs:663-673). + +**`DESIGN`** — `src/membership_index.rs:84` +*What:* As stated, with one softening: the remove()-path scenario where leaf values (1) are freed as page ids usually aborts earlier with a misleading ChecksumMismatch (inner.delete/any_present descend the same bogus depth before free_subtree runs), so the sharpest unmitigated instance is a corrupt interior child pointer at the depth-1 boundary of free_subtree, which is pushed into `freed` without any read or validation. +*Why:* Inconsistent hardening across the crate's three radixes against the exact threat model the repo tests everywhere (checksum-valid, structurally wrong pages): freemap_tree, overflow, and data_page fail closed with typed CorruptPage; handle_table and membership_index fail open. For `free_subtree` the failure mode is corruption amplification — bogus ids (potentially live data pages or even superblock slot 1) enter `txn_freed_pages` and get marked reusable at commit. +*Fix:* Add the freemap_tree-style positional type check (`buf[0]` vs expected MembershipLeaf/MembershipInterior/HandleTable+FLAG) on every descent read in find_leaf/insert_recursive/delete_recursive/iter/free_subtree, returning CorruptPage on mismatch. + +**`DESIGN`** — `src/overflow.rs:183` +*What:* As stated, with one precision: the failing page cannot arise from any legitimate Chisel write or from the stale-handle scenario (a genuinely Overflow-typed page always carries a truthfully-written total_length, replicated per page). Reaching the panic requires on-disk corruption/tampering that also passes the XXH3 checksum. Within the module's own I14 standard — corrupt-but-checksummed chains surface as typed CorruptPage, never panic/abort — the gap is real: total_length is the one header field read() trusts unbounded before allocating, and a cheap bound (next_page_id * OVERFLOW_PAYLOAD, the same universe bound reclaim_orphans uses) would close it. +*Why:* The module's own hardening standard (I14: corrupt-but-checksummed chains must surface as typed CorruptPage, never a panic/abort) is contradicted for the one field it trusts most. A well-formed chain can never carry more bytes than the file holds, so a cheap plausibility bound exists and is already used elsewhere (reclaim_orphans uses cache.next_page_id()). +*Fix:* Bound total_length against a file-derived maximum (cache.next_page_id() * OVERFLOW_PAYLOAD → CorruptPage if exceeded), or clamp the initial with_capacity and let Vec grow as pages are validated. + +**`DESIGN`** — `src/error.rs:397` +*What:* Accurate as stated. Minor precision note: the add_key path (keys.rs:151) reaches InvalidEncryptionKey via an explicit `.map_err`, not the blanket From impl, but the effect (BadKeyLength/Kdf on the new credential reported as "does not match any key slot") is identical. +*Why:* Creating a brand-new encrypted DB with bad Argon2Params yields 'key does not match any key slot' — nonsensical on create where no slots exist yet; the same conflation hits add_key's wrap of the NEW credential (transaction/keys.rs:151 map_err → InvalidEncryptionKey). The comment above the From impl claims a CryptoError here 'is always a key-or-KDF problem on intact on-disk data', which is true, but collapsing 'your params are invalid' into 'your key is wrong' sends users debugging the wrong thing. +*Fix:* Validate `argon2_params` (and raw-key length) in open()/the setter alongside the existing superblock_count check, or map Kdf/BadKeyLength to a distinct operational variant (e.g. InvalidKeyParameters) instead of the blanket InvalidEncryptionKey. + +**`SMELL`** — `src/page_io.rs:336` +*What:* `read_page_unit_into` validates `buf.len() == stride` only via `debug_assert_eq!`, while its sibling `write_page_unit` (line 370) does a real runtime check returning an error. In release, a wrong-size buf silently prefix-reads on the File backing (`read_exact(buf)` reads buf.len() bytes) but panics on the Memory backing (`copy_from_slice` length mismatch, line 351). +*Why:* The two backings diverge on the same caller bug — silent short data on File vs a reachable panic on Memory — and the read path is asymmetric with the write path's validation philosophy. Today the single caller (PageCache::load_page) passes `&mut on_disk[..stride]` exactly, so this is latent, but the panic in the Memory branch is reachable non-test code and the File branch's silent misread is the worse failure mode. +*Fix:* Promote the debug_assert to the same runtime length check `write_page_unit` uses (or take `&mut [u8; ENC_PAGE_SIZE]`-style typed buffers), so both backings fail identically and loudly. + +**`SMELL`** — `src/transaction/recovery.rs:337` +*What:* The comment at recovery.rs:334-336 says a `decrypt_body` tag failure "means corruption, not a wrong key (the slot already authenticated the DEK)", yet the code maps it to `ChiselError::InvalidEncryptionKey`, whose own doc (error.rs:170-173) and Display text ("wrong passphrase or raw key") assert the opposite condition. `map_err(|_| ...)` also drops the underlying CryptoError. +*Why:* An operator hitting this path is told to hunt for the right key when the file body is actually corrupt (or a buffer/AAD-pairing regression re-appeared — the exact bug class the comment at recovery.rs:299-306 documents fixing). Misdiagnosis of the previously-shipped bug class is masked behind the wrong-key error. +*Fix:* Return a corruption-flavored variant (e.g. `CorruptSuperblock` with a body-auth defect note) instead of `InvalidEncryptionKey`, matching the comment's own diagnosis. + +**`NIT`** — `src/page_cache.rs:991` +*What:* On a PLAINTEXT database, a spillway blob whose length mismatches PAGE_SIZE is surfaced as `ChiselError::DecryptionFailed` (load_page spillway branch line 991, and flush drain line 538) even though no cipher is involved. +*Why:* Both are unreachable-in-practice programming-error guards (blob length is fixed by payload_size), but the variant misdirects diagnosis: an operator seeing DecryptionFailed on an unencrypted DB will chase key/crypto problems instead of the real slot-size invariant break. `CorruptPage` (already used for the analogous LRU/entries desync at line 1167) fits better. +*Fix:* Map the plaintext-arm try_into failures to `CorruptPage { page_id }` instead of DecryptionFailed in both load_page and the flush drain loop. + +**`NIT`** — `src/error.rs:263` +*What:* The `SpillwayFull` Display has a `limit_bytes == 0` arm claiming '0 is the spillway disabled sentinel', but the disabled-spillway overflow path returns `CacheFull` instead (page_cache.rs:1138-1142, and the CacheFull variant doc at error.rs:53-58 says exactly that); the only production constructor of SpillwayFull uses `self.max_bytes` of a live spillway (spillway.rs:237), which is nonzero whenever a spillway exists. +*Why:* The special-cased message describes a state that appears unreachable, and it contradicts the CacheFull doc two variants up — a reader auditing which error fires when the spillway is disabled gets two different answers from the same file. +*Fix:* Either delete the limit_bytes==0 arm (CacheFull owns the disabled case) or note in the arm that it is defensive-only and CacheFull is the expected disabled-path error. + +### api-design + +**`DESIGN`** — `python/src/transaction.rs:135` +*What:* PyTransaction's data operations (allocate, read, update, delete, delete_many, allocate_tagged, set_root_name, savepoint, etc., lines 135–227) do not check the `finished` guard; only commit()/rollback()/__exit__ do. A finished Transaction object still forwards every operation to the db. +*Why:* After `t1 = db.transaction(); t1.commit(); t2 = db.transaction()`, a stray `t1.allocate(b"x")` silently writes into t2's transaction — the exact 'called the wrong object' bug class that the I22/I24 AlreadyFinishedError guard was added to surface, applied to commit/rollback but not to the data ops. If no new transaction is active the engine at least raises NoActiveTransactionError, but the aliasing case is silent data misattribution. +*Fix:* Check `finished` at the top of each forwarding method and raise AlreadyFinishedError, consistent with the commit/rollback policy. + +**`SMELL`** — `src/error.rs:221` +*What:* `LockFailed` is listed in the 'Fatal — database integrity is in question' block and `is_fatal()` returns true for it, but it is only produced at open time when another process holds the advisory flock (page_io.rs:145). Meanwhile `FileNotFound` — the other purely-open-time caller mistake — is classified operational. +*Why:* The enum header (lines 4-11) defines Fatal as 'integrity invariants have been violated; stop using the handle', and names the Fatal/Operational split a breaking-change contract for callers doing `if e.is_fatal()` triage. Lock contention violates no integrity invariant — the correct recovery is 'retry later/close the other handle', yet is_fatal() tells the caller the database is suspect. There is no poisoning consequence (no handle exists yet), so the misclassification is pure caller-facing semantics — and per the header, fixing it later is a breaking change, so pre-1.0 is the time. +*Fix:* Move LockFailed to the Operational block (updating is_fatal(), the I104 test's documented_is_fatal, and the fatal-count tripwire of 10), or document in the Fatal block why open-time contention is deliberately conservative. + +**`NIT`** — `src/lib.rs:353` +*What:* `Chisel::open(path: &Path, ...)` takes a bare `&Path` rather than `impl AsRef`, unlike std::fs and virtually every storage crate (rusqlite, redb, sled). +*Why:* Callers must write `Chisel::open(Path::new("db"), ...)` for the common string-literal case; `AsRef` is a non-breaking generalization (existing `&Path` call sites keep compiling). +*Fix:* Change the signature to `pub fn open(path: impl AsRef, options: Options)` and take `path.as_ref()` once at the top. + +**`NIT`** — `python/chisel/chisel.pyi:166` +*What:* Chisel.handles() is stubbed as -> Iterable[int] while Transaction.handles() (line 226) is -> list[int]; the runtime returns a list in both cases (db.rs:400 returns Vec, transaction.rs:199 forwards to it). +*Why:* Type-checked callers of db.handles() lose len()/indexing on a value that is in fact a list, and the two stubs for the same underlying method disagree. +*Fix:* Change Chisel.handles() to -> list[int]. + +### performance + +**`DESIGN`** — `python/src/db.rs:652` +*What:* Per-op engine calls (including commit's 3 fsyncs, defrag, and add_key/rotate_key/remove_key) hold the GIL for their full duration; only open() detaches (db.rs:276). With passphrase keys, rotate_key runs Argon2id (19 MiB, t=2) once or twice under the GIL — roughly 100-300 ms, not seconds — freezing all other Python threads. The GIL-hold is an explicitly documented design tradeoff (db.rs:634-651 names the consequence and both fix paths), but that comment predates the encryption work (2026-06-20 vs PR #85), so the memory-hard KDF case was never weighed; the comment should at least be updated to name key ops alongside commit/defrag. +*Why:* A passphrase rotate_key or a large commit freezes every other Python thread in the process for the full KDF/fsync duration — seconds-scale for Argon2id. The encryption work postdates the 'per-op calls hold the GIL' comment, which was reasoned about quick engine ops, not a deliberately slow memory-hard KDF. +*Fix:* Move the mutex acquisition inside py.detach (detach → lock → engine call → unlock → reattach); the closure only needs Chisel: Send, which open() already proves. Do NOT detach while holding the guard — that inverts the GIL/Mutex order and can deadlock. At minimum, detach around the three key-management methods. + +**`NIT`** — `src/page_cache.rs:1198` +*What:* maybe_evict Phase B builds `spill_blob: Vec` with `entry.buf.as_ref().to_vec()` in the plaintext arm purely to type-unify with the encrypted arm's sealed array; `Spillway::spill` takes `&[u8]`. +*Why:* An avoidable 8 KB heap allocation + copy on every plaintext spill. Spills are I/O-adjacent so it is not hot-hot, but under sustained cache pressure this runs once per evicted page. +*Fix:* Branch at the spill call instead of unifying the buffer: `match &self.cipher { Some(c) => spw.spill(id, &c.seal(...)), None => spw.spill(id, entry.buf.as_ref()) }` (the encrypted arm's stack array already suffices). + +**`NIT`** — `bench/src/runner.rs:245` +*What:* apply_op materializes the payload with vec![0u8; *size] inside the timed region for Allocate (line 245) and Update (line 267), while the Read path got careful black_box fencing (I90). Document-store updates run up to 4 MiB, so a per-op allocation+memset is charged to every engine's timed number. +*Why:* The cost is identical across engines (so cross-engine ratios compress rather than invert) but it inflates absolute per-op latencies, and for chisel-mem — whose whole purpose is isolating pure CPU/engine cost — a 4 MiB memset is a non-trivial fraction of the measured op. +*Fix:* Pre-build one max-size zero buffer per workload and pass &payload[..size] slices into allocate/update, keeping payload construction out of the timed window. + +### tests + +**`DESIGN`** — `tests/spillway_integration.rs:22` +*What:* No test anywhere combines encryption with the production on-disk spillway sidecar. Chisel::open always uses SpillwayLocation::Path (src/lib.rs:381), but every encrypted-spillway test (src/page_cache.rs:1985 fresh_encrypted_cache_with_spillway) hardcodes SpillwayLocation::InMemory, and tests/spillway_integration.rs never sets encryption_key (grep confirms zero 'encryption' hits in the spillway integration tests). +*Why:* The spillway sidecar is a second file that receives page payloads mid-transaction. The at-rest-encryption promise ('no plaintext hits disk') is exactly about this path, and the only coverage is unit-level against an in-memory buffer. A regression that routed plaintext bytes into the Path-backed sidecar (e.g. a drain refactor picking the wrong payload_size branch) would pass the entire suite. +*Fix:* Add one integration test: open an encrypted DB with a ~4-page cache, allocate sentinel-patterned values until pages spill, scan the sidecar file for the sentinel bytes (must be absent), then commit + reopen + read back. + +**`SMELL`** — `tests/encryption_open.rs:106` +*What:* key_supplied_for_plaintext_db_errors asserts only `err.is_err()`. The intended contract — Chisel::open with a key on a plaintext DB returns EncryptionNotSupported (src/transaction/recovery.rs:326) — is not pinned by any test; encryption_roundtrip.rs pins the wrong-key (InvalidEncryptionKey) and no-key (NoEncryptionKey) variants but not this one. +*Why:* The test would still pass if the code regressed to returning a misleading error (CorruptSuperblock, InvalidEncryptionKey, FileSizeMismatch) for the key-on-plaintext case, breaking the documented error contract callers dispatch on without any red test. +*Fix:* Match the variant: `assert!(matches!(err.unwrap_err(), ChiselError::EncryptionNotSupported))`. Consider mirroring in python/tests/test_encryption.py, which also lacks this case entirely. + +**`NIT`** — `src/crypto/mod.rs:593` +*What:* The PageCipher seal/open tests use fixed inputs (e.g. the round-trip at line ~593 and length check at line 650); there is no property test over random bodies/page-ids, unlike the oracle proptests in freemap_tree.rs:833, membership_index.rs:1173, data_page.rs:501, and freemap.rs:283. +*Why:* proptest is already a dev-dep and the seal→open identity plus tamper/wrong-page-id-must-fail properties are the crypto layer's core contract; a randomized pin is nearly free and catches padding/AAD-construction edge cases fixed vectors miss. +*Fix:* Add a small proptest: random 8192-byte body + random page_id → seal → open == identity; open with a different page_id or one flipped ciphertext bit must return Err. + +### ci-automation + +**`SMELL`** — `.github/workflows/ci.yml:62` +*What:* Three stale comments in ci.yml: (1) line 62 — audit job says "Currently the root crate ships with only `xxhash-rust` and `libc` as production deps ... so the practical risk surface is small"; the root crate now has 10 runtime deps including the whole crypto stack (chacha20poly1305, argon2, hkdf, sha2, zeroize, getrandom, base64ct, rustc-hash); (2) lines 66-73 — "Permissions notes" describe `pull-requests: write` / `checks: write` for an annotation-posting action, but the job runs `cargo audit` directly (the rustsec/audit-check action was removed per line 85-86) and declares no permissions block; (3) line 92 — msrv job claims 1.82 is "the floor stabilized by Option::is_none_or in src/page_cache.rs"; `is_none_or` is used nowhere in src/. +*Why:* The audit comment's "risk surface is small" rationale is now the opposite of reality — the crypto deps are precisely why the audit job matters; the permissions and is_none_or notes describe configurations/code that no longer exist, misleading anyone editing the workflow. +*Fix:* Refresh the three comments: audit surface now includes the RustCrypto stack; drop the action-permissions note; msrv rationale is "conservative pin, true floor ~1.74 io::Error::other" per Cargo.toml:39-40. + +**`NIT`** — `.github/workflows/wheels.yml:113` +*What:* The sdist job runs `maturin sdist -o ../dist` and uploads the tarball, but nothing ever pip-installs or builds the sdist. The wheel path is import-tested per Python version (CIBW_TEST_COMMAND); the sdist is not, and it is the one artifact whose correctness depends on maturin vendoring the `chisel = { path = ".." }` parent crate into the tarball. +*Why:* A broken sdist (missing parent-crate sources, a classic path-dep failure) would only be discovered by an end user at `pip install chisel --no-binary` time, after the release is tagged. +*Fix:* Add one step after the build: `pip install dist/*.tar.gz && pytest python/tests` (needs a Rust toolchain, already present on the runner). + +**`NIT`** — `.github/workflows/ci.yml:88` +*What:* Both the ci.yml audit job (line 88) and the wheels.yml cargo-test-gate (line 30) run `cargo install cargo-audit --locked`, compiling cargo-audit from source on every run; the audit job also has no Swatinem/rust-cache step, so nothing is reused across runs. +*Why:* Adds several minutes of pure toolchain-compile time to every push/PR and every tag build for a tool whose binary is prebuilt and cacheable. +*Fix:* Install a prebuilt binary (e.g. `taiki-e/install-action@cargo-audit`) or add rust-cache with `cache-all-crates` to both jobs. + +### docs-vs-reality + +**`BUG`** — `README.md:363` +*What:* README.md:363 falsely states the MINOR write-refusal gate is "not yet wired up"; it shipped as the I29 write-gate in src/transaction/recovery.rs:409-411 (force_read_only on file-MINOR > binary-MINOR, mutations return ReadOnlyMode), consistent with THEORY.md:132 (repo root). Only the README sentence needs updating. +*Why:* This is the user-facing compatibility contract, in a README refreshed three commits ago (#86). A user reading README concludes that opening a newer-minor file risks clobbering unknown fields on write; in reality mutations return ReadOnlyMode. The two design docs disagree with each other on a safety guarantee. +*Fix:* Update README's format-compatibility section to say the read-only write-gate is implemented (matching THEORY.md:132 and recovery.rs:409); drop the "lands with the first post-1.0 minor bump" sentence. + +**`DESIGN`** — `bench/benches/freemap_churn.rs:193` +*What:* The comments at freemap_churn.rs lines 14, 126, and 193 claim the churn metrics are persisted to bench/results/aux_metrics.jsonl via AuxMetricsWriter, but this bench never uses that writer (it exists in bench/src/runner.rs and is used only by micro_grid.rs); the pages_allocated and file-size deltas are computed in the timed closures and discarded via black_box, so only wall-clock timing is reported. +*Why:* The bench's two stated purposes — trend-tracking the flat-high-water property and reclamation pages_allocated — are unfulfilled: only wall-clock timing reaches Criterion's output. A freemap reclamation regression (file growing per commit) would be invisible unless it also changed timing. The comments tell a reader the safety net exists when it does not. +*Fix:* Either wire an AuxMetricsWriter (as micro_grid does) to emit the deltas per case, or delete the three aux-file claims and state that only timing is tracked. + +**`SMELL`** — `src/page_cache.rs:965` +*What:* The module header (lines 29-30) declares "The cache is a STRICT bound" and `evict_clean_to_cap` enforces `entries.len() <= max_pages`, but `load_page` runs `maybe_evict()` BEFORE inserting (line 965), then inserts the new entry (lines 994/1041), leaving `entries.len() == max_pages + 1` with no subsequent eviction. `new_page`/`claim_page` do the opposite order (insert then evict) and end at the cap. +*Why:* For a read-heavy workload the steady state is persistently max_pages+1 resident pages, not max_pages: every cold miss evicts down to the cap and then inserts one over. It never trims back until the next miss/allocation, so the stated strict cap is off by one on the dominant read path. Harm is one extra 8 KB page, but the invariant the header (and the spillway design spec reference) leans on is not what the code enforces, and the load_page comment ("does not temporarily push us two entries over") tacitly admits one-over without saying it is permanent. +*Fix:* Either evict after insertion in load_page (matching new_page's insert-then-evict order), or amend the header/spec comment to state the real bound is max_pages+1 on the read path. + +**`SMELL`** — `src/page_io.rs:221` +*What:* `set_stride` swallows the `seek(End(0))` error with `unwrap_or(0)`, justified by the comment "Infallible by signature (the encrypted-open bootstrap has no Result to thread into)". That claim is false: all four production call sites (transaction/recovery.rs:67, 258, 284, 289) sit inside `Result`-returning functions (`create`, `open_existing`), so the error could be threaded through trivially. +*Why:* The comment defends a design constraint that does not exist. And the fail-closed story is weaker than advertised: after the fallback seeds `cached_page_count = 0`, `write_page_unit` (which never bounds-checks) silently re-seeds the count to `page_id + 1` — a wrong, low value — so the promised InvalidPageId fail-closed can be masked by any intervening write before it fires. InvalidPageId is fatal (error.rs:224) so it eventually poisons, but the path is roundabout and the reported error misattributes a broken fd as a bad page id. +*Fix:* Make `set_stride` return `Result<()>` and propagate the seek error at the four recovery.rs call sites; delete the infallible-by-signature comment. + +**`SMELL`** — `src/transaction/recovery.rs:307` +*What:* `open_existing` re-implements superblock winner selection inline (`candidates.iter().filter_map(deserialize).max_by_key(txn_counter)`, recovery.rs:307-313) to keep the raw buffer paired for AAD reconstruction. `Superblock::select` is now called only from tests (recovery_tests.rs:77, superblock/mod.rs tests), yet six production comments — mod.rs:13, recovery.rs:172/191, lifecycle.rs:173, error.rs:99, lib.rs:508 — all describe recovery as "runs Superblock::select()". +*Why:* Two copies of the load-bearing recovery rule (checksum-filter + highest-counter + last-max tie-break) must now be kept in sync by hand; the unit tests that pin select()'s behavior no longer exercise the code recovery actually runs. A future change to select() (e.g. extra validation) would silently not apply to open_existing. +*Fix:* Change `Superblock::select` to return the winning index (or `(Superblock, usize)`) and call it from open_existing so there is one selection implementation; the comments then become true again. + +**`SMELL`** — `src/superblock/crypto_header.rs:81` +*What:* KeySlot::aad()'s doc comment says the AAD "Binds the wrap to its salt/params/nonce so a slot can't be transplanted between DBs." The AAD contains only the slot's own state/kdf_id/argon2/salt/nonce — no DB identity of any kind. +*Why:* The claimed property does not hold: a key-slot copied from DB A into DB B's slot table authenticates fine under DB A's credential and unwraps to DEK_A (the failure only surfaces later when decrypt_body rejects the wrong DEK). The AAD's real property is anti-param-tampering within a slot, which the wrap_dek doc in crypto/mod.rs states correctly. A future reader relying on the claimed cross-DB binding could remove the downstream check that actually provides containment. +*Fix:* Reword the comment to "binds the wrap to this slot's own KDF metadata so params/salt/nonce can't be tampered"; if cross-DB binding is actually wanted, add a per-DB identifier to the slot AAD (format change). + +**`SMELL`** — `src/handle_table.rs:594` +*What:* The sparse-allocation comment in `insert_recursive` claims "The newly allocated child is already a fresh page, so it IS its own COW clone — no further copy needed." The code then passes that fresh child to `insert_recursive` (line 622), whose first action (lines 552-567) unconditionally allocates ANOTHER page, copies the fresh child into it, and pushes the fresh child onto `freed` — i.e. a further copy is made and the just-materialized page is immediately superseded. +*Why:* Comment asserts the opposite of what the code does. The behavior itself is benign (the superseded fresh page is reclaimed at commit) but costs one extra alloc+8KB copy per materialized node; the membership twin (membership_index.rs:284-287) documents the identical behavior honestly as "immediately re-COWed ... benign, first-touch only". A reader trusting this comment would mis-reason about the freed-list contents and allocation counts. +*Fix:* Either fix the comment to match the membership twin's wording, or skip the redundant re-COW by threading a 'freshly materialized' flag (the comment then becomes true and one alloc+copy per new subtree node is saved). + +**`SMELL`** — `src/freemap.rs:9` +*What:* The file header claims "data-page allocation AND the handle-table / membership-index COW paths prefer `FreeMap::allocate_first` (reusing a page freed by a prior committed transaction)", and the I35 note (lines 61-62) says "The production allocator path uses `allocate_first` + `mark_free`." In reality `FreeMap::allocate_first` has zero production callers (grep: only its own tests and a tests.rs comment); the production allocator is `FreeMapTree::allocate_first`, which composes `FreeMap::first_free_bit_from` + `FreeMap::clear_bit` (freemap_tree.rs:469-481, 508, 437). The I35 note's claim that `is_free` is reached "only from src-tests" is also stale: `FreeMapTree::is_free` → `FreeMap::is_free` is production-called by the defrag orphan sweep (transaction/freemap.rs reclaim_orphans, via freemap_tree.rs:240-245). +*Why:* Comment rot from the multi-page-freemap rewrite: the header names the wrong function as the production allocation entry point, which misleads anyone tracing the allocator or auditing which leaf primitives are dead code (the #[allow(dead_code)] rationale block is built on the stale claims). +*Fix:* Rewrite the header to name `FreeMapTree::allocate_first` (via first_free_bit_from/clear_bit) as the production path, and update the I35 dead-code note to reflect that is_free/first_free_bit_from/clear_bit now have production callers through the tree, leaving only capacity/allocate_first as test-only. + +**`SMELL`** — `src/stats.rs:30` +*What:* Stats field docs contradict how the struct is populated. `total_pages` (line 20) claims 'matching Superblock.total_pages', and `file_size_bytes` (lines 22-29) claims 'Raw size of the database file on disk. May exceed total_pages * PAGE_SIZE when a previous crash left orphan pages'. In reality both come from the same number: `Chisel::stats` sets total_pages = txm.file_page_count() (physical file length / stride, page_cache.rs:668-669 → io.page_count()) and file_size_bytes = that same count × PAGE_SIZE (lib.rs:850), so file_size_bytes can NEVER exceed total_pages*PAGE_SIZE, and total_pages is the physical count (which is what may exceed the superblock's), not Superblock.total_pages. +*Why:* The field docs describe the exact inverse of the implementation: an operator reasoning about crash orphans from these docs draws the wrong conclusion from both fields. Additionally, for encrypted DBs the on-disk stride is ENC_PAGE_SIZE = 8232 (page_io.rs:21), so file_size_bytes = count × 8192 underreports the real on-disk size by ~0.5%, further undermining the 'raw size on disk' claim (lib.rs:884 `file_size_bytes()` doc says 'on-disk size' too). +*Fix:* Rewrite the two field docs to state what is computed (physical page count from file length; logical bytes = count × PAGE_SIZE), and either document or correct the encrypted-stride underreport (multiply by the io stride for the 'on disk' figure). + +**`SMELL`** — `src/lib.rs:137` +*What:* The `Options` rustdoc says 'External callers must construct via `Options { ..Options::default() }` rather than a full struct literal', but functional-record-update syntax on a `#[non_exhaustive]` struct is a hard error (E0639) outside the defining crate. The internal comment at lines 218-220 states the opposite, correctly: 'External crates can't construct via a struct literal — even with `..Options::default()`'. +*Why:* This is the user-facing rustdoc for the primary configuration type; it instructs downstream users to write code that does not compile. The two comments in the same file contradict each other. +*Fix:* Change the rustdoc to point at the supported paths: chained setters (`Options::default().cache_max_bytes(...)`) or `let mut o = Options::default(); o.field = ...` (fields are pub, so field mutation still works). + +**`SMELL`** — `src/lib.rs:605` +*What:* `Chisel::tag` doc says 'Returns 0 for untagged handles', but the signature is `Result>` and the body maps a stored 0 to `None` (line 610, `.map(Tag::new)` with the inline comment 'stored 0 -> None'). +*Why:* Stale doc from before the Tag/Option newtype boundary (I126): a caller reading the rustdoc looks for a zero Tag that is unconstructable by design — the whole point of `Tag(NonZeroU32)` is that 0 is never returned. +*Fix:* Reword to 'Returns Ok(None) for untagged handles.' + +**`SMELL`** — `python/src/db.rs:639` +*What:* The with_inner_* rationale comment says 'Chisel owns Cell internally so `&Chisel` is not Sync' and 'The GIL prevents CROSS-THREAD re-entry into the RefCell' — but the RefCell was replaced by Mutex> in I75 (line 114), and the Mutex already provides the exclusivity that makes Sync irrelevant; with_inner_mut_io has &mut Chisel, for which only Send is required (open()'s py.detach at line 276 relies on exactly that). +*Why:* The stated constraint is not the real one, and the comment's suggested fixes ((a) make Chisel Sync-safe, (b) move GIL release into the engine) are heavier than needed. Worse, it omits the actual hazard of a naive fix: detaching while holding the Mutex guard creates a GIL/Mutex lock-order inversion deadlock (thread B holds GIL blocked on the mutex, thread A holds the mutex waiting to reattach). A future maintainer following this comment could ship that deadlock. +*Fix:* Rewrite the comment: the blocker is lock ordering, not Sync; the safe pattern is lock-inside-detach. Delete the stale RefCell reference. + +**`SMELL`** — `python/src/db.rs:91` +*What:* The `inner` field comment says 'After close(), is_poisoned reports true and (future) mutating methods will raise PoisonedError.' The code raises ClosedError (closed_err(), line 680), not PoisonedError, and the methods are not future — they exist. +*Why:* Directly contradicts the module header (lines 4–9), errors.rs's I25 note (line 139), and the actual behavior. A reader trusting this comment would write `except PoisonedError` handlers that miss the closed case. +*Fix:* Change the comment to say subsequent calls raise ClosedError. + +**`SMELL`** — `src/transaction/tests.rs:210` +*What:* poisoned_manager_rejects_every_public_entry_point claims (comment at lines 203–208) to assert the Poisoned rejection 'for each method independently so a future refactor that forgets to wrap a new entry point will fail loudly', but entry points added since — add_key/rotate_key/remove_key (src/transaction/keys.rs:134/164/201), set_root_name/get_root_name/clear_root_name, stats, counters — are absent from its list. +*Why:* The comment's guarantee is now false: the exact scenario it was written to catch (new entry point forgets the poison guard) already happened silently for eight methods. The key ops each carry an early `poisoned.get()` check (keys.rs:139/169/202) that no test exercises directly — only the deeper rewrite_crypto_header guard is tested (keys.rs:302), so removing those early-outs, or a new entry point skipping check_alive, would not fail loudly. +*Fix:* Extend the test with the missing entry points (an encrypted fresh_manager variant is needed for the key ops), or shrink the comment's claim to match reality. + +**`SMELL`** — `bench/benches/freemap_churn.rs:141` +*What:* bench_delete_churn_flat is documented as '(in-memory engine)' (line 114) and registers BenchmarkId::new("in-memory", ...) (line 141), but the setup opens a tempfile-backed Chisel::open(tf.path(), ...) (lines 147-155) with real F_FULLFSYNC commits — churn_cycle's own doc (line 82) says in-memory means 6 no-op fsyncs per cycle. The justification comment 'in-memory Vec opens need special handling in Chisel' is false: Chisel::open_in_memory_with_options is used routinely by the bench crate's own adapter (bench/src/chisel_engine.rs:53). Same doc block also says the body is 'inside iter_custom' (it uses iter_batched) and that the throughput unit is 'round-trips per second' while the code sets Throughput::Elements(live_count), i.e. per-record. +*Why:* The persisted Criterion benchmark ID 'in-memory' labels numbers that are dominated by real F_FULLFSYNC cost; anyone reading trends will attribute file-I/O cost to the pure CPU/memory regime. The surrounding comment rot (iter_custom, round-trip units) compounds the misread. +*Fix:* Either open the engine via Chisel::open_in_memory_with_options to match the label, or rename the BenchmarkId to 'chisel-file' (as groups 2/3 use) and fix the doc block's iter_custom/throughput-unit claims. + +**`SMELL`** — `bench/benches/micro_grid.rs:51` +*What:* The skip rationale is stale on two levels. TX_BUDGET_BYTES (lines 50-54) says cells are skipped 'to avoid Chisel's CacheFull... cache hard ceiling is ~16 MB', and lines 412-416 say update/delete-1000pertx 'exceed Chisel's 2048-page cache ceiling... not measurable under default cache settings' — but every bench engine now opens with the spillway enabled at 1024x cache budget (bench/src/chisel_engine.rs:40), and runner.rs states repeatedly that 'with the spillway enabled... no strict cache ceiling applies'; the test populate_snapshot_chisel_large_size_chunks pushes 24 MiB through single-engine chunks successfully. The file header additionally claims a '270-cell micro grid' over '9 row groups' while only 6 groups are registered (and discover.rs:257 says '165 cells'). +*Why:* Grid cells (all 1000-per-tx cells at >=16KB, plus two whole rows) are silently missing from the published grid for a constraint the harness itself documents as removed; the header's 270/9 claim tells a reader coverage exists that doesn't. +*Fix:* Re-test whether the 1000-per-tx cells run under the spillway and re-enable them (or update TX_BUDGET_BYTES/skip comments to state the real current reason), and fix the header cell/row counts. + +**`SMELL`** — `bench/src/runner.rs:75` +*What:* supports_internal_counters's doc says 'Currently only ChiselStrict (the other engines are black-box)' but the code matches ChiselStrict | ChiselMemory. The companion test (lines 647-656) iterates EngineMode::ALL — which excludes ChiselMemory — and its assertion message repeats 'only ChiselStrict reports internal counters', while another test in the same file (run_scenario_cell_in_memory_mode_is_wired_and_does_work) asserts ChiselMemory DOES report counters. +*Why:* Comment claims X while code does Y; the test's iteration domain hides the discrepancy, so a reader trusting either the doc or the test message gets the wrong model of which modes fill the Chisel-internals appendix. +*Fix:* Update the doc to 'ChiselStrict and ChiselMemory' and extend the test to cover ChiselMemory explicitly (assert true) rather than relying on ALL's exclusion. + +**`SMELL`** — `THEORY.md:178` +*What:* Three false claims in the MSRV decision (lines 176-180): (1) "Adopting a real Cargo workspace — deferred (I61)" — I61 in fact CREATED the workspace; root Cargo.toml declares `[workspace] members = [".", "python", "bench"]` and its own comment credits I61; (2) "the published library (no getrandom/proptest in its tree)" — `getrandom = "0.2"` is a direct runtime dependency of the library (Cargo.toml:84, used in src/crypto/mod.rs); (3) line 180: "driven by actual language usage (`is_none_or` is 1.82+)" — `is_none_or` appears nowhere in src/ (grep: zero hits); Cargo.toml:39-40 says the true stdlib floor is ~1.74 (io::Error::other). +*Why:* THEORY.md is the "why" document readers are told to trust for durable mental models; it asserts the opposite of the current build layout, a false dependency-tree claim about the published crate, and a nonexistent language-feature floor — all in a doc refreshed at commit 9efb735. +*Fix:* Rewrite the MSRV decision paragraph: workspace exists (resolver=2, default-members excludes python); getrandom 0.2 is in the library tree but predates edition2024; 1.82 is a conservative pin above the ~1.74 io::Error::other floor. + +**`SMELL`** — `THEORY.md:190` +*What:* "It lives in a `bench/` subcrate that is a *sibling* to `python/`, not a plain workspace member drawn into the engine's own build in a way that would auto-run its 10–25 minutes of tests on every `cargo test`." Reality: bench IS a workspace member and a default-member (Cargo.toml:20,31), and root `cargo test` DOES run bench's tests — README:79/84 and ARCHITECTURE.md:133 both say so, and ci.yml:120-127 documents removing the standalone bench-tests job for exactly that reason. The 10-25 min figure is the Criterion benches, which `cargo test` does not run. +*Why:* Directly contradicts README, ARCHITECTURE, Cargo.toml, and CI on how the build is structured; a reader following THEORY would not expect bench test failures from a root `cargo test`. +*Fix:* Update the history paragraph: bench started as a sibling crate, became a default-members workspace member under I61; root `cargo test` runs its equivalence tests, while the long Criterion benches only run under `cargo bench`. + +**`SMELL`** — `src/lib.rs:137` +*What:* The public rustdoc on `Options` says "External callers must construct via `Options { ..Options::default() }` rather than a full struct literal" — but with `#[non_exhaustive]`, functional-update syntax is ALSO a struct literal and fails with E0639 in downstream crates. The crate's own internal comment 80 lines later (lib.rs:217-220) states this: "External crates can't construct via a struct literal — even with `..Options::default()`"; README:305 agrees (chained setters are the supported path). +*Why:* This is rendered rustdoc on the primary configuration type; a user copying the documented construction pattern gets a compile error, and the same file contradicts itself. +*Fix:* Change the doc sentence to point at the chained setters (`Options::default().cache_max_bytes(…)…`), matching the I36 comment and README. + +**`SMELL`** — `src/page.rs:107` +*What:* Comment on FORMAT_MINOR_VERSION says the newer-minor write check "is deferred until the first 1.1 release (at which point the gate grows a 'newer minor ⇒ refuse writes' arm)" — the arm exists at src/transaction/recovery.rs:409-411 (shipped with the I29 write-gate). +*Why:* Same rot as the README claim, but on the constant every format change starts from; a developer bumping MINOR would wrongly believe they must also build the gate. +*Fix:* Replace "deferred until the first 1.1 release" with a pointer to the implemented gate in transaction/recovery.rs. + +**`NIT`** — `src/page_cache.rs:187` +*What:* `PageCache::new` does `io.page_count().unwrap_or(0)` and the doc comment (lines 173-175) explains a tradeoff: "we'd rather construct a usable cache and surface the underlying I/O error on the next real operation than fail the constructor". But post-I51/I123, `PageIo::page_count` is a pure Cell read that always returns `Ok` (page_io.rs:521-523). +*Why:* The comment documents a fallback behavior that is dead code — there is no I/O error to defer. A reader auditing error paths wastes time reasoning about an unreachable branch. +*Fix:* Change to `io.page_count().expect("infallible post-I51")` or have page_count stop returning Result; delete the tradeoff comment. + +**`NIT`** — `src/transaction/mod.rs:121` +*What:* The `Savepoint.freed_pages` doc says it is tracked "so a future freemap reclamation pass (R2) can restore freed-but-not-yet-reclaimed pages if a savepoint is rolled back to". R2 has landed, and rollback_to does not restore anything from the record — the field's actual live roles are scoping frees for rollback_to/release and the I27 commit-time flatten (commit.rs:35-50). +*Why:* A stale forward-reference to a shipped feature describes a mechanism that never materialized in that form, misleading a reader auditing savepoint/freemap interactions — the most delicate area of this module. +*Fix:* Rewrite the sentence to describe the current contract: frees are moved into the savepoint record so rollback_to drops only post-savepoint frees, and commit flattens all records back into txn_freed_pages (I27). + +**`NIT`** — `src/lib.rs:349` +*What:* `Chisel::open`'s `# Errors` section omits `UnsupportedPageSize`, which error.rs:152-158 documents as 'Raised at open time when the superblock's page_size field does not match PAGE_SIZE'. The type-level doc (lines 315-316) explicitly promises that constructor errors 'are fully enumerated in place'. +*Why:* The one method that claims full error enumeration is missing an open-time variant, so a caller matching open() errors from the docs will not handle it. +*Fix:* Add UnsupportedPageSize to the reopen-parsing error list in open()'s # Errors section. + +**`NIT`** — `python/src/lib.rs:14` +*What:* Header claims 'Registration order matters: errors must be registered before db so that to_py_err can reach the exception types (they are module-level Python objects, resolved by string lookup at call time)'. create_exception! types are lazily-initialized statics, not resolved via module string lookup; the only genuine ordering dependency is the IO_ERROR_CLASS OnceLock, and even that cannot be observed out of order since no user code runs mid-init (errors.rs:352–358 says the fallback is unreachable). +*Why:* The mechanism described is wrong, and the claim that order 'matters' contradicts to_py_err's own comment that the ordering hazard cannot happen. Misleading for anyone reordering module init. +*Fix:* Say the order is conventional, with the one real (init-time-only) dependency being IO_ERROR_CLASS population in errors::register. + +**`NIT`** — `src/lib.rs:1` +*What:* The crate contains zero rustdoc code examples: no ``` fence appears anywhere under src/, so `cargo test --doc` runs nothing. The entire public surface (Chisel::open, begin/allocate/commit, encryption_key, add_key/rotate_key) is documented as prose only. +*Why:* For a library with a curated re-export surface and an explicit API-stability contract (lib.rs:29–39), compile-tested examples are the only doc form that cannot rot; prose signatures and Options descriptions can silently drift from the code. +*Fix:* Add a doctest to the Chisel type or lib.rs module docs covering open → begin → allocate → commit → read (tempfile in a hidden setup line), and one for the encryption_key builder. + +**`NIT`** — `ARCHITECTURE.md:82` +*What:* In the layer-model mermaid graph every edge follows dependency --> dependent (e.g. `lru --> page_cache` = page_cache depends on lru), but the edge `page_cache --> spillway` is inverted: spillway.rs imports only error/page/crypto, while page_cache.rs:132 owns the `Option` field — i.e. page_cache depends on spillway, not vice versa. +*Why:* The doc explicitly sells the graph as "strict bottom-up dependency graph ... read the codebase in dependency order and you never have to forward-reference"; this edge sends a reader to spillway.rs expecting a page_cache dependency that isn't there. +*Fix:* Flip the edge to `spillway --> page_cache` (optionally add the missing `crypto --> spillway` / `page --> spillway` edges). + +### cargo-hygiene + +**`NIT`** — `src/transaction/recovery.rs:602` +*What:* `build_create_cipher` hardcodes `stride: 8232` in the CryptoHeader literal, while every other site in this file uses `crate::crypto::ENC_PAGE_SIZE` (recovery.rs:67, 98). +*Why:* If ENC_PAGE_SIZE ever changed, the header's self-reported stride — which open_existing trusts to set the IO stride (recovery.rs:256) — would silently diverge from the layout the pages were actually written at. +*Fix:* Use `crate::crypto::ENC_PAGE_SIZE as u32` for the stride field. + +### idiomaticity + +**`SMELL`** — `src/transaction/recovery.rs:625` +*What:* unwrap_first_matching_slot is a line-for-line duplicate of CryptoHeader::unlock (unlock's own doc at crypto_header.rs:186 admits it is "byte-identical to the inline trial in recovery.rs"), differing only in kdf_id matching style (literal 1/2 vs KdfId casts) and in discarding the slot index. +*Why:* Two hand-maintained copies of the security-critical key-trial loop can drift — a future fix to one (e.g. the Argon2 param clamp above, or a new kdf_id) must be applied twice or the open path and the key-management path diverge in which slots they accept. +*Fix:* Delete unwrap_first_matching_slot and have recovery call header.unlock(k), ignoring the returned index. + +**`SMELL`** — `python/src/db.rs:222` +*What:* open()'s encryption_key coercion (lines 222–240) is a byte-for-byte inline duplicate of the py_key helper (lines 63–77), differing only in the error-message prefix. py_key's own doc comment (lines 59–62) claims the factoring exists so 'the binding has one key vocabulary' and that 'duplicating the coercion inline would be both verbose and a maintenance hazard.' +*Why:* The stated invariant is already violated by the file that states it. If key coercion ever changes (e.g., accepting bytearray, normalizing str), open() and the rotation methods will drift apart silently — exactly the hazard the comment predicts. +*Fix:* Have open() call py_key(&obj.bind(py)) and remap the TypeError message (or parameterize py_key with the argument name). + +### security + +**`BUG`** — `src/transaction/recovery.rs:258` +*What:* open_existing takes the crypto-header's stride field verbatim (`sb.encryption.map(|h| h.stride as usize)`) and calls `cache.io_mut().set_stride(stride)` with no validation. `PageIo::set_stride` (src/page_io.rs:226) computes `len / stride as u64`. The header comment in src/superblock/crypto_header.rs:11 claims stride is "validated by the engine"; no such check exists anywhere (only the two hardcoded ENC_PAGE_SIZE fallback sites are safe). +*Why:* stride is a plaintext field protected only by the forgeable XXH3 page checksum — the codebase itself established this trust boundary when it bounds-checked ct_len in decrypt_body for exactly this reason. A forged stride of 0 is a guaranteed division-by-zero panic at open (DoS, violates the poison-not-panic error model); a huge stride drives multi-GiB read-buffer allocations. Also a first-class comment-vs-code mismatch. +*Fix:* Before set_stride, require header.stride == ENC_PAGE_SIZE as u32 (the only value ever written) and treat a mismatch like an unsupported format / corrupt slot; that also makes the crypto_header.rs comment true. + +**`DESIGN`** — `src/superblock/crypto_header.rs:209` +*What:* As stated, except the twin lives at src/transaction/recovery.rs:625-653 (derive_kek call at line 637), not src/superblock/recovery.rs. Everything else — params-before-auth ordering, u32::MAX m_cost accepted by argon2 0.5.3, re-stampable XXH3 checksum — checks out. Fix is a small read-time ceiling on the slot's Argon2 params (or clamp in derive_kek) before deriving. +*Why:* An attacker who edits a slot's m_cost/t_cost/p_cost and re-stamps the non-cryptographic XXH3 checksum turns every subsequent open into an OOM/allocation-abort (or minutes of grinding). Unauthenticated-KDF-param DoS is inherent to the construction, but an unbounded cost parameter converts "corrupt file fails to open" into "process dies". +*Fix:* Clamp attacker-controlled params with sanity ceilings before deriving (e.g. m_cost <= a few GiB, t_cost/p_cost small maxima); reject out-of-range slots the same way as an unknown kdf_id (skip/continue), so a tampered slot degrades to InvalidEncryptionKey. + +**`DESIGN`** — `src/crypto/mod.rs:321` +*What:* Accurate as written; one refinement: 're-authenticates forever' holds until a (currently deferred) full DEK rotation/re-encryption — KEK rotation does not invalidate old sealed images. +*Why:* An attacker with file access can splice stale versions of individual pages into a current database, producing a mixed state that never existed at any commit (stale freemap or tree pages that pass AEAD and then corrupt structure silently). The design spec's documented non-goal covers only substitution of a "wholly older, validly-signed database image"; per-page temporal splicing is strictly stronger and is not covered by the spec's "cryptographic tamper-detection" / anti-relocation claims. +*Fix:* At minimum, extend the spec §9 boundary to name per-page replay explicitly. If it should be defended, bind pages to a commit epoch in the AAD (costs rewriting reachable pages on epoch bump) or hash-chain page tags into the sealed superblock body. + +**`SMELL`** — `src/superblock/crypto_header.rs:146` +*What:* CryptoHeader::deserialize treats ANY nonzero algorithm byte as an encrypted DB and the engine never compares it against ALGO_XCHACHA20POLY1305 anywhere (the constant is exported but unreferenced by validation code). algorithm is also outside both the slot AAD and the body AAD. +*Why:* A file stamped with an unknown algorithm (future format, or a flipped byte with re-stamped checksum) is silently processed as XChaCha20-Poly1305 and fails with the misleading InvalidEncryptionKey ("wrong passphrase") instead of an unsupported-algorithm error. Explicit algorithm validation is the standard defense that keeps a future algorithm-2 file from being misinterpreted by this binary. +*Fix:* In the open path (or deserialize), reject algorithm != ALGO_XCHACHA20POLY1305 with a distinct unsupported-format error instead of proceeding. + +## Open questions + +- Is the persistent max_pages+1 residency after cold loads (finding 1) an accepted design point? The load_page comment hints one-over is tolerated "temporarily", but nothing trims it back until the next miss, and no test pins the read-path cap. +- spill() inserts into `slots` before write_slot; a write_slot I/O error leaves the page dual-resident (restored dirty in cache AND mapped to a garbage spillway slot). This is only safe because IoError is fatal and poisons before the next drain — confirmed by error.rs, but worth a comment or a slots.remove on the error path if the poison model is ever weakened. +- rollback_inner sets active_txn=false only after the two fallible recover_depth re-derivations (lifecycle.rs:276-292); if those could ever return an operational (non-fatal) error such as CacheFull, the manager would be left half-rolled-back (roots restored, packer/freemap state not yet reset, active_txn still true) without being poisoned. After discard_all_dirty the cache is nearly all evictable so this looks unreachable in practice, but whether Options enforces a cache-cap floor that guarantees it is not resolvable from the transaction module alone. +- Is the missing stride/algorithm validation a later-phase task that the crypto_header.rs comment ("validated by the engine") anticipates, or was it believed already implemented? The comment and the code currently disagree. +- Is per-page temporal replay intended to be covered by the spec's "wholly older database image" non-goal, or does the team consider AEAD's tamper-detection claim to include stale-page splicing? Code alone cannot tell whether this boundary was consciously accepted at page granularity. +- Are corrupt-but-checksummed pages formally in the supported threat model for handle_table/membership descent (they are for freemap_tree/overflow/data_page, per comments and tests)? If the asymmetry is a deliberate scope cut, the type-validation DESIGN finding downgrades to a doc gap. +- Does Chisel support 32-bit targets? freemap.rs bit_position (`page_id as usize`) and overflow.rs (`total_length ... as usize`) silently truncate on 32-bit, which for mark_free would set a wrong bitmap bit (freeing a live page); on 64-bit both are sound. +- The u64::MAX insert runaway is unreachable via engine-assigned monotonic handles — is the RadixU64/HandleTable insert API considered internal-only forever, or might a future feature (user-chosen keys) expose it? +- Is LockFailed's fatal classification a deliberate conservative choice (e.g. to force callers to treat any open-time refusal as stop-the-world), or leftover from before the Fatal/Operational contract was documented? +- Is Stats::file_size_bytes intended as logical bytes (count x 8192) or physical on-disk bytes? For encrypted DBs (stride 8232) the two diverge and the docs claim the physical meaning while the code computes the logical one. +- Can Spillway::max_bytes ever be 0 on a live spillway via set_spillway_max_bytes(0) after a spillway was lazily created (which would make the SpillwayFull{limit_bytes:0} Display arm reachable after all)? The page_cache disabled-guard appears to route that case to CacheFull, but the resize/lazy-construction interleaving was not fully traced. +- Is multi-threaded Python embedding actually a supported use case (test_threading.py asserts handle migration, and the Mutex exists for it), or does the single-client design philosophy make the GIL-held Argon2id/fsync finding moot? If multiple threads are out of scope, finding 1 drops to a documentation issue. +- Is the stateless-wrapper aliasing of finished Transaction objects (finding 2) a deliberate accepted tradeoff recorded somewhere (ADR/ISSUES.md) beyond the design note in transaction.rs, or was the I22/I24 guard simply never extended to data ops? +- Is the missing encrypted+on-disk-sidecar spillway integration test deliberately deferred to a later encryption phase (the page_cache.rs comments reference 'Task 3.3/3.4'), or an oversight? +- Is there a planned crash-window test for add_key/rotate_key (torn superblock write during rewrite_crypto_header, recovering via the sibling slot)? Rotation is a single-superblock write, so a torn-rotate recovery test would mirror torn_slot_0_encrypted_db_recovers_via_sibling but for the key path. +- Are cross-engine strict comparisons meant to be authoritative on macOS dev machines, or only on Linux CI? That decides whether the redb F_BARRIERFSYNC gap needs a code/doc fix or just a summary footnote. +- Is the micro-grid's 1000-per-tx row-skip set (TX_BUDGET_BYTES + the two unregistered rows) intentionally retained pending a spec revision, or simply stale from before the spillway landed? The bench spec ('270 cells', '9 rows') isn't in the repo files reviewed, so intent can't be resolved from code alone. +- The unsafe u64->Handle slice cast at bench/src/chisel_engine.rs:99 verified sound (Handle is #[repr(transparent)] over plain u64 with size/align static asserts in src/handle.rs:25-41 that name the bench transmute) — no finding, noted here so it isn't re-flagged. +- Does ISSUES.md's I29 entry record the write-gate as shipped? README.md:363 and src/page.rs:107 both still describe it as future work, so it's unclear which document was treated as the source of truth during the #86 doc refresh. +- Has the sdist ever been verified to vendor the parent path-dep crate (chisel = { path = ".." })? No CI step or release note confirms an sdist install works. +- bench.yml's 45-minute timeout must cover two fat-LTO release builds plus two ~10-25 min scenario runs; CI is currently green, but the headroom on a slow ubuntu-latest runner is unknown from the code alone. + +## What I didn't review + +- **`src/transaction/tests.rs`, `src/recovery_tests.rs` internals** beyond spot-checks — read for coverage gaps, not line-by-line correctness of every assertion. +- **`bench/src/summary/` renderers** — skimmed; the review focused on measurement *validity* (durability parity, black_box) over report formatting. +- **`.codebase-memory/adr.md`** — the ADR graph is MCP-only by decision (I129); used as rationale context, not audited. +- **Third-party crate internals** (RustCrypto, pyo3, redb, rusqlite) — trusted as dependencies; only Chisel's *use* of them was reviewed.