fix(storage): complete metadata I/O and surface backend errors - #18
Merged
farhan-syah merged 7 commits intoJul 26, 2026
Conversation
A backend that reports transferring more bytes than the caller's remaining buffer has broken the read_at/write_at contract, not corrupted the on-disk format. Give that case its own error variant instead of a generic io::Error, and share the progress-checking logic between reads and writes. Also add a borrowed-handle variant of the read loop for callers that only have a &F, since a future holding &F across an await is Send only when F: Sync, which VfsFile does not require.
A file cut short mid-slot is the same recoverable condition as a slot that fails its HK-MAC check: the A/B protocol is designed to survive exactly one unusable slot. Add read_header_slot, which maps a short/EOF read to an absent slot so the caller's usual decode rejects it like any other damaged copy, and route both header-reading call sites through it so the surviving slot still opens the database.
Segment header/footer/index reads only borrow their file handle, so they were hand-rolling the same read-until-complete loop that read_exact_at already implements. Replace them with the read_exact_at_borrowed macro to remove the duplication.
Reading an entire prefix scan into memory to validate it at open, or to sum segment bytes for stats(), sizes an allocation by however many counters or segments the embedder happens to have. Add collect_prefix_batch_from to the B+ tree for bounded, resumable prefix scans, and stream both catalog walks through it. Also replace a saturating segment-bytes sum with a checked one that reports overflow instead of silently wrapping.
Replace the repeated literal 16 with SPILL_TAG_LEN, and read the tag into a fixed-size array by copying instead of a fallible slice conversion, since the split is already exact by construction.
farhan-syah
force-pushed
the
codex/upstream/metadata-error-propagation
branch
from
July 26, 2026 21:48
1ee68f3 to
57e56e5
Compare
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.
Summary
Completes positional reads and writes before treating authenticated metadata as
present or durable, and stops several public operations from translating real
filesystem failures into benign states.
VfsFile::read_atandwrite_atreturn the number of bytes transferred. Asuccessful call does not promise the whole caller buffer was completed, and
backends are allowed to satisfy a request in more than one call. The old
behavior was inconsistent: some readers rejected one short call, others
zero-filled or decoded the remainder, and writers commonly ignored the returned
length. That matters for every authenticated record — a short A/B header write
can report a commit durable before the inactive slot is complete, and a short
segment or spill write can publish metadata naming a truncated encrypted frame.
A second class of issue came from broad fallback branches: segment open mapped
every VFS error to
NotFound, compaction skipped every live-segment open error,stats()reported zeros when the underlying read failed, and mode acquisitiontranslated every lock failure into contention. Those translations make operators
diagnose the wrong condition.
No public API, on-disk format, feature flag, or dependency changes.
Exact positional I/O
read_exact_atandwrite_all_atretry from the exact byte offset after apartial transfer, reject zero progress as
UnexpectedEof/WriteZero, reject abackend reporting more bytes than the remaining buffer, and reject offset
overflow. Both directions share one progress validator, so the rule exists once.
Both take
&mut Frather than&F.read_atonly needs&self, but a futureholding
&Facross an await isSendonly whenF: Sync, whichVfsFiledoesnot require. The two segment paths that read through a handle they only borrow
use
read_exact_at_borrowed!, which runs the same loop in place — a functionwould have spread an
F: Syncbound through every segment reader for no gain.Callers now completing exactly: header bootstrap/open/commit, open-time and
restore-mode header probing, main-page reads, segment creation, data/index
writes and footer publication, segment header/footer/index/data reads, and spill
frame append and read. Spill ciphertext and its tag are handled as one
contiguous frame, so no handle is returned for a file holding half of one.
A backend reporting an impossible byte count is a contract violation, not an
on-disk defect. It surfaces as
PagedbError::VfsContractViolated { operation, detail }rather than a free-textio::Error; offset overflow reusesArithmeticOverflow.A truncated header slot is absent, not fatal
Requiring an exact read on both A/B slots would fail the open of a file
truncated mid-slot even when the other slot is intact — discarding the header
the protocol keeps a spare copy of precisely for this. A short slot read is the
same condition as a slot failing its HK-MAC: absent.
read_header_slotreportsit that way and lets the surviving copy win; only when neither slot is usable
does the header layer report corruption. Every other backend error propagates.
Honest error propagation
Segment open maps only an actual not-found result to
PagedbError::NotFound;permission and backend errors stay intact. Full compaction requires both the
live-segment open and its length query to succeed, so a catalog entry naming an
inaccessible file is no longer silently skipped.
Db::stats()returns an errorwhen the free-list chain, main file, or segment catalog cannot be read — a
metric value of zero once again means the observed value was zero. Lock
acquisition centralizes one narrow rule:
AlreadyLockedbecomes themode-specific contention error and every other error propagates unchanged.
Persisted metadata validation
Open recovery scans the named-counter prefix and decodes every authenticated
counter value. The scan is validation-only: it never derives a counter from the
page-nonce anchor and never rewrites catalog state. It runs for standalone,
follower, and read-only opens, because malformed authenticated metadata is not
safe merely because a handle lacks repair authority.
Commit-history keys are an exact eight-byte big-endian commit id. Both the
reclamation-floor query and the
begin_read_atfallback reject shorter andlonger keys as catalog corruption. The historical-read fallback uses
BTree::first_keyinstead of materializing the full retained-history index,preserving the O(height) intent.
Validation streams; it does not accumulate
How many counters an embedder has named, or segments a realm has linked, is the
embedder's business — neither open nor a metrics call may size an allocation by
it.
BTree::collect_prefix_batch_fromis the bounded counterpart toscan_prefix: rows sharing a prefix sort contiguously, so it stops at the firstkey outside the prefix rather than reading the rest of the tree. Counter
validation and the
stats()segment aggregation both page through it in boundedbatches.
Historical finding disposition
Restores direct current-architecture regression proof for BUG-032 through
BUG-039, BUG-044, BUG-045, BUG-048 through BUG-063, BUG-099, and BUG-100.
BUG-041 is adapted rather than copied literally. The old recovery path treated
the page-nonce anchor as if it were a named application counter; the corrected
tests prove both sides of the actual contract — read-only open performs no
counter rewrite, and a higher nonce anchor does not overwrite a persisted named
counter.
BUG-040, BUG-042, BUG-043, and BUG-047 are deliberately excluded. Upstream
commit
f30565eintentionally removed catalog-backed reader pins. CurrentPageDB publishes one atomic reader snapshot, admits readers through the
visibility gate, tracks active readers in the owning
Dbhandle, and useslong-lived mode locks for cross-process exclusion. Reintroducing catalog pin
inserts, stale-pin cleanup, or compaction scans of removed durable-pin rows
would add durable writes to
begin_readand recreate recovery state the currentdesign removed. The applicable admission, compaction, tombstone, and
cross-process lock behavior remains covered by current upstream tests.
Regression proof
The historical test module was first run against untouched production code: 32
executed, 13 passed, 19 failed. Exact positional I/O fixed ten; open,
compaction, stats, and lock propagation fixed the rest bar the three obsolete
durable-pin assumptions, which were ruled out rather than transplanted.
Tests inject one-shot short transfers and one-shot failures at the exact
boundary under review, and each verifies both halves of the contract: the
operation does not report success or a benign substitute before the required
state is complete, and a retry succeeds and produces usable state. This checks
the externally observable database result rather than mock call counts.
Four further RED tests exercised the metadata findings —
stats()returned asuccessful
DbStatswhile omitting a malformed row's bytes, open recovery hadno counter-row validation boundary, the retained-history reuse floor silently
disabled itself on a malformed key, and
begin_read_atpanicked slicing aone-byte key. All four now return typed
PagedbError::Corruptionresults, forboth undersized and oversized history keys.
Header-slot truncation is covered directly: one test proves an intact slot still
opens a file truncated inside the other, and one proves a file with no readable
slot still reports corruption rather than leaking an end-of-file error.
Non-goals
There is deliberately no retry of a failed or short page read inside the
pager's read loop. That loop absorbs a torn read — a full-length buffer of
mixed old and new bytes, which is why retrying can succeed. A transfer that ends
short or errors is not that condition, and because
observer_retry_countdefaults to 3 for every handle opened with default options, retrying it would
silently mask a one-shot backend fault.
stats_surfaces_free_list_read_error_ then_retry_succeedspins that boundary.Compatibility and risk
The primary compatibility change is intentional: operations that previously
returned a benign value after an I/O failure now return the original error.
Callers already handling PageDB errors need no change; callers that treated a
zero statistic or
NotFoundas authoritative now receive the accurate failure.The exact-I/O loops are bounded by the caller buffer and reject non-progress, so
a broken backend cannot cause an infinite loop. Existing VFS implementations
remain source-compatible — no trait method or bound changed.
Open performs one authenticated prefix scan over persisted counter rows, adding
work proportional to the number of named counters but bounded in memory, and
avoiding deferral of corruption discovery until an arbitrary counter is first
accessed. The scan performs no writes and does not traverse unrelated catalog
row kinds. The historical fallback becomes cheaper, reading only the leftmost
B+ tree path instead of collecting the complete history.
Verification
On this branch, rebased onto
mainata812d42:cargo nextest run --all-features --no-fail-fast: 531 passed, 5 skipped;PAGEDB_INVARIANT_CHECKS=1: 531 passed, 5 skipped;cargo fmt --all --check: pass;cargo clippy --all-targets --all-features -- -D warnings: pass;cargo check --no-default-features: pass;wasm32-unknown-unknown --features opfsandaarch64-apple-darwinchecks:pass;
cargo doc --no-deps --all-features: no new warnings (the seven pre-existingbroken intra-doc links are unchanged);
Review focus
src/vfs/traits.rs— progress, overflow, and the borrowed-handle rule;src/pager/header.rs— treating a short slot as absent rather than fatal;src/segment/andsrc/txn/write/spill.rs— authenticated completion andcontiguous frame handling;
src/txn/db/open/modes.rs,misc.rs,src/compaction/full.rs— narrow errorclassification;
src/btree/tree/scan.rs,src/txn/db/catalog.rs— bounded prefix streaming;tests/durability/metadata_errors.rsandtests/header.rs— one-shotfailure, retry, corruption, and state-preservation proof.
The change favors rejection over partial recovery whenever an operation cannot
prove the state it is about to report.