fix(recovery): fail closed during deep-walk reachability - #12
Merged
farhan-syah merged 3 commits intoJul 26, 2026
Merged
Conversation
presempathy-awb
force-pushed
the
codex/upstream/deep-walk-diagnostics
branch
from
July 26, 2026 13:16
166f892 to
d0d3b2d
Compare
…erved pages Introduce a canonical page_space module defining the reserved page-id range (headers + apply-journal) and use it everywhere a raw `< 4` comparison stood in for it. collect_all_page_ids and its overflow-chain walk now fail closed on any reference into reserved space or a chain cycle instead of silently truncating, and deep-walk's diagnostic pass gains the same structural coverage (leaf overflow chains, decode failures) it was previously missing. Commit now panics rather than proceeding if the post-commit reachability walk itself errors, since a short reachable set would otherwise pass the freed-page check vacuously.
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.
fix(recovery): fail closed during deep-walk reachability
Summary
pagedb-fsck --deeptreated the live-tree reachability walk as a best-effortpage collector: an unreadable node was skipped, an undecodable leaf or internal
body was skipped, and an overflow chain that could not be read — or that
revisited a page it had already walked — simply stopped. The returned set then
looked complete to its caller.
That is the wrong failure mode for an integrity checker. A partial set its
caller cannot distinguish from a complete one does not merely lose detail; it
reads as a clean bill of health.
This PR makes the walk authoritative.
Ok(())fromBTree::collect_all_page_idsnow means every reachable node and overflow pageauthenticated as its own kind, decoded structurally, and pointed only at
allocatable pages. Anything else is a corruption error that surfaces in
DeepWalkReport.Alongside that, dangling-child localization becomes conditional: a healthy tree
is traversed once, and only a tree whose authoritative walk failed gets a
second, bounded, authenticated pass that adds parent/child context to the
failure already recorded.
What was fixed
The reachability walk is authoritative
collect_all_page_idsusesread_node_guard— the same authenticated node readnormal tree operations use — and the authenticated kind selects the decoder.
Read and decode failures propagate.
Each pointer now carries its referrer, so a failure names both ends. Overflow
roots are collected before the page guard drops; the root authenticates as
PageKind::OverflowRootviaoverflow::read_root_page, and non-root linksunder the fixed
PageKind::Overflowbinding. The pre-cedd6e6v1 fallback isdeliberately not restored.
The walk keeps its own visited set rather than doubling the caller's
accumulator as one. That accumulator spans every tree in the database and
arrives pre-seeded with the reserved pages, so reusing it would let the caller's
contents silently truncate the walk.
Reserved pages are not a benign destination
Page ids 0..=3 are the A/B structural headers and the apply-journal. They use a
different envelope format entirely, so a live tree pointer that reaches one is a
wild pointer or a use-after-free that recycled a reserved id — never normal.
The boundary had five independent hardcoded
4s across the crate, and the twowalks that checked it disagreed about what it meant. It now has one definition,
src/pager/page_space.rs, documenting what owns each reserved id and why. Bothcollect_all_page_idsandfind_danglingenforce it, as doesLeaf::encode'sexisting write-path assertion.
Zero keeps its two legitimate meanings — an absent internal child slot, and a
chain terminator — but is corruption as an overflow root: an
Overflowvaluealways owns at least its root page.
Failures are named
A cyclic overflow chain previously reported
CorruptionDetail::HeaderUnverifiable,a variant documented as "main.db A/B header HK-MAC failed on both copies". An
operator would have been told their database header was unverifiable, and the
page id of the cycle — the one actionable fact — was discarded.
Two variants now carry their context, with canonical constructors:
Both callers fail closed
WriteTxn::commit's per-commit use-after-free invariant discarded the walk'serror with
let _ =. With the walk now fail-closed, a corrupt tree yielded ashort reachable set and the freed-page assertion below it passed vacuously —
the same false-clean this PR exists to remove, at the second of two call sites.
It now panics at the source commit, like the
find_danglingcheck above it.Localization covers the whole tree
The diagnostic pass (
diagnose_tree_structure) runs only after a tree'sauthoritative walk has failed; its job is to say where, not whether. It
descends leaf overflow chains as well as internal children, so a malformed leaf
or a cycle now gets a located issue instead of only the tree-level message.
Every page it inspects is authenticated first. A recycled child is identified by
reading it as a B+ tree node and failing — never by trusting a raw on-disk kind
byte, which is unauthenticated and therefore not evidence. The previous
implementation's raw VFS kind-byte read is removed.
The pass keeps a visited set and a budget derived with saturating arithmetic
from
next_page_id; exhausting the budget is itself a reported issue ratherthan a silent break.
Regression tests
All five construct authenticated corruption. Flipping an AEAD tag would
exercise the physical page verifier, not the structural gap being closed here.
deep_walk_reports_aead_valid_malformed_live_btree_rootdeep_walk_reports_a_cycle_in_a_live_overflow_chaindeep_walk_identifies_the_dangling_child_pageFreechild is named together with its referring parentdeep_walk_reports_an_internal_child_in_a_reserved_pagedeep_walk_reports_an_overflow_root_in_a_reserved_pageOverflowvalue with no root page is rejected by nameEach was confirmed to fail without its specific fix, not merely observed
green:
Two notes on how those tests are written. The reserved-page tests assert the
named corruption detail rather than "an error occurred" — reading page 1 as a
node fails its AEAD anyway, and that generic failure would not tell an operator
which pointer was wild, so only the named assertion pins the fix. And
Leaf::encodealready asserts a non-reserved overflow root, so the write pathwas never the gap; that test forges the bytes directly, because what is under
test is catching a page that reached disk some other way.
The four existing
fsck_deepintegration tests and theoverflow_walk_offsetregression are unchanged and green.
Verification
Run against this branch with
mainmerged in:The invariant-checks run matters specifically: it exercises the new
WriteTxn::commitpanic path and confirms it does not fire on healthy commits.Scope
No change to page layout, AAD, encryption, allocation, free-list format,
recovery publication order, public API, or CLI syntax. No new dependency,
feature, or workflow.
CorruptionDetailgains two variants; it is#[non_exhaustive], so that is not a breaking change.No README change: this repairs the existing documented deep integrity walk
rather than adding a user-facing mode.
Known behavior change
When a tree's walk fails, the reachable set is necessarily partial, so every
AEAD-valid live page falls out as an orphan — a few hundred on a 10k-record
database.
orphan_page_idsis report-only (no destructive consumer), the floodis always accompanied by the reachability-failure issue that explains it, and
is_clean()already keys offpage_issuesrather than orphans. Noisy on analready-corrupt database, and strictly preferable to the previous silence.