[Store] Make file-per-key metadata scans resilient

[feichai0017]

Description

Fixes #2982.

• Rebuild file-per-key counters transactionally during ScanMeta() so rescans do not double-count and failed traversals preserve the previous totals.
• Skip malformed or empty object files and serialize scans against offload and explicit eviction mutations.

Module

• Mooncake Store (mooncake-store)

Type of Change

• Bug fix

How Has This Been Tested?

Test commands:

pre-commit run --files mooncake-store/include/storage_backend.h mooncake-store/src/storage_backend.cpp mooncake-store/tests/storage_backend_test.cpp
cmake --build build-focused --target storage_backend_test
./build-focused/mooncake-store/tests/storage_backend_test --gtest_brief=1

Test results (Ubuntu):

• Unit tests pass (storage_backend_test: 73/73)
• Integration tests pass (not applicable)
• Manual testing done

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh
• I have run pre-commit run --all-files and all hooks pass (touched-file hooks pass)
• I have updated the documentation (not applicable)
• I have added tests to prove my changes are effective
• For changes >500 LOC: I have filed an RFC issue (not applicable; under 500 LOC)

AI Assistance Disclosure

• No AI tools were used
• AI tools were used: Codex assisted with investigation, implementation, testing, and review.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gemini-code-assist]

Code Review

This pull request introduces concurrent write safety and metadata scan isolation in StorageBackendAdaptor using a std::shared_mutex. It also improves robustness in ScanMeta by catching exceptions from malformed files, checking for empty keys, and reporting directory traversal errors. A high-severity bug was identified in ScanMeta where reusing std::error_code variables for both the directory iterator and loop-body operations can cause errors to be silently cleared during iterator increments. It is recommended to use separate local std::error_code variables inside the loop.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Systematic Error Clearing Bug in Directory Traversal

There is a subtle but significant bug in how std::error_code is handled during the directory traversal in ScanMeta.

The error codes (ec_leaf, ec_d1, ec_root) are passed to both:

1. The iterator itself (e.g., fs::directory_iterator(leaf, ec_leaf) and it.increment(ec_leaf)).
2. Member functions of directory_entry inside the loop body (e.g., it->is_regular_file(ec_leaf), fs::file_size(p, ec_leaf), and it1->is_directory(ec_root)).

The Issue:

If an error occurs inside the loop body (for example, fs::file_size fails and sets ec_leaf), the code executes continue.
During the continue, the loop's increment expression it.increment(ec_leaf) is executed. According to the C++ standard, directory_iterator::increment(ec) clears ec on success.
Consequently, any error that occurred inside the loop body is silently cleared and ignored, unless it happened on the very last file in the directory (where the loop terminates immediately without another increment, leaving ec_leaf set).

This leads to highly non-deterministic behavior where ScanMeta may or may not fail depending on the order of files returned by the filesystem.

Suggested Fix:

Use a separate, local std::error_code variable (e.g., ec_file) for any operations inside the loop body, and handle those errors explicitly (either by breaking/returning if they are fatal, or logging and continuing if they are non-fatal) so they do not interfere with the iterator's state.

For example:

std::error_code ec_file;
if (!it->is_regular_file(ec_file) || ec_file) continue;
uintmax_t sz = fs::file_size(p, ec_file);
if (ec_file) continue;

[feichai0017]

Handled in ac103c9: traversal and per-entry file operations now use separate std::error_code instances.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 85.88235% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mooncake-store/src/storage_backend.cpp	73.33%	12 Missing ⚠️

📢 Thoughts on this report? Let us know!

[he-yufeng]

Nice hardening — this is squarely in the same "tolerate a corrupt/partial on-disk metadata tree instead of crashing or miscounting" space as the earlier bucket-scan fixes. Two things worth a look, one confirmation and one consistency question.

Counting fix looks right. Moving from the in-place total_keys++ / total_size += to local scanned_keys / scanned_size that are assigned once at the end makes a re-scan idempotent — the previous code accumulated on top of the existing totals every time ScanMeta ran, so a second scan double-counted. Good catch, and the scan_mutex_ shared/unique split to isolate a full scan from concurrent offload/evict reads is the right shape.

1. Does struct_pb::from_pb actually throw on a malformed record? The new try { from_pb } catch (const std::exception&) only helps if a corrupt buffer surfaces as a thrown exception. A lot of the yalantinglibs deserialize entry points report failure through a returned std::errc / expected rather than throwing, in which case a truncated file would sail past the catch, leave kv partially/default-initialized, and then be treated as a real record (or skipped only by the kv.key.empty() guard). Worth confirming the deserialize overload in use here throws — if it can also fail by return value, the resilience is only half there and the return should be checked too.

2. Partial flush() vs. rolled-back totals on a mid-scan failure. flush() hands each accumulated batch to handler(...) during the walk, but total_keys / total_size are only committed after the whole tree scans cleanly. If a later directory hits ec_leaf / ec_d1 / ec_root and the function returns FILE_READ_FAIL, the batches already flushed have been consumed by the handler, yet the totals are left at their old values (never updated to reflect the partial work). So on a partial failure the handler's persisted state and the adaptor's counters can diverge. Is the contract that the caller treats a failed ScanMeta as "retry a full scan" and the handler is idempotent enough to absorb the re-flush? If so it'd be good to say that in a comment; if not, the flushed-but-uncounted window looks like a real inconsistency (handler sees keys the counters don't). Either committing the partial totals before returning, or not flushing until the scan is known-good, would close it.

Neither blocks the direction — the scan is clearly more robust than before. Mostly want to make sure the deserialize failure mode is really exception-based and that the partial-flush semantics are intentional.

[feichai0017]

@he-yufeng Confirmed that this struct_pb::from_pb overload returns void and throws on malformed input. ac103c9 also documents the full-scan retry contract: counters commit only after a complete walk, while Master replica registration is idempotent.