Cleanups in the superblock, format versioning and recovery

[Xof]

This issue groups 5 related findings.

SUPERBLOCK-RECOVERY-5 — sb_identity_aad's doc claims it binds the cleartext bootstrap fields, but page_size stays cleartext and is excluded from the AAD

Location: src/superblock/mod.rs:325 (doc) — the AAD body at 334-342 and the cleartext write at 406 are both correct as written · Severity: SMELL · Category: comment-accuracy · Status: NEW

What the code does. The doc asserts coverage of the cleartext set: "The four bootstrap fields that stay cleartext in both encrypted and plaintext DBs are included" and "These four MUST stay cleartext even in an encrypted DB precisely because they are the AAD" (mod.rs:326-333). The AAD body covers magic, format_version, txn_counter, superblock_count (mod.rs:335-341). But serialize_encrypted leaves FIVE fields cleartext — it writes buf[48..52].copy_from_slice(&self.page_size.to_le_bytes()); (mod.rs:406), and its own comment at mod.rs:401-402 acknowledges "page_size at 48..52 is a cleartext bootstrap field". page_size is therefore cleartext, is consumed at open (if sb.page_size != PAGE_SIZE as u32, recovery.rs:406), and is authenticated by nothing but the forgeable XXH3 page checksum.

Why it is a problem. Two concrete costs. (a) A single flipped byte in 48..52 plus a recomputed XXH3 turns a healthy encrypted database into a permanent UnsupportedPageSize failure with no fallback to a sibling slot — the AEAD that would have caught it never sees the field. (b) A maintainer reading this doc concludes that every cleartext bootstrap field is AAD-bound, and will add the next cleartext field (a second stride hint, a flags word) assuming it is authenticated — writing exactly the bug this comment was meant to prevent.

Direction of a fix. Either add page_size to sb_identity_aad (bytes 20..24 are already reserved for exactly this) and bump the format version, or correct the doc to say five fields stay cleartext and state explicitly that page_size is deliberately outside the AAD and why.

SUPERBLOCK-RECOVERY-6 — diagnose's documented guarantees are wrong in both directions — it can label data pages as corrupt slots and can omit real slots

Location: src/superblock/mod.rs:588, src/superblock/mod.rs:598, src/superblock/mod.rs:601 · Severity: SMELL · Category: comment-accuracy

What the code does. The doc promises "this returns one SlotDefect per genuine superblock slot", that non-slot pages are excluded because "including them in the defect list would falsely label intact data pages as corrupt superblocks", and that with the MIN fallback "we never under-report" (mod.rs:571-587). The implementation derives its bound from the FIRST buffer whose raw bytes 308..312 happen to land in range — over ALL candidates including non-slot pages: .find(|&n| (MIN_SUPERBLOCKS..=MAX_SUPERBLOCKS).contains(&n)).unwrap_or(MIN_SUPERBLOCKS) (mod.rs:589-599), then reports buffers[..bound.min(buffers.len())] (mod.rs:601).

Why it is a problem. Over-report: in a DB with N=2 whose two slots are mangled past recognition, if the data page at index 2 happens to carry a value in 2..=16 at byte offset 308, bound becomes that value and pages 2..bound — ordinary intact data pages — are emitted as BadMagic superblock defects, the exact outcome the doc says is avoided. Under-report: in a DB with N=4 where all four slots are mangled including their count fields, bound falls back to 2 and the operator's CorruptSuperblock diagnostic silently omits half the slots. Both mislead whoever is triaging an unopenable file.

Direction of a fix. Take the count only from buffers at indices < MAX that also pass the magic check, or drop the heuristic and report all read candidates tagged with whether they are inside the recoverable slot range; either way, correct the doc's two guarantees to match.

SUPERBLOCK-RECOVERY-7 — A forged crypto-header stride is a hard error on the intact-page-0 path and silently ignored on the torn-page-0 path, and the trailing comment misstates which value was applied

Location: src/transaction/recovery.rs:296, src/transaction/recovery.rs:265, src/transaction/recovery.rs:350 · Severity: SMELL · Category: correctness

What the code does. The anchor path validates the advertised stride and refuses anything else: if stride != crate::crypto::ENC_PAGE_SIZE { return Err(ChiselError::CorruptSuperblock { defects: Vec::new() }); } (recovery.rs:265-269). The torn-slot-0 fallback instead hardcodes the stride and accepts the candidates on the mere PRESENCE of a header, never comparing the sibling's advertised value: cache.io_mut().set_stride(crate::crypto::ENC_PAGE_SIZE); ... if encrypted_stride(&enc_candidates).is_some() { candidates = enc_candidates; } (recovery.rs:296-299). The comment 50 lines later then claims "The IO stride was already switched to header.stride during the bootstrap read above" (recovery.rs:350-351), which is only true on the anchor path.

Why it is a problem. Two identical inputs get two different verdicts purely on whether page 0 happens to be torn: a file whose header advertises stride 12345 is rejected as CorruptSuperblock when page 0 is intact, and opened normally (at 8232) when page 0 is torn. Neither outcome is unsafe today, but the divergence means the I144 guard is not actually a guard on the header field — it is a guard on one of two code paths — and the comment tells the next maintainer the winner's header.stride was honored when in the fallback it was never consulted at all.

Direction of a fix. Validate the winner's header.stride == ENC_PAGE_SIZE once, after selection, on both paths (the anchor check then becomes a fast pre-check), and reword the comment at recovery.rs:350 to say the stride is the constant ENC_PAGE_SIZE, not the header's value.

SUPERBLOCK-RECOVERY-8 — Spillway::slot_size() is dead code whose doc names a caller that does not exist

Location: src/spillway.rs:214, src/spillway.rs:212, src/page_cache.rs:522 · Severity: SMELL · Category: correctness · Status: NEW

What the code does. The method is annotated #[allow(dead_code)] yet its doc asserts a live consumer: "On-disk slot size in bytes: SLOT_HEADER_SIZE + payload_size. Used by Task 3.3/3.4 to size drain buffers for encrypted DBs." (spillway.rs:211-216). Grepping the tree, slot_size() has no call sites at all — the drain path it names sizes its buffer from the crypto constant directly: let unit: [u8; ENC_PAGE_SIZE] = blob.as_slice().try_into() (page_cache.rs:522). The sibling SLOT_SIZE const (spillway.rs:60) is likewise doc'd as used "by callers that pass PAGE_SIZE as payload_size" but appears only in this file's own #[cfg(test)] module.

Why it is a problem. The #[allow(dead_code)] plus a confident "used by" doc makes an unused accessor look load-bearing, so nobody deletes it and a reader hunting the drain sizing logic goes to slot_size() and finds it is not what the drain uses. The two size sources (this method's SLOT_HEADER_SIZE + payload_size and the drain's ENC_PAGE_SIZE) can drift with no compiler complaint precisely because the method is never called.

Direction of a fix. Delete slot_size() (and SLOT_SIZE if only tests use it, moving it into the test module), or make the drain path call it so the doc becomes true and there is a single sizing source.

SUPERBLOCK-RECOVERY-9 — Property-test comment claims PartialEq is not derived on Superblock, but it is, and another test in the same file relies on it

Location: src/superblock/mod.rs:1199, src/superblock/mod.rs:176, src/superblock/mod.rs:808 · Severity: NIT · Category: comment-accuracy · Status: NEW

What the code does. The proptest body carries the justification "PartialEq isn't derived on Superblock, so compare structurally — easier to diagnose if a single field round-trips wrong." (mod.rs:1199-1201). The struct is declared #[derive(Debug, Clone, PartialEq, Eq)] pub struct Superblock (mod.rs:176), and test_superblock_roundtrip in the same module does assert_eq!(sb, sb2); (mod.rs:808), which only compiles because PartialEq IS derived.

Why it is a problem. The stated reason is false, so the field-by-field comparison looks mandatory rather than a diagnostics preference. The real hazard is the reverse of what the comment implies: the hand-rolled comparison enumerates fields explicitly, so a newly added Superblock field is silently omitted from the round-trip property while assert_eq! would have caught it — and the comment actively discourages switching to the form that would.

Direction of a fix. Correct the comment to say the structural comparison is a deliberate diagnostics choice (PartialEq is available), and note that a new field must be added to the list — or just use prop_assert_eq!(parsed, sb) and let the derive cover future fields.

---

_{Filed from the clean-slate deep review of 2026-07-29. Full context, verification notes, and the delta against ISSUES.md are in docs/reviews/review-20260729-183138.md. Baseline at review time: 681 tests passing, clippy and fmt clean — none of these are toolchain-visible.}