Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
feichai0017 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf376e
[Store] Prevent file-per-key path collisions
feichai0017 Jul 17, 2026
b63b817
[Store] Handle missing legacy files during lookup
feichai0017 Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions mooncake-store/include/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,4 +540,10 @@ std::string ResolveMooncakeHostId(const std::string& local_hostname);
std::string ResolvePathFromKey(const std::string& key,
const std::string& root_dir,
const std::string& fsdir);

// Resolves the pre-digest file-per-key layout. Kept for reading cache files
// written before ResolvePathFromKey switched to stable digest filenames.
std::string ResolveLegacyPathFromKey(const std::string& key,
const std::string& root_dir,
const std::string& fsdir);
} // namespace mooncake
72 changes: 68 additions & 4 deletions mooncake-store/src/storage_backend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1421,10 +1421,39 @@ StorageBackendAdaptor::EvictAboveDiskWatermark(

tl::expected<bool, ErrorCode> StorageBackendAdaptor::IsExist(
const std::string& key) {
auto path = ResolvePathFromKey(key, file_storage_config_.storage_filepath,
file_per_key_config_.fsdir);
namespace fs = std::filesystem;
return fs::exists(path);
const auto path = ResolvePathFromKey(
key, file_storage_config_.storage_filepath, file_per_key_config_.fsdir);
if (fs::exists(path)) {
return true;
}

const auto legacy_path = ResolveLegacyPathFromKey(
key, file_storage_config_.storage_filepath, file_per_key_config_.fsdir);
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);
}
Comment on lines +1433 to +1440

@gemini-code-assist gemini-code-assist Bot Jul 17, 2026

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);
    }

@feichai0017 feichai0017 Jul 18, 2026

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.

std::string kv_buf;
auto load_result = storage_backend_->LoadObject(legacy_path, kv_buf,
static_cast<int64_t>(size));
if (!load_result) {
return tl::make_unexpected(load_result.error());
}

try {
KVEntry kv;
struct_pb::from_pb(kv, kv_buf);
return kv.key == key;
} catch (const std::exception& e) {
LOG(ERROR) << "Failed to decode legacy file for key: " << key
<< ", error: " << e.what();
return tl::make_unexpected(ErrorCode::FILE_READ_FAIL);
}
}

tl::expected<void, ErrorCode> StorageBackendAdaptor::BatchLoad(
Expand All @@ -1435,6 +1464,14 @@ tl::expected<void, ErrorCode> StorageBackendAdaptor::BatchLoad(
auto path =
ResolvePathFromKey(kv.key, file_storage_config_.storage_filepath,
file_per_key_config_.fsdir);
if (!std::filesystem::exists(path)) {
const auto legacy_path = ResolveLegacyPathFromKey(
kv.key, file_storage_config_.storage_filepath,
file_per_key_config_.fsdir);
if (std::filesystem::exists(legacy_path)) {
path = legacy_path;
}
}

kv.value.resize(slice.size);

Expand All @@ -1447,7 +1484,25 @@ tl::expected<void, ErrorCode> StorageBackendAdaptor::BatchLoad(
return tl::make_unexpected(r.error());
}

struct_pb::from_pb(kv, kv_buf);
try {
struct_pb::from_pb(kv, kv_buf);
} catch (const std::exception& e) {
LOG(ERROR) << "Failed to decode file for key: " << key
<< ", error: " << e.what();
return tl::make_unexpected(ErrorCode::FILE_READ_FAIL);
}

if (kv.key != key) {
LOG(ERROR) << "Stored key does not match requested key";
return tl::make_unexpected(ErrorCode::INVALID_KEY);
}
if (kv.value.size() != slice.size) {
LOG(ERROR)
<< "Stored value size does not match destination for key: "
<< key << ", expected: " << slice.size
<< ", got: " << kv.value.size();
return tl::make_unexpected(ErrorCode::INVALID_PARAMS);
}

if (!kv.value.empty()) {
std::memcpy(slice.ptr, kv.value.data(), kv.value.size());
Expand Down Expand Up @@ -1536,6 +1591,15 @@ tl::expected<void, ErrorCode> StorageBackendAdaptor::ScanMeta(

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

@gemini-code-assist gemini-code-assist Bot Jul 17, 2026

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;
                }

@feichai0017 feichai0017 Jul 18, 2026

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.


const auto canonical_path = ResolvePathFromKey(
kv.key, file_storage_config_.storage_filepath,
file_per_key_config_.fsdir);
if (p.lexically_normal() !=
fs::path(canonical_path).lexically_normal() &&
fs::exists(canonical_path)) {
continue;
}
storage_backend_->UpdateFileRecordKey(p.string(), kv.key);

total_keys++;
Expand Down
27 changes: 24 additions & 3 deletions mooncake-store/src/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@
#include <sys/socket.h>
#include <unistd.h>
#include <boost/algorithm/string.hpp>
#include <xxhash.h>

#include <algorithm>
#include <array>
#include <atomic>
#include <cerrno>
#include <csignal>
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <memory>
Expand Down Expand Up @@ -766,9 +769,9 @@ static std::string SanitizeKey(const std::string &key) {
return sanitized_key;
}

std::string ResolvePathFromKey(const std::string &key,
const std::string &root_dir,
const std::string &fsdir) {
std::string ResolveLegacyPathFromKey(const std::string &key,
const std::string &root_dir,
const std::string &fsdir) {
// Compute hash of the key
size_t hash = std::hash<std::string>{}(key);

Expand All @@ -788,4 +791,22 @@ std::string ResolvePathFromKey(const std::string &key,

return full_path.lexically_normal().string();
}

std::string ResolvePathFromKey(const std::string &key,
const std::string &root_dir,
const std::string &fsdir) {
const XXH128_hash_t hash = XXH3_128bits(key.data(), key.size());
std::array<char, 33> digest{};
std::snprintf(digest.data(), digest.size(), "%016llx%016llx",
static_cast<unsigned long long>(hash.high64),
static_cast<unsigned long long>(hash.low64));

const std::string digest_string(digest.data());
namespace fs = std::filesystem;
const fs::path dir_path =
fs::path(digest_string.substr(0, 2)) / digest_string.substr(2, 2);
return (fs::path(root_dir) / fsdir / dir_path / digest_string)
.lexically_normal()
.string();
}
} // namespace mooncake
134 changes: 134 additions & 0 deletions mooncake-store/tests/storage_backend_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <gtest/gtest.h>

#include <filesystem>
#include <array>
#include <iostream>
#include <limits>
#include <ranges>
Expand All @@ -23,6 +24,28 @@
namespace fs = std::filesystem;
namespace mooncake::test {

static std::pair<std::string, std::string> FindLegacyPathCollision(
const std::string& root_dir, const std::string& fsdir) {
constexpr std::array<char, 10> kChars = {'_', '/', '\\', ':', '*',
'?', '"', '<', '>', '|'};
std::unordered_map<std::string, std::string> key_by_path;
const std::string tenant_prefix("default\0", 8);
for (size_t value = 0; value < 10000; ++value) {
size_t remaining = value;
std::string key = tenant_prefix;
for (size_t i = 0; i < 4; ++i) {
key.push_back(kChars[remaining % kChars.size()]);
remaining /= kChars.size();
}
const auto path = ResolveLegacyPathFromKey(key, root_dir, fsdir);
auto [it, inserted] = key_by_path.emplace(path, key);
if (!inserted && it->second != key) {
return {it->second, key};
}
}
return {};
}

class StorageBackendTest : public ::testing::Test {
protected:
std::string data_path;
Expand Down Expand Up @@ -708,6 +731,117 @@ TEST_F(StorageBackendTest, AdaptorBatchOffloadAndBatchLoad) {
}
}

TEST_F(StorageBackendTest, AdaptorSeparatesLegacyPathCollisions) {
FileStorageConfig cfg;
cfg.storage_filepath = data_path;
FilePerKeyConfig backend_config;
backend_config.fsdir = "file_per_key_collision";
backend_config.enable_eviction = false;

auto [first_key, second_key] =
FindLegacyPathCollision(cfg.storage_filepath, backend_config.fsdir);
ASSERT_FALSE(first_key.empty());
ASSERT_EQ(ResolveLegacyPathFromKey(first_key, cfg.storage_filepath,
backend_config.fsdir),
ResolveLegacyPathFromKey(second_key, cfg.storage_filepath,
backend_config.fsdir));
ASSERT_NE(ResolvePathFromKey(first_key, cfg.storage_filepath,
backend_config.fsdir),
ResolvePathFromKey(second_key, cfg.storage_filepath,
backend_config.fsdir));

StorageBackendAdaptor adaptor(cfg, backend_config);
ASSERT_TRUE(adaptor.Init());
ASSERT_TRUE(adaptor.ScanMeta(
[](const std::vector<std::string>&,
std::vector<StorageObjectMetadata>&) { return ErrorCode::OK; }));

std::string first_value(64, 'a');
std::string second_value(64, 'b');
std::unordered_map<std::string, std::vector<Slice>> batch = {
{first_key, {{first_value.data(), first_value.size()}}},
{second_key, {{second_value.data(), second_value.size()}}},
};
auto offload_result = adaptor.BatchOffload(
batch,
[](const std::vector<std::string>&,
std::vector<StorageObjectMetadata>&) { return ErrorCode::OK; });
ASSERT_TRUE(offload_result);
EXPECT_EQ(offload_result.value(), 2);

std::array<char, 64> first_output{};
std::array<char, 64> second_output{};
std::unordered_map<std::string, Slice> loads = {
{first_key, {first_output.data(), first_output.size()}},
{second_key, {second_output.data(), second_output.size()}},
};
ASSERT_TRUE(adaptor.BatchLoad(loads));
EXPECT_EQ(std::string(first_output.data(), first_output.size()),
first_value);
EXPECT_EQ(std::string(second_output.data(), second_output.size()),
second_value);
}

TEST_F(StorageBackendTest, AdaptorValidatesLegacyFallbackKey) {
FileStorageConfig cfg;
cfg.storage_filepath = data_path;
FilePerKeyConfig backend_config;
backend_config.fsdir = "file_per_key_legacy_fallback";
backend_config.enable_eviction = false;

auto [stored_key, colliding_key] =
FindLegacyPathCollision(cfg.storage_filepath, backend_config.fsdir);
ASSERT_FALSE(stored_key.empty());

StorageBackendAdaptor adaptor(cfg, backend_config);
ASSERT_TRUE(adaptor.Init());
ASSERT_TRUE(adaptor.ScanMeta(
[](const std::vector<std::string>&,
std::vector<StorageObjectMetadata>&) { return ErrorCode::OK; }));

std::string value(64, 'v');
std::unordered_map<std::string, std::vector<Slice>> batch = {
{stored_key, {{value.data(), value.size()}}},
};
ASSERT_TRUE(adaptor.BatchOffload(
batch,
[](const std::vector<std::string>&,
std::vector<StorageObjectMetadata>&) { return ErrorCode::OK; }));

const auto canonical_path = ResolvePathFromKey(
stored_key, cfg.storage_filepath, backend_config.fsdir);
const auto legacy_path = ResolveLegacyPathFromKey(
stored_key, cfg.storage_filepath, backend_config.fsdir);
fs::create_directories(fs::path(legacy_path).parent_path());
fs::rename(canonical_path, legacy_path);

auto stored_exists = adaptor.IsExist(stored_key);
ASSERT_TRUE(stored_exists);
EXPECT_TRUE(stored_exists.value());
auto colliding_exists = adaptor.IsExist(colliding_key);
ASSERT_TRUE(colliding_exists);
EXPECT_FALSE(colliding_exists.value());

std::array<char, 64> output{};
std::unordered_map<std::string, Slice> valid_load = {
{stored_key, {output.data(), output.size()}},
};
ASSERT_TRUE(adaptor.BatchLoad(valid_load));
EXPECT_EQ(std::string(output.data(), output.size()), value);

std::unordered_map<std::string, Slice> wrong_load = {
{colliding_key, {output.data(), output.size()}},
};
auto wrong_result = adaptor.BatchLoad(wrong_load);
ASSERT_FALSE(wrong_result);
EXPECT_EQ(wrong_result.error(), ErrorCode::INVALID_KEY);

ASSERT_TRUE(fs::remove(legacy_path));
auto removed_exists = adaptor.IsExist(stored_key);
ASSERT_TRUE(removed_exists);
EXPECT_FALSE(removed_exists.value());
}

TEST_F(StorageBackendTest, AdaptorBatchOffload_PartialSuccess) {
FileStorageConfig cfg;
cfg.storage_filepath = data_path;
Expand Down
15 changes: 15 additions & 0 deletions mooncake-store/tests/utils_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,23 @@
#include <unistd.h>
#include <errno.h>

#include <filesystem>

using namespace mooncake;

TEST(UtilsTest, ResolvePathFromKeyUsesBoundedStableDigest) {
const std::string root = "/tmp/mooncake";
const std::string fsdir = "file_per_key";
const std::string key(4096, '/');

const auto first = ResolvePathFromKey(key, root, fsdir);
const auto second = ResolvePathFromKey(key, root, fsdir);
EXPECT_EQ(first, second);
EXPECT_EQ(std::filesystem::path(first).filename().string().size(), 32);
EXPECT_NE(first, ResolvePathFromKey(key + "x", root, fsdir));
EXPECT_GT(ResolveLegacyPathFromKey(key, root, fsdir).size(), first.size());
}

TEST(UtilsTest, ByteSizeToString) {
EXPECT_EQ(byte_size_to_string(999), "999 B");
EXPECT_EQ(byte_size_to_string(2048), "2.00 KB");
Expand Down
Loading