feat(salvage): total recovery for SSTs and blob files with in-place ECC autoheal - #587
feat(salvage): total recovery for SSTs and blob files with in-place ECC autoheal#587polaz wants to merge 140 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 130 files, which is 30 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (130)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c1ccabef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…CC autoheal Hardens recovery, salvage, and manifest repair into a deterministic pipeline that always yields a valid, openable tree, for both SST tables and vlog blob files. - Block-level SST salvage keeps readable blocks and drops corrupt ones, re-emitting survivors under the tree's comparator, encryption, and dictionary context; a fast surface copies clean blocks verbatim and heals single-block ECC in place, a faithful surface decodes and rewrites. - Blob-file salvage gives vlog files the same record-granular treatment, reporting dropped records and an offset remap so surviving value handles can be re-targeted instead of invalidated. - Manifest recovery by directory scan is total: a punched SST is recovered restricted to its live suffix (sidecar-proven bound, else derived from the punch geometry), unrecoverable files are set aside, and a table whose blob file cannot be recovered is excluded rather than left dangling. - One policy knob, allow_resurrection (default drop), governs the ambiguous cases; no branch requires a manual step. - Corrected reads emit heal hints and a background pass rewrites healed blocks under fresh parity, guarded by a tri-state attestation marker. - Checkpoints sync reflinked clones before publishing them, and prefix reclamation never punches an inode shared with a checkpoint. - A bitrot heal fuzzer and its CI job assert the read path never panics and never returns a wrong value, dumping the exact failing SST for repro. Closes #568 Closes #570
- actions/checkout, actions/github-script, actions/create-github-app-token, dependabot/fetch-metadata move from full-SHA pins to major version tags: first-party GitHub orgs gain nothing from SHA pinning, while every patch release churns a Dependabot bump PR - third-party actions (dtolnay, Swatinem, taiki-e, codecov, release-plz, benchmark-action) stay SHA-pinned: they are the actual supply-chain risk
A tight-space-restricted columnar SST has its consumed prefix hole-punched, so those data blocks read as zeros. Table::columnar_scan walked the complete block index and tried to decode the punched prefix, failing the whole scan with a block-header error instead of serving the live suffix. - Skip data blocks wholly below the restriction bound (the same key-based clamp the row-oriented scans apply), advancing the positional row base by each skipped block's zone-map row count so delete-bitmap positions keep addressing the same rows - Mask the straddling block's sub-bound rows (the punch is block-aligned, the bound is a key) - Fail loudly when a skipped block has no zone-map row count while positional deletes are present, instead of silently desyncing every later delete position (this also hardens the pre-existing predicate-skip path) Carries a regression test that punches a columnar SST's prefix and scans the restricted view, with deleted rows on both sides of the bound
A flush / compaction-produced columnar segment physically stores every MVCC version of an overwritten key. The tree-level scan's singleton path (a segment overlapping no other) streamed those rows verbatim, so an all-visible scan returned stale duplicate versions, and a predicate could resurrect an older matching version when the newest one failed it — the newest-wins dedup ran only in the overlap-merge path. - ParsedMeta now reads the writer's (already persisted) key_count meta key, optional for legacy tables; key_count == item_count proves one version per key - Singletons that can hold duplicates get per-key newest-visible dedup: rows sit in internal-key order (key asc, seqno desc), so the first visible row of each key run is the newest visible version, with the run carried across batch boundaries - The predicate runs AFTER dedup on that path, mirroring the merge path (and zone-map predicate skip is disabled there for the same reason) - Provably-unique segments (bulk ingest, fully deduped compaction output) keep the zero-copy verbatim fast path Carries regression tests for the duplicate-version scan, the straddling snapshot, and the predicate-after-dedup ordering
A tight-space relocation's blob frontier lives only in the manifest's blob_restrictions record. When the manifest was lost, recover_blob_files rebuilt every blob with frontier 0 and a whole-file digest over the zeroed prefix: repair reported success, but a later relocation scan started inside the punched region and errored (or reached the live suffix only through a tainted resync that relocation rejects). Unlike the SST bound (a key, unrecoverable from the block-aligned punch and therefore carried by its sidecar), the blob frontier is a byte offset at a frame boundary, so the punch geometry recovers it exactly: - an unpunched file short-circuits to frontier 0 on its first non-zero data byte (zero extra read cost on the common path) - a zeroed data-section prefix is walked as zero runs anchored by validly decoding frames; the frontier is the end of the LAST anchored run, so a partially completed punch (intact-but-consumed frames between holes) still resolves, while a zero-filled value payload inside the live suffix can never move the frontier (structure-anchored, not run-length-anchored) - the recorded digest covers the live suffix from the derived frontier, matching what reopen_restricted records, and the snapshot encoder re-persists the restriction from the recovered live_data_start - transient I/O keeps propagating for retry on every new read The transient-propagation test now writes a real blob file (a garbage file classifies persistent-unreadable at the probe, before any faulted read) and arms WouldBlock instead of Interrupted, which std's read_exact would retry forever. Carries a regression test punching a real blob file's first frame and asserting the derived frontier, the suffix digest, and the unpunched sibling's frontier 0
…onale - manifest-recovery.md gains the blob-frontier resolution rules: geometry recovers the byte-offset frontier exactly (structure-anchored zero runs), unlike the SST key bound which needs its sidecar - the VersionEdit encoder documents why the appended blob-frontier section is deliberately not behind a bumped format version: a binary without blob-restriction support cannot serve a punched store anyway, so its trailing-data rejection is the fail-fast, while rollback stays possible for stores that never used tight-space blob reclaim
A manifest repair over a large store runs for minutes to hours with no liveness signal until the final report. RecoveryProgress is the observation seam: a shared handle of relaxed atomic counters, wired via Config::with_recovery_progress (repair) or SalvageOptions::progress (standalone salvage), polled via snapshot() from any other thread. Ticked as the work proceeds: - table / blob files as they are discovered and recovered by the scan - data blocks as the salvage walk inspects, re-emits, or drops them (published per block, so continue-heavy walk paths stay covered) - ECC-healed blocks (SalvageBlock now carries an explicit ecc_recovered flag; verbatim-ineligible clean reads no longer alias with heals) - KV entries and columnar columns as recovered rows are emitted Without a handle the paths publish nothing (zero overhead). Patrol-scrub in-place heals stay observable through the metrics counters instead. Carries a behavioural test covering table, salvage-walk, and blob-file counters through a real manifest-loss repair
…ring - nest the scanner's or-pattern, switch the chunked zero-scan cast to allow (target-width-dependent lint, expect is unfulfilled on 64-bit) - SalvageOptions test initializers gain the new progress field - test lint hygiene: no indexing, no redundant clone, expect() gated
30c1cca to
1ddd0e8
Compare
|
@codex review |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
coordinode-lsm-tree/src/vlog/mod.rs
Lines 236 to 237 in 1ddd0e8
Manifest repair takes the numeric filename as id, but this recovery helper parses the authenticated metadata without checking meta.id == id and then constructs the BlobFile under the filename's ID. If blob files were renamed or swapped while the manifest was unavailable, repair therefore publishes a valid-looking manifest that routes SST handles to the wrong physical file; when the misplaced file happens to contain the same key and compatible offset/length, the reader can return its stale value because it validates the key and frame checksum but not the frame seqno against the SST entry. Treat an ID mismatch as an unreadable file and quarantine it rather than rebinding its contents.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Every VersionEdit carries the FULL restriction set of its version (the encoder derives restrictions and blob_restrictions by iterating the new version's tables / blob files), but replay merged entries into the recovered maps instead of replacing them. A removed restricted blob file's frontier therefore outlived it, and because blob file ids are reused (the id counter reseeds from the maximum live id), the stale frontier attached to an unrelated whole file added later under the same id: integrity checks hashed only its suffix and the next snapshot persisted the bogus restriction. The SST map had the same latent carry-forever behaviour, shielded only by never-reused table ids. Replay now replaces both maps from each edit, which advances a bound per slice exactly as before (the later full set carries the higher bound) and additionally drops lifted entries. Carries regression tests for a removed blob file's frontier (including id reuse by an unrestricted replacement) and a lifted SST restriction
A blob file whose frontier probe, streaming checksum, or metadata recovery failed persistently was reported unreadable but left in blobs/ while the rebuilt manifest omitted its id. The next successful open then classified that path as an orphan and DELETED it, so the report's path vanished and the operator lost the only source from which intact records might later be salvaged. Such files now move to the repair quarantine directory before the unreadable report is recorded, exactly like the unreadable-SST path; a failed quarantine aborts the repair (the file must not be both omitted and left in place). Transient I/O keeps propagating for retry. Carries a regression test asserting the bad blob leaves blobs/ and lands in repair-quarantine
Two directory entries parsing to the same blob id (1 and 01) were handled by silently skipping the second: both files stayed in blobs/ while the rebuilt manifest recorded one checksum, so on the next open directory iteration order decided which physical file served reads — a stale duplicate could shadow the recovered one, and both carried the kept file's manifest checksum. The scan now collects and orders candidates before recovering (id, canonical spelling first, then name), so resolution no longer depends on FS iteration order; a second entry that physically ALIASES the kept file (symlink / case-folded spelling) is still skipped silently, while a distinct physical duplicate is quarantined and reported, mirroring the SST duplicate treatment. A candidate whose canonical sibling failed recovery still gets its own chance. Carries a regression test with two distinct valid blob files named 1 and 01: the canonical file is kept and its checksum recorded, the duplicate leaves blobs/
dropped_data_extent_is_zeroed accepted ANY header-length zero run inside a surrendered extent as hole-punch evidence. SST values are arbitrary bytes, so an ordinary unpunched table whose value embeds a header-sized zero run after the damaged offset satisfied the length test, and under the default no-resurrection policy repair then quarantined an otherwise usable salvaged replacement as punched-bound-lost. A qualifying run now also has to be structure-anchored: it counts only when it ends where intact structure begins — a decodable block header (magic + type + the header's own checksum), the next dropped extent, or the data-section end. A punched block's run always terminates at one of those; a zero run inside a value payload ends mid-payload at bytes that do not decode. Carries a regression test with an unpunched SST whose values embed header-sized zero runs (with a fixture self-check proving the raw bytes really contain such a run), alongside the existing deep-punch detection test that now exercises the header-anchor accept path
In a KV-separated repair, salvaged was derived from the candidate map before the pass that quarantines tables referencing unrecoverable blob files. A block-salvaged table dropped by that filter still counted, so the report could claim salvaged > recovered — telling an operator data was restored when the salvaged copy was in fact set aside. Candidates now carry their completeness through the dependency filter, and salvaged is derived from the tables that actually land in the rebuilt manifest, keeping it a true subset of recovered. Carries a regression test with a block-corrupt SST whose blob files are wrecked: recovered == 0 and salvaged == 0
…n fails The tight-space relocation re-opens each consumed stale blob file as a restricted view (hashing its live suffix) after run_subcompaction has finalized the slice's output SSTs and blob files — but before the pre-install rollback closure existed, so a reopen failure propagated without retracting those unreferenced outputs. They leaked until the next open's orphan sweep, pinning disk space under the exact condition that engaged tight-space in the first place: scarce free space, where persistent read errors plus retries could consume the remainder. The reopen loop now sits below the rollback closure and routes its failure through it, exactly like the restricted SST reopen. Carries a regression test driving a real sliced blob relocation into an injected reopen failure (a test-only failpoint at the precise step) and asserting the tables/ and blobs/ file sets are unchanged afterwards
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86487633e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…bust The rollback regression test asserted exact before/after equality of the tables/ and blobs/ file sets. On CI (different compaction thread counts and merge scheduling) earlier merges / slices of the same compaction legitimately install before the failpoint fires — their outputs are referenced and their consumed inputs deleted — so the exact-equality form was flaky across hosts while the fix itself held. The assertion now checks the actual leak-free invariant: every file that appeared since the snapshot must be referenced by the current version. A missed rollback still fails it (verified by surgically reverting the rollback locally: the leaked output SST is caught as neither pre-existing nor referenced)
The live tables_recovered counter ticked when an intermediate candidate was recorded, before duplicate displacement and blob-dependency filtering. A candidate later quarantined by either pass stayed counted (the cumulative counter never decrements), so the progress snapshot could report more recovered tables than the rebuilt manifest holds, contrary to the field's documented meaning. The counter is now published once, from the final survivor set — the same derivation the report's recovered/salvaged counts use — so it can never overcount. Per-file liveness during the scan stays covered by tables_discovered and the per-block salvage counters. Extends the blob-filtered-salvage regression test with a progress handle asserting tables_recovered == 0
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7c8252f5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ontier The blob-dependency filter only checked that a referenced blob file ID was recovered. A pre-relocation SST file left behind by a crash (its manifest removal committed, its file deletion not yet run) still holds ValueHandles into the prefix the relocation punched; with the manifest lost, repair republished that SST because the blob id exists, and a read through such a handle dereferences zeroed bytes. The same held for an all-zero corrupt region recovered as a fully consumed blob. When at least one recovered blob file carries a derived frontier, each candidate table's indirections are now scanned and a table holding a handle below its blob's live-data frontier is set aside with a report, exactly like the missing-blob-id case. With no punched blobs recovered (the common path) the scan does not run at all. Also reattaches salvage_blocks' rustdoc and cfg_attr to the function itself; an earlier edit had left them on the PublishedProgress helper. Carries a regression test: a punched first blob frame plus a pre-relocation SST and a lost manifest must yield recovered == 0 with the stale handle named in the report
…ling unshare_for_heal reproduced a restricted table's hole pattern from the LOGICAL restriction bound alone, treating every data block below the punch offset as an existing hole. A tight-space slice that committed but failed its restriction-sidecar write deliberately leaves the input unpunched (punching without the sidecar would force a lossy conservative bound on a later manifest-loss repair) — yet a heal detach of such a table omitted its intact prefix blocks from the copy, physically punching the file without the required sidecar. Each candidate extent is now probed against the source: an all-zero extent (genuinely punched) stays a hole so the reclaimed space is not re-allocated on the near-full tight-space disk, while an intact sub-bound block (its header magic alone reads non-zero) is copied verbatim, preserving the committed-but-unpunched state the recovery rules depend on. Carries a regression test covering both states: an unpunched restricted table's prefix survives the detach byte-for-byte, and a genuinely punched extent remains a hole
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
coordinode-lsm-tree/src/tree/columnar_scan.rs
Line 163 in 8b5fce0
When columnar mode flushes rows together with a remove_range, the resulting SST retains that range tombstone until a later compaction converts it into a delete bitmap. This selection loop includes the SST, but no subsequent path reads its range tombstones, so columnar_scan returns covered rows that normal reads suppress. Apply snapshot-visible range tombstones while merging the selected segments, or reject RT-bearing segments instead of returning deleted data.
coordinode-lsm-tree/src/checkpoint.rs
Lines 715 to 717 in 8b5fce0
When a caller chooses a fresh target beneath the source tree's tables/ or blobs/ directory, this proceeds to create the checkpoint there without checking its relationship to src_root. For example, $DB/tables/checkpoint succeeds but leaves a nonnumeric directory among the SST files; the next source-tree open scans that entry as a table ID and fails recovery. On a shared filesystem namespace, reject targets that resolve inside the source's live data directories before creating them.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lost `lost_coverage` published the on-disk LOCAL maximum as an excluded table's highest sequence number. For a bulk-ingested SST that is normally zero: every entry sits at local seqno 0 and the real base lives in a manifest-only offset, which the repair scan deliberately opens with as `0` — and whose loss is precisely why such a table is quarantined. An operator scoping the possibly superseded history by that value would stop far below the affected range. The bound is now `Option<SeqNo>` and reported as `None` for exactly those tables, so the whole history of that key range reads as affected. The key range itself stays knowable and is still reported. Carries a regression test on a quarantined bulk-ingested table asserting the range is present and the bound is unknown.
A projected seqno column carried the value as STORED. A bulk-ingested segment stores every row at local seqno 0 and takes its ordering from a per-segment offset, so a caller reading that column got `0` — a commit sequence number the tree never had, and one no other read surface would agree with. None of the emit paths rewrote the column. Every path now translates it: the verbatim, masked and dedup paths add their segment's offset, while the overlap path writes each surviving row's effective seqno — already computed for its newest-version dedup — since that union spans segments with different offsets. Masking still compares in local space (one subtraction per segment rather than one addition per row), so only the emitted column is translated, and a segment with a zero offset is untouched. Carries a regression test over two ingested segments asserting each row carries its own segment's effective seqno.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d78d47910c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The direct-block order guard demanded a strictly decreasing seqno for equal user keys, in-block and across the block edge. One write batch may add several merge operands for a key, though, and they share the batch's seqno — a flush stores every one of them at it. Salvage triggered by damage elsewhere in the SST therefore rejected an otherwise clean block as out of internal-key order and DROPPED it, losing valid operands and changing the value the repaired tree merges to. An equal seqno is now valid when both sides are merge operands, on both the in-block window and the cross-edge check. Every other equal-seqno pair stays a violation: for those kinds the internal key ties with no tie-breaker, so their order — and the value a read serves — would not be reproducible. Carries a regression test per position: the operands sharing one block, and the run straddling a block boundary.
… presence The republish check only asked whether the sidecar file EXISTS. Presence proves nothing about content: a truncated or checksum-corrupt sidecar passed, and so did a valid-but-STALE one — the shape a second tight-space slice leaves when its own write fails, recording a LOWER bound than the manifest holds. A later manifest-loss repair then either derives a conservative bound (dropping up to one live block) or, worse, honors the stale one and restricts less than reality, resurrecting consumed rows. The sidecar is now read and compared against the manifest: absent, unreadable, or disagreeing (stale bound, or another table's id) all republish, while one that already records this table and this bound is left alone so a healthy tree does not churn the file on every open. Carries a regression test that plants a valid sidecar with a stale bound and asserts the next open replaces it with the manifest's.
Stopping the top-down pass at the first failure is right — punching below an unreclaimed extent breaks the hole pattern a sidecar-less repair reads — but the whole queued item went with it, including the failed extent and every one below it. Nothing could retry them, so the consumed prefix stayed allocated until the table was retired, under the low-space condition that chose the tight-space path. The pass still stops; the failed extent and the untried remainder are now retained for the same retry that already covers a shared inode. A punch failure is usually transient, so the next drain or tight-space compaction completes the reclaim. Carries a regression test with three extents whose middle punch fails once: the top one lands, the rest are retained, and the retry reclaims them.
There was a problem hiding this comment.
💡 Codex Review
coordinode-lsm-tree/src/tree/columnar_scan.rs
Lines 712 to 715 in 218e68d
When overlapping columnar SSTs contain the same key at the same effective seqno, this comparator declares the rows equal and the later dedup keeps whichever segment happened to be combined first. group_by_overlap orders segments by minimum key rather than table recency, so an older SST containing a,k@10 can precede a newer SST containing b,k@10; columnar_scan then returns the older value for k even though the normal read path serves the newer L0 source. Carry source precedence into the row metadata and use it as the tie-break after effective seqno.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Boundary suppression decides key identity, so it must use the tree's comparator. Under a comparator that folds spellings together (case folding, locale collation), the lost newest version can be spelled `A` while the older version that follows is spelled `a`: byte comparison keeps the older one and salvage republishes a version its newer one had already replaced. `suppress_shadowed_boundary` and `suppress_columnar_boundary` now take the table's `SharedComparator` and drop every entry the comparator calls equal to the shadowed key, on both the row and the columnar emit paths. Carries a regression test with a case-folding comparator, red before the change: the older `a` survived the loss of `A`.
`scan_since_seqno` advertises a consistent snapshot of `[target, watermark]`, but the seqno cap alone cannot deliver one: `apply_batch` takes the seqno from the caller, so a write can commit at or below the cap after the cap was taken. Walking the lock-free memtable live then sees that write or misses it depending on where its node lands relative to the cursor, splitting a single batch across the boundary, and a consumer that advanced past the watermark loses the remainder for good. The cap, the active memtable's entries and its range tombstones are now taken together under the version-history write guard, which excludes writers; the guard is released before mapping, since resolving blob indirections is I/O. Carries a regression test that starts a writer from inside the capture and asserts it cannot commit, plus the resulting scan holding exactly the pre-scan state. Red before the change with a read guard.
There was a problem hiding this comment.
💡 Codex Review
coordinode-lsm-tree/src/tree/columnar_scan.rs
Lines 712 to 715 in 287f624
When overlapping columnar SSTs contain the same key at the same effective seqno but with different values or a newer tombstone—reachable because callers assign seqnos—this comparison returns Equal, so the stable sort keeps their concatenation order. group_by_overlap orders segments by minimum key rather than source precedence, meaning an older table with a smaller minimum key can win and columnar_scan can return a value that the normal point-read path shadows with the newer L0 source. Carry source recency into the row metadata and use it as the tie-break after effective seqno.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A repair run had a third outcome: files moved into a sibling
`repair-quarantine/` directory for someone to deal with later. That is
the "recovery dead-ends in fix-it-by-hand" anti-pattern, and it broke
re-derivability in two concrete ways.
The scan displaced sources BEFORE the manifest commit, so a crashed run
handed its retry a weaker world than the one it was given: the source of
a table was no longer under `tables/`, where the retry scans, and its
keys silently vanished from the rebuilt manifest. And a marked set-aside
was read BACK by a later run, making the resurrection flag a state
machine spread across runs instead of an input to one.
A run now has exactly two outcomes: a committed tree that opens, or an
error.
- The scan mutates nothing. Every file the rebuilt manifest will not name
is recorded and removed AFTER the commit: a foreign name, a duplicate
id, a table no bound can make safe, a source its replacement
supersedes. A removal the filesystem refuses fails the repair, because
the file left behind is an orphan the next open must sweep and an open
that cannot sweep it does not open.
- A table's replacement is built at `{id}.repair-tmp` — a name no scan
adopts — and swapped onto `{id}` once the manifest naming it is
durable. Publishing under a fresh id instead would leave a crash with
the source AND its half-published copy both readable, and the retry
would rebuild one history into L0 twice, applying its merge operands
twice on read. A leftover temp is garbage: a run whose manifest names
its id finishes the swap, any other run drops it.
- The resurrection flag decides within the run that reads the bytes.
Nothing is stashed for a later run to reconsider.
Recovering the CONTENT of a damaged file is replication, a checkpoint
plus journal replay, or a backup. It was never a directory hidden beside
the tree: nothing ever read it back except the mechanism that broke
re-derivability.
Test suite and docs follow the model, including a regression test that a
run failing before its commit leaves the source exactly where the retry
finds it.
The blob-handle rewrite drops every older version of a key whose newest record the salvage lost — the lost record was the chain's head, so the survivors would republish a value its newer version had replaced, or undo a deletion. It decided "same key" by byte equality. Under a comparator that folds spellings together, the headless `A` did not suppress the older `a` that followed it: the next iteration cleared the suppression and kept that version, resurrecting exactly what the no-resurrection policy exists to prevent. `rewrite_block_indirections` now takes the table's comparator, matching the boundary suppression on the row and columnar paths. Carries a regression test with a case-folding comparator, red before the change: the older `a` survived the loss of `A`.
A deferred reclaim probed the link count and then punched, with nothing holding a checkpoint out of the gap between the two. A table's or blob file's `Drop` is protected by the live version object whose lifetime spans that sequence; a queued reclaim holds no such object. So a checkpoint starting right after the probe read `1` hard-linked the file, and the punch then zeroed the inode its immutable snapshot shares — corrupting a snapshot under the digest its manifest already recorded. `reclaim_or_retain` now holds the mutation window across the whole probe-and-punch sequence, which is what a checkpoint's link window excludes, so no checkpoint generation can open inside it. Carries a regression test that opens a link window and asserts the retry blocks until it closes; red before the change, where the punch ran straight through the open window.
`derive_blob_frontier` promoted a zeroed run to punch geometry on structure alone — a frame decoding cleanly at the run's end. Ordinary corruption that zeroes the leading blob records and leaves the next one intact has exactly that shape, so repair fabricated a frontier, dropped every handle below it, and reported success over the loss. A data section zeroed end to end went further and read as a completed relocation, which REMOVES the file and every table referencing it. A punch DEALLOCATES, so each zero run must read back as a hole. The run is probed at its midpoint (its ends can reach into the allocated blocks around the hole), and a backend that cannot answer leaves it unproven, which keeps the zeros classified as damage. This is the rule the SST classifiers already apply; the blob path never got it. Carries two regression tests — a zeroed prefix and a wholly zeroed section, both fully allocated — red before the change, where they read as `Punched` and `FullyConsumed`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e62b9b48e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
"Is this the same key as the previous one?" was answered by a bare `==` in twelve places — MVCC version collapsing on reads and in compaction, the writer's key count, the row cache, CDC event ordering, and three salvage paths. Nothing named the relation or said why bytes are the right answer, so each site looked like an oversight a reviewer could file against, and two of them had already been "fixed" to ask the comparator instead — leaving salvage deciding identity differently from the read path it exists to reproduce. Bytes are not a shortcut for ordering. The `UserComparator` contract requires `compare(a, b) == Equal` to imply byte equality, so for any valid comparator the two relations are the same one, and the invariant is not negotiable: filters and the locator index are built over `hash64(user_key)` of the raw bytes, so a comparator equating two spellings would make a point lookup for one hash to an entry the other never wrote. No identity relation chosen downstream can repair that — an engine cannot hash an equivalence class it cannot enumerate. `same_user_key` now states this once, next to the contract it rests on, and every site calls it. The comparator threading added to salvage is gone with the two tests that exercised a contract-violating comparator. No behaviour changes for any valid comparator.
Ten methods across KeyRange, Run, and Level existed twice: a bytewise body and a `_cmp` body differing only in how two keys are ordered. The byte twins had no production callers — every live path already passes the tree's comparator — so they survived purely as a place for the next fix to land in one copy of two. The largest pair (`range_overlap_indexes`) duplicated 54 lines of partition-point logic. Each `_cmp` body is now generic over `C: UserComparator + ?Sized`, and the bytewise name is a one-line wrapper calling it with `DefaultUserComparator`. Callers passing `&dyn UserComparator` compile unchanged (a trait object implements its own trait); the wrapper monomorphizes to the exact code the hand-written byte version compiled to, since `DefaultUserComparator::compare` is an inlined `a.cmp(b)` — so the default-comparator path pays nothing for the sharing. `RunReader::new` already had this shape; the rest now match it.
There was a problem hiding this comment.
💡 Codex Review
coordinode-lsm-tree/src/tree/mod.rs
Line 2123 in 82a8154
When compaction has run with an MVCC GC watermark above target_seqno, older versions and tombstones in the requested interval may already have been physically dropped or had their sequence numbers zeroed, but this still returns a successful iterator over the remaining records. A CDC consumer therefore silently receives an incomplete or mis-sequenced history despite the method's promise to return every committed change at or after the target. Persist and check a retained-history watermark, or otherwise reject targets whose history may have been reclaimed instead of presenting the result as a complete CDC stream.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two scanners walk the same directory and each carried its own parser for
the names the engine owns: `Tree::open`'s recovery sweep and manifest
repair's table scan both matched `{id}.heal-attest[.tmp]`,
`{id}.healtmp-{n}`, `{id}.restrict-bound[.tmp]`, `{id}.repair-tmp` and
the bare `{id}` — ~40 lines of suffix-stripping each, which had to agree
exactly: a kind added to one scanner but not the other is a file one path
deletes while the other depends on it.
`file::TableDirEntry::classify` now states the grammar once — exact-shape
ownership, longer suffixes before their prefixes, both healtmp halves
numeric — and both scanners match on the resulting kind. What each DOES
with a kind (sweep, preserve, adopt, reject, finish a pending swap)
remains its own policy, visible as one match per scanner instead of a
ladder of string probes.
Also folds the repair scan's remaining inline "report unreadable + queue
post-commit removal" pairs into the existing `set_aside_path` helper —
eleven copies of the two-push shape become calls to the one place that
states the rule.
There was a problem hiding this comment.
💡 Codex Review
coordinode-lsm-tree/src/tree/columnar_scan.rs
Lines 720 to 721 in 828d740
When overlapping SSTs contain different values for the same key at the same effective seqno, this comparison treats the versions as equal and the stable sort preserves segment order. That order is not recency order: group_by_overlap first sorts segments by minimum key, so an older segment with a smaller minimum can precede and win over a newer segment. Explicit tied seqnos are supported, and point reads give the newer L0 source precedence, so columnar_scan can return a different value; carry source recency into this tie-break.
coordinode-lsm-tree/src/tree/columnar_scan.rs
Lines 165 to 168 in 828d740
When a visible range tombstone covers rows in an older columnar SST, this scan collects only table row segments and never gathers or applies range tombstones from the captured version. This remains wrong after the deletion itself is flushed into an SST, so it is not merely the documented omission of active memtable rows: a normal point/range read reports the covered keys absent while columnar_scan emits them. Collect the visible table range tombstones for the snapshot and suppress covered rows using the same seqno rules as the regular read path.
coordinode-lsm-tree/src/abstract_tree.rs
Lines 1347 to 1348 in 828d740
When the remaining quota or physical free space is larger than the fixed reserved band but smaller than the value being inserted, this pre-check succeeds because it accounts only for the existing on-disk footprint and already-pending memtables; it never includes the incoming key/value size. For example, with 1.5 MiB remaining and an empty memtable, the 1 MiB reserve admits a 2 MiB value, after which its flush can exceed the configured budget or hit ENOSPC despite use of the admission-gated API. Pass the proposed write size into the check or reserve it atomically before inserting.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`Table::recover` carried 14 positional arguments — three integers and two booleans in a row among them — so a transposed `tree_id`/`table_id` or `pin_filter`/`pin_index` compiled cleanly and surfaced only as misbehavior at runtime. Sixty call sites each restated every neutral default. `RecoverParams` now names every field, with `RecoverParams::new` filling the neutral context (no ingest offset, transient tree id 0, nothing pinned, no encryption) so a call states only what it actually passes. `recover_inner` destructures the struct into the same locals, leaving the recovery body untouched. Repair's two transient opens share one `repair_recover_params` helper; the table test module shares `test_recover_params` for its dominant shape.
remove_range cloned the active memtable handle under a short-lived read guard and inserted after releasing it, so the insert ran outside the writer exclusion the CDC scan's capture relies on: a range deletion preempted between the two steps could land while (or right after) the scan captured the memtable, backdated at or below the returned watermark, and a consumer that advanced past the watermark would lose the deletion for good. The guard now spans the insert, mirroring append_entry, which also keeps a concurrent rotate_memtable from sealing the memtable mid-insert. Carries a regression test that parks the writer between obtaining the memtable and inserting (test seam), proving the read guard is held at that point and that the committed deletion surfaces as a CDC event.
|
Superseded by #588: same tree, squashed to one commit — this PR's review history (117 threads, all resolved) had grown past the point of usability. |
There was a problem hiding this comment.
💡 Codex Review
coordinode-lsm-tree/src/tree/columnar_scan.rs
Lines 167 to 168 in 2f4d602
When a columnar SST contains a range tombstone that has not yet been materialized into its positional delete bitmap—for example, immediately after inserting rows, calling remove_range, and flushing but before compaction—this scan collects only the tables' columnar rows and never evaluates visible_range_tombstones(). It therefore returns keys covered by the newer tombstone even though point and ordinary range reads report them deleted; gather visible range tombstones from the snapshot and suppress each affected row by its effective seqno.
coordinode-lsm-tree/src/tree/columnar_scan.rs
Lines 719 to 722 in 2f4d602
When two overlapping columnar ingestions have the same effective seqno—which is possible after the public sequence counter is reset with set—this ordering has no source-recency tie-break. Because group_by_overlap previously sorted the sources by minimum key, the first row retained for a tied key can come from the older table solely because that table's range starts earlier, while the normal L0 point-read path keeps the newer run on equal seqnos. Carry the table/run recency into the merged rows and use it after effective seqno so the projected scan returns the same value as the tree.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Hardens recovery, salvage, and manifest repair into a total, deterministic
pipeline that always yields a valid, openable tree, for both SST tables and
vlog blob files.
SST salvage
ones, re-emitting the survivors into a fresh, verifiable SST under the
tree's comparator, encryption, and dictionary context.
and heals single-block ECC in place; the faithful surface decodes and
rewrites when a block cannot be copied through.
tombstones), preserving correctness over a lossy rewrite.
batch may add several merge operands for a key, and they share the batch's
seqno, so a flush stores them all at it. Rejecting that would drop an
otherwise clean block and change the value the repaired tree merges to. The
tie is allowed in-block and across a block edge, and only between operands —
for other kinds the internal key ties with no tie-breaker, so their order
would not be reproducible.
walk suppresses the boundary key from the block it emits after a drop: a key
whose newest version died with the dropped block is dropped outright rather
than rolled back to the older version that opened the next block (which
would resurrect a deleted key or shadow the write that replaced it). The
suppression holds until a surviving entry with a DIFFERENT key proves the
version run ended, so a chain spanning several blocks cannot re-surface in
the second one; it applies on every emit path (row, columnar, and the
delete-masked columnar one); a filtered block is re-emitted row by row
rather than byte-copied, since its bytes still carry the suppressed key; and
losing a blob record takes the rest of that key's chain with it, because the
record it lost was the chain's head. A lost region recorded before the walk
(a resynced-past gap) arms the suppression by OFFSET, against the block that
actually follows it. What the suppression is armed WITH is the lost block's
own separator, and its absence — a gap-probed region, or a block whose
separator did not survive — arms the unknown boundary that suppresses the
following block's first key instead. Key identity here is the table's
comparator, not the bytes: under a comparator that folds spellings together,
an older version spelled differently from the lost newest one is still the
same key and is suppressed with it.
Blob-file salvage (KV separation)
salvage_blob_filegives vlog blob files the same block-granular treatmentas SSTs: it walks the file record by record, keeps every intact record, and
drops only the damaged ones instead of condemning the whole file.
BlobSalvageReportreturns the dropped records (Vec<DroppedBlob>) and anoffset_remapmapping every salvaged record's source offset to itsrelocation in the rewritten file — new offset AND new on-disk size (the
re-emit re-compresses, and a live read cross-checks the handle's size
against the frame header) — so the SST value handles that point into the
blob file can be rewritten rather than invalidated.
payload does not fully land is dropped, because keeping part of it would
fabricate a value and a bogus remap entry.
is unrecoverable is excluded (and its file removed once the manifest is
durable) rather than wired into a
rebuilt manifest as a dangling reference, and blob frontiers survive across
recovery so a reclaimed blob file verifies against its live suffix.
re-derived from the punch geometry: the zeroed data-section prefix is
anchored at a validly decoding frame (a zero-filled value payload can never
move the frontier; a partially completed punch is walked hole by hole), the
digest covers the live suffix from the derived frontier, and the rebuilt
snapshot re-persists the restriction — so a later relocation resumes exactly
where the punch stopped instead of erroring inside the zeroed prefix. An
unpunched file short-circuits on its first non-zero data byte at zero extra
read cost. Zeros through the whole data section — with nothing live anchored
below them, since reclaim punches top-down — mean the relocation completed
and only the file's removal lagged the crash: repair completes that drop
rather than publishing an empty-suffix handle whose whole-file
metadata blob GC could never retire; if the removal fails, the repair
fails with that error instead of committing a manifest (left in
blobs/,the next open's orphan sweep would hit the same failure — success would
describe a tree that cannot open). The same rule covers leftover salvage
temps. A partially punched file recovers
with its garbage accounting SEEDED from the validation scan (whole-file
metadata minus live-suffix totals), so the consumed prefix — which no
future compaction can observe — still counts toward
is_deadand thefile remains retirable once its suffix handles are gone. See
docs/manifest-recovery.md§ Blob frontier resolution.while a checkpoint's deletion pause is active: both the SST and the blob
tight-space prefix punches probe the link count and the pause before
punching, so a checkpoint's hard-linked copy can never be zeroed (MemFs
answers the probe instead of inheriting the Unsupported default, which had
silently disabled the blob punch too). Standing down does not DISCARD the
reclaim: the intent lives in the view that is dropping, so it is handed to
the pause, which re-probes the link count and punches once the checkpoint's
window closes — otherwise the space would be stranded permanently, in
exactly the tight-space situation that needs it. A reclaim that still cannot
be proven safe THEN (the checkpoint's link is still there, or the probe
cannot answer) is RETAINED rather than dropped: unlinking the checkpoint only
decrements the link count while the live restricted table keeps holding the
inode, so nothing else would ever free the consumed prefix. A punch that FAILS
mid-pass retains just as much: the pass still stops there (punching below an
unreclaimed extent would break the top-down hole pattern a sidecar-less repair
reads), but the failed extent and the untried remainder are kept for the same
retry rather than discarded. Tight-space
compaction drains that backlog before it plans, since the space is what it is
short of. The residual probe-to-punch window is
closed by lifetimes: the checkpoint captures its version under the held link
window and that version pins every handle it links.
plain repair() no longer blesses an SST whose corruption sits in a lazily
read data block (the freshly computed digest would launder it past
verify_integrity while reads fail). The salvage flag only decides what
happens to a damaged table — rewritten (on) or dropped with a reason
pointing at the salvage-enabled repair (off); rotted-parity-but-readable
tables stay admitted, entering the normal attributable-heal path.
manifest digest, and refuses the refresh when that sync fails: a heal whose
write landed but whose sync did not must never have its post-heal digest
recorded over bytes a power loss can still discard.
There is no third state, no directory of files for someone to deal with
later, and no step that ends in "fix this by hand". Every file the rebuilt
manifest does not name is removed before the run reports success — a foreign
name, a duplicate id, a table no bound can make safe, a source its
replacement supersedes — because a file left behind is an orphan the next
open must sweep, and an open that cannot sweep it does not open. A removal
the filesystem refuses is therefore an error, not a warning. Recovering the
CONTENT of a damaged file is replication, a checkpoint plus journal replay,
or a backup; it is never a copy the engine hides beside the tree.
reconcile, and the scan mutates nothing. The manifest commit is the only act
that publishes anything, and nothing before it displaces a source: a table's
replacement is built at
{id}.repair-tmp— a name no scan adopts — andswapped onto
{id}afterwards, while a salvaged blob takes a fresh id besideits untouched original. Publishing a table under a fresh id instead would
leave a crash with the source AND its half-published copy both readable, and
the retry, unable to tell them apart, would rebuild one history into L0
twice, applying its merge operands twice on read. A leftover temp is
garbage: the run whose committed manifest names its id finishes the swap
(from an
open()as well as a repair), any other run drops it. A copy thatcannot be completed (its restriction could not be re-imposed) is removed on
the spot, and a removal that fails propagates — an undiscardable
half-finished replacement is the one thing that could corrupt the retry.
not a state machine spread across runs. A table dropped only because the flag
was off goes with that decision; nothing is stashed for a later run to
reconsider, which is what keeps every later report describing the world the
operator actually handed the engine.
the rest are removed: an unreferenced one holds nothing reachable, and
admitting it would strand it forever, since repair cannot rebuild the
fragmentation stats blob GC needs to retire a file.
them survive elsewhere — a value it had overwritten, or a key its tombstone
had deleted, becomes visible again. Nothing on disk distinguishes that from
"the key was simply never rewritten", and covering the range with a synthetic
deletion instead would destroy intact data (a flushed table spans most of the
keyspace with seqnos above every older level, so one corrupt block would
erase most of the tree, irreversibly).
RepairReport::lost_coveragetherefore NAMES each excluded table's key range and highest seqno, so a
caller knows exactly where a superseded value may now be served. The seqno
bound is
Nonewhen the table's own sequence base lived in the lost manifest(a bulk-ingested SST stores every entry at local seqno 0), since the on-disk
value there is a fabricated zero that would scope the affected history far
too low. The
allow_resurrectionflag governs ambiguous VISIBILITY (a lostrestriction bound, a lost or forged delete mask), not lost bytes; its docs
now say so.
scan_since_seqnomirrors the read path, which is what a consumer replayingit has to reproduce. Merge operands are all delivered: the read path collects
every physically stored operand for a key and never deduplicates them by
seqno, and
apply_batchtakes a caller-chosen seqno without requiring it tobe unique, so identical operands can legitimately sit in one source or in
two. Idempotent events — a write, a deletion, a range deletion — collapse
across sources instead, since replaying them twice reaches the same state and
the read path shadows the copies rather than compounding them; that is the
manifest-loss-repair shape, where both the inputs and the outputs of an
unfinished compaction are published. Events sharing a seqno are emitted
oldest SOURCE first, so the value the tree serves is applied last: two
sources can hold different values for one key at one seqno, and deciding
that by payload bytes would hand precedence to byte order. Within ONE source
a tied run keeps the order that source applies it — a batch may add several
operands for a key under its shared seqno, and an order-sensitive merge
operator would otherwise converge somewhere the tree never was. Within one
seqno a
range deletion is emitted FIRST — ahead of source recency, so a deletion in
the newer source still leads — because suppression is strictly
entry.seqno < tombstone.seqno: the tree keeps a write made at thetombstone's own seqno, so replaying the deletion last would drop it. The
snapshot watermark is an INCLUSIVE bound, so an entry written at the maximum sequence
number — which defines that watermark — is delivered from an SST exactly as
from a memtable. The snapshot is taken under the version-history write guard:
the watermark, the active memtable's entries and its range tombstones are
captured together with writers excluded, because
apply_batchtakes theseqno from the caller and could otherwise commit at or below the watermark
mid-walk, splitting one batch across the boundary and losing the remainder
for a consumer that had advanced past it. The guard is released before
mapping, since resolving blob indirections is I/O.
blobs/never make the tree unopenable: thescan sweeps the staging file its atomic write renames from instead of
aborting on a name that does not parse as a blob id.
through a single binding point shared with flush, bulk ingest, and recovery.
A compaction output previously carried no heal-hint sink, so a
confirmed-persistent ECC correction while reading it was applied in memory
but could never queue that SST for a durable healing rewrite. Bulk
ingestion binds the BLOB files it publishes as well as its tables: without
the deletion pause, one file's
Dropcan unlink it while a checkpoint iscapturing, and a prefix punch can zero bytes the checkpoint already linked.
hashing damaged content would launder the corruption past every later
integrity check while reads of the affected values still fail. The gate
verifies four independent properties — clean framing/checksums, payload
decompression (a re-stamped checksum over an undecodable compressed payload
frames cleanly), key order under the tree comparator (reordered frames
break the relocation scanner's sorted-input contract), and metadata
counters against the scanned frames (blob GC's dead-file arithmetic trusts
them): exact equality for unpunched files, and lower bounds for punched
ones, whose whole-file metadata must count at least the live suffix's
items and bytes and whose key range must contain the scanned suffix. A file that fails validation is salvaged instead — its
intact records are re-emitted into a compacted replacement under a fresh id
(the damaged original is removed once the manifest names it), and the SSTs
referencing it are rewritten onto the new offsets, dropping only entries
whose record was lost (those keys read as absent, never as an error). The
same rewrite drops a stale handle below a punched file's frontier. Blob
salvage now also recovers COMPRESSED sources (each record is decompressed,
proving it round-trips, then re-emitted under the same descriptor),
including dictionary-compressed blobs on the repair path — repair passes
the tree's configured dictionary; only the true standalone case without a
dictionary fails closed.
unreadable blob files and distinct duplicate spellings of one id are
recorded (deterministically, canonical name kept) and removed after the
commit, never left in
blobs/for the next open's orphan sweep to tripover. The SST punch guard is
structure-anchored (a zero run counts only when it ends at a decodable
block header, the extent end, or the data end) and evidence-backed (the run
must cover a proven hole — zeros alone are the shape of a reclaim, not the
proof, since destroyed bytes read the same; a backend that cannot answer
leaves the run unproven, so the zeros stay damage and the table is
salvaged), the report's
salvagedcount is derived after blob-dependency filtering so it stays a subset of
recovered, and edit replay replaces the restriction sets wholesale (eachedit carries its version's full set), so a removed blob file's frontier can
never attach to a later file reusing its id.
pre-install rollback as the SST reopen, retracting the slice's finalized
outputs instead of leaking them under the exact low-space condition that
engaged tight-space.
deferred, and the user compaction filter now defers the same way (the next
normal compaction applies it). The slice output is therefore a strict
superset shadowing any surviving input prefix, so an unpunched, sidecarless
input republished whole by a manifest-loss repair can never resurrect a
removed record. Shadowing is not enough on its own, though: merge operands
are deliberately never deduplicated across sources, so publishing both
histories would apply the consumed prefix's operands twice. The post-commit
sidecar-failure window is therefore CLOSED rather than tolerated — the
manifest is the authority for the bound, so an open that finds a restricted
table whose
.restrict-boundfile is absent, unreadable, or DISAGREEING(a stale bound from a later slice whose own write failed, or another table's
id) republishes it from the manifest — presence alone proves nothing, and a
stale bound restricts less than reality. An input stays unpunched until its
sidecar exists.
reuse: its
scan()starts at the restriction bound and numbers rows fromzero, while the relocation copies the whole physical section (punched prefix
included) and publishes it unrestricted. Restricted inputs take the
copy-on-write path instead.
Columnar scans under restriction and MVCC
Table::columnar_scanclamps to a tight-space-restricted view's livesuffix: data blocks wholly below the bound (hole-punched, reading as zeros)
are stepped over with their zone-map row counts preserving positional
delete mapping, and the straddling block's sub-bound rows are masked.
segment: a flush / compaction product physically stores every version of an
overwritten key, so singletons that can carry duplicates (the writer's
persisted
key_countdiffering fromitem_count) get per-keynewest-visible selection, with the predicate running after dedup exactly
like the overlap-merge path. Provably-unique segments keep the zero-copy
verbatim fast path.
every path: a bulk-ingested segment stores every row at local seqno 0 and
takes its ordering from a per-segment offset, so the stored value would name
a commit the tree never had. The overlap path writes each row's effective
seqno (already computed for its dedup), since that union spans segments with
different offsets. Masking still compares in local space — one subtraction
per segment rather than one addition per row — so only the emitted column is
translated.
(dedup, verbatim, overlap): the scan used to pick that row without reading
the value type and emit it, returning a key the point read reports absent —
indistinguishable from a live row with an empty value unless the caller
projected the type column. Only a segment whose metadata counts deletions
decodes it, so one without them keeps its columns and its fast path.
rows are the merge's inputs, so the scan would return the raw operand where
a read returns the merged value, having already dropped the base row. A tree
carrying a merge operator is therefore refused rather than served
unresolved chains — the guard is on the operator, since without one the read
path returns the newest entry unchanged too and nothing diverges.
Recovery progress
RecoveryProgress(wired viaConfig::with_recovery_progressorSalvageOptions::progress) exposes live counters a UI thread can pollwhile a repair runs: tables and blob files as they are discovered and
recovered, blocks as the salvage walk inspects / re-emits / drops them,
ECC-healed blocks, and recovered KV entries and columnar columns. The
recovered counts are published once the surviving set is final — after
duplicate displacement and blob-dependency / reference filtering — so a
snapshot can never claim more than the rebuilt manifest holds.
In-place ECC autoheal
under fresh parity, guarded by a tri-state attestation marker so an
inconclusive check preserves rather than deletes it.
Manifest recovery
tables/is total: every branchyields a valid tree, and re-running is deterministic.
The
.restrict-boundsidecar is written strictly after the slice'sinstall commits, so a valid sidecar proves a committed restriction and
its exact bound is honored directly, with no dead-prefix probing; absent
a trustworthy sidecar, the bound derives from the punch geometry — but only
once the ZEROED BLOCK'S OWN EXTENT proves it was punched (a new extent-local
Fshole probe,lseek(SEEK_DATA)on Linux), since corruption that destroysa data block leaves the same read-as-zeros shape a completed punch does, and
a file-wide allocation total attributes nothing to a particular block — on a
filesystem with transparent compression it reads low for an ordinary file.
The same evidence gates the leading-bytes probe, so ordinary corruption there
is salvaged rather than mistaken for a punch with a lost bound and dropped. The readable suffix is never
discarded, even when it is itself corrupt (salvaged, then re-restricted).
Ids named by a surviving sidecar are never handed to a rewritten copy, so no
replacement can inherit a restriction belonging to another file.
allow_resurrection(default drop),governs the ambiguous cases: a lost restriction bound and an
unauthenticated or concealed delete mask. Off, ambiguous data is dropped
to avoid resurrecting superseded or deleted rows; on, it is kept. Neither
setting requires a manual step.
deterministically. Genuinely unrecoverable files (foreign name, redundant
duplicate, undecodable) are dropped, still leaving a valid tree.
was found under, so a per-level route's tables are handled on their own
tier rather than looked for in the primary namespace.
manifest beside the files it could not remove or swap. The next repair (or
the next
open(), for a pending swap) finishesthat cleanup before it scans — the committed manifest is the tree's own
authority on which files count, exactly as an open treats everything it does
not name as an orphan — so a retry cannot rebuild one history twice. A
manifest that does not load cleanly is the case repair exists for and is not
consulted at all.
sequence numbers: callers may assign seqnos explicitly, so a table's highest
seqno says nothing about which table is newer. Ids are allocated in
increasing order, so descending id is the recency order — and being total,
it also makes a repeated repair over the same files reproduce the same tree.
The scan itself is
ordered for the same reason, canonical
{id}spelling first, so an alternatespelling of one id can never displace the writer's own file.
for the block magic instead of decoding a header at every byte — a
multi-gigabyte punched prefix would otherwise take the diagnostic out of
reach on exactly the files it exists to inspect. It also matches the
restriction sidecar to the file it sits beside (from the caller's id, else
from the SST's numeric name): an unmatchable sidecar never skips blocks, so a
copied or stale one cannot make a zeroed leading block read as reclaim. What
a matching sidecar permits is bounded by the table's own index — where its
bound actually falls — not by the first zeros: the punch runs
highest-block-first and stops at its first failure, so a table with a
committed restriction and no hole at all is a supported state, and there the
first zeros are destroyed data.
Checkpoint interaction
durability before treating it as complete.
shared with a checkpoint, so reclaiming space in the live tree can never
hole-punch a checkpoint's data.
Tooling and CI
A heal fuzzer drives single-bit corruption across a fixed corpus of SSTs
(varied block size, per-KV checksum, columnar layout, compression,
encryption, Page-ECC), asserting recover-and-scan never panics and never
returns a wrong value, and dumping the exact failing SST for repro.
The fuzzer needs a CI home, so this PR adds the
fuzz-healjob that runsit on a fixed budget with retries disabled (a retry would overwrite the
reproducer dump or mask a non-replaying failure) and surfaces the failing
SST inline in the log.
Pinning policy: first-party GitHub actions (
actions/*,dependabot/*)are pinned by major version tag (
actions/checkout@v7) — SHA-pinning aGitHub-owned org adds no supply-chain protection while churning a
Dependabot bump PR per patch release. Third-party actions (dtolnay,
Swatinem, taiki-e, codecov, release-plz, benchmark-action) stay pinned to
full commit SHAs;
release-plz/actionis aligned to v0.5.131. No workflowbehaviour changes beyond the new job.
Coverage policy: the patch status uses a fixed 80% target instead of the
auto compare-to-project bar (~92%), which recovery/verification diffs cannot
meaningfully meet — they are dominated by defensive error-classification
arms (Display impls, double-fault plumbing, exotic-corruption classifiers)
whose fail-closed contract the representative refuse/abort tests already
pin. The heal fuzzer harness is excluded from coverage: it runs under the
dedicated uninstrumented fuzz-heal job, so its lines can only ever read as
unexecuted.
Testing
--all-features(nextest),no_std(alloc) clean,cargo doc --all-features -D warningsand doctests clean, clippyall-features/all-targets clean.
docs/manifest-recovery.mddiagrams therecovery algorithm and its invariants.
Related
FormatVersionreserves the retireddiscriminants as numbers, not as public variants: legacy decoding lives only
in that tool, while recovery, salvage, patrol scrub and verify stay V5-only.
Closes #568
Closes #570