fix(txn): validate freed pages before durable publication - #14
Merged
farhan-syah merged 3 commits intoJul 26, 2026
Merged
Conversation
presempathy-awb
force-pushed
the
codex/upstream/freed-page-invariant
branch
from
July 26, 2026 14:57
6453f17 to
7ea444a
Compare
A rejected candidate commit previously restored only the commit-history fields, leaving next_page_id and friends advanced past pages the abandoned candidate had already claimed. Capture and restore the full writer state so the next writer bump-allocates cleanly instead of leaking page ids, and harden the regression test to assert the child process actually ran the invariant check rather than trusting a bare exit status.
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(txn): validate freed pages before durable publication
Summary
PageDB's opt-in freed-page invariant (
PAGEDB_INVARIANT_CHECKS) ran aftercommit_headerhad already swapped the durable A/B header. When it fired, thecaller saw a failed commit while the store had published that exact commit —
reopening reported it as authoritative.
This moves the check ahead of the free-list rewrite, the pager flush, and the
header swap, so a rejected candidate is never durably published. It also walks
the commit-history tree, which was omitted, and unwinds the writer state the
candidate had advanced before it panics.
Changed file:
src/txn/write/commit.rs. No dependency, lockfile, public API,storage-format, VFS, recovery-protocol, CI, or normal-path behaviour change.
The defect
Reproduced against
mainby taking itscommit.rsunmodified and appendingonly the regression test:
An invariant meant to pinpoint allocator corruption was itself producing an
outcome the caller could not act on: commit reported failure, the store said
otherwise.
Two smaller defects came with it. The check omitted the commit-history root,
even though retained history pages are live authenticated B+ tree pages in the
same allocator namespace. And
std::env::vartreated a present non-Unicodevalue as "disabled".
What the fix does
The invariant now runs after the stall policy accepts the commit and before any
durable effect. It checks three roots — data, catalog, commit-history — and for
each nonzero root performs two walks whose results are both authoritative.
On violation the four writer fields the candidate advanced are restored and the
commit panics, as the existing opt-in contract requires.
Dropthen discardsthe dirty pages, the durable header and free-list root are untouched, and the
next writer rebuilds its allocator caches from the unchanged durable chain.
Why exactly four fields
InvariantRollbackcapturesnext_page_id,commit_history_root_page_id,commit_history_root_version, andcommit_history_count. Those are preciselywhat tree materialization and
write_commit_history_entryassign. Everythingelse the pre-invariant stretch of
committouches is either transaction-localand dies with the
WriteTxn, or — like the sharedfree_page_cacheandfree_page_consumed— cleared and rebuilt from the durable free-list by thenext
begin_write, so it needs no rollback of its own.Restoring
next_page_idmatters beyond tidiness: without it the id spacedrifts upward on every rejected candidate, leaking page ids and growing the
file. It is safe because nothing durable references the abandoned pages.
Why both walks stay
find_danglingandcollect_all_page_idsare complementary, not redundant.find_danglingselects a node's decoder from the node body's own header byteand reports the first bad pointer together with its parent — that parent
context is what localizes a use-after-free.
collect_all_page_idsselects thedecoder from the authenticated envelope kind, so it additionally catches a
page whose envelope and body disagree, which the first walk cannot see.
Its error is propagated rather than dropped: a traversal failure returns a
short reachable set, and the freed-page check would then clear the commit
vacuously — the same false-clean this PR exists to remove.
Failures are a private typed enum (
Dangling,Traversal,ReachableFreed)so the helper is testable without parsing panic text. PageDB's public error
surface is unchanged.
Tests
invariant_failure_precedes_durable_publicationinvariant_rejects_a_reachable_commit_history_pageinvariant_rejects_an_unreadable_rootinvariant_rejects_a_root_whose_authenticated_kind_contradicts_its_bodyThe kind-contradiction test re-persists the fixture's real leaf root bytes under
the
BTreeInternalenvelope kind, assertsfind_danglingreturnsNone, andrequires a
Traversalviolation. That is the mis-tagged-page class, not acontrivance — and it documents why neither walk may be deleted as redundant.
The durable-publication test uses a subprocess because mutating a
process-global environment variable inside a parallel test process is unsafe.
The parent requires evidence the child actually ran:
libtest exits 0 when a filter matches nothing, so a bare
status.success()would keep reporting green after any rename of the child, the module, or the
file — silently deleting the regression. Confirmed by pointing the filter at a
nonexistent name: the hardened parent fails, the status-only version passed.
Verification
The invariant-enabled run is the one that matters: it exercises the newly
walked commit-history root on every commit in the suite, and confirms the
pre-publication check does not fire on healthy commits.
One of the five skipped tests is the ignored child helper, which its
non-ignored parent executes as a separate process.
Compatibility
With
PAGEDB_INVARIANT_CHECKSabsent the diagnostic stays off and the commitpath is unchanged — one environment probe, no state capture, no walk.
With it present, failure behaviour tightens deliberately: the commit fails
before durable publication instead of after, commit-history pages join the
live-versus-free check, a traversal failure is fatal at the pre-publication
boundary, and a present non-Unicode value enables the check.
The invariant remains diagnostic and proportional to the live trees; it is not
intended as an always-on production feature.
Known adjacent issue, deliberately not fixed here
read_node_guardreports an envelope/body kind disagreement asCorruptionDetail::HeaderUnverifiable, a variant documented as "main.db A/Bheader HK-MAC failed on both copies". That variant is used as a de-facto
generic malformed-bytes detail at roughly seventy sites across
catalog/,btree/,recovery/,rekey/, andsnapshot/. Repairing it here alone wouldmake one file inconsistent with the rest of the crate; doing it properly is a
crate-wide taxonomy change and belongs in its own PR.