docs: correct ARCHITECTURE.md and the docs it contradicts (#98)

README.md

@@ -126,7 +126,7 @@ The macOS APFS runtime gap is real: Chisel uses `fcntl(F_FULLFSYNC)` (durable th
 
 ### Handles
 
-A handle is a stable `u64` returned by `allocate()`. It maps through a radix-tree **handle table** rooted in the superblock to a `(page, slot)` location in a slotted data page. This indirection means values can move internally — during `update()` to a larger value, or during `defrag()` — without changing the handle. Deleted handles are retired and never reused within a database's lifetime.
+A handle is a stable `u64` returned by `allocate()`. It maps through a radix-tree **handle table** rooted in the superblock to a `(page, slot)` location in a slotted data page. This indirection means values can move internally — during `update()` to a larger value, or during `defrag()` — without changing the handle. A handle that reached a commit is retired on delete and never reused. One minted by a transaction that rolls back (or is lost to crash recovery) *is* re-minted, because the counter rewinds with the rest of the roots snapshot — so commit before recording a handle outside the database.
 
 ### Transactions
 
@@ -180,13 +180,16 @@ Names are bounded in length and must be valid UTF-8 without embedded NUL; the ta
 `defrag()` consolidates sparse data pages: it re-inserts values from pages whose live-slot count falls below a threshold so those pages become fully free and can be reclaimed. It runs inside an active transaction so it composes with other work and commits atomically.
 
 ```rust
-use chisel::defrag::DefragOptions;
+use chisel::DefragOptions;
 
 db.begin()?;
-let stats = db.defrag(DefragOptions {
-    sparse_threshold: 0.25,
-    max_pages: 0,  // 0 = no cap on values relocated
-})?;
+// DefragOptions is #[non_exhaustive]; build it with the chained setters
+// rather than a struct literal.
+let stats = db.defrag(
+    DefragOptions::default()
+        .sparse_threshold(0.25)  // relocate pages under 25% full, per page
+        .max_values(0),          // 0 = no cap on values relocated
+)?;
 db.commit()?;
 ```
 
@@ -330,20 +333,26 @@ let options = Options {
 
 **Fatal errors** — storage integrity is in question. Drop the handle and reopen.
 
-`IoError`, `ChecksumMismatch`, `CorruptSuperblock`, `FileSizeMismatch`, `LockFailed`, `UnsupportedFormatVersion`, `UnsupportedPageSize`, `CorruptPage`, `InvalidPageId`, `DecryptionFailed`, `Poisoned`.
+`IoError`, `ChecksumMismatch`, `CorruptSuperblock`, `FileSizeMismatch`, `LockFailed`, `UnsupportedFormatVersion`, `UnsupportedPageSize`, `CorruptPage`, `InvalidPageId`, `DecryptionFailed`.
 
 `DecryptionFailed { page_id }` is fatal: an AEAD authentication failure while decrypting an already-read page means the ciphertext or session key can no longer be trusted, so it poisons the handle exactly like `ChecksumMismatch` (see the poison model below). It is distinct from the operational `InvalidEncryptionKey`, which fires at open time when the supplied key unwraps no key slot — before any data page is served.
 
 Use `ChiselError::is_fatal()` to classify at runtime.
 
+**`Poisoned` is in neither tier**, and `is_fatal()` returns `false` for it. That is deliberate: `is_fatal()` answers "should this error poison the handle?", and by the time you see `Poisoned` the handle already is — re-poisoning is meaningless. But it also means `Poisoned` is *not* recoverable-and-continue either: it is terminal, and the only response is the same drop-and-reopen a fatal error calls for. Match it explicitly alongside `is_fatal()`, as the snippet below does; routing it into an "operational, keep going" arm produces a loop where every call returns `Poisoned` forever.
+
 ### Poison model
 
 On any fatal error — including a failed commit-protocol fsync — the `Chisel` handle becomes **poisoned**. Every subsequent call returns `ChiselError::Poisoned`, regardless of whether it is a read or a write. The only legal recovery is to drop the handle and call `Chisel::open` again; the shadow-paging recovery path then restores the database to the last durable state.
 
+`commit` is the one method where the operational/fatal split above does not apply. Its only non-poisoning error is `NoActiveTransaction`, checked before the protocol starts. Once `cache.flush()` has run the manager is in a partial-commit state, so *every* error it can then return poisons the handle — including `CacheFull` and `SpillwayFull`, which are operational anywhere else. Do not catch those from `commit` and call `rollback` to recover: the handle is already poisoned and `rollback` will tell you so.
+
 ```rust
 match db.commit() {
     Ok(()) => (),
-    Err(e) if e.is_fatal() => {
+    // `Poisoned` is matched explicitly: is_fatal() is false for it, so it
+    // would otherwise fall into the operational arm and loop forever.
+    Err(e) if e.is_fatal() || matches!(e, ChiselError::Poisoned) => {
         drop(db);
         db = Chisel::open(path, Options::default())?;
         // Chisel is now at its last-committed state; retry the work if needed.

src/defrag.rs

@@ -136,8 +136,12 @@ pub struct DefragStats {
 ///   1. Short-circuit if the handle-table root is empty — nothing to do.
 ///   2. Compute the set of SPARSE data pages up front via
 ///      `txm.sparse_data_pages(sparse_threshold)`. A page is sparse if
-///      its live-slot count is at or below `threshold × max_observed`.
-///      Dense pages are left alone.
+///      `live_slots / stored_slots < sparse_threshold` — a per-page density
+///      against the page's OWN stored-slot count, not against the densest
+///      page in the database. (The older relative-to-densest rule was
+///      abandoned because a lone sparse page scores 1.0 against itself and
+///      was never collected; see `sparse_data_pages`.) Dense pages are
+///      left alone.
 ///   3. Snapshot the initial set of data-page IDs (not a count) so we can
 ///      report `pages_freed` at the end as the set difference against the
 ///      final set — a net count delta is the wrong metric here, see the

src/handle_table.rs

@@ -45,10 +45,17 @@
 //        swap in phase 2 — this is what makes shadow paging crash-safe.
 //
 // Handle allocation policy (enforced by transaction.rs, not this module):
-// handles are monotonic from `next_handle` and never reused, starting at 1
-// (handle 0 is reserved as the "no handle" sentinel). Delete writes
-// a tombstone entry (HandleFlags::Deleted) in place; the leaf slot is not
-// freed. This keeps handles stable forever but means the tree only grows.
+// handles are monotonic from `next_handle`, starting at 1 (handle 0 is
+// reserved as the "no handle" sentinel), and are never reused ONCE
+// COMMITTED. Delete writes a tombstone entry (HandleFlags::Deleted) in
+// place; the leaf slot is not freed. This keeps committed handles stable
+// forever but means the tree only grows.
+//
+// The "once committed" qualifier is not pedantry: `next_handle` is a field
+// of `Roots`, so rollback, rollback_to and crash recovery all rewind it
+// along with everything else, and an id minted by a transaction that never
+// commits is handed out again. HandleEntry has no generation field, so the
+// re-minted id is indistinguishable from the original.
 
 use crate::error::{ChiselError, Result};
 use crate::page::{
@@ -82,10 +89,17 @@ const MAX_DEPTH: u32 = 6;
 
 // Page flags byte (buf[1]): distinguishes leaf from interior. Stored in the
 // page header so `open_existing` can walk the tree to recover depth without
-// needing the depth to be persisted separately in the superblock. Currently
-// forensic-only — no live code reads it (the depth walk uses child-pointer
-// presence); kept because a hex-dump reader can tell leaf from interior at
-// a glance.
+// needing the depth to be persisted separately in the superblock.
+//
+// This byte is READ AT RUNTIME and is load-bearing: `recover_depth` below
+// uses `buf[1] != FLAG_INTERIOR` as the primary terminator of its left-spine
+// walk, with the zero-child check only as the secondary terminator. The walk
+// runs on every open and every rollback, and its result is the descent depth
+// for all subsequent lookups — so stop writing the flag in `grow`, or
+// repurpose byte 1 for something else, and `recover_depth` returns 0 for a
+// depth-N tree: every committed handle mis-descends and `lookup` reports
+// Ok(None) for live data, with no checksum or type-tag error to signal it.
+// (It is also legible in a hex dump, which is a bonus, not the reason.)
 //
 // NOTE on per-page version byte (I31): handle-table pages keep the flag at
 // byte 1 and put the I31 per-page format version at byte 2. Every other
@@ -100,8 +114,10 @@ pub(crate) const FLAG_INTERIOR: u8 = 0x02;
 
 /// Per-entry state tag. `Deleted` functions as a tombstone — the slot stays
 /// allocated in the leaf, so the corresponding handle value is permanently
-/// burned (never reused). `Overflow` signals that `page_id` points to the
-/// first page of an overflow chain rather than a data page slot.
+/// burned and never reused (see the module header for why that guarantee
+/// covers committed handles only). `Overflow` signals that `page_id` points
+/// to the first page of an overflow chain rather than a data page slot; such
+/// an entry owns no data-page slot at all, and `slot_index` is an unused 0.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum HandleFlags {
     Live,

src/lib.rs

@@ -572,10 +572,21 @@ impl Chisel {
     /// "two_fsync" label from the original spec.
     ///
     /// # Errors
-    /// `NoActiveTransaction` if none is open. Operationally, `CacheFull` or
-    /// `SpillwayFull` if the transaction's working set exceeds the cache /
-    /// spillway caps. A failure inside the fsync/superblock protocol is fatal
-    /// and poisons the handle — the previous committed state stays intact.
+    /// `NoActiveTransaction` if none is open — and that is the ONLY error this
+    /// method can return without poisoning the handle. It is checked before
+    /// the commit protocol starts; everything after that point poisons.
+    ///
+    /// This inverts the crate-wide operational/fatal contract described on
+    /// [`Chisel`], and it does so deliberately. Once `cache.flush()` has run,
+    /// the manager is in a partial-commit state, so even an ordinarily
+    /// operational error — `CacheFull` or `SpillwayFull` from a working set
+    /// that exceeds the caps — leaves nothing safe to continue from.
+    /// `commit` therefore poisons on *any* error it can reach.
+    ///
+    /// Practically: do not catch `CacheFull` from `commit` and call
+    /// `rollback` to recover, the way the operational contract would suggest.
+    /// The handle is already poisoned and `rollback` will return `Poisoned`.
+    /// Drop the handle and reopen; the previous committed state is intact.
     pub fn commit(&mut self) -> Result<()> {
         self.txm.commit()
     }
@@ -625,10 +636,18 @@ impl Chisel {
     }
 
     /// Store `value` and return a freshly minted stable handle. Handles are
-    /// u64 identifiers assigned from a monotonic counter in the superblock;
-    /// they are never reused within a database's lifetime and are stable
-    /// across updates, defrag, and reopens. Physical location may change;
-    /// the handle will not.
+    /// u64 identifiers assigned from a monotonic counter in the superblock,
+    /// and are stable across updates, defrag, and reopens. Physical location
+    /// may change; the handle will not.
+    ///
+    /// A handle is never reused **once the transaction that minted it has
+    /// committed**, including after the value is deleted. Before that, it can
+    /// be: the counter lives in the roots snapshot, so `rollback`,
+    /// `rollback_to` and crash recovery all rewind it, and the next
+    /// `allocate` hands the same id out again for different bytes. Nothing in
+    /// the entry distinguishes the two — there is no generation counter — so
+    /// only commit the transaction before recording a handle anywhere outside
+    /// the database (a log, an external index, another process).
     ///
     /// Values up to `transaction::MAX_INLINE_VALUE` are packed into a slot
     /// on a data page (R1 packing — multiple values share a page); larger

tests/transactions.rs

@@ -141,3 +141,56 @@ dual_backing_test!(test_nested_savepoints, test_nested_savepoints_body);
 // under the I35 pub→pub(crate) reshape. The Chisel-public-API equivalent
 // (open path + commit + reopen + read) is covered by `test_chisel_reopen`
 // in tests/basic_ops.rs.
+
+// A handle's "never reused" guarantee covers COMMITTED handles only. The
+// counter that mints them (`next_handle`) lives in the roots snapshot, so
+// every rewind path restores it and the id is handed out again — for
+// different bytes, with nothing in the entry to distinguish the two.
+//
+// This is correct behaviour (a rolled-back allocate never happened), but it
+// is the exact thing ARCHITECTURE.md's "Handle stability" section and
+// `Chisel::allocate`'s rustdoc now warn callers about, so it is pinned here.
+// The crash-recovery arm of the same property is pinned by
+// `src/recovery_tests.rs`; this covers the two in-process rewind paths.
+fn test_uncommitted_handle_is_reminted_body(b: &Backing) {
+    let mut db = open_chisel(b);
+
+    // Rewind path 1: rollback.
+    db.begin().unwrap();
+    let h = db.allocate(b"rolled-back").unwrap();
+    db.rollback().unwrap();
+
+    db.begin().unwrap();
+    let h2 = db.allocate(b"different-bytes").unwrap();
+    db.commit().unwrap();
+
+    assert_eq!(
+        h, h2,
+        "next_handle rewinds with the roots snapshot, so a rolled-back id is re-minted"
+    );
+    assert_eq!(
+        db.read(h).unwrap(),
+        b"different-bytes",
+        "the re-minted handle resolves to the NEW value — a caller holding the old \
+         id reads unrelated data with no error"
+    );
+
+    // Rewind path 2: rollback_to a savepoint.
+    db.begin().unwrap();
+    db.savepoint("sp").unwrap();
+    let h3 = db.allocate(b"inside-savepoint").unwrap();
+    db.rollback_to("sp").unwrap();
+    let h4 = db.allocate(b"after-rollback-to").unwrap();
+    db.commit().unwrap();
+
+    assert_eq!(
+        h3, h4,
+        "rollback_to restores the savepoint's roots, next_handle included"
+    );
+    assert_eq!(db.read(h3).unwrap(), b"after-rollback-to");
+}
+
+dual_backing_test!(
+    test_uncommitted_handle_is_reminted,
+    test_uncommitted_handle_is_reminted_body
+);