feat(salvage): faithful + fast recovery and in-place ECC autoheal#575
Open
polaz wants to merge 83 commits into
Open
feat(salvage): faithful + fast recovery and in-place ECC autoheal#575polaz wants to merge 83 commits into
polaz wants to merge 83 commits into
Conversation
Block salvage opened the source and rewrote the recovered copy with no
encryption / dictionary context (`recover_inner(.., None, None, ..)` and a
plain writer), so an encrypted or zstd-dictionary SST could not be salvaged
at all: the source could not be decrypted / decompressed to read its blocks,
and the rewritten copy would not match an encrypted / dictionary reopen.
During repair this was doubly broken — `try_salvage_table` reopened the
salvaged copy with `config.encryption` / `config.zstd_dictionary` while
salvage had written it plaintext / dictionary-less.
Thread the recovery + write context through salvage:
- Add `SalvageOptions { encryption, zstd_dictionary }` and
`salvage_sst_with_options`; the internal `salvage_with_context` forwards the
provider + dictionary into both the source open (`recover_inner`) and the
destination `Writer` (`use_encryption` / `use_zstd_dictionary`).
- `repair::try_salvage_table` passes the tree's `config.encryption` /
`config.zstd_dictionary`, which it already holds for the reopen, so a
damaged encrypted / dictionary table is now block-salvaged instead of
quarantined whole-file.
- `salvage_sst` keeps its simple signature (empty context, back-compatible).
Tests: an encrypted SST and a dictionary SST each fail to salvage without the
context and block-salvage a corrupt block with it, the recovered copy
reopening under the same encryption / dictionary.
Part of #568
Claude-Session: https://claude.ai/code/session_01VuBftVDYEigwgvYEe1G6fK
Encrypted block AAD binds the table identity, but salvage opened the source and wrote the recovered copy with a hardcoded table id (`0` into recovery, `table.id()` into the writer). So an encrypted SST sealed under a non-zero table id could not be decrypted during salvage, and repair — which reopens the copy under the tree's `Config` — would mismatch. Carry the table id in `SalvageOptions` and pass it into both recovery (`recover_inner`) and the destination writer. `repair::try_salvage_table` supplies the table's real id; the standalone API defaults to `0`. Regression test: an encrypted SST sealed under a non-zero table id recovers nothing under the wrong id (the AAD cannot be decrypted) and block-salvages under the right id, the copy reopening under that id. Claude-Session: https://claude.ai/code/session_01VuBftVDYEigwgvYEe1G6fK
…d copy Salvage configured the destination writer with only data-block compression + ECC, dropping the source's index compression, restart interval, per-KV checksum footers, and — most importantly — its columnar layout: a columnar SST was recovered as a row-major copy, losing the PAX layout and zone map. Add `Writer::mirror_from(&ParsedMeta)`, which seeds a writer from a source table's persisted descriptor (data + index compression, ECC, data-block restart interval, columnar layout with a regenerated zone map, per-KV checksum algorithm), and use it in salvage. A columnar source now salvages into a columnar copy (the recovered rows transpose back into PAX blocks). Layout choices not recorded in metadata (partitioned filter/index, bloom policy, hash ratio, locator, prefix extractor) still fall back to writer defaults. Per-field value sub-columns collapse to a single value column in this row round-trip; preserving them verbatim is a separate step. Test: the columnar-source salvage now asserts the recovered copy is columnar. Claude-Session: https://claude.ai/code/session_01VuBftVDYEigwgvYEe1G6fK
Repair only rebuilt the blob-file manifest around whole files; a single bit-rotted record cost the whole file. Add `salvage_blob_file`, which walks a blob file record by record and re-emits every record whose checksum verifies, recording the rest in a `BlobSalvageReport`. The scanner re-synchronizes at the next frame after a checksum mismatch, so a bit-rotted record costs only itself; a structural break (bad frame magic / header CRC / a frame past the data section) cannot be resynced, so the walk stops there and reports it. A compressed blob source is rejected (fail-closed) for now: the scanner yields on-disk compressed bytes that this path cannot faithfully recompress yet — mirroring how SST salvage fails closed on range tombstones. Adds `BlobFile::compression()` (crate-internal) to detect the source layout. Tests: healthy round-trip, a corrupt record dropped while the rest recover, and a compressed source rejected. Claude-Session: https://claude.ai/code/session_01VuBftVDYEigwgvYEe1G6fK
Slice 2 recovered a columnar source as columnar but round-tripped through rows, collapsing per-field value sub-columns into a single value column (so a projected columnar scan over the recovered copy saw one column instead of the original sub-columns). Re-emit each columnar block as a delete-masked `ColumnBatch` verbatim instead. - Add `Writer::write_columnar_block_verbatim`: writes a pre-built `ColumnBatch` as one columnar block storing its sub-columns AND per-row seqnos as-is (refactors `write_columnar_batch` to share a body gated by a `require_zero_seqno` flag; the bulk-ingest path keeps its seqno-0 contract, the salvage path stores real seqnos). This is the canonical columnar-writer "write a batch" shape (cf. Arrow/Parquet). - Add `Table::load_columnar_block_masked`: loads one columnar block as a delete-masked `ColumnBatch` (preserving sub-columns) with per-block isolation. - Salvage's walk takes the columnar path for a columnar source: load masked batch -> write verbatim, dropping only the blocks that fail to decode. Test: a columnar source with a fixed-4 value sub-column salvages into a copy whose per-SST projected scan still returns that sub-column (id 3), proving it is preserved verbatim rather than collapsed. Claude-Session: https://claude.ai/code/session_01VuBftVDYEigwgvYEe1G6fK
Salvage re-emitted every recovered block through a full decode + re-encode + recompression, paying the encoder cost even for blocks that read back perfectly. Add a copy-through fast path: a block that passes its checksum without ECC recovery has its raw on-disk bytes byte-copied into the recovered SST, skipping the decode + re-encode entirely. Only ECC-recovered blocks (whose on-disk bytes are faulty) are re-emitted from their healed payload; corrupt-beyond-recovery blocks are still dropped. A columnar SST that carries materialized positional deletes falls back to the delete-masked re-emit (a verbatim copy would resurrect deleted rows, since the bitmap is not carried into the recovered copy). - Writer: extract `account_direct_block` as the single source of truth for direct-block bookkeeping (key-order guard, chunk flush, per-row shape / seqno / filter / locator accounting), shared by the columnar-ingest block path and the new `append_verbatim_data_block` primitive that copies raw block bytes and registers the index entry without re-encoding. - Table: `salvage_load_block` reads a block recovery-aware (cache-bypassing, so the recovery status reflects the medium) and, on a clean read, captures the raw bytes + header + inner-zstd layout for the verbatim copy. - Report `blocks_copied_verbatim` so an operator can see how cheaply a mostly-healthy SST was recovered. Tests: clean row + columnar blocks copy byte-for-byte (sub-columns preserved), an ECC-recovered block is re-encoded rather than copied, and the delete / encryption / drop paths stay green. Part of #568
`Aes256GcmProvider`'s `key_chain` / `key_epoch` / `suite_id` are read only by `encrypt_block_aad` / `decrypt_block_aad`, which are `zstd_any`-gated (they wrap the zstd `SkippableFrame`). Building with `encryption` but without zstd left the three fields written-but-never-read, tripping `dead_code` (and `-D warnings`). Gate the trio with the entry points that consume them. The opaque `encrypt` / `decrypt` path keeps using `cipher` directly, so the manifest crypto path is unaffected; without zstd the provider already falls back to the trait default for block AAD.
A patrol scrub could correct a bit-rotted Page-ECC block on read, but the only way to PERSIST the fix was a full-file healing recompaction (O(file), rewrites and re-encodes every healthy block). There was no in-place, O(damage) self-heal. Add an opt-in `heal_in_place` scrub mode that writes the corrected block back at its existing offset, leaving every healthy block untouched: - `Block::heal_frame`: reads a block's on-disk frame and, when its checksum fails but Page-ECC recovers it, returns the corrected frame to write back (header + RS/SEC-DED-recovered data + recomputed parity). Byte-length-identical to the original (the payload length is preserved and parity length is a function of it), so it overwrites in place without shifting later blocks. Works purely at the framed-bytes level (checksum/parity cover the on-disk, post-compression/encryption bytes), so no decompress/decrypt is needed. - `Table::heal_data_blocks_in_place`: opens the SST read+write (cache-bypassing, like scrub), heals each correctable block, and `sync_data`s it before moving on so a crash mid-heal leaves the block in its prior, still-RS-correctable state. Uncorrectable / unreadable blocks are reported and left for block salvage. - `PatrolScrubOptions::heal_in_place` selects it; `PatrolScrubReport::blocks_healed_in_place` counts the persisted corrections. A correctable fault is restored byte-for-byte (RS reconstructs the original data, parity is recomputed deterministically), so a healed SST is bit-identical to its pre-fault state — verified in the test alongside the uncorrectable-left-for- salvage path. Closes #570
📝 WalkthroughWalkthroughThis PR extends SST salvage with runtime encryption, dictionary, table-ID, columnar, delete-bitmap, and blob-file support; adds faithful copy-through and layout mirroring; and introduces Page-ECC verification and in-place healing integrated with patrol scrub. ChangesSalvage context and repair integration
Table recovery and faithful writer output
Salvage walk and blob recovery
Page-ECC verification and in-place healing
Patrol scrub integration
Estimated code review effort: 5 (Critical) | ~110 minutes Sequence Diagram(s)sequenceDiagram
participant Repair as try_salvage_table
participant Salvage as salvage_with_context
participant Table as Table::recover_inner
participant Writer as Writer::mirror_from
participant Block as Table::salvage_load_block
Repair->>Salvage: SalvageOptions with runtime context
Salvage->>Table: recover_inner with table_id, encryption, dictionary
Table-->>Salvage: recovered source table
Salvage->>Writer: mirror persisted layout
Salvage->>Block: load and verify data block
Block-->>Salvage: clean raw frame or recovered data
Salvage->>Writer: copy frame or re-emit block
Salvage-->>Repair: SalvageReport
sequenceDiagram
participant Scrub as patrol_scrub
participant Selector as scan_one
participant Table as Table
participant Block as Block.heal_frame
participant Fs as filesystem
Scrub->>Selector: scan_one with heal_in_place
Selector->>Table: heal_data_blocks_in_place
Table->>Block: verify and recover frame
Block-->>Table: healed frame or scrub result
Table->>Fs: write frame at original offset
Fs-->>Table: sync_data
Table-->>Scrub: PatrolScrubReport
Estimated code review effort: 5 (Critical) | ~110 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
polaz
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
| Filename | Overview |
|---|---|
| src/salvage.rs | Adds context-aware SST salvage, delete-mask fail-closed behavior, verbatim block copy-through, blob link preservation, and record-level blob salvage. |
| src/table/mod.rs | Adds recovery-aware block loading, delete position verification, masked columnar block loading, and in-place ECC healing support. |
| src/table/writer/mod.rs | Adds source-layout mirroring and direct block append validation for salvaged SST output. |
| src/vlog/blob_file/writer.rs | Cleans up partial blob files when writer initialization fails after destination creation. |
| src/repair.rs | Routes repair salvage through the new recovery context and requires warning-free block verification. |
| src/verify.rs | Adds encryption-aware SST verification and Page-ECC parity trailer mismatch reporting. |
| src/scrub.rs | Adds patrol scrub options and reporting for in-place Page-ECC healing. |
Reviews (18): Last reviewed commit: "fix(table): bound linked_blob_files reco..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The shared direct-block bookkeeping rejected equal user keys as "not strictly increasing". That contract is correct for columnar *ingest* (a bulk load of unique keys), but the verbatim copy-through salvage path reuses the same helper, and a real data block legitimately holds several MVCC versions of one user key (same key, descending seqno). Salvaging such a block therefore errored out instead of recovering it. Move the strictly-unique-key check into the columnar-ingest path (`write_columnar_block_inner`), where it belongs, and let the shared `account_direct_block` accept any validly-ordered entries (its per-key accounting already treats repeats correctly — a repeated user key is counted once). The verbatim path no longer needs a comparator. Adds a regression test that salvages a block holding three versions of one key and recovers all of them, plus coverage for the columnar ECC-recovered re-encode path.
Add table-level tests driving `heal_data_blocks_in_place` directly: - a single-bit SEC-DED fault is healed in place and the SST is restored byte-for-byte (exercises the SEC-DED branch of the heal primitive); - a corrected block whose write-back fails (via a fault-injecting `Fs`) is not counted as healed and is reported as an uncorrectable finding, left for block salvage rather than silently lost.
…al blobs Columnar copy-through keyed the verbatim-vs-re-emit choice on `delete_bitmap().is_empty()`. A salvage-mode open degrades a corrupt delete bitmap to empty, so that predicate is also true when the bitmap was lost, and the verbatim path then byte-copies the physical block (including positionally-deleted rows) into a recovered SST that carries no bitmap, resurrecting deleted data. Gate the fast path on whether the SST actually carries a delete-bitmap section (`Table::has_delete_bitmap_section`): a delete-bearing SST always re-emits (a degraded bitmap still recovers all rows live, the documented salvage degradation, never via a verbatim copy), and only a genuinely delete-free SST is eligible for copy-through. `salvage_blob_file` left a partial destination behind when a record write or `finish` failed. Remove the incomplete output on the error path, the same way the SST salvage path does, so a retry caller never finds a half-written blob file. Regression tests: a corrupt-delete-bitmap SST recovers with zero verbatim copies, and a failing destination write removes the partial blob file.
…ombstone paths - in-place heal on a non-ECC SST is a no-op (blocks carry no parity, nothing to reconstruct); - an in-place heal whose read+write open fails records an error finding and scans nothing; - salvaging a block with a weak tombstone followed by a value for the same key recovers both entries through the verbatim copy-through path.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/table/writer/mod.rs (1)
1300-1315: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winStrictly-increasing key check wrongly applies to the verbatim salvage path — gate it behind
require_zero_seqno.The comment states the verbatim path "does NOT go through here," but
write_columnar_block_verbatim→write_columnar_block_inner(.., false)reaches this loop. The loop rejects any non-Lesscomparison, so two rows sharing a user key (MVCC versions) returnInvalidHeader("... strictly increasing keys").That contradicts this path's contract:
write_columnar_block_verbatimis documented to "keep... its MVCC versions," andaccount_direct_blockexplicitly permits equal user keys. Salvage of a columnar SST whose block holds multiple versions of one key (flush-produced, multi-seqno) callswriter.write_columnar_block_verbatim(&batch, comparator)?— the error propagates and fails recovery of that block. Only the bulk-ingest path (require_zero_seqno == true) requires strict uniqueness.🐛 Proposed fix: gate the strict-order check on the bulk-ingest path
- // Columnar ingest is a bulk load of strictly-unique, ascending keys (the - // verbatim salvage path, which can carry repeated user keys across MVCC - // versions, does NOT go through here). Check within the batch and against - // the writer's prior key before any state mutation. - let mut prev = self.current_key.as_ref(); - for e in &entries { - if let Some(p) = prev - && comparator.compare(p.as_ref(), e.key.user_key.as_ref()) - != core::cmp::Ordering::Less - { - return Err(crate::Error::InvalidHeader( - "columnar batch ingest requires strictly increasing keys", - )); - } - prev = Some(&e.key.user_key); - } + // Columnar ingest is a bulk load of strictly-unique, ascending keys; the + // verbatim salvage path (`require_zero_seqno == false`) can carry repeated + // user keys across MVCC versions, so enforce strict ordering only for the + // bulk-ingest path. Check within the batch and against the writer's prior + // key before any state mutation. + if require_zero_seqno { + let mut prev = self.current_key.as_ref(); + for e in &entries { + if let Some(p) = prev + && comparator.compare(p.as_ref(), e.key.user_key.as_ref()) + != core::cmp::Ordering::Less + { + return Err(crate::Error::InvalidHeader( + "columnar batch ingest requires strictly increasing keys", + )); + } + prev = Some(&e.key.user_key); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/table/writer/mod.rs` around lines 1300 - 1315, The strict ascending-key validation in write_columnar_block_inner is being applied to both bulk ingest and verbatim salvage, but it should only run when require_zero_seqno is true. Update the loop that compares self.current_key and entries to skip the InvalidHeader check for the write_columnar_block_verbatim path so equal user keys (MVCC versions) are preserved. Keep the strict-order enforcement only for the bulk-load/zero-seqno flow and ensure the existing salvage behavior remains compatible with account_direct_block and write_columnar_block_verbatim.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/salvage.rs`:
- Around line 103-108: Update the documentation for the salvage metrics in the
`blocks_copied_verbatim`/`blocks_salvaged` comment to reflect that the
non-verbatim remainder is not only ECC-recovered blocks. Reword the text so it
also covers clean blocks that are re-emitted to apply delete masks, keeping the
explanation aligned with `blocks_salvaged` and `blocks_copied_verbatim` in
`src/salvage.rs`.
- Around line 470-473: The salvage re-emit path in `salvage.rs` is still using
`write_columnar_block_verbatim`, which rejects equal user keys and breaks valid
MVCC blocks with duplicate versions. Update the
`table.load_columnar_block_masked(...)` re-emit flow to use or add a salvage
writer that allows duplicate user keys as long as internal-key order is
preserved, and make sure the same fix is applied to the other re-emit branch
around the second occurrence. Add a columnar salvage test that covers
duplicate-version blocks (including delete-bearing or ECC-recovered data) to
verify salvage succeeds.
In `@src/salvage/tests.rs`:
- Around line 1047-1050: The salvage_sst() test is too strict because it assumes
missing dictionary input must return an error, but salvage_blocks() can still
complete successfully with zero recovered entries when data blocks fail with
ZstdDictMismatch. Update the test in salvage/tests.rs around the salvage_sst()
assertion to follow the zero-table-id case below: keep using salvage_sst() with
the source and dest, but assert on the returned report’s recovered/entries count
being zero instead of expecting is_err().
In `@src/scrub/ecc_tests.rs`:
- Line 215: The test helpers in ecc_tests still use a u64-to-usize cast that
needs an explicit Clippy suppression, and this codebase prefers #[expect] over
#[allow] for new lint suppressions. Update both helper functions around the
corrupt_pos calculation to add #[expect(clippy::cast_possible_truncation, reason
= "...")] on the relevant helper definitions, keeping the cast unchanged and
using the existing test helper symbols to place the suppression correctly.
In `@src/table/block/mod.rs`:
- Around line 1697-1736: The in-place heal path in heal_data_blocks_in_place
relies on debug_assert_eq! for frame-size validation, which disappears in
release builds and can allow malformed repaired blocks to be written with the
wrong length. Add a runtime check after rebuilding the frame, using the same
trailer/classification logic as the read path (via the header/post-header
handling around read_payload_and_verify and expected_parity_len) to ensure the
rebuilt frame is byte-identical in length to the original block. If the length
differs, return an error instead of continuing, and keep the fix localized
around the frame reconstruction and block_size validation in
src/table/block/mod.rs.
- Around line 1677-1681: The heal path in the block reader allocates directly
from handle.size(), which can allow a corrupt on-disk handle to trigger an
oversized buffer. In the block-loading logic around the buffer creation in this
module, apply the same pre-allocation size cap used by from_file_with_recovery
before calling alloc::vec![], and return an error for oversized values instead
of allocating. Use the existing block/header handling symbols in this area
(including handle.size(), Header::MIN_LEN, and the recovery/read path helpers)
to keep the fix consistent.
---
Outside diff comments:
In `@src/table/writer/mod.rs`:
- Around line 1300-1315: The strict ascending-key validation in
write_columnar_block_inner is being applied to both bulk ingest and verbatim
salvage, but it should only run when require_zero_seqno is true. Update the loop
that compares self.current_key and entries to skip the InvalidHeader check for
the write_columnar_block_verbatim path so equal user keys (MVCC versions) are
preserved. Keep the strict-order enforcement only for the bulk-load/zero-seqno
flow and ensure the existing salvage behavior remains compatible with
account_direct_block and write_columnar_block_verbatim.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e0ab1b1-b838-49c5-ad45-09f1cce1a8c5
📒 Files selected for processing (12)
src/encryption/mod.rssrc/repair.rssrc/salvage.rssrc/salvage/tests.rssrc/scrub.rssrc/scrub/ecc_tests.rssrc/scrub/tests.rssrc/table/block/mod.rssrc/table/mod.rssrc/table/tests.rssrc/table/writer/mod.rssrc/vlog/blob_file/mod.rs
…icate keys A block re-emitted through `write_columnar_block_verbatim` can hold several MVCC versions of one user key. The test writes such a batch and expects it to succeed; it fails on current code because the strictly-increasing-key check wrongly applies to the verbatim path.
`write_columnar_block_inner` ran the strictly-increasing-key check on every caller, but `write_columnar_block_verbatim` (the salvage re-emit path, `require_zero_seqno == false`) routes through it too. That rejected a block holding multiple MVCC versions of one key, failing recovery of any columnar SST whose block carries duplicate-key versions (flush-produced, multi-seqno). Gate the check on `require_zero_seqno`: only bulk ingest requires strictly-unique ascending keys. The verbatim path keeps its documented contract of preserving MVCC versions; `account_direct_block` already accepts validly-ordered equal keys.
…untime `Block::heal_frame` allocated a read buffer straight from `handle.size()`, so a corrupt on-disk handle could request an oversized buffer. Apply the same pre-allocation size cap `from_file_with_recovery` uses (max payload + encryption overhead + parity + header), rejecting an absurd size before allocating. The rebuilt-frame length check was a `debug_assert`, which vanishes in release builds and could let a wrong-length frame overwrite the block in place. Make it a runtime check that returns an error instead. Also clarify the `blocks_copied_verbatim` doc: the non-verbatim remainder covers both ECC-recovered blocks and the clean blocks of a delete-bearing columnar SST (re-emitted with the delete mask applied), not only ECC-recovered ones.
…nal cast lint - salvage_recovers_a_dictionary_sst_with_the_dictionary: note that salvage without the dictionary errors at open (recover_inner ZstdDict fail-fast), so the assertion is `is_err`, not a zero-recovered report. - ecc_tests header: spell out why the cast_possible_truncation suppression stays `#[allow]` and not `#[expect]` (the lint only fires on 32-bit, so `#[expect]` would be unfulfilled on the 64-bit host).
Member
Author
|
@coderabbitai The outside-diff Critical (src/table/writer/mod.rs 1300-1315) is fixed in cca02b7: the strictly-increasing-key check is now gated on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5
git show cca02b786 --stat 2>/dev/null || echo "commit not found locally, trying fetch"Length of output: 1668 🏁 Script executed: #!/bin/bash
git show cca02b7 -- src/table/writer/mod.rs | head -100
echo "---test---"
git log --all --oneline | grep -i 15eb5e2
git show 15eb5e2 --stat
rg -n "write_columnar_block_verbatim_accepts_mvcc_duplicate_keys" -A5 src/Length of output: 4683
|
- write_columnar_batch_enforces_the_ingest_contract: a row-mode writer, a non-zero per-row seqno, and non-increasing keys are each rejected. - write_columnar_batch_records_entry_precision_locator: bulk ingest with an Entry-precision locator records a per-key slot (the per-entry-index arm of the direct-block accounting).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/table/block/mod.rs (1)
1720-1768: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the ECC trailer before returning clean.
The new length check only runs after a correction. If the declared payload+parity verifies but the block handle includes extra trailing bytes,
read_payload_and_verifyreturnsNoneandheal_framereports the block clean, unlike the normal read path’s trailer classification.Proposed fix
- if !block_has_parity(&header, transform) { + let has_ecc = block_has_parity(&header, transform); + if !has_ecc { return Ok(None); } let ecc_params = block_ecc_params(&header, transform); let ecc_length = expected_parity_len(header.data_length, ecc_params); + let actual_payload_plus_ecc = block_size.saturating_sub(header_len); + let actual_data_len = usize::try_from(header.data_length) + .map_err(|_| crate::Error::InvalidHeader("Block"))?; + classify_block_trailer( + has_ecc, + actual_payload_plus_ecc, + actual_data_len, + ecc_length, + &handle, + )?; // `read_payload_and_verify` consumes exactly `data_length + ecc_length`As per coding guidelines, missing validation of on-disk metadata must be flagged; as per path instructions,
src/**/*.rsrequires panic-free error handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/table/block/mod.rs` around lines 1720 - 1768, The heal path in `heal_frame` only checks the rebuilt length after a correction, so extra trailing bytes can be missed when `read_payload_and_verify` returns `None`. Add the same post-payload/trailer validation used by the normal read path before treating a block as clean, and make sure `heal_frame` still returns an error instead of `Ok(None)` when the declared payload-plus-parity does not consume the full on-disk block.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/table/block/mod.rs`:
- Around line 1720-1768: The heal path in `heal_frame` only checks the rebuilt
length after a correction, so extra trailing bytes can be missed when
`read_payload_and_verify` returns `None`. Add the same post-payload/trailer
validation used by the normal read path before treating a block as clean, and
make sure `heal_frame` still returns an error instead of `Ok(None)` when the
declared payload-plus-parity does not consume the full on-disk block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4daf56a2-5a9b-4b0d-a853-9f9b47872a3a
📒 Files selected for processing (6)
src/salvage.rssrc/salvage/tests.rssrc/scrub/ecc_tests.rssrc/table/block/mod.rssrc/table/writer/mod.rssrc/table/writer/tests.rs
An over-sized block handle (extra trailing bytes beyond header + data + parity) must be rejected by heal_frame, not reported clean. The test fails on current code because heal_frame skips the trailer classification the normal read path performs.
… fail closed An unreadable data block inside a delete-bearing columnar SST makes every later delete position unverifiable: the block's actual row count is unknowable, and trusting the zone map's claim for it would let a checksum-repatched count on exactly that block shift the masks of all later readable blocks undetected. The current validation advances by the zone map's claim, so the default salvage still emits delete-masked rows it cannot prove faithful.
The verification chain advanced past an unreadable block using the zone map's claimed row count, which re-opened the hole it exists to close: a checksum-repatched count on exactly that block shifts the masks of all later readable blocks undetected (the claimed starts stay internally consistent while the ACTUAL positions diverge). An unreadable block now fails the verification outright — the default salvage refuses, and the explicit allow_delete_resurrection opt-in recovers the readable rows live. Regression test in 5e17c99.
…must salvage The encrypted verify path scrubs through the table, and the scrub silently corrects an RS-recoverable fault on read (corrections_applied > 0 with no errors) while the corrupt bytes stay on disk — repair then accepts the table as verified. The unencrypted out-of-band verifier flags the same checksum mismatch and drives the table through salvage; the encrypted path must do the same.
…yload The len/4 heuristic can land the flip in a parity trailer, which a clean-checksum read never validates — the scrub then reports the table clean and the test exercises nothing. Offset 40 is deterministically inside the first data block's payload (the data region starts at file offset 0, headers are ~33 bytes), so the read must RS-correct it.
The encrypted verify path accepted a table whose scrub CORRECTED an RS-recoverable fault on read (corrections_applied > 0, no errors): the fault was healed in memory but the corrupt bytes stayed on disk, while the unencrypted out-of-band verifier flags the same checksum mismatch and drives the table through salvage. A scrub correction now fails the verify too, so a persistent correctable fault in an encrypted Page-ECC SST is rewritten cleanly through salvage instead of surviving every repair. Regression test in 294af83 (vector pinned in 70df171); falsified against the pre-fix condition.
The encrypted verify path scrubs only data blocks; the bloom filter section loads lazily on point reads, so a corrupt encrypted filter survives repair_with_salvage and later reads fail on it (the unencrypted out-of-band verifier covers the filter section).
The encrypted block-verify gate scrubbed only data blocks; the bloom filter section loads lazily on point reads, so a corrupt encrypted filter survived repair_with_salvage and later point reads failed on it (the unencrypted out-of-band verifier covers the filter section). Table::filter_blocks_verified loads the unpinned whole-filter block, or every partition of a partitioned filter, through the table's own provider-wired transform, and the encrypted verify gate requires it — a corrupt filter now drives the table through salvage into a rebuild with a fresh filter. Regression test in 22ab2b9.
The salvaged blob file is written compacted: after the first dropped record every later record lands at a new offset, while existing SST entries still hold ValueHandle::offset values into the SOURCE file — replacing the file under the same id would point every later handle at the wrong frame. BlobSalvageReport::offset_remap now maps each salvaged record's source frame offset to its offset in the recovered file (a source offset absent from the map is a lost record), so a caller can re-target handles before swapping the file in; the docs state the non-drop-in contract explicitly. The behavior test lands with the change (a failing-first test cannot compile against the pre-change API, so it would prove nothing): it verifies the remap against actual scans of both files, including the post-drop shift, and that the dropped record has no target.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc9ff5b19e
ℹ️ 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".
…files An SST from a KV-separated tree names every referenced blob file in its linked_blob_files section; blob GC / relocation consults it to decide whether a blob is still referenced. The salvage writer currently starts with an empty link list and never copies the source's, so the recovered copy omits the section — GC could then rewrite or delete a blob that only this table still references, silently breaking its indirections.
… copy An SST from a KV-separated tree names every referenced blob file in its linked_blob_files section; blob GC / relocation consults it (via list_blob_file_references) to decide whether a blob is still referenced. The salvage writer started with an empty link list, so the recovered copy omitted the section and GC could rewrite or delete a blob that only this table still references, silently breaking its indirections. The links are copied conservatively: a dropped block may remove the last reference to some blob, so the copy can over-reference — that only makes GC retain a blob longer, never break a live indirection. Regression test in b9da6ec.
Bit rot confined to a block's parity trailer reads as Clean (the payload checksum passes; parity is only consulted on a payload mismatch), so the heal pass reports no findings and leaves dead ECC on disk — a later payload fault in that block can no longer be recovered. The pass holds the read+write handle and the payload is untouched, so a size-preserving trailer rebuild is exactly the heal it exists to perform.
A clean payload checksum never validates the parity trailer (parity is only consulted on a payload mismatch), so rot confined to the trailer read as Clean and the heal pass left dead ECC on disk — a later payload fault in that block could no longer be recovered. The Clean arm now re-reads the raw frame, compares its trailer against freshly computed parity (raw_block_parity_delta, shared with the salvage verbatim gate), and on a mismatch persists the rebuilt trailer at its on-disk position (header + payload untouched, size-preserving), counting it in blocks_healed_in_place. Regression test in 370759d.
The encrypted verify accepts a table whose FILTER or SIDE-section blocks (index TLI, meta, zone map, ...) carry a persistent ECC-correctable fault: those loads silently correct the fault in memory (EccStatus::Corrected is hidden behind an Ok), while the corrupt bytes stay on disk. The unencrypted out-of-band verifier flags the same raw checksum mismatch and drives salvage; the encrypted path must apply the same standard to every block-format section.
…n walk Block headers and payload checksums are PLAINTEXT — encryption seals only payloads — so the out-of-band section walk works on encrypted files as-is; the provider (and the AAD-bound table id) are needed only to decode the meta block for the per-SST ECC descriptor. verify_sst_file_with_context threads that context through, and repair's block-verify gate now uses ONE uniform path for encrypted and unencrypted tables. This replaces the encrypted special-case (data-block scrub + corrections check + explicit filter loads) with a strictly stronger check: every section — data, index/TLI, filter, zone map, delete bitmap, locator, meta — verifies against its raw on-disk checksum, which flags even a persistent ECC-CORRECTABLE fault that a live read would silently heal in memory while the corrupt bytes stayed on disk. The now-redundant Table::filter_blocks_verified is removed. Regression tests in 1e7610e (filter + TLI correctable faults).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82e3f81db0
ℹ️ 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".
…ck with deletes A delete-bearing columnar SST holding a checksum-clean block whose ColumnBatch does not decode (leading row-count u32 intact, column framing broken, checksum re-stamped) must fail the default salvage: the position verifier only reads the leading four bytes, so the walk drops the block yet keeps masking later blocks at positions it could not prove. Fails until the verifier fully decodes each block.
The delete-position verifier advanced by each block's leading LE u32 row count without decoding the ColumnBatch, so a checksum-repatched block with intact count bytes but broken column framing passed the verification; the walk then dropped it as undecodable while still masking later blocks against positions derived from the unproven count. Decode each block fully and fail closed when it does not decode — its actual row count is unknowable, same as an unreadable block.
… unrecognized-ECC repair gate Two holes in the block-verify gate repair relies on: - a rotted Page-ECC parity trailer under a clean payload checksum is invisible to the out-of-band walk (the trailer is drained, never compared), so the SST verifies healthy while its ECC is dead; - an unrecognized-ECC descriptor makes the walk skip every SST-block section with only a WARNING, and the repair gate checks is_ok() alone, stamping effectively unchecked bytes into the rebuilt manifest instead of salvaging the table. Both tests fail until the walk compares trailers against freshly computed parity and the repair gate requires a warning-free report.
…ings The out-of-band block walk drained each Page-ECC parity trailer without comparing it, so rot confined to parity read as a healthy block while its ECC was dead. The walk now reads the trailer and, on a build with the ECC codecs, verifies it against parity freshly computed over the checksum-clean payload, reporting a mismatch as the new BlockVerifyError::EccParityMismatch. Repair's block-verify gate additionally required only is_ok(), which ignores warnings: an unrecognized-ECC table (every SST-block section skipped, bytes unchecked) was stamped into the rebuilt manifest as-is. The gate now also requires a warning-free report, routing such tables to salvage.
verify_sst_file_with_context passed max_enc_overhead = 0 to the section walk even when an encryption provider was supplied, while the writer/read path allows a block's on-disk payload to exceed the 256 MiB plaintext cap by up to the provider's max_overhead(). A healthy encrypted block just over the cap would be false-flagged as HeaderCorrupted and the table quarantined/salvaged (or dropped when salvage is unsupported). Thread the provider's overhead into the scan, mirroring the live scrub path. No regression test: reproducing this requires a single block whose encrypted payload exceeds 256 MiB, which is prohibitively heavy for the suite; the change mirrors the already-tested live-scrub sizing.
…ping the block Within salvage_load_block the raw verbatim re-read follows a first, recovery-aware read that has already produced a verified decoded block. A transient I/O error (or corrupt header) on that second read currently propagates as Err, so the salvage walk drops a block that is provably recoverable via re-encoding. Fails until the re-read failure falls back to the re-encode path (verbatim = None) like any other re-read validation mismatch.
salvage_load_block propagated an Err from the raw verbatim re-read (transient I/O error or corrupt header), dropping a block whose first, recovery-aware read had already produced a verified decoded payload. Reserve Err for that initial verified read: any re-read failure now disqualifies only the byte-copy (verbatim = None) and the walk re-encodes the verified payload, same as a checksum or parity mismatch on the re-read frame.
…r handle pick Method-path form for the AEAD overhead lookup and find_map for the first data-block handle in the salvage re-read test (lint-clean under redundant_closure_for_method_calls and filter_map_next).
…able blob links A KV-separated source whose linked_blob_files section is unreadable (count header claims more records than the section holds) fails the salvage AFTER Writer::new has already created the destination, and the early ? return bypasses the cleanup wrapper — leaving a partial SST at dest for a later repair or retry to trip over. Fails until the links are read before the destination is created.
list_blob_file_references() ran after Writer::new had created dest but before the cleanup-wrapped block walk, so its early ? return on an unreadable linked_blob_files section leaked a partial SST at the destination (in the repair path that is the original table path, which a later run would re-open and re-quarantine). Read the source's links before anything is created at dest.
list_blob_file_references pre-allocated Vec::with_capacity from the section's untrusted count header; a corrupt or forged count (u32::MAX) requested a multi-GB reservation, aborting the process (SIGABRT) on allocators that do not overcommit instead of failing as invalid data. Validate the declared count against the remaining section bytes (32 bytes per record) before reserving, mirroring the columnar decoder's column-count bound. Surfaced by the salvage regression test for unreadable blob links, which passed on macOS (overcommit) and aborted on Linux CI.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 726f44e80e
ℹ️ 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".
Comment on lines
+1171
to
+1175
| match self.raw_block_parity_delta(&raw, &raw_header) { | ||
| // Trailer matches (or no ECC): nothing to persist. | ||
| Ok(None) => {} | ||
| // Trailer rot: persist the freshly computed parity at | ||
| // its on-disk position (header + payload unchanged). |
There was a problem hiding this comment.
When the initial scrub read is clean but this second raw reread observes a transient or newly persistent payload bit flip, raw_block_parity_delta recomputes parity over those unverified bytes and the Ok(Some(fresh)) arm writes that parity trailer back. That can turn a healthy block with a transient reread fault into one with a bad parity trailer, or make ECC agree with a newly corrupted payload instead of preserving the original recovery data. Recompute and compare the reread payload checksum against raw_header.checksum before accepting fresh for an in-place parity rebuild.
Useful? React with 👍 / 👎.
Comment on lines
+1450
to
+1451
| #[cfg(feature = "page_ecc")] | ||
| if payload_clean && let Some(scheme) = block_ecc { |
There was a problem hiding this comment.
In a build compiled without page_ecc, this comparison is compiled out, and the current code only consumes the parity trailer without adding any warning or error. When repair_with_salvage(true) verifies a Page-ECC SST in that build, block_verify_passes can see is_ok() && !has_warnings() and stamp a rebuilt manifest over parity bytes that were never checked; parity-only rot is then accepted instead of routing through salvage, which already knows how to re-encode without ECC on parity-less builds. Fresh evidence: the current parity check is entirely behind this #[cfg(feature = "page_ecc")] gate while the no-feature path records no warning.
Useful? React with 👍 / 👎.
Comment on lines
+1260
to
+1262
| let write_back = file | ||
| .seek(SeekFrom::Start(block_offset)) | ||
| .and_then(|_| file.write_all(&frame)); |
There was a problem hiding this comment.
When an SST has an existing hard-linked checkpoint (or any other extra hard link), this in-place overwrite mutates the checkpoint's copy too because both paths share the same inode. Checkpoints rely on SST files being immutable after they are linked; the delete path already avoids truncating shared inodes, but the heal path does not check hard_link_count before writing. In that context, fall back to a rewrite/scheduled heal instead of modifying the linked file in place.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/repair.rs`:
- Around line 537-542: Update the inline comment describing encrypted-table
verification near block_verify_passes and verify_sst_file_with_context so it
accurately states that both encrypted and unencrypted tables use the same
encryption-aware out-of-band walk, with the recovered table only supplying
metadata.id; remove the claim that verification occurs through a table-self
scrub.
In `@src/salvage/tests.rs`:
- Around line 1294-1308: Extend the salvage tests to verify logical visibility
of deleted rows, not only physical counts: in src/salvage/tests.rs lines
1294-1308, read and assert visibility of a deleted key outside the corrupt
block; at lines 1417-1430, read surviving deleted keys after dropping the
poisoned block; and at lines 1540-1550, assert keys at deleted positions 5, 50,
and 150 are visible. Use the existing read/query helpers and preserve the
current count assertions.
In `@src/scrub/ecc_tests.rs`:
- Around line 389-438: Update heal_in_place_restores_a_rotted_parity_trailer to
capture the original trailer byte before flipping it, then assert the healed
on-disk byte equals that saved value rather than only asserting it differs from
the rotted value.
In `@src/table/util.rs`:
- Around line 436-443: Update the FRAME integrity comment near the scrub logic
to remove the inaccurate description of crate::verify::verify_sst_file as a
semantic or body-decoding pass. Keep the explanation focused on the shared
frame-level validation depth, or reference only a verifier that actually decodes
block bodies.
In `@src/verify.rs`:
- Around line 816-820: Update verify_sst_file_with_context and its warning/error
reporting plus scan_sst_blocks calls to use the supplied table_id instead of the
hardcoded 0. Preserve the existing verification behavior while ensuring all
encrypted repair diagnostics identify the actual table.
In `@src/verify/block_verify_tests.rs`:
- Around line 225-282: Strengthen
verify_sst_file_detects_a_rotted_parity_trailer by asserting the report contains
the specific parity-trailer mismatch error, rather than only checking that
report.is_ok() is false. Keep the existing trailer-byte corruption setup and
match the established error variant or message produced by parity validation, so
unrelated verification failures cannot satisfy the regression.
In `@tests/repair.rs`:
- Around line 899-909: Extend the repair test after the report assertions to
reopen the repaired table using the same configuration and perform point reads,
following the pattern used by the non-ECC filter test around its point-read
checks. Assert those reads succeed so the rebuilt filter is actually loaded and
exercised before accepting recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6450d56a-6d90-4486-9de9-c2dfd2a8a378
📒 Files selected for processing (20)
.config/nextest.tomldocs/data-integrity.mdsrc/fs/fault_fs.rssrc/repair.rssrc/repair/tests.rssrc/salvage.rssrc/salvage/tests.rssrc/scrub.rssrc/scrub/ecc_tests.rssrc/table/meta.rssrc/table/mod.rssrc/table/tests.rssrc/table/util.rssrc/table/writer/mod.rssrc/table/writer/tests.rssrc/verify.rssrc/verify/block_verify_tests.rssrc/vlog/blob_file/writer.rstests/repair.rstools/sst-dump/src/main.rs
Comment on lines
+537
to
+542
| // rather than keep a table that errors on read. The verify must | ||
| // be ENCRYPTION-AWARE: the out-of-band file walk cannot decode | ||
| // an encrypted meta block, so it would misreport every healthy | ||
| // encrypted table as corrupt and rewrite it on every repair — | ||
| // encrypted tables verify through the recovered table itself | ||
| // (its cache-bypassing, provider-wired data-block scrub). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale inline comment: encrypted tables now use the out-of-band walk, not a table-self scrub.
This comment states encrypted tables "verify through the recovered table itself (its cache-bypassing, provider-wired data-block scrub)," but the guarded call is block_verify_passes(...) → verify_sst_file_with_context(...), the same encryption-aware out-of-band walk used for unencrypted tables (the recovered table only supplies metadata.id). This contradicts block_verify_passes's own docstring ("One uniform path for encrypted and unencrypted tables"). Reword so it doesn't imply a separate encrypted-verify path on this security-relevant gate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/repair.rs` around lines 537 - 542, Update the inline comment describing
encrypted-table verification near block_verify_passes and
verify_sst_file_with_context so it accurately states that both encrypted and
unencrypted tables use the same encryption-aware out-of-band walk, with the
recovered table only supplying metadata.id; remove the claim that verification
occurs through a table-self scrub.
Comment on lines
+1294
to
1308
| let report = salvage_sst_with_options(&source, dest.clone(), &fs, &options)?; | ||
| assert_eq!( | ||
| report.dropped.len(), | ||
| 1, | ||
| "exactly the corrupt block is dropped: {report:?}", | ||
| ); | ||
| assert!( | ||
| report.entries_salvaged < u64::from(n), | ||
| "the dropped block's rows are lost: {report:?}", | ||
| ); | ||
| assert_eq!( | ||
| reopen_item_count(dest, &fs)?, | ||
| report.entries_salvaged, | ||
| u64::from(n - deleted), | ||
| "every live row is recovered, the deleted prefix is skipped", | ||
| "the recovered copy reopens with every salvaged row live", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify delete resurrection through logical reads.
The tests only validate physical counts, so they can pass while deleted rows remain hidden.
src/salvage/tests.rs#L1294-L1308: read a deleted key outside the corrupt block.src/salvage/tests.rs#L1417-L1430: read surviving deleted keys after the poisoned block is dropped.src/salvage/tests.rs#L1540-L1550: assert keys at deleted positions5,50, and150are visible.
As per path instructions, “Verify test coverage of edge cases and error paths.”
📍 Affects 1 file
src/salvage/tests.rs#L1294-L1308(this comment)src/salvage/tests.rs#L1417-L1430src/salvage/tests.rs#L1540-L1550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/salvage/tests.rs` around lines 1294 - 1308, Extend the salvage tests to
verify logical visibility of deleted rows, not only physical counts: in
src/salvage/tests.rs lines 1294-1308, read and assert visibility of a deleted
key outside the corrupt block; at lines 1417-1430, read surviving deleted keys
after dropping the poisoned block; and at lines 1540-1550, assert keys at
deleted positions 5, 50, and 150 are visible. Use the existing read/query
helpers and preserve the current count assertions.
Source: Path instructions
Comment on lines
+389
to
+438
| /// Bit rot confined to a block's PARITY trailer reads as Clean (the payload | ||
| /// checksum passes and parity is only consulted on a payload mismatch), so | ||
| /// without an explicit trailer check the heal pass would leave dead ECC on | ||
| /// disk — a later payload fault could no longer be recovered. `heal_in_place` | ||
| /// must verify each clean block's trailer against freshly computed parity and | ||
| /// PERSIST a rebuilt trailer on a mismatch (the pass holds the read+write | ||
| /// handle; the payload is untouched, so the rewrite is size-preserving). | ||
| #[test] | ||
| fn heal_in_place_restores_a_rotted_parity_trailer() -> crate::Result<()> { | ||
| use crate::coding::Decode; | ||
|
|
||
| let dir = tempfile::tempdir()?; | ||
| let (sst_path, block) = write_ecc_sst(dir.path()); | ||
|
|
||
| // Flip one byte INSIDE the first data block's parity trailer (right after | ||
| // its `data_length` payload): the payload checksum still verifies, so the | ||
| // block reads back Clean. | ||
| let mut bytes = std::fs::read(&sst_path)?; | ||
| let base = block.offset().0 as usize; | ||
| let Some(mut cursor) = bytes.get(base..) else { | ||
| panic!("first data block within the file"); | ||
| }; | ||
| let header = Header::decode_from(&mut cursor)?; | ||
| let trailer_pos = base + Header::header_len(header.block_type) + header.data_length as usize; | ||
| let Some(slot) = bytes.get_mut(trailer_pos) else { | ||
| panic!("parity trailer within the file"); | ||
| }; | ||
| let rotted = *slot ^ 0xFF; | ||
| *slot = rotted; | ||
| std::fs::write(&sst_path, &bytes)?; | ||
|
|
||
| // Reopen (fresh caches + fds) and heal in place. | ||
| let tree = open_ecc_tree(dir.path()); | ||
| let report = patrol_scrub(&tree, &PatrolScrubOptions::default().heal_in_place(true)); | ||
|
|
||
| assert!( | ||
| report.blocks_healed_in_place >= 1, | ||
| "the rotted parity trailer is rebuilt and persisted: {report:?}", | ||
| ); | ||
| assert_eq!(report.uncorrectable_blocks, 0, "{report:?}"); | ||
| assert!(report.is_ok(), "a parity rebuild is a heal, not a finding"); | ||
|
|
||
| // The on-disk trailer byte is restored (no longer the rotted value). | ||
| let healed = std::fs::read(&sst_path)?; | ||
| let Some(&now) = healed.get(trailer_pos) else { | ||
| panic!("parity trailer within the healed file"); | ||
| }; | ||
| assert_ne!(now, rotted, "the rotted trailer byte was rewritten on disk"); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify that healing restores the original parity byte.
assert_ne!(now, rotted) also accepts arbitrary incorrect parity. Save the original byte and assert exact restoration.
Proposed fix
- let rotted = *slot ^ 0xFF;
+ let original = *slot;
+ let rotted = original ^ 0xFF;
...
- assert_ne!(now, rotted, "the rotted trailer byte was rewritten on disk");
+ assert_eq!(now, original, "the original parity byte was restored");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Bit rot confined to a block's PARITY trailer reads as Clean (the payload | |
| /// checksum passes and parity is only consulted on a payload mismatch), so | |
| /// without an explicit trailer check the heal pass would leave dead ECC on | |
| /// disk — a later payload fault could no longer be recovered. `heal_in_place` | |
| /// must verify each clean block's trailer against freshly computed parity and | |
| /// PERSIST a rebuilt trailer on a mismatch (the pass holds the read+write | |
| /// handle; the payload is untouched, so the rewrite is size-preserving). | |
| #[test] | |
| fn heal_in_place_restores_a_rotted_parity_trailer() -> crate::Result<()> { | |
| use crate::coding::Decode; | |
| let dir = tempfile::tempdir()?; | |
| let (sst_path, block) = write_ecc_sst(dir.path()); | |
| // Flip one byte INSIDE the first data block's parity trailer (right after | |
| // its `data_length` payload): the payload checksum still verifies, so the | |
| // block reads back Clean. | |
| let mut bytes = std::fs::read(&sst_path)?; | |
| let base = block.offset().0 as usize; | |
| let Some(mut cursor) = bytes.get(base..) else { | |
| panic!("first data block within the file"); | |
| }; | |
| let header = Header::decode_from(&mut cursor)?; | |
| let trailer_pos = base + Header::header_len(header.block_type) + header.data_length as usize; | |
| let Some(slot) = bytes.get_mut(trailer_pos) else { | |
| panic!("parity trailer within the file"); | |
| }; | |
| let rotted = *slot ^ 0xFF; | |
| *slot = rotted; | |
| std::fs::write(&sst_path, &bytes)?; | |
| // Reopen (fresh caches + fds) and heal in place. | |
| let tree = open_ecc_tree(dir.path()); | |
| let report = patrol_scrub(&tree, &PatrolScrubOptions::default().heal_in_place(true)); | |
| assert!( | |
| report.blocks_healed_in_place >= 1, | |
| "the rotted parity trailer is rebuilt and persisted: {report:?}", | |
| ); | |
| assert_eq!(report.uncorrectable_blocks, 0, "{report:?}"); | |
| assert!(report.is_ok(), "a parity rebuild is a heal, not a finding"); | |
| // The on-disk trailer byte is restored (no longer the rotted value). | |
| let healed = std::fs::read(&sst_path)?; | |
| let Some(&now) = healed.get(trailer_pos) else { | |
| panic!("parity trailer within the healed file"); | |
| }; | |
| assert_ne!(now, rotted, "the rotted trailer byte was rewritten on disk"); | |
| Ok(()) | |
| } | |
| /// Bit rot confined to a block's PARITY trailer reads as Clean (the payload | |
| /// checksum passes and parity is only consulted on a payload mismatch), so | |
| /// without an explicit trailer check the heal pass would leave dead ECC on | |
| /// disk — a later payload fault could no longer be recovered. `heal_in_place` | |
| /// must verify each clean block's trailer against freshly computed parity and | |
| /// PERSIST a rebuilt trailer on a mismatch (the pass holds the read+write | |
| /// handle; the payload is untouched, so the rewrite is size-preserving). | |
| #[test] | |
| fn heal_in_place_restores_a_rotted_parity_trailer() -> crate::Result<()> { | |
| use crate::coding::Decode; | |
| let dir = tempfile::tempdir()?; | |
| let (sst_path, block) = write_ecc_sst(dir.path()); | |
| // Flip one byte INSIDE the first data block's parity trailer (right after | |
| // its `data_length` payload): the payload checksum still verifies, so the | |
| // block reads back Clean. | |
| let mut bytes = std::fs::read(&sst_path)?; | |
| let base = block.offset().0 as usize; | |
| let Some(mut cursor) = bytes.get(base..) else { | |
| panic!("first data block within the file"); | |
| }; | |
| let header = Header::decode_from(&mut cursor)?; | |
| let trailer_pos = base + Header::header_len(header.block_type) + header.data_length as usize; | |
| let Some(slot) = bytes.get_mut(trailer_pos) else { | |
| panic!("parity trailer within the file"); | |
| }; | |
| let original = *slot; | |
| let rotted = original ^ 0xFF; | |
| *slot = rotted; | |
| std::fs::write(&sst_path, &bytes)?; | |
| // Reopen (fresh caches + fds) and heal in place. | |
| let tree = open_ecc_tree(dir.path()); | |
| let report = patrol_scrub(&tree, &PatrolScrubOptions::default().heal_in_place(true)); | |
| assert!( | |
| report.blocks_healed_in_place >= 1, | |
| "the rotted parity trailer is rebuilt and persisted: {report:?}", | |
| ); | |
| assert_eq!(report.uncorrectable_blocks, 0, "{report:?}"); | |
| assert!(report.is_ok(), "a parity rebuild is a heal, not a finding"); | |
| // The on-disk trailer byte is restored (no longer the rotted value). | |
| let healed = std::fs::read(&sst_path)?; | |
| let Some(&now) = healed.get(trailer_pos) else { | |
| panic!("parity trailer within the healed file"); | |
| }; | |
| assert_eq!(now, original, "the original parity byte was restored"); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scrub/ecc_tests.rs` around lines 389 - 438, Update
heal_in_place_restores_a_rotted_parity_trailer to capture the original trailer
byte before flipping it, then assert the healed on-disk byte equals that saved
value rather than only asserting it differs from the rotted value.
Comment on lines
+436
to
+443
| /// | ||
| /// Scope: FRAME integrity only — checksum, decompress, decrypt, ECC. The | ||
| /// block body (restart trailer, entry framing, value-type tags) is | ||
| /// deliberately NOT decoded here: a scrub is a lightweight bit-rot patrol, | ||
| /// and any body-level fault in a checksum-clean block is (a) not producible | ||
| /// by media corruption (the checksum covers the payload) and (b) the domain | ||
| /// of the full semantic pass, [`crate::verify::verify_sst_file`]. Both the | ||
| /// plain patrol scrub and the in-place heal share exactly this depth. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not describe verify_sst_file as a semantic pass.
verify_sst_file is also frame-level: it walks headers, checks payload hashes, and validates parity without decoding restart trailers, entries, or value tags. Remove this link or reference an actual body-decoding verifier.
Proposed clarification
-/// domain of the full semantic pass, [`crate::verify::verify_sst_file`]. Both the
-/// plain patrol scrub and the in-place heal share exactly this depth.
+/// domain of callers that fully decode and validate block bodies.
+/// [`crate::verify::verify_sst_file`] is likewise a frame-level verifier.
+/// Both plain patrol scrub and in-place heal share exactly this depth.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// | |
| /// Scope: FRAME integrity only — checksum, decompress, decrypt, ECC. The | |
| /// block body (restart trailer, entry framing, value-type tags) is | |
| /// deliberately NOT decoded here: a scrub is a lightweight bit-rot patrol, | |
| /// and any body-level fault in a checksum-clean block is (a) not producible | |
| /// by media corruption (the checksum covers the payload) and (b) the domain | |
| /// of the full semantic pass, [`crate::verify::verify_sst_file`]. Both the | |
| /// plain patrol scrub and the in-place heal share exactly this depth. | |
| /// | |
| /// Scope: FRAME integrity only — checksum, decompress, decrypt, ECC. The | |
| /// block body (restart trailer, entry framing, value-type tags) is | |
| /// deliberately NOT decoded here: a scrub is a lightweight bit-rot patrol, | |
| /// and any body-level fault in a checksum-clean block is (a) not producible | |
| /// by media corruption (the checksum covers the payload) and (b) the domain | |
| /// of callers that fully decode and validate block bodies. | |
| /// [`crate::verify::verify_sst_file`] is likewise a frame-level verifier. | |
| /// Both plain patrol scrub and in-place heal share exactly this depth. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/table/util.rs` around lines 436 - 443, Update the FRAME integrity comment
near the scrub logic to remove the inaccurate description of
crate::verify::verify_sst_file as a semantic or body-decoding pass. Keep the
explanation focused on the shared frame-level validation depth, or reference
only a verifier that actually decodes block bodies.
Comment on lines
+816
to
+820
| pub(crate) fn verify_sst_file_with_context( | ||
| fs: &dyn crate::fs::Fs, | ||
| path: &std::path::Path, | ||
| encryption: Option<&dyn crate::encryption::EncryptionProvider>, | ||
| table_id: crate::TableId, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Propagate the supplied table_id into verification reports.
The new context API accepts the real table ID, but warnings, errors, and scan_sst_blocks still use 0. Encrypted repair diagnostics will therefore identify the wrong table.
Proposed fix
- table_id: 0,
+ table_id,
...
- table_id: 0,
+ table_id,
...
- table_id: 0,
+ table_id,
...
- match scan_sst_blocks(fs, path, 0, max_enc_overhead, ecc, ecc_unrecognized) {
+ match scan_sst_blocks(
+ fs,
+ path,
+ table_id,
+ max_enc_overhead,
+ ecc,
+ ecc_unrecognized,
+ ) {
...
- table_id: 0,
+ table_id,Also applies to: 835-899
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/verify.rs` around lines 816 - 820, Update verify_sst_file_with_context
and its warning/error reporting plus scan_sst_blocks calls to use the supplied
table_id instead of the hardcoded 0. Preserve the existing verification behavior
while ensuring all encrypted repair diagnostics identify the actual table.
Comment on lines
+225
to
+282
| /// Rot confined to a Page-ECC parity trailer is invisible to the payload | ||
| /// checksum (the checksum covers the payload only; parity is consulted just | ||
| /// for recovery on a mismatch), so a walk that merely SKIPS the trailer | ||
| /// reports the SST as healthy while its ECC is dead — a later payload fault | ||
| /// on that block would no longer be recoverable. The out-of-band walk must | ||
| /// compare each clean block's trailer against freshly computed parity and | ||
| /// flag a mismatch. | ||
| #[cfg(feature = "page_ecc")] | ||
| #[test] | ||
| fn verify_sst_file_detects_a_rotted_parity_trailer() { | ||
| use crate::coding::Decode; | ||
| use crate::table::block::Header; | ||
|
|
||
| let dir = tempfile::tempdir().unwrap(); | ||
| { | ||
| let any = Config::new( | ||
| dir.path(), | ||
| SequenceNumberCounter::default(), | ||
| SequenceNumberCounter::default(), | ||
| ) | ||
| .data_block_compression_policy(CompressionPolicy::all(CompressionType::None)) | ||
| .page_ecc(true) | ||
| .ecc_scheme(crate::runtime_config::EccScheme::ReedSolomon { | ||
| data_shards: 4, | ||
| parity_shards: 2, | ||
| }) | ||
| .open() | ||
| .unwrap(); | ||
| for i in 0u64..2_000 { | ||
| let key = format!("k{i:08}"); | ||
| let val = format!("v{i:08}"); | ||
| any.insert(key.as_bytes(), val.as_bytes(), 1 + i); | ||
| } | ||
| any.flush_active_memtable(2_001).unwrap(); | ||
| drop(any); | ||
| } | ||
| let sst_path = pick_first_sst_path(dir.path()); | ||
|
|
||
| // The first data block sits at file offset 0 (the writer opens the file | ||
| // with the `data` section). Its parity trailer follows the payload: | ||
| // header_len + data_length. | ||
| let mut bytes = std::fs::read(&sst_path).unwrap(); | ||
| let mut cursor = bytes.as_slice(); | ||
| let header = Header::decode_from(&mut cursor).unwrap(); | ||
| let trailer_pos = Header::header_len(header.block_type) + header.data_length as usize; | ||
| let slot = bytes | ||
| .get_mut(trailer_pos) | ||
| .expect("parity trailer within the file"); | ||
| *slot ^= 0xFF; | ||
| std::fs::write(&sst_path, &bytes).unwrap(); | ||
|
|
||
| let report = verify_sst_file(&sst_path); | ||
| assert!( | ||
| !report.is_ok(), | ||
| "a rotted parity trailer under a clean payload checksum must be \ | ||
| flagged (dead ECC), got {report:?}", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the specific parity-mismatch error.
!report.is_ok() can pass because of an unrelated failure, so this regression does not prove that parity-trailer validation ran.
Proposed fix
assert!(
- !report.is_ok(),
+ report.errors.iter().any(|error| matches!(
+ error,
+ BlockVerifyError::EccParityMismatch { .. }
+ )),
"a rotted parity trailer under a clean payload checksum must be \
flagged (dead ECC), got {report:?}",
);As per coding guidelines, corruption tests must tamper the relevant on-disk field and assert the error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Rot confined to a Page-ECC parity trailer is invisible to the payload | |
| /// checksum (the checksum covers the payload only; parity is consulted just | |
| /// for recovery on a mismatch), so a walk that merely SKIPS the trailer | |
| /// reports the SST as healthy while its ECC is dead — a later payload fault | |
| /// on that block would no longer be recoverable. The out-of-band walk must | |
| /// compare each clean block's trailer against freshly computed parity and | |
| /// flag a mismatch. | |
| #[cfg(feature = "page_ecc")] | |
| #[test] | |
| fn verify_sst_file_detects_a_rotted_parity_trailer() { | |
| use crate::coding::Decode; | |
| use crate::table::block::Header; | |
| let dir = tempfile::tempdir().unwrap(); | |
| { | |
| let any = Config::new( | |
| dir.path(), | |
| SequenceNumberCounter::default(), | |
| SequenceNumberCounter::default(), | |
| ) | |
| .data_block_compression_policy(CompressionPolicy::all(CompressionType::None)) | |
| .page_ecc(true) | |
| .ecc_scheme(crate::runtime_config::EccScheme::ReedSolomon { | |
| data_shards: 4, | |
| parity_shards: 2, | |
| }) | |
| .open() | |
| .unwrap(); | |
| for i in 0u64..2_000 { | |
| let key = format!("k{i:08}"); | |
| let val = format!("v{i:08}"); | |
| any.insert(key.as_bytes(), val.as_bytes(), 1 + i); | |
| } | |
| any.flush_active_memtable(2_001).unwrap(); | |
| drop(any); | |
| } | |
| let sst_path = pick_first_sst_path(dir.path()); | |
| // The first data block sits at file offset 0 (the writer opens the file | |
| // with the `data` section). Its parity trailer follows the payload: | |
| // header_len + data_length. | |
| let mut bytes = std::fs::read(&sst_path).unwrap(); | |
| let mut cursor = bytes.as_slice(); | |
| let header = Header::decode_from(&mut cursor).unwrap(); | |
| let trailer_pos = Header::header_len(header.block_type) + header.data_length as usize; | |
| let slot = bytes | |
| .get_mut(trailer_pos) | |
| .expect("parity trailer within the file"); | |
| *slot ^= 0xFF; | |
| std::fs::write(&sst_path, &bytes).unwrap(); | |
| let report = verify_sst_file(&sst_path); | |
| assert!( | |
| !report.is_ok(), | |
| "a rotted parity trailer under a clean payload checksum must be \ | |
| flagged (dead ECC), got {report:?}", | |
| ); | |
| } | |
| /// Rot confined to a Page-ECC parity trailer is invisible to the payload | |
| /// checksum (the checksum covers the payload only; parity is consulted just | |
| /// for recovery on a mismatch), so a walk that merely SKIPS the trailer | |
| /// reports the SST as healthy while its ECC is dead — a later payload fault | |
| /// on that block would no longer be recoverable. The out-of-band walk must | |
| /// compare each clean block's trailer against freshly computed parity and | |
| /// flag a mismatch. | |
| #[cfg(feature = "page_ecc")] | |
| #[test] | |
| fn verify_sst_file_detects_a_rotted_parity_trailer() { | |
| use crate::coding::Decode; | |
| use crate::table::block::Header; | |
| let dir = tempfile::tempdir().unwrap(); | |
| { | |
| let any = Config::new( | |
| dir.path(), | |
| SequenceNumberCounter::default(), | |
| SequenceNumberCounter::default(), | |
| ) | |
| .data_block_compression_policy(CompressionPolicy::all(CompressionType::None)) | |
| .page_ecc(true) | |
| .ecc_scheme(crate::runtime_config::EccScheme::ReedSolomon { | |
| data_shards: 4, | |
| parity_shards: 2, | |
| }) | |
| .open() | |
| .unwrap(); | |
| for i in 0u64..2_000 { | |
| let key = format!("k{i:08}"); | |
| let val = format!("v{i:08}"); | |
| any.insert(key.as_bytes(), val.as_bytes(), 1 + i); | |
| } | |
| any.flush_active_memtable(2_001).unwrap(); | |
| drop(any); | |
| } | |
| let sst_path = pick_first_sst_path(dir.path()); | |
| // The first data block sits at file offset 0 (the writer opens the file | |
| // with the `data` section). Its parity trailer follows the payload: | |
| // header_len + data_length. | |
| let mut bytes = std::fs::read(&sst_path).unwrap(); | |
| let mut cursor = bytes.as_slice(); | |
| let header = Header::decode_from(&mut cursor).unwrap(); | |
| let trailer_pos = Header::header_len(header.block_type) + header.data_length as usize; | |
| let slot = bytes | |
| .get_mut(trailer_pos) | |
| .expect("parity trailer within the file"); | |
| *slot ^= 0xFF; | |
| std::fs::write(&sst_path, &bytes).unwrap(); | |
| let report = verify_sst_file(&sst_path); | |
| assert!( | |
| report.errors.iter().any(|error| matches!( | |
| error, | |
| BlockVerifyError::EccParityMismatch { .. } | |
| )), | |
| "a rotted parity trailer under a clean payload checksum must be \ | |
| flagged (dead ECC), got {report:?}", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/verify/block_verify_tests.rs` around lines 225 - 282, Strengthen
verify_sst_file_detects_a_rotted_parity_trailer by asserting the report contains
the specific parity-trailer mismatch error, rather than only checking that
report.is_ok() is false. Keep the existing trailer-byte corruption setup and
match the established error variant or message produced by parity validation, so
unrelated verification failures cannot satisfy the regression.
Source: Coding guidelines
Comment on lines
+899
to
+909
| let report = config(dir.path()).repair_with_salvage(true)?; | ||
| assert_eq!( | ||
| report.salvaged, 1, | ||
| "a persistent correctable filter fault drives the table through salvage: {:?}", | ||
| report.unreadable_files, | ||
| ); | ||
| assert_eq!( | ||
| report.recovered, 1, | ||
| "the rewritten table joins the manifest" | ||
| ); | ||
| Ok(()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the rebuilt filter before declaring recovery successful.
recovered == 1 only proves the table was admitted; the filter loads lazily. Reopen with the same configuration and perform point reads, as the non-ECC filter test does at Lines 1062–1076.
As per path instructions, “Verify test coverage of edge cases and error paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/repair.rs` around lines 899 - 909, Extend the repair test after the
report assertions to reopen the repaired table using the same configuration and
perform point reads, following the pattern used by the non-ECC filter test
around its point-read checks. Assert those reads succeed so the rebuilt filter
is actually loaded and exercised before accepting recovery.
Source: Path instructions
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
1 participant
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 the salvage / autoheal recovery surface for Page-ECC, encrypted,
dictionary-compressed, and columnar SSTs, adding both a reactive new-file fast
path and a proactive in-place self-heal.
Recovery now has two complementary primitives:
recover every readable block into a fresh, fully-valid copy, quarantine the
rest. Clean blocks are copied through verbatim (raw bytes byte-copied,
skipping decode + re-encode + recompression) after their ECC parity trailer
(when present) verifies against freshly computed parity; ECC-recovered or
parity-rotted blocks are re-encoded from their healed payload.
persist each Page-ECC correction by writing the corrected block back at its
existing offset (size-preserving), leaving every healthy block untouched —
O(damage), not O(file). Uncorrectable blocks are reported and left for salvage.
What's included
the provider + dictionary + table id into both the read and the rewrite.
Encrypted-block AAD binds the table id, so a non-zero-id source decrypts and the
recovered copy reopens consistently.
compression, ECC, restart interval, columnar layout + zone map, per-KV checksum
footers). On a build without
page_ecc, ECC is mirrored as off (no parity canbe emitted) and such blocks are re-encoded rather than byte-copied.
into a columnar copy with its PAX sub-columns intact, not a row-major downgrade.
A malformed columnar block (frame decodes, row materialization fails) is
dropped like any other block-local failure instead of aborting the salvage.
positional delete bitmap cannot be applied FAITHFULLY (unreadable bitmap, a
bitmap whose positioning zone map is unreadable, or a zone map whose claimed
per-block row counts do not match the actual decoded counts — each block is
FULLY decoded during position verification, so a checksum-repatched block
with intact leading count bytes but broken column framing is unverifiable,
not trusted) fails the salvage by default:
recovering "all rows live" would resurrect deleted rows. The degradation is an
explicit opt-in (
SalvageOptions::allow_delete_resurrection,sst-dump salvage --allow-delete-resurrection); automated repair never opts in and quarantinessuch tables instead. A readable bitmap is applied on read via the delete-masked
re-emit, so a verbatim copy never resurrects deleted rows.
bit-rotted record, so one bad record costs only itself. A pre-existing file at
the destination survives a failed open (only a partial file this call created
is cleaned up). The salvaged file is compacted and therefore not a drop-in
replacement while SST
ValueHandles point into the source:BlobSalvageReport::offset_remapmaps every salvaged record's source offsetto its new one so callers can re-target handles. A KV-separated source's linked_blob_files section is carried into the salvaged SST so blob GC never drops a still-referenced blob.
ECC-recovered ones re-encoded; reported via
SalvageReport::blocks_copied_verbatim.A failure of the verbatim re-read (transient I/O, corrupt header) only
disqualifies the byte-copy: the already-verified first read is re-encoded
instead of dropping the block.
PatrolScrubOptions::heal_in_place: corrected blocksare written back at their offset and
sync_data'd before moving on, so a crashmid-heal leaves the block in its prior, still-RS-correctable state. Reported via
PatrolScrubReport::blocks_healed_in_place. A transient correction (cleanconfirming re-read) is a no-op, not a finding; non-ECC tables still get their
integrity checked via the checksum-verifying scrub path. Rot confined to a
PARITY trailer (invisible to a clean payload checksum) is detected and the
trailer rebuilt in place, so the block's ECC stays live.
regardless of its stored table id (no out-of-band id knowledge needed) and
stamps the recovered copy with the SOURCE's stored id;
SalvageOptions::table_idis only the open/AAD context for encrypted sources. Direct-block emission also
validates equal-key block boundaries (seqnos must keep strictly decreasing).
plaintext, so the full out-of-band section walk works on encrypted files; the
provider is needed only to decode the meta block (ECC descriptor). Repair's
block-verify gate uses ONE uniform path for encrypted and unencrypted tables:
every section (data, index/TLI, filter, zone map, delete bitmap, locator,
meta) verifies against its raw on-disk checksum, flagging even persistent
ECC-correctable faults that a live read would silently heal in memory. A
healthy encrypted SST is no longer quarantined and rewritten on every repair.
The walk also VERIFIES each Page-ECC parity trailer against parity freshly
computed over the checksum-clean payload (new
BlockVerifyError::EccParityMismatch), so rot confined to a trailer nolonger reads as a healthy block with dead ECC; it sizes encrypted blocks
with the provider's AEAD overhead (mirroring the live read path); and the
repair gate requires a WARNING-free report, so an unrecognized-ECC table
(whose SST-block sections the walk cannot check) is salvaged under a
recognized descriptor instead of stamping unchecked bytes into the rebuilt
manifest.
Aes256GcmProviderAAD-bound state onzstd_any(it is only read onthe block-AAD path, which is itself zstd-gated) so an
encryption-without-zstdbuild is warning-clean.
Testing
cargo nextest run --all-features: 2561 passed.cargo clippy --all-features --all-targets -- -D warnings: clean.byte-for-byte vs its pre-fault state, and an uncorrectable block is left
untouched for salvage.
cargo doc(default + all-features), doctests, and the no-stdthumbv7emalloccheck: clean.Closes #568
Closes #570
Summary by CodeRabbit
New Features
--allow-delete-resurrectionfor explicitly opting into recovery when delete metadata is damaged.Bug Fixes
Documentation