Skip to content

fix(storage): complete metadata I/O and surface backend errors - #18

Merged
farhan-syah merged 7 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/metadata-error-propagation
Jul 26, 2026
Merged

fix(storage): complete metadata I/O and surface backend errors#18
farhan-syah merged 7 commits into
NodeDB-Lab:mainfrom
presempathy-awb:codex/upstream/metadata-error-propagation

Conversation

@presempathy-awb

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

Copy link
Copy Markdown
Contributor

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_at and write_at return the number of bytes transferred. A
successful 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 acquisition
translated 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_at and write_all_at retry from the exact byte offset after a
partial transfer, reject zero progress as UnexpectedEof / WriteZero, reject a
backend 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 F rather than &F. read_at only needs &self, but a future
holding &F across an await is Send only when F: Sync, which VfsFile does
not 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 function
would have spread an F: Sync bound 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-text io::Error; offset overflow reuses
ArithmeticOverflow.

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_slot reports
it 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 error
when 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: AlreadyLocked becomes the
mode-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_at fallback reject shorter and
longer keys as catalog corruption. The historical-read fallback uses
BTree::first_key instead 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_from is the bounded counterpart to
scan_prefix: rows sharing a prefix sort contiguously, so it stops at the first
key outside the prefix rather than reading the rest of the tree. Counter
validation and the stats() segment aggregation both page through it in bounded
batches.

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 f30565e intentionally removed catalog-backed reader pins. Current
PageDB publishes one atomic reader snapshot, admits readers through the
visibility gate, tracks active readers in the owning Db handle, and uses
long-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_read and recreate recovery state the current
design 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 a
successful DbStats while omitting a malformed row's bytes, open recovery had
no counter-row validation boundary, the retained-history reuse floor silently
disabled itself on a malformed key, and begin_read_at panicked slicing a
one-byte key. All four now return typed PagedbError::Corruption results, for
both 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_count
defaults 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_succeeds pins 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 NotFound as 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 main at a812d42:

  • cargo nextest run --all-features --no-fail-fast: 531 passed, 5 skipped;
  • the same suite under 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 opfs and aarch64-apple-darwin checks:
    pass;
  • cargo doc --no-deps --all-features: no new warnings (the seven pre-existing
    broken intra-doc links are unchanged);
  • checklist-scaffold gate: pass.

Review focus

  1. src/vfs/traits.rs — progress, overflow, and the borrowed-handle rule;
  2. src/pager/header.rs — treating a short slot as absent rather than fatal;
  3. src/segment/ and src/txn/write/spill.rs — authenticated completion and
    contiguous frame handling;
  4. src/txn/db/open/modes.rs, misc.rs, src/compaction/full.rs — narrow error
    classification;
  5. src/btree/tree/scan.rs, src/txn/db/catalog.rs — bounded prefix streaming;
  6. tests/durability/metadata_errors.rs and tests/header.rs — one-shot
    failure, 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.

presempathy-awb and others added 7 commits July 27, 2026 05:28
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
farhan-syah force-pushed the codex/upstream/metadata-error-propagation branch from 1ee68f3 to 57e56e5 Compare July 26, 2026 21:48
@farhan-syah
farhan-syah merged commit cd32f61 into NodeDB-Lab:main Jul 26, 2026
19 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