feat(salvage): faithful + fast recovery and in-place ECC autoheal#575
Open
polaz wants to merge 140 commits into
Open
feat(salvage): faithful + fast recovery and in-place ECC autoheal#575polaz wants to merge 140 commits into
polaz wants to merge 140 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (30)
📝 WalkthroughWalkthroughThis PR extends SST salvage with encryption, dictionary, table-ID, columnar, delete-bitmap, and blob-file context; adds faithful copy-through and layout mirroring; and introduces Page-ECC parity verification and in-place healing integrated with patrol scrub. ChangesSalvage and repair integration
Page-ECC verification and healing
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 salvage_load_block
Repair->>Salvage: SalvageOptions with runtime context
Salvage->>Table: Recover source with table ID and encryption
Table-->>Salvage: Recovered source table
Salvage->>Writer: Mirror persisted layout
Salvage->>Block: Verify and load data block
Block-->>Salvage: Verbatim frame or recovered data
Salvage->>Writer: Copy or re-emit block
Salvage-->>Repair: SalvageReport
sequenceDiagram
participant Scrub as patrol_scrub
participant Selector as scan_one
participant Table as heal_data_blocks_in_place
participant Block as Block::heal_frame
participant Fs as filesystem
Scrub->>Selector: Select in-place healing
Selector->>Table: Scan ECC data blocks
Table->>Block: Verify and recover frame
Block-->>Table: Healed frame or error
Table->>Fs: Write frame at original offset
Fs-->>Table: Persist result
Table-->>Scrub: PatrolScrubReport
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 recovery options, delete-aware columnar salvage, verbatim-copy gating, derived blob links, and cleanup for failed salvage outputs. |
| src/repair.rs | Adds encryption-aware block verification, ECC grading, and quarantine-before-salvage repair decisions. |
| src/table/mod.rs | Adds delete-position verification and recovery-mode handling used by safe salvage opens. |
| src/vlog/blob_file/writer.rs | Cleans up files created during failed blob writer initialization. |
| src/scrub.rs | Adds in-place ECC healing and checksum refresh reporting for scrub runs. |
| src/verify.rs | Expands SST block verification to report ECC parity and descriptor states used by repair. |
Reviews (35): Last reviewed commit: "style(salvage): drop a leftover writer s..." | 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.
![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
coordinode-lsm-tree/src/repair.rs
Lines 825 to 829 in a6262c6
Reject SSTs that reference unrecovered blob files
When repairing a KV-separated tree where a recovered SST still contains ValueHandles to blob file N but that blob file is missing or its metadata/SFA is unreadable, this only records the blob error and still persists all recovered SSTs with a BlobFileList that omits N. The reopened BlobTree then has index entries whose handles cannot be resolved (Accessor::get returns None, and resolve_value_handle panics), and orphan cleanup can delete an unmanifested blob copy. Cross-check recovered table blob references against this map before writing the manifest, and quarantine/drop the dependent SSTs or abort the repair when a required blob was not recovered.
ℹ️ 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".
scrub_block discards the decoded block without checking its role against the caller's expected type (load_block performs that swap-defence check): an index entry misdirected at another checksum-valid block of a different role scrubs as clean, so misdirection corruption is invisible to the patrol scrub. Fails until scrub_block rejects a role mismatch.
scrub_block discarded the decoded block without comparing its role to the caller's expected type, unlike load_block's swap-defence check: an index entry misdirected at another checksum-valid block of a different role scrubbed as clean, hiding misdirection corruption from the patrol scrub. The scrub and heal walks also always passed BlockType::Data, which only worked because of that missing check — a columnar segment's data blocks are sealed as BlockType::Columnar. scrub_block now rejects a role mismatch as an uncorrectable finding, and both walks pass the table's real data-block role (Columnar for a columnar segment, Data otherwise) via the new data_block_role() helper. Makes scrub_block_rejects_a_block_of_the_wrong_role pass.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/table/mod.rs (1)
835-876: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winReject malformed frames before in-place parity repair.
raw_block_parity_deltareturnsOk(Some(fresh))even when the on-disk trailer has the wrong length. The clean-heal path then writesfreshat the computed trailer offset; a truncated trailer can overwrite the beginning of the next block, violating the size-preserving heal contract. Also validate that the second-read header still hasself.data_block_role()before using its lengths.Use checked arithmetic, require
trailer.len() == fresh.len(), and treat mismatches as unverifiable. Add a tempdir-backed regression test asserting adjacent block bytes remain unchanged.As per path instructions,
src/**/*.rsmust use memory-safe, panic-free error handling; malformed on-disk frames must be rejected rather than repaired in place.Also applies to: 1234-1305
🤖 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/mod.rs` around lines 835 - 876, Update raw_block_parity_delta and the related clean-heal flow to reject unverifiable frames before writing: validate the second-read header has self.data_block_role(), use checked arithmetic for all header/data/trailer ranges, and require trailer.len() == fresh.len(), returning Err(()) on any mismatch. Add a tempdir-backed regression test covering adjacent blocks and assert malformed-frame repair leaves neighboring block bytes unchanged.Source: 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.
Inline comments:
In `@src/table/tests.rs`:
- Around line 791-795: The assertion around the role-mismatch test must verify
the specific error rather than accepting any failure. Update the outcome check
to match `crate::Error::InvalidTag(("BlockType", _))`, while preserving the
test’s requirement that the checksum-clean block with the wrong role is
rejected.
---
Outside diff comments:
In `@src/table/mod.rs`:
- Around line 835-876: Update raw_block_parity_delta and the related clean-heal
flow to reject unverifiable frames before writing: validate the second-read
header has self.data_block_role(), use checked arithmetic for all
header/data/trailer ranges, and require trailer.len() == fresh.len(), returning
Err(()) on any mismatch. Add a tempdir-backed regression test covering adjacent
blocks and assert malformed-frame repair leaves neighboring block bytes
unchanged.
🪄 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: d1c2f7b0-76b7-4f24-98bc-2d6f7c2f071f
📒 Files selected for processing (3)
src/table/mod.rssrc/table/tests.rssrc/table/util.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1f10004fe
ℹ️ 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".
… healable raw_block_parity_delta compares the on-disk trailer against freshly computed parity without checking their lengths match: a frame whose trailer is shorter than the expected parity grades as a healable mismatch, and the in-place heal would write the full-length rebuilt parity at the trailer offset — past the frame's end, into the next block's bytes — violating the size-preserving heal contract. Fails until a length mismatch is rejected as unverifiable.
raw_block_parity_delta compared the on-disk trailer against freshly computed parity without checking their lengths: a frame whose trailer length disagreed with the expected parity graded as a healable mismatch, and the clean-heal arm would write the full-length rebuilt parity at the trailer offset — past the frame's end, into the next block's bytes. A trailer-length mismatch is now unverifiable (never healed), the payload-end offset uses checked arithmetic so a forged data_length on a 32-bit target fails as unverifiable instead of wrapping, and the clean-heal arm verifies the re-read header still carries the table's data-block role before trusting its lengths (a role change between the two reads is surfaced as a finding). Makes raw_block_parity_delta_rejects_a_trailer_length_mismatch pass.
… test The assertion accepted any failure; it now requires the specific InvalidTag(BlockType) the role check produces, so an unrelated read error cannot satisfy the regression.
PatrolScrubReport::is_ok() only counts uncorrectable blocks, so a scrub whose findings include a failed manifest-digest refresh (or an index walk that never enumerated the data blocks) still reports ok — a caller following the public status treats the scrub as clean while the manifest keeps a stale digest. Fails until is_ok() also requires an empty findings list.
PatrolScrubReport::is_ok() only counted uncorrectable blocks, so a sweep whose findings included a failed manifest-digest refresh or an unreadable block index (a table whose data blocks were never even enumerated) still reported ok to callers following the public status. is_ok() now also requires an empty findings list; the docs spell out what each finding class means for the caller. Makes the strengthened heal_in_place_reports_a_failed_checksum_refresh assert pass.
…ndered A blob frame whose header CRC and data checksum are internally consistent but whose real_val_len disagrees with the stored bytes is rejected by the live blob reader, yet the salvage scanner yields it as Ok and the rewrite restamps a consistent length — counting a record the live read path treats as corrupt as salvaged. Fails until the blob walk validates the declared value length against the recovered bytes and drops mismatches.
The blob scanner yields a frame whose header CRC and data checksum are internally consistent even when its real_val_len disagrees with the stored bytes; the live blob reader rejects exactly that frame, so re-emitting it restamped a consistent length and laundered a record live reads treat as corrupt. The blob walk now drops such a frame through the corrupt-record path (the salvage only handles uncompressed sources, where the declared and stored lengths must be equal); the walk continues past it. Makes salvage_blob_file_drops_a_frame_with_a_forged_value_length pass.
… verbatim The blob-link union keeps source-list ids no recovered indirection references. A forged record naming a blob that exists nowhere then lands in the salvaged table's linked_blob_files, giving downstream consumers a table that references a blob absent from the blob-file manifest. A dropped block's indirections do not exist in the copy, so a source-only id can never be needed by it. Fails until the copy's links are derived from the recovered indirections exactly.
The blob-link union kept source-list ids no recovered indirection references, on the theory that over-referencing merely delays GC. That was wrong in both directions: a dropped block's indirections do not exist in the copy (so a source-only id can never be needed by it), and a forged record can plant a reference to a blob that exists nowhere — a corrupt manifest reference downstream consumers must never see. The salvaged table's linked_blob_files section is now exactly the links derived from the recovered rows' indirections; the source's section is no longer read at all (its unreadability previously needed a dedicated best-effort path). Makes salvage_drops_source_only_blob_links pass.
Member
Author
|
@coderabbitai Re: reject malformed frames before in-place parity repair (src/table/mod.rs raw_block_parity_delta + the clean-heal arm) — fixed test-first: 550ce4b adds the repro (a frame one byte short of its trailer graded as a healable mismatch), 03c3b37 requires trailer.len() == fresh.len() (a mismatch is unverifiable, never healed — the rebuild can no longer write past the frame's end), computes the payload-end offset with checked arithmetic so a forged data_length cannot wrap on 32-bit targets, and makes the clean-heal arm verify the re-read header still carries the table's data-block role before trusting its lengths (a role change between the two reads is surfaced as a finding). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- referenced commits ---'
git show --stat --oneline 550ce4b6b 03c3b37a0
echo '--- relevant changes ---'
git show --format=fuller --find-renames 550ce4b6b 03c3b37a0 -- src/table/mod.rs src/table/tests.rs
echo '--- symbol map ---'
ast-grep outline src/table/mod.rs --items all --match 'raw_block_parity_delta|heal_data_blocks_in_place|data_block_role' --view expanded
ast-grep outline src/table/tests.rs --items all --match 'parity|heal|trailer' --view expanded
echo '--- current implementation occurrences ---'
rg -n -C 12 'raw_block_parity_delta|trailer\.len\(\)|checked_add|data_block_role\(' src/table/mod.rs
echo '--- current regression-test occurrences ---'
rg -n -C 8 'short|trailer|parity.*heal|heal.*parity|role.*re-read|role.*reread' src/table/tests.rs src/scrub/ecc_tests.rsLength of output: 1353 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- current HEAD and recent relevant history ---'
git rev-parse --short HEAD
git log --oneline -30 -- src/table/mod.rs src/table/tests.rs | sed -n '1,80p'
echo '--- relevant symbols in the current implementation ---'
ast-grep outline src/table/mod.rs --items all --match 'raw_block_parity_delta|heal_data_blocks_in_place|data_block_role' --view expanded || true
echo '--- current implementation ---'
rg -n -C 18 'raw_block_parity_delta|trailer\.len\(\)|checked_add|data_block_role\(' src/table/mod.rs
echo '--- matching regression tests ---'
rg -n -C 12 -i 'short.*trailer|trailer.*short|parity.*heal|heal.*parity|re-read.*role|reread.*role|wrong.*role' src/table/tests.rs src/scrub/ecc_tests.rsLength of output: 40392
Therefore, I can’t verify or resolve this finding yet. Please update the PR branch to include |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a424c5d37
ℹ️ 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".
…ading the MID fallback The verify probe's meta loop returns on the FIRST copy that decodes: a tail meta whose table id is correct but whose ECC descriptor alone was forged (checksum restamped) passes the id cross-check, so its forged unrecognized descriptor is returned without ever trying the intact MID mirror — and a healthy range-tombstone SST is graded degraded-unscanned and quarantined. Fails until the probe falls back to the mirror when a copy decodes with an unrecognized descriptor.
…riptor The ECC-descriptor probe returned on the first meta copy that decoded, so a tail whose table id was intact (passing the id cross-check) but whose descriptor alone was forged to an unrecognized scheme dictated that forged descriptor without the intact MID mirror ever being tried — grading a healthy range-tombstone SST degraded-unscanned and quarantining it. The probe now remembers an unrecognized descriptor and keeps trying the mirror; only when every decodable copy agrees is the scheme reported unrecognized. A forge TO a recognized scheme stays undetectable this way — nothing distinguishes it from a legitimate descriptor without cross-copy arbitration. Makes repair_with_salvage_keeps_a_healthy_rt_sst_with_a_forged_tail_descriptor_only pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 378-383: Remove the redundant self-rebinding `let writer =
writer;` after the optional `use_zstd_dictionary` assignment in the writer
construction flow, leaving the existing `Writer::new` and conditional
configuration unchanged.
🪄 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: ba033c36-e116-4a52-91b3-29822d51fc64
📒 Files selected for processing (8)
src/repair/tests.rssrc/salvage.rssrc/salvage/tests.rssrc/scrub.rssrc/scrub/ecc_tests.rssrc/table/mod.rssrc/table/tests.rssrc/verify.rs
The binding stopped being mutated when the blob-link registration moved into the walk; the rebind that once demoted it from mut is dead.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Member
Author
|
@coderabbitai re-review |
|
Action performedFull review triggered. |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 30 minutes. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 3 minutes. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
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 blob frame whose declared value length disagrees with its stored bytes (a shape the live blob reader rejects) drops as corrupt instead of being restamped into a "salvaged" record. A KV-separated source's blob links are EXACTLY those derived from the recovered indirections of every emitted block — the source's linked_blob_files section is not consulted at all (a forged count word can under-report, hiding a blob GC would delete; a forged record can over-report an id that exists nowhere; and a dropped block's indirections do not exist in the copy, so no source-only id can ever be needed by it); an entry tagged as an indirection whose value fails to decode drops its block instead of laundering corrupt content into the copy. A repair salvage passes the durable file-name id into the open (
SalvageOptions::expected_stored_id), so a checksum-clean tail meta with a forged table_id falls back to the intact MID mirror instead of poisoning the recovered copy's identity; standalone salvage keeps accepting the source's stored id.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. The gate also requires the per-block
EccStatus::Ok— a frame with an unattributable trailer (e.g. an over-sizedforged index handle leaking the next section's bytes) re-encodes its
verified payload instead of offering overlong raw bytes the writer would
reject.
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. The scrub and heal
walks pass the table's real data-block role (Columnar for a columnar
segment, Data otherwise) and
scrub_blockenforces the same role check asload_block, so an index entry misdirected at a checksum-valid block of adifferent role is an uncorrectable finding instead of scrubbing clean. 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; the rebuild first
re-verifies the re-read header's role and its payload against the checksum,
so a fault landing between the two reads is surfaced, never persisted into
fresh parity, and a trailer whose length disagrees with the expected parity
is unverifiable (never healed — the size-preserving write can never reach
past the frame's end). The
in-place write is hard-link safe (checkpoints share the inode): every write
restores the block's ORIGINAL bytes, so a linked checkpoint heals along with
the live file. After a scan that healed blocks in place AND left the file
free of known corruption (no uncorrectable blocks, no findings), the scrub
recomputes the table's full-file digest through its
Fshandle and persistsit to the manifest via a version upgrade (the refreshed digest rides on the
Tablewrapper so older snapshots keep the digest they were recoveredunder, and the tight-space restricted reopen carries it forward), so a
manifest rebuilt over the rotted file no longer flags the healed table on a
later
verify_integrity. A partially-healed file is never restamped — itsstale digest keeps integrity scans failing until the remaining corruption is
actually repaired. A refresh that FAILS (digest recompute or manifest
install) is a scrub finding (
ScrubError::ChecksumRefreshFailed), not aswallowed log line — the healed bytes are durable, and re-running the scrub
refreshes the digest idempotently.
PatrolScrubReport::is_ok()now fails onANY finding (not just uncorrectable blocks), so an unreadable block index or
a failed digest refresh never reads as a clean sweep.
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 grades the report (clean / degraded-but-readable /
degraded-unscanned / corrupt): 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. The
ECC-descriptor probe applies the same caller-known-id cross-check as
recovery (repair passes the recovery-checked id even for unencrypted
reads), so a checksum-clean forged tail meta falls back to the intact MID
mirror instead of dictating a forged descriptor to the walk; standalone
id-less tools keep the previous skip. The probe also keeps trying the
mirror when a copy decodes with an UNRECOGNIZED descriptor (a
descriptor-only forge passes the id check), reporting unrecognized only
when every decodable copy agrees.
Degraded-but-readable covers warning-bearing clean reports AND parity-only
rot (every payload checksum clean, only the trailers dead) — such a table
that salvage cannot faithfully re-emit (range tombstones) is kept as-is
rather than dropped over dead parity. Degraded-UNSCANNED (unrecognized
descriptor: NOTHING about the data was verified) is terminal when salvage
cannot re-emit the table: a range-tombstone SST under an unrecognized
descriptor is quarantined (
repair-quarantine/) with the reason surfacedin
unreadable_files— no partially-verified table reaches the rebuiltmanifest; recovery is manual recompaction under a supported scheme. A FAILED
quarantine move aborts the whole repair before any manifest is installed
(an omitted-but-still-in-place file would be deleted by the next open's
orphan cleanup); a salvage error after a successful move is recorded, since
the original is then safely preserved in quarantine. The
salvage row path additionally verifies per-KV checksum footers before
emitting, so a re-stamped block with a stale per-KV digest drops instead
of being laundered into the copy. Columnar delete-position verification
rejects a decoded zero-row batch (the writer never emits empty blocks), so
a tampered zone map claiming zero rows cannot silently erase a block's
rows past their delete markers. On a build without the ECC codecs, a recognized scheme's
trailers cannot be recomputed — the walk emits the new
BlockVerifyWarning::ParityUnverifiable, routing such tables throughsalvage (a parity-less rewrite on that build) under the same grading. The
derived parity-trailer length is capped like
data_length(arecognized-but-absurd shard layout cannot drive a multi-GB reservation),
and the writer enforces the same cap at block emission, so no legitimate
SST can ever trip it; diagnostics carry the caller-supplied table id, and
sst-dump verifytags parity-only rot asEccParityMismatch. Blob salvagevalidates scanner output (a CRC-consistent frame with an empty key drops as
corrupt instead of panicking the walk).
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: 2596 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