Skip to content

[Store] Prevent file-per-key path collisions#2984

Open
feichai0017 wants to merge 2 commits into
kvcache-ai:mainfrom
feichai0017:fix/file-per-key-path-collision
Open

[Store] Prevent file-per-key path collisions#2984
feichai0017 wants to merge 2 commits into
kvcache-ai:mainfrom
feichai0017:fix/file-per-key-path-collision

Conversation

@feichai0017

@feichai0017 feichai0017 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #2980.

  • Use a bounded XXH3-128 filename for file-per-key objects so distinct keys cannot alias through path normalization.
  • Keep legacy files readable and removable, validate their embedded key, and prefer the canonical file when both forms exist.

Module

  • Mooncake Store (mooncake-store)

Type of Change

  • Bug fix

How Has This Been Tested?

Test commands:

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

Test results (Ubuntu):

  • Unit tests pass (storage_backend_test: 74/74; utils_test: 12/12)
  • 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 AI review requested due to automatic review settings July 17, 2026 13:05
@feichai0017
feichai0017 requested review from XucSh and YiXR as code owners July 17, 2026 13:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request transitions the file-per-key storage layout to use stable digests generated via XXH3 128-bit hashing, replacing the legacy path resolution. It introduces legacy path fallback mechanisms in IsExist and BatchLoad to maintain backward compatibility with existing cache files, and adds validation to prevent collisions. The review feedback suggests wrapping struct_pb::from_pb in a try-catch block within ScanMeta to prevent corrupted files from crashing the metadata scanning process, and handling std::errc::no_such_file_or_directory gracefully in IsExist when checking file size to handle concurrent deletions robustly.

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.

Comment on lines 1593 to 1594
KVEntry kv;
struct_pb::from_pb(kv, buf);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

struct_pb::from_pb can throw an exception if the file content is corrupted or not a valid protobuf message. In ScanMeta, which scans the entire storage directory, an unhandled exception will cause the entire metadata scanning process to fail or crash the application. Wrapping it in a try-catch block and continuing the loop ensures robustness against individual corrupted files.

                KVEntry kv;
                try {
                    struct_pb::from_pb(kv, buf);
                } catch (const std::exception& e) {
                    LOG(ERROR) << "Failed to decode file: " << p.string()
                               << ", error: " << e.what();
                    continue;
                }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This pre-existing ScanMeta hardening is handled separately in #2987, with malformed-file coverage, so this PR stays focused on path collisions.

Comment on lines +1437 to +1441
std::error_code ec;
const auto size = fs::file_size(legacy_path, ec);
if (ec) {
return tl::make_unexpected(ErrorCode::FILE_READ_FAIL);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If fs::file_size fails because the file was concurrently deleted or does not exist (e.g., std::errc::no_such_file_or_directory), IsExist should return false instead of failing with ErrorCode::FILE_READ_FAIL. This makes the existence check more robust against concurrent evictions.

    std::error_code ec;
    const auto size = fs::file_size(legacy_path, ec);
    if (ec) {
        if (ec == std::errc::no_such_file_or_directory) {
            return false;
        }
        return tl::make_unexpected(ErrorCode::FILE_READ_FAIL);
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled in b63b817: IsExist() now treats a concurrently removed legacy file as absent, with regression coverage.

@codecov-commenter

codecov-commenter commented Jul 17, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 91.32948% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
mooncake-store/src/storage_backend.cpp 71.42% 14 Missing ⚠️
mooncake-store/tests/storage_backend_test.cpp 99.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: File-per-key path mapping can collide and return another object's data

3 participants