fix: bound txn_counter on the write side too (review follow-up to #151)

src/error.rs

@@ -176,6 +176,23 @@ pub enum ChiselError {
         stored: u32,
         max: u32,
     },
+    // Raised when a commit (or a crypto-header rewrite) would push
+    // `txn_counter` past `MAX_TXN_COUNTER`, the bound `Superblock::validate`
+    // enforces on the read side.
+    //
+    // Fatal, and refused BEFORE any commit I/O, because the alternative is
+    // writing a superblock this same binary would refuse to read: the database
+    // would either lose an acknowledged commit on the next open (silent
+    // fallback to an older valid slot) or become permanently unopenable once
+    // every slot has been written past the bound.
+    //
+    // Legitimately unreachable — a file that opens has 2^32 increments of
+    // headroom, and a reopen re-validates. It IS reachable adversarially, on
+    // the first commit after opening a file forged at exactly MAX_TXN_COUNTER,
+    // which is precisely the case this exists to make clean.
+    TxnCounterExhausted {
+        current: u64,
+    },
 
     // Operational — the caller supplied the wrong key material or none, or
     // asked an unencrypted-only build to open an encrypted DB. The on-disk
@@ -241,6 +258,7 @@ impl ChiselError {
                 | ChiselError::InvalidPageId { .. }
                 | ChiselError::UnsupportedPageSize { .. }
                 | ChiselError::InvalidFreemapDepth { .. }
+                | ChiselError::TxnCounterExhausted { .. }
                 | ChiselError::DecryptionFailed { .. }
         )
     }
@@ -331,6 +349,10 @@ impl fmt::Display for ChiselError {
                 f,
                 "page size mismatch: file was written with {stored}-byte pages, this build uses {compiled}-byte pages"
             ),
+            ChiselError::TxnCounterExhausted { current } => write!(
+                f,
+                "txn_counter {current} is at the maximum this format supports; the database can accept no further commits"
+            ),
             ChiselError::InvalidFreemapDepth { stored, max } => write!(
                 f,
                 "superblock declares freemap depth {stored}, which exceeds the maximum {max} (corrupt or forged superblock)"
@@ -594,6 +616,7 @@ mod tests {
                 | ChiselError::InvalidPageId { .. }
                 | ChiselError::UnsupportedPageSize { .. }
                 | ChiselError::InvalidFreemapDepth { .. }
+                | ChiselError::TxnCounterExhausted { .. }
                 | ChiselError::DecryptionFailed { .. } => true,
             }
         }
@@ -640,6 +663,7 @@ mod tests {
                 compiled: 0,
             },
             ChiselError::InvalidFreemapDepth { stored: 0, max: 0 },
+            ChiselError::TxnCounterExhausted { current: 0 },
             ChiselError::NoEncryptionKey,
             ChiselError::InvalidEncryptionKey,
             ChiselError::EncryptionNotSupported,
@@ -654,13 +678,15 @@ mod tests {
                 "is_fatal() disagrees with the documented Fatal/Operational block for {e:?}"
             );
         }
-        // Tripwire: exactly 11 variants are fatal today. If this count moves, the
+        // Tripwire: exactly 12 variants are fatal today. If this count moves, the
         // Fatal/Operational split changed — confirm that was intentional (it is a
         // breaking change for callers doing error-class matching, per the header).
-        // Last moved 10 -> 11 by InvalidFreemapDepth, which is fatal because a
-        // superblock field outside its representable range cannot be resolved by
-        // retrying: every sibling slot carries the same value.
-        assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 11);
+        // Last moved 11 -> 12 by TxnCounterExhausted (a database that cannot
+        // accept another commit is terminal for the handle); before that,
+        // 10 -> 11 by InvalidFreemapDepth, fatal because a superblock field
+        // outside its representable range cannot be resolved by retrying —
+        // every sibling slot carries the same value.
+        assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 12);
     }
 
     // Phase 4: the three operational encryption errors are recoverable (the

src/recovery_tests.rs

@@ -2179,3 +2179,73 @@ fn a_forged_txn_counter_in_one_slot_loses_to_its_healthy_sibling() {
     db.allocate(b"and it still commits").unwrap();
     db.commit().unwrap();
 }
+
+#[test]
+fn a_database_forged_at_the_boundary_refuses_to_commit_rather_than_writing_past_it() {
+    // The read-side bound alone is only half an invariant, and the half it
+    // leaves open is worse than the hole it closed.
+    //
+    // MAX_TXN_COUNTER itself is VALID — the check is `>`, not `>=` — so a file
+    // forged at exactly the boundary opens cleanly, with `diagnose` silent.
+    // Without a matching write-side check, the commits that follow are
+    // acknowledged and fsynced at MAX+1, MAX+2 … producing superblocks this
+    // same binary refuses to read. One commit past the bound is silently
+    // discarded on the next open (selection falls back to an older valid slot);
+    // once every slot has been written past it, the database is permanently
+    // unopenable. Both outcomes are strictly worse than the original forged-
+    // u64::MAX panic, which at least failed loudly and immediately.
+    //
+    // So: refuse the commit, before any I/O, and leave the last durable state
+    // exactly as it was.
+    let file = NamedTempFile::new().unwrap();
+    let path = file.path().to_owned();
+    let handle;
+    {
+        let mut db = Chisel::open(&path, Default::default()).unwrap();
+        db.begin().unwrap();
+        handle = db.allocate(b"still here afterwards").unwrap();
+        db.commit().unwrap();
+    }
+
+    // Forge only the WINNING slot. Forging both would tie their counters, and
+    // `max_by_key` returns the last maximum — so selection would pick the
+    // create-seed slot and the database would come back empty for reasons that
+    // have nothing to do with what this test is about.
+    let newest = {
+        let mut f = fs::File::open(&path).unwrap();
+        let mut best = (0u64, 0u64);
+        for slot in 0..2u64 {
+            let mut buf = [0u8; PAGE_SIZE];
+            f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap();
+            f.read_exact(&mut buf).unwrap();
+            let sb = Superblock::deserialize(&buf).expect("slot must be valid");
+            if sb.txn_counter >= best.1 {
+                best = (slot, sb.txn_counter);
+            }
+        }
+        best.0
+    };
+    forge_txn_counter(&path, newest, crate::superblock::MAX_TXN_COUNTER);
+
+    // Opens: the boundary value is legal.
+    let mut db = Chisel::open(&path, Default::default())
+        .expect("MAX_TXN_COUNTER is inside the accepted range and must open");
+
+    db.begin().unwrap();
+    db.allocate(b"this commit cannot be represented").unwrap();
+    match db.commit() {
+        Err(ChiselError::TxnCounterExhausted { current }) => {
+            assert_eq!(current, crate::superblock::MAX_TXN_COUNTER);
+        }
+        Ok(()) => panic!("committing past MAX_TXN_COUNTER must be refused, not persisted"),
+        Err(other) => panic!("expected TxnCounterExhausted, got {other:?}"),
+    }
+    drop(db);
+
+    // The refusal wrote nothing: the database still opens and still holds the
+    // last durable state. This is the assertion that actually distinguishes
+    // "refused" from "wrote a superblock nobody can read".
+    let db = Chisel::open(&path, Default::default())
+        .expect("a refused commit must leave the file exactly as it was");
+    assert_eq!(db.read(handle).unwrap(), b"still here afterwards");
+}

src/superblock/mod.rs

@@ -76,6 +76,29 @@ pub const DEFAULT_SUPERBLOCK_COUNT: u32 = 2;
 pub const TXN_COUNTER_RESERVE: u64 = 1 << 32;
 pub const MAX_TXN_COUNTER: u64 = u64::MAX - TXN_COUNTER_RESERVE;
 
+/// The counter the next superblock write should carry, or a typed error if
+/// bumping would leave the reserved band.
+///
+/// This exists because the read-side bound in `validate` is only half an
+/// invariant. Bounding what we ACCEPT without bounding what we WRITE lets the
+/// engine persist a superblock it will later refuse to read: forge a slot at
+/// exactly `MAX_TXN_COUNTER` (which validates, and opens cleanly), and the next
+/// commits are acknowledged and fsynced at MAX+1, MAX+2 — after which every
+/// slot fails `validate` and the database is permanently unopenable. One commit
+/// past the bound is quieter and worse: the reopen silently falls back to an
+/// older valid slot, discarding an acknowledged commit.
+///
+/// So the rule is: never write a superblock we would not read back. Callers
+/// must consult this BEFORE doing any commit I/O, so a refusal costs nothing
+/// and leaves the last durable state untouched.
+pub fn next_txn_counter(current: u64) -> Result<u64, crate::error::ChiselError> {
+    let next = current.saturating_add(1);
+    if next > MAX_TXN_COUNTER {
+        return Err(crate::error::ChiselError::TxnCounterExhausted { current });
+    }
+    Ok(next)
+}
+
 // Byte offset of the superblock_count field within the serialized
 // superblock page. Placed AFTER the named-root table (which ends at
 // NAMED_ROOTS_END = 308). Deserialization rejects any value outside
@@ -805,6 +828,48 @@ mod tests {
             .copy_from_slice(&99u32.to_le_bytes());
         page::stamp_checksum(&mut bad_count);
         assert_eq!(validate(&bad_count), Err(SuperblockDefect::BadCount(99)));
+
+        // The bound is `>`, so MAX_TXN_COUNTER itself must be accepted. Pinning
+        // both sides matters here: a database forged at exactly the boundary is
+        // the case that opens cleanly and then exercises the write-side guard.
+        let mut at_bound = good;
+        at_bound[8..16].copy_from_slice(&MAX_TXN_COUNTER.to_le_bytes());
+        page::stamp_checksum(&mut at_bound);
+        assert_eq!(validate(&at_bound), Ok(()));
+
+        let mut over_bound = good;
+        over_bound[8..16].copy_from_slice(&(MAX_TXN_COUNTER + 1).to_le_bytes());
+        page::stamp_checksum(&mut over_bound);
+        assert_eq!(
+            validate(&over_bound),
+            Err(SuperblockDefect::BadTxnCounter(MAX_TXN_COUNTER + 1))
+        );
+    }
+
+    // The write side of the same invariant. Bounding what we ACCEPT without
+    // bounding what we WRITE lets the engine persist superblocks it will refuse
+    // to read back, which is worse than refusing the commit: one commit past the
+    // bound is silently discarded on the next open (fallback to an older valid
+    // slot), and two make the file permanently unopenable.
+    #[test]
+    fn next_txn_counter_refuses_to_leave_the_readable_range() {
+        assert_eq!(next_txn_counter(0).unwrap(), 1);
+        assert_eq!(
+            next_txn_counter(MAX_TXN_COUNTER - 1).unwrap(),
+            MAX_TXN_COUNTER
+        );
+        assert!(matches!(
+            next_txn_counter(MAX_TXN_COUNTER),
+            Err(crate::error::ChiselError::TxnCounterExhausted {
+                current
+            }) if current == MAX_TXN_COUNTER
+        ));
+        // Saturating, not wrapping: u64::MAX must not roll over to 0 and be
+        // mistaken for a fresh database by "highest counter wins".
+        assert!(matches!(
+            next_txn_counter(u64::MAX),
+            Err(crate::error::ChiselError::TxnCounterExhausted { .. })
+        ));
     }
 
     #[test]
@@ -1206,7 +1271,11 @@ mod tests {
     proptest::proptest! {
         #[test]
         fn prop_serialize_deserialize_roundtrip(
-            txn_counter in 0u64..u64::MAX,
+            // Same reasoning as superblock_count below: values above
+            // MAX_TXN_COUNTER make deserialize() return None by design, so
+            // sampling the full u64 range would fail the round-trip property
+            // roughly once in 2^32 draws — a landmine rather than a signal.
+            txn_counter in 0u64..=MAX_TXN_COUNTER,
             root_handle_table_page in 0u64..u64::MAX,
             root_freemap_page in 0u64..u64::MAX,
             total_pages in 0u64..u64::MAX,

src/transaction/commit.rs

@@ -32,6 +32,15 @@ pub(super) struct CommitCtx<'a> {
 }
 
 pub(super) fn run_commit(ctx: &mut CommitCtx<'_>) -> Result<()> {
+    // Refuse a commit that could not be read back BEFORE doing any work. The
+    // counter this commit will stamp is decided here, at the top, so exhausting
+    // the range costs no I/O and leaves the last durable state exactly as it
+    // was. See `superblock::next_txn_counter` for why writing past the bound is
+    // worse than refusing: it produces superblocks this binary rejects on the
+    // next open, silently discarding an acknowledged commit or bricking the file
+    // outright once every slot has been written past it.
+    let next_counter = crate::superblock::next_txn_counter(*ctx.txn_counter)?;
+
     // I27: flatten every still-active savepoint's `freed_pages`
     // back into `txn_freed_pages` before persist_freemap consumes
     // it. savepoint_inner moves `txn_freed_pages` INTO the
@@ -123,14 +132,15 @@ pub(super) fn run_commit(ctx: &mut CommitCtx<'_>) -> Result<()> {
     // What makes it unreachable now is an actual invariant rather than an
     // argument about counting: `Superblock::validate` rejects any slot whose
     // counter exceeds MAX_TXN_COUNTER, so a file that opened at all has at least
-    // TXN_COUNTER_RESERVE (2^32) increments of headroom. Exhausting that takes
-    // 4.3 billion commits in one session, since a reopen re-validates.
-    // The `expect` therefore stays: it now documents a guarded invariant rather
-    // than asserting an unguarded hope.
-    *ctx.txn_counter = ctx
-        .txn_counter
-        .checked_add(1)
-        .expect("txn_counter overflowed u64 (2^64 commits) — unreachable");
+    // TXN_COUNTER_RESERVE (2^32) increments of headroom.
+    //
+    // `next_txn_counter` was computed at the TOP of this function, before any
+    // I/O, so the refusal case costs nothing and leaves the last durable state
+    // untouched. Bounding only what we ACCEPT and not what we WRITE would let a
+    // file forged at exactly MAX_TXN_COUNTER (which validates, and opens
+    // cleanly) take acknowledged, fsynced commits at MAX+1, MAX+2 — superblocks
+    // this same binary then refuses to read. There is no panic left here at all.
+    *ctx.txn_counter = next_counter;
     let total_pages = cache.file_page_count()?;
     let sb = Superblock {
         magic: page::MAGIC,

src/transaction/keys.rs

@@ -61,17 +61,18 @@ impl TransactionManager {
         // future changes.
         cache.flush()?;
 
-        // I119: use checked_add, not `+= 1`. A wrapped counter corrupts
-        // Superblock::select's "highest counter wins" rule on recovery.
+        // I119: a wrapped counter corrupts Superblock::select's "highest counter
+        // wins" rule on recovery. This used to be `checked_add(1).expect(...)`
+        // justified by a "structurally unreachable" claim that was wrong — the
+        // counter is read from the file at open, so a forged value made it
+        // reachable. See the matching site in commit.rs for the full story.
         //
-        // Reachable from a forged superblock until `Superblock::validate` grew
-        // its MAX_TXN_COUNTER bound; see the long note at the matching site in
-        // commit.rs for why the old "structurally unreachable" claim was wrong
-        // and what actually guarantees the headroom now.
-        self.txn_counter = self
-            .txn_counter
-            .checked_add(1)
-            .expect("txn_counter overflowed u64 (2^64 commits) — unreachable");
+        // Bounded rather than panicking now, and bounded on the WRITE side so
+        // this never persists a header the next open would reject. Note this
+        // rewrite is metadata-only but still consumes a counter, so key rotation
+        // draws from the same headroom pool as commit.
+        let next_counter = crate::superblock::next_txn_counter(self.txn_counter)?;
+        self.txn_counter = next_counter;
 
         let total_pages = cache.file_page_count()?;
         let r = &self.committed_roots;