Cleanups in the freemap, allocation and defrag

[Xof]

This issue groups 7 related findings.

FREEMAP-2 — ARCHITECTURE.md says handle-table COW pages bypass the freemap and always extend; the code routes them through cow_alloc

Location: ARCHITECTURE.md:597 (also :587, :595) · Severity: SMELL · Category: docs-vs-reality · Status: REGRESSION of I118 (duplicate of TXN-COMMIT-2)

What the code does. ARCHITECTURE.md:597 states: "Overflow pages and handle-table COW pages do not go through allocate_data_page (they call cache.new_page directly and always extend), but their frees still feed the freemap on commit". The code says the opposite for the handle table (and membership index): cow_alloc's doc is "Freemap-aware page allocator shared by data-page allocation and the handle-table / membership-index COW paths" (src/transaction/freemap.rs:20-21), and every handle-table / membership site builds let mut alloc = |c: &mut PageCache| self.freemap.cow_alloc_into(c, &mut tree, reuse); (src/transaction/freemap.rs:638, src/transaction/staging.rs:50/97/172, src/transaction/mutate.rs:198). src/freemap.rs:8-19 documents the current behavior correctly ("the handle-table / membership-index COW paths prefer FreeMap::allocate_first... so they reach a bounded steady-state page count rather than growing one page per mutation"). Only overflow still extends directly (src/overflow.rs:92).

Why it is a problem. ARCHITECTURE.md is the stated design reference. A maintainer sizing the file's steady-state growth, or debugging why a reused id came back from claim_page on a handle-table COW, is told by the top-level doc that this path can only extend. The doc also still names allocate_data_page (ARCHITECTURE.md:587, :595), a function that no longer exists anywhere in src/ — only in three historical comments (src/transaction/freemap.rs:28, src/transaction/packing.rs:78, :271).

Direction of a fix. Rewrite ARCHITECTURE.md:587-597 to say that data-page, handle-table and membership-index COW allocation all go through cow_alloc (reuse-before-extend), that only overflow still calls cache.new_page directly, and replace the allocate_data_page references with cow_alloc / insert_into_data_page.

FREEMAP-3 — commit.rs claims persist marks the prior commit's deferred structural frees free in the tree, contradicting the two-free-streams invariant

Location: src/transaction/commit.rs:80-82 · Severity: SMELL · Category: comment-accuracy · Status: NEW

What the code does. src/transaction/commit.rs:80-83 describes step 0 as: "persist the freemap tree. This marks txn_freed_pages (plus the prior commit's deferred structural frees) free in a COW of the committed tree". FreemapRecycle::persist does no such thing — it loops only over the data frees: for id in txn_freed_pages.iter().copied() { self.mark_free_committed_path(cache, roots, id)?; } (src/transaction/freemap.rs:347-349). The unit's own header states the inverse as a load-bearing rule: "structural_superseded / pending_structural_frees / structural_reuse (FREEMAP-page frees) ... These are NOT marked free in the tree" (src/transaction/freemap.rs:311-315), and "Reusing a DEAD page (vs. a free bit in the tree) preserves the extend-only TERMINATION guarantee" (src/transaction/freemap.rs:127-129).

Why it is a problem. This is the comment most likely to produce a real bug. A maintainer who trusts commit.rs and 'restores' the supposedly-missing behavior — adding for id in &self.pending_structural_frees { mark_free_committed_path(...) } to persist — puts pool pages into the bitmap while they are simultaneously poppable by structural_extend (src/transaction/freemap.rs:99). The same page would then be handed out once as a data page by cow_alloc and once as a freemap COW target, i.e. a silent double-allocation. It also re-opens the self-reference recursion the extend-only rule exists to prevent (src/freemap_tree.rs:21-27).

Direction of a fix. Change commit.rs:80-83 to say persist marks only txn_freed_pages free, and that the prior commit's deferred structural frees are CONSUMED as COW targets by structural_extend, never marked free in the bitmap. Cross-reference the TWO FREE-STREAMS block.

FREEMAP-5 — FreeMap::allocate_first has no production caller, yet two header comments name it as the production allocator path

Location: src/freemap.rs:9, src/freemap.rs:62, src/freemap.rs:127 · Severity: SMELL · Category: comment-accuracy

What the code does. src/freemap.rs:8-11 says the allocation paths "prefer FreeMap::allocate_first (reusing a page freed by a prior committed transaction)", and the I35 note says "The production allocator path uses allocate_first + mark_free" (src/freemap.rs:62-63). A crate-wide grep for FreeMap:: shows FreeMap::allocate_first appears only inside src/freemap.rs's own #[cfg(test)] mod (lines 220, 222, 224) and in one stale comment (src/freemap.rs:178). The real tree allocator never calls it: FreeMapTree::allocate_first finds the id with FreeMap::first_free_bit_from (src/freemap_tree.rs:508) and claims it with FreeMap::clear_bit (src/freemap_tree.rs:437). The same I35 note claims is_free is "reached only from src-tests today" and cites "src/transaction.rs's I27/I28 regression tests" — but src/transaction.rs no longer exists (it is the src/transaction/ module), and FreeMap::is_free IS on a production path: FreeMapTree::is_free (src/freemap_tree.rs:244) is called by FreemapRecycle::reclaim_orphans (src/transaction/freemap.rs:465), the defrag orphan sweep. FreeMapTree::is_free's own doc (src/freemap_tree.rs:236-239) states this correctly, so the two files disagree.

Why it is a problem. FreeMap::allocate_first (and capacity) are genuinely dead production code hidden behind a blanket #[allow(dead_code)] on the whole impl (src/freemap.rs:66), so the compiler cannot flag them. A maintainer optimizing or hardening 'the production allocator' would work on allocate_first and change nothing that runs, while first_free_bit_from + clear_bit — the code that actually decides which page id is handed out — goes untouched. The inverted claim about is_free risks the opposite error: deleting or weakening a predicate the orphan sweep depends on to avoid double-reclaiming a page.

Direction of a fix. Fix src/freemap.rs:8-11 and :59-65 to name first_free_bit_from + clear_bit as the production path and is_free as production-reachable via FreeMapTree::is_free / reclaim_orphans; then either delete FreeMap::allocate_first and capacity or move the #[allow(dead_code)] onto just those two so future dead code is still caught.

FREEMAP-7 — FreemapRecycle::rollback claims every freemap page COW'd this transaction sits above the watermark; pool-reused pages sit below it

Location: src/transaction/freemap.rs:516, src/transaction/freemap.rs:99 · Severity: SMELL · Category: comment-accuracy · Status: NEW

What the code does. FreemapRecycle::rollback's doc justifies clearing the session set with: "any freemap pages this aborted transaction COW'd sit above the watermark and were just truncated, so their ids must not be treated as in-place-mutable next transaction" (src/transaction/freemap.rs:515-517). structural_extend prefers a pooled id over extension — if let Some(id) = structural_reuse.pop() { ... cache.claim_page(id)?; Ok(id) } (src/transaction/freemap.rs:99-103) — and those ids come from pending_structural_frees, i.e. freemap pages superseded by a PRIOR commit, which are below the current watermark by construction. What actually drops them is discard_all_dirty, not the truncate: rollback_inner runs cache.discard_all_dirty(); cache.truncate(self.committed_roots.total_pages)?; (src/transaction/lifecycle.rs:265-266), and the (a)/(b) comment above it says step (a) exists precisely to "catch pages REUSED from the freemap whose id is less than the watermark" (src/transaction/lifecycle.rs:250-253). The same false premise is repeated at src/transaction/lifecycle.rs:294-296.

Why it is a problem. The action (clearing session_owned) is correct; the stated reason is not. A maintainer optimizing rollback who believes the truncate alone suffices for freemap pages could drop or reorder discard_all_dirty — which would leave the aborted transaction's freemap-leaf contents dirty at a pooled id, to be flushed by a later commit. The same premise would also read as license to let the savepoint path (rollback_to_inner, which calls truncate only) COW the freemap; today that is safe only because cow_alloc disables reuse under savepoints (src/transaction/freemap.rs:55, src/page_cache.rs:687-692).

Direction of a fix. Reword to: pages EXTENDED this transaction are dropped by the watermark truncate; pages drawn from structural_reuse sit below the watermark and are dropped by discard_all_dirty — both reasons the session set must be cleared. Fix the mirrored comment at src/transaction/lifecycle.rs:294.

FREEMAP-8 — persist's early-return is justified by a claim that the supersede streams are non-empty only when there were frees; allocation alone fills them

Location: src/transaction/freemap.rs:336, src/transaction/freemap.rs:61 · Severity: SMELL · Category: comment-accuracy · Status: NEW

What the code does. FreemapRecycle::persist returns early on if txn_freed_pages.is_empty() (src/transaction/freemap.rs:344-346), justified as: "A no-op when nothing was freed (the recycle/supersede streams are only ever non-empty when there were frees, so the single emptiness check suffices)" (src/transaction/freemap.rs:335-337). That premise is false. A transaction that frees nothing but allocates still fills structural_superseded: cow_alloc calls tree.allocate_first(cache, hint, &mut extend) (src/transaction/freemap.rs:61), which COWs the containing leaf via clear_bit -> cow_descend -> cow_node, pushing the old leaf id onto pending_superseded (src/freemap_tree.rs:402), which put_tree drains into structural_superseded (src/transaction/freemap.rs:213-214). defrag.rs's own test comment describes exactly this: "step-5 update() calls allocate fresh data pages via cow_alloc -> allocate_first ... The OLD (pre-COW) leaf id lands in structural_superseded" (src/defrag.rs:344-347).

Why it is a problem. No bug today: the streams are promoted by FreemapRecycle::commit (src/transaction/commit.rs:200), which runs unconditionally, so the early return skips nothing that matters. But the comment states a property of the recycle that a future change could lean on — e.g. moving stream promotion into persist, or asserting structural_superseded.is_empty() when txn_freed_pages is empty — and that change would silently drop a commit's dead freemap pages out of the recycle, turning steady-state freemap churn back into unbounded file growth.

Direction of a fix. Replace the parenthetical with the true reason: persist only marks DATA frees, so with no frees there is nothing to mark; the structural streams are promoted separately and unconditionally by FreemapRecycle::commit.

FREEMAP-10 — ARCHITECTURE.md's defrag section names two API items that do not exist: DefragOptions::max_pages and compact()

Location: ARCHITECTURE.md:609, ARCHITECTURE.md:583 · Severity: SMELL · Category: docs-vs-reality · Status: REGRESSION of I122 and I130

What the code does. ARCHITECTURE.md:609 says "The cap parameter (DefragOptions::max_pages) bounds the number of values relocated in one pass, despite the legacy name (kept for API stability; see C4 in ISSUES.md)." The field is pub max_values: usize (src/defrag.rs:80) with builder max_values (src/defrag.rs:100) — a crate-wide grep for max_pages returns nothing, including the PyO3 binding, which reads max_values (python/src/db.rs:519). ARCHITECTURE.md:583 says "freed slots become tombstones until compact() reclaims the space"; there is no compact function anywhere in src/ or python/src/ — the reclaiming caller is defrag() itself via update() relocation (src/defrag.rs:250-251).

Why it is a problem. Both lines send a reader looking for symbols that do not exist, and :609's parenthetical actively argues for keeping a name that was already changed — so a maintainer trusting it would either rename max_values back to max_pages (a breaking change to the public struct and the Python binding, which extracts the attribute by name at python/src/db.rs:519) or file a compatibility issue for a non-problem.

Direction of a fix. Update ARCHITECTURE.md:609 to name DefragOptions::max_values and drop the legacy-name caveat; replace the compact() reference at :583 with defrag() (or with the packer's slot-release / relocation mechanism).

FREEMAP-6 — cow_descend enforces its in-range precondition only in prose; find_leaf's bounds guard has no counterpart on the write path

Location: src/freemap_tree.rs:308, src/freemap_tree.rs:360, src/freemap_tree.rs:218 · Severity: NIT · Category: api-design · Status: NEW

What the code does. The read path guards the child slot index: "Defense-in-depth: a validated depth keeps child_idx in bounds, but guard the slot index so a corrupt one reads as "absent" rather than slicing past the page. if child_idx >= PTRS_PER_INTERIOR { return Ok(None); }" (src/freemap_tree.rs:216-220), and rejects out-of-reach ids up front via if cap != u64::MAX && id >= cap (src/freemap_tree.rs:201). The COW write path has neither. cow_descend computes let child_idx = (remaining / span) as usize; (src/freemap_tree.rs:308) and passes it straight to read_child / write_child, which index DATA_PAGE_HEADER_SIZE + index * PTR_SIZE with no bound (src/freemap_tree.rs:66, :71). At depth 0 the loop never runs and the leaf op is applied to remaining % LEAF_CAPACITY (src/freemap_tree.rs:360), silently aliasing an out-of-range id onto some other page's bit. The only protection is prose: "Assumes id is in range (callers grow first if needed)" (src/freemap_tree.rs:279-280) and "Assumes id is in range; use mark_free_growing when it may exceed capacity" (src/freemap_tree.rs:407-408).

Why it is a problem. Not reachable today: the only production entry is mark_free_growing, which grows first (src/freemap_tree.rs:454-457), and clear_bit is only called by allocate_first on an id scan_from just proved free. But mark_free is pub on a pub(crate) type, so one new caller that forgets _growing gets, at depth 0, a silently corrupted bitmap (id 65344+7 clears/sets the bit for page 7 — a live page marked free, i.e. a double-hand-out), and at depth 1 with id >= LEAF_CAPACITY*1021 a child_idx of 1021+ that makes write_child slice past the 8192-byte buffer and panic. The invariant that keeps this safe lives only in a doc comment, while the identical invariant on the read path is enforced in code.

Direction of a fix. Either make mark_free private and expose only mark_free_growing, or have cow_descend return CorruptPage/a typed error when id >= self.capacity() or child_idx >= PTRS_PER_INTERIOR, mirroring find_leaf's guard.

---

_{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.}