Skip to content

fix(recovery): fail closed during deep-walk reachability - #12

Merged
farhan-syah merged 3 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/deep-walk-diagnostics
Jul 26, 2026
Merged

fix(recovery): fail closed during deep-walk reachability#12
farhan-syah merged 3 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/deep-walk-diagnostics

Conversation

@presempathy-awb

@presempathy-awb presempathy-awb commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

fix(recovery): fail closed during deep-walk reachability

Summary

pagedb-fsck --deep treated the live-tree reachability walk as a best-effort
page 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(()) from
BTree::collect_all_page_ids now means every reachable node and overflow page
authenticated 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_ids uses read_node_guard — the same authenticated node read
normal 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::OverflowRoot via overflow::read_root_page, and non-root links
under the fixed PageKind::Overflow binding. The pre-cedd6e6 v1 fallback is
deliberately 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 two
walks 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. Both
collect_all_page_ids and find_dangling enforce it, as does Leaf::encode's
existing 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 Overflow value
always 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:

ReservedPageReferenced { parent_page_id, child_page_id }
OverflowChainCycle { root_page_id, page_id }

Both callers fail closed

WriteTxn::commit's per-commit use-after-free invariant discarded the walk's
error with let _ =. With the walk now fail-closed, a corrupt tree yielded a
short 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_dangling check above it.

Localization covers the whole tree

The diagnostic pass (diagnose_tree_structure) runs only after a tree's
authoritative 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 rather
than 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.

Test What it pins
deep_walk_reports_aead_valid_malformed_live_btree_root A valid envelope over an invalid node body is not silently omitted
deep_walk_reports_a_cycle_in_a_live_overflow_chain A chain that loops fails closed, and the repeated page is located
deep_walk_identifies_the_dangling_child_page A recycled Free child is named together with its referring parent
deep_walk_reports_an_internal_child_in_a_reserved_page A child pointer into the reserved region is rejected by name
deep_walk_reports_an_overflow_root_in_a_reserved_page An Overflow value with no root page is rejected by name

Each was confirmed to fail without its specific fix, not merely observed
green:

Stubbed out Result
The walk's error, swallowed as before all five fail
The reserved-page checks both reserved-page tests fail
Cycle localization the cycle test fails

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::encode already asserts a non-reserved overflow root, so the write path
was 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_deep integration tests and the overflow_walk_offset
regression are unchanged and green.

Verification

Run against this branch with main merged in:

cargo nextest run --all-features --no-fail-fast
  421 passed; 4 skipped

PAGEDB_INVARIANT_CHECKS=1 cargo nextest run --all-features --no-fail-fast
  421 passed; 4 skipped

cargo clippy --all-targets --all-features -- -D warnings
  clean

cargo fmt --all --check
  clean

cargo test --doc --all-features
  clean

cargo check --target wasm32-unknown-unknown --lib --features opfs
  clean (4 pre-existing warnings in journal/segment code, unchanged)

The invariant-checks run matters specifically: it exercises the new
WriteTxn::commit panic 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. CorruptionDetail gains 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_ids is report-only (no destructive consumer), the flood
is always accompanied by the reachability-failure issue that explains it, and
is_clean() already keys off page_issues rather than orphans. Noisy on an
already-corrupt database, and strictly preferable to the previous silence.

…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.
@farhan-syah
farhan-syah merged commit 3bdc2be into NodeDB-Lab:main Jul 26, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants