docs: correct ARCHITECTURE.md and the docs it contradicts (#98) - #133
Merged
Conversation
Eleven findings from the 2026-07-29 review. Two pairs overlapped, so they are fixed once each. Invented API (DOCS-COMMENTS-1, TXN-COMMIT-4 — a regression of I130). ARCHITECTURE described a `DataPage::compact()` and a `SLOT_FLAG_DEAD` constant in four places, plus an old->new slot-index remapping contract. Neither exists: data_page.rs defines only `SLOT_FLAG_LIVE`, nothing ever clears it, and there is no `fn compact` anywhere. The invented contract is the inverse of a load-bearing invariant — slot indices are immutable for the page's lifetime because the handle table stores `(page_id, slot_index)` — so a maintainer implementing the documented `compact()` would renumber slots in a live page and any entry the remap loop missed would resolve to the wrong value or escalate to a fatal `CorruptPage`. Replaced with the real model: an append-only directory, liveness tracked out-of-band in `SlotPacker`, whole-page reclamation, and defrag as relocate-via-update-until-the-page-drains. Also records that a deleted inline value is not scrubbed from its page, which the old text hid and which matters to the encryption threat model. Freemap routing (DOCS-COMMENTS-4, TXN-COMMIT-2 — a regression of I118's doc side). ARCHITECTURE named a function `allocate_data_page` that no longer exists (three references, all dead ends for a grep) and said handle-table COW pages "call `cache.new_page` directly and always extend". They do the opposite, and that routing is the fix for a real page leak — HT COW pages that always extended grew the file one page per mutation. Rewritten against the current call graph: `cow_alloc` / `FreemapRecycle::cow_alloc_into` is the single freemap-aware allocator, used by data-page inserts, handle-table COW and membership-index COW alike; overflow chains alone still bypass it. The savepoint gate is also not a data-page carve-out — `reuse = savepoints.is_empty()` is evaluated at all five call sites. Handle reuse (HANDLES-INDEX-1). Five places asserted handles are "never reused within a database's lifetime". `next_handle` is a field of `Roots`, so rollback, rollback_to and crash recovery all rewind it and the id is re-minted for different bytes — with no generation field to distinguish them. The engine is right (a rolled-back allocate never happened) so the docs are the wrong side: the guarantee is now scoped to committed handles, with the consequence for callers embedding a handle externally spelled out, and the generation-counter alternative named as the format change it would be. `tests/transactions.rs` pins both in-process rewind paths; `src/recovery_tests.rs` already pinned the crash-recovery one. Overflow values (TXN-COMMIT-3). ARCHITECTURE said an overflow value occupies a data-page slot holding a chain-head pointer. No data page or slot is allocated at all — the `HandleEntry` itself carries `HandleFlags::Overflow` with `slot_index = 0` unused. The R1 accounting depends on that: adding slot accounting for the phantom slot would decrement a count that was never incremented and push a still-referenced page to `txn_freed_pages`. Handle-table flag byte (DOCS-COMMENTS-3). Two places called byte 1 "forensic-only — no runtime code reads it". `recover_depth` reads it first, as the primary terminator of its left-spine walk, with the zero-child check only secondary; the constant was made `pub(crate)` for exactly that. The walk runs on every open and rollback and yields the descent depth for all lookups, so a comment inviting its removal is a live trap: `lookup` would return Ok(None) for live data with no checksum or type-tag error. Error contract (DOCS-COMMENTS-5, DOCS-COMMENTS-6). README listed `Poisoned` as fatal and told callers to classify with `is_fatal()`, which returns false for it — so README's own recovery snippet routed a poisoned handle into the "operational, keep going" arm, an unrecoverable loop the README taught you to build. `Poisoned` is now described as terminal-but-not-fatal, with the rationale, and the snippet matches it explicitly. Separately, `commit`'s `# Errors` advertised `CacheFull`/`SpillwayFull` as operational while `lifecycle.rs` poisons on *any* post-protocol error; both the rustdoc and README now state that `NoActiveTransaction` is commit's only non-poisoning error. Two name errors: `DefragOptions::max_pages` does not exist (the field is `max_values`), corrected in ARCHITECTURE and in README's example — which also could not compile, using a struct literal against a `#[non_exhaustive]` type and importing from the `pub(crate)` `defrag` module. And `defrag`'s rustdoc described a `threshold x max_observed` sparseness rule the implementation explicitly abandoned, in favour of a per-page density the same file documents correctly elsewhere. Closes #98.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #98 (11 findings, DESIGN/docs).
Two pairs of findings overlapped (DOCS-COMMENTS-1 / TXN-COMMIT-4 on
compact(),and DOCS-COMMENTS-4 / TXN-COMMIT-2 on freemap routing), so they are fixed once
each. Every claim was checked against the code before rewriting.
An invented API that inverts a real invariant
ARCHITECTURE described a
DataPage::compact()and aSLOT_FLAG_DEADconstantin four places, plus an old→new slot-index remapping contract. Neither exists —
data_page.rsdefines onlySLOT_FLAG_LIVE, nothing ever clears it, andgrep -rn "fn compact" src/is empty. The file's own header says the opposite:This is the worst kind of doc error, because the invented contract is the
inverse of a load-bearing invariant: slot indices are immutable for the
page's lifetime precisely because the handle table stores
(page_id, slot_index).A maintainer implementing the documented
compact()would renumber slots insidea live page, and any handle-table entry the remap loop missed would resolve to
the wrong value or escalate to a fatal
CorruptPage. The doc invited the onechange the format forbids.
Replaced with the real model: append-only directory, liveness tracked
out-of-band by
SlotPacker, whole-page reclamation, and defrag asrelocate-via-
update-until-the-page-drains. Also now records that a deletedinline value keeps its bytes on the page — the old text implied a tombstone
overwrote them, and it matters to the encryption threat model.
Freemap routing — documented backwards, and it was a real leak
ARCHITECTURE named a function
allocate_data_pagein three places. It doesn'texist (only "formerly"/"historical" mentions in comments), so every reader
grepping for it hits a dead end. Worse,
:597said handle-table COW pages"call
cache.new_pagedirectly and always extend" — the exact opposite of whatthey do, and that routing is the fix for a real page leak (HT COW pages that
always extended grew the file one page per mutation).
So a maintainer auditing reclamation against the doc would conclude the handle
table is expected to leak, and not treat unbounded growth as a bug; or would
"restore" the documented behaviour and reintroduce it.
Rewritten against the current call graph:
cow_alloc/FreemapRecycle::cow_alloc_intois the single freemap-aware allocator, reachedby data-page inserts, handle-table COW and membership-index COW alike; overflow
chains alone still bypass it. The savepoint gate is also not a data-page
carve-out —
reuse = savepoints.is_empty()is evaluated at all five call sites."Handles are never reused" is only true after commit
Five places asserted handles are never reused "within a database's lifetime".
next_handleis a field ofRoots, sorollback,rollback_toand crashrecovery all rewind it, and the id is re-minted for different bytes — with no
generation field to tell them apart:
The engine is right (a rolled-back allocate never happened), so the docs are the
wrong side. The guarantee is now scoped to committed handles, with the
consequence for callers spelled out, and the generation-counter alternative
named as the on-disk format change it would be.
New test —
tests/transactions.rs::test_uncommitted_handle_is_remintedpins both in-process rewind paths (
rollbackandrollback_to), assertingh == h2and that reading the old handle returns the new bytes.src/recovery_tests.rsalready pinned the crash-recovery arm.Overflow values own no slot
ARCHITECTURE said an overflow value occupies a data-page slot holding a
chain-head pointer. No data page or slot is allocated at all — the
HandleEntrycarries
HandleFlags::Overflowwithslot_index = 0unused. R1 accountingdepends on it: adding slot accounting for the phantom slot would decrement a
count that was never incremented, pushing a still-referenced page to
txn_freed_pages.The handle-table flag byte is not decoration
Two places called byte 1 "forensic-only — no runtime code reads it". Eleven
lines below one of them, the constant is
pub(crate)specifically so the depthwalk can use it, and
recover_depthreads it as its primary terminator(
if buf[1] != FLAG_INTERIOR { break; }) — the zero-child check is onlysecondary. That walk runs on every open and rollback and yields the descent
depth for all lookups, so a maintainer who trusts the comment and stops writing
the flag in
growmakeslookupreturnOk(None)for live data, with nochecksum or type-tag error to signal it.
The error contract disagreed with itself
Poisoned: README listed it as fatal and said to classify withis_fatal()— which returnsfalsefor it, deliberately (the manager isalready dead; re-poisoning is meaningless). So README's own recovery snippet
routed a poisoned handle into the "operational, keep going" arm: an
unrecoverable loop the README taught you to build. It is now described as
terminal-but-not-fatal, with the rationale, and the snippet matches
Poisonedexplicitly.
commit: its# ErrorsadvertisedCacheFull/SpillwayFullasoperational, while
lifecycle.rspoisons on any post-protocol error. A callerfollowing the documented recovery calls
rollback()on an already-poisonedhandle. Both the rustdoc and README now state that
NoActiveTransactioniscommit's only non-poisoning error, and say why the inversion is deliberate.
Two name errors
DefragOptions::max_pagesdoes not exist (the field ismax_values), correctedin ARCHITECTURE and in README's example — which additionally could not compile:
it used a struct literal against a
#[non_exhaustive]type and imported fromthe
pub(crate)defragmodule. Verified the corrected form compiles and runs.And
defrag's rustdoc described athreshold × max_observedsparseness rulethe implementation explicitly abandoned — the same file documents the real
per-page density rule correctly two places over. The abandoned rule never fires
for the case the change was made to handle (a lone sparse page scores 1.0
against itself), so callers were tuning against a rule that does not exist.
Verification
cargo testall green,cargo clippy --workspace --all-targets -- -D warningsclean,
cargo fmt --checkclean.Note on the stack
PR #132 was incomplete when first opened — a
git addthere listed thepre-rename stub path, failed as a whole, and staged only the rename. It has been
completed with a follow-up commit on the same branch, and this branch is rebased
on top of it. Both are unmerged, so no history was rewritten.