From f5ccd2cc82fac5aa7ede2650bf0cfe0ea982b054 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Tue, 23 Jun 2026 11:40:29 -0700 Subject: [PATCH] =?UTF-8?q?docs:=20commenting=20pass=20=E2=80=94=205=20WHY?= =?UTF-8?q?-comments=20on=20invariants=20the=20recent=20work=20left=20impl?= =?UTF-8?q?icit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A LEAVE-default pass over the engine (already heavily commented by three prior passes). Five high-value additions where a tradeoff/invariant lived only in a test or a callee: - page_cache.rs: dirty_count counts ONLY in-cache dirty entries, never spilled pages — that is what makes the dirty_count==entries.len() eviction short-circuit sound. - overflow.rs: the wrong-type + zero-length guards MUST precede the Vec::with_capacity, so an untrusted total_length can't drive a speculative giant allocation. - membership_index.rs: pack the POST-insert inner.depth, not the stale pre-insert depth (invisible at depth 0; pinned by inner_grow_roundtrip). - transaction/fault.rs: fail_next_handle_table_op is consumed in handle_table_insert_candidate, which both allocate_inner and update_inner route through. - python/errors.rs: add TagMismatchError to the exception-hierarchy doc. --- python/src/errors.rs | 2 ++ src/membership_index.rs | 4 ++++ src/overflow.rs | 5 +++++ src/page_cache.rs | 10 ++++++++++ src/transaction/fault.rs | 4 +++- 5 files changed, 24 insertions(+), 1 deletion(-) diff --git a/python/src/errors.rs b/python/src/errors.rs index 4d6136a..72c4eac 100644 --- a/python/src/errors.rs +++ b/python/src/errors.rs @@ -37,6 +37,8 @@ // TransactionInProgressError // ClosedError (I25: db.close() raced a live txn/sp) // AlreadyFinishedError (I22/I24: double-drive a finished txn/sp) +// TagMismatchError (delete_tagged supplied a tag that does +// not match the handle's stored tag) // FatalError (drop-and-reopen recovery only) // IoError (ALSO subclasses builtin OSError — see register) // ChecksumMismatchError diff --git a/src/membership_index.rs b/src/membership_index.rs index d7a1c7e..429f577 100644 --- a/src/membership_index.rs +++ b/src/membership_index.rs @@ -638,6 +638,10 @@ impl MembershipIndex { inner_root = inner.create_root(cache, alloc)?; } let new_inner_root = inner.insert(cache, inner_root, handle, 1, alloc, freed)?; + // Pack the POST-insert `inner.depth`, not the stale `inner_depth` from + // unpack_inner: this insert may have grown the inner tree, bumping its + // depth. Persisting the old depth would make later readers descend the + // wrong number of levels (invisible at depth 0; see inner_grow_roundtrip). let packed = pack_inner(new_inner_root, inner.depth); let new_outer_root = outer.insert(cache, root, tag as u64, packed, alloc, freed)?; self.outer_depth = outer.depth; diff --git a/src/overflow.rs b/src/overflow.rs index 3890e8d..3c372ca 100644 --- a/src/overflow.rs +++ b/src/overflow.rs @@ -175,6 +175,11 @@ impl Overflow { }); } let max_pages = total_length.div_ceil(OVERFLOW_PAYLOAD); + // Ordering matters: the wrong-type and zero-length guards above run + // BEFORE this allocation, so an untrusted `total_length` (e.g. a + // stale handle pointing at a non-overflow page whose bytes 16..24 + // read as u64::MAX) can never drive a speculative giant allocation + // here. The per-page loop guard alone would be too late. let mut result = Vec::with_capacity(total_length); let mut current_page = first_page; diff --git a/src/page_cache.rs b/src/page_cache.rs index a44c75a..8287973 100644 --- a/src/page_cache.rs +++ b/src/page_cache.rs @@ -85,6 +85,16 @@ pub struct PageCache { /// allocation in a write-heavy transaction (where all pages are /// dirty and no victim exists), trivially making page-allocation /// O(n) per call. With this counter the early-out is O(1). + /// + /// INVARIANT: this counts ONLY dirty entries currently resident in + /// `entries`, never pages that have been spilled to the spillway. + /// That is exactly why the `dirty_count == entries.len()` + /// short-circuit is sound — both sides measure the in-cache set. A + /// Phase-B spill decrements this as it removes the victim from + /// `entries` (and re-increments on a failed spill that restores it), + /// so `dirty_count <= entries.len()` always holds; `forget_above` + /// (truncate's spillway prune) deliberately does NOT touch it, + /// because spilled pages were never counted here. dirty_count: usize, /// I52 (ISSUES.md, 2026-05-22): reusable scratch buffer for /// `flush()` and `discard_all_dirty()`. Both functions iterate diff --git a/src/transaction/fault.rs b/src/transaction/fault.rs index 5eec3b7..1e1f795 100644 --- a/src/transaction/fault.rs +++ b/src/transaction/fault.rs @@ -10,7 +10,9 @@ //! - `fail_next_handle_table_op`: companion for the FORWARD step — the next //! `allocate_inner` handle-table insert returns a non-fatal `CacheFull`, exercising //! the prepare-abort/unwind path of the step carrying the eager depth bump -//! (HandleTable::grow). +//! (HandleTable::grow). Consumed inside `handle_table_insert_candidate`, which both +//! `allocate_inner` and `update_inner` route through, so either path's forward step +//! can be the one that trips it (the test that arms it controls which fires first). //! - `fail_next_update_value_write`: for `update_inner` — the next update returns a //! non-fatal `CacheFull` at the NEW-value-write step (the first fallible step with //! the fix; pre-fix it landed AFTER the old location was freed), proving the old