Skip to content

fix(txn): validate freed pages before durable publication - #14

Merged
farhan-syah merged 3 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/freed-page-invariant
Jul 26, 2026
Merged

fix(txn): validate freed pages before durable publication#14
farhan-syah merged 3 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/freed-page-invariant

Conversation

@presempathy-awb

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

Copy link
Copy Markdown
Contributor

fix(txn): validate freed pages before durable publication

Summary

PageDB's opt-in freed-page invariant (PAGEDB_INVARIANT_CHECKS) ran after
commit_header had already swapped the durable A/B header. When it fired, the
caller 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 main by taking its commit.rs unmodified and appending
only the regression test:

PAGEDB INVARIANT VIOLATED: commit 2 freed page 4 but it is still reachable ...

assertion `left == right` failed:
an invariant failure must not durably publish its candidate commit
  left: 2
 right: 1

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::var treated a present non-Unicode
value 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. Drop then discards
the 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

InvariantRollback captures next_page_id, commit_history_root_page_id,
commit_history_root_version, and commit_history_count. Those are precisely
what tree materialization and write_commit_history_entry assign. Everything
else the pre-invariant stretch of commit touches is either transaction-local
and dies with the WriteTxn, or — like the shared free_page_cache and
free_page_consumed — cleared and rebuilt from the durable free-list by the
next begin_write, so it needs no rollback of its own.

Restoring next_page_id matters beyond tidiness: without it the id space
drifts 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_dangling and collect_all_page_ids are complementary, not redundant.

find_dangling selects a node's decoder from the node body's own header byte
and reports the first bad pointer together with its parent — that parent
context is what localizes a use-after-free. collect_all_page_ids selects the
decoder 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

Test What it pins
invariant_failure_precedes_durable_publication A rejected candidate is neither reported nor durably published
invariant_rejects_a_reachable_commit_history_page The history tree participates in the live-versus-free check
invariant_rejects_an_unreadable_root The strict dangling-pointer diagnosis is preserved
invariant_rejects_a_root_whose_authenticated_kind_contradicts_its_body A traversal failure fails closed, and the two walks are provably distinct

The kind-contradiction test re-persists the fixture's real leaf root bytes under
the BTreeInternal envelope kind, asserts find_dangling returns None, and
requires a Traversal violation. That is the mis-tagged-page class, not a
contrivance — 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:

assert!(report.contains("1 passed"), "the child helper did not run; ...");

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

cargo nextest run --all-features --no-fail-fast
  446 passed; 5 skipped

PAGEDB_INVARIANT_CHECKS=1 cargo nextest run --all-features --no-fail-fast
  446 passed; 5 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-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_CHECKS absent the diagnostic stays off and the commit
path 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_guard reports an envelope/body kind disagreement as
CorruptionDetail::HeaderUnverifiable, a variant documented as "main.db A/B
header 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/, and snapshot/. Repairing it here alone would
make one file inconsistent with the rest of the crate; doing it properly is a
crate-wide taxonomy change and belongs in its own PR.

@presempathy-awb
presempathy-awb force-pushed the codex/upstream/freed-page-invariant branch from 6453f17 to 7ea444a Compare July 26, 2026 14:57
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.
@farhan-syah
farhan-syah merged commit 4a6c853 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