From bb9bf738a0c6f5700937fbee99c672245ceb422a Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Fri, 6 Nov 2026 16:28:54 +0100 Subject: [PATCH] Serialize IDisk::checkAccess: disks created in parallel share the probe file Every disk probes its access with the same file name, clickhouse_access_check_. Since custom disks are created outside storage_policies_mutex (https://github.com/Altinity/ClickHouse/pull/2573), two tables whose custom disks sit on one backing disk are started in parallel at server start, and their probes overwrite and unlink each other's file: the tables fail to attach with FILE_DOESNT_EXIST "While checking access for disk ...". Seen in test_filesystem_cache on https://github.com/Altinity/ClickHouse/pull/2805 (tables test and test_size_in_name, cache disks over hdd_blob). A process-wide mutex around the probe; config disks are created one after another anyway. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Mikhail Filimonov (cherry picked from commit 086efc9536359a6cfe4baeb40355abd420ec35f1) --- src/Disks/IDisk.cpp | 4 ++ src/Disks/tests/gtest_disk_access_check.cpp | 64 +++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 src/Disks/tests/gtest_disk_access_check.cpp diff --git a/src/Disks/IDisk.cpp b/src/Disks/IDisk.cpp index 7c6a05853f22e..44390495fa9db 100644 --- a/src/Disks/IDisk.cpp +++ b/src/Disks/IDisk.cpp @@ -247,6 +247,10 @@ void IDisk::checkAccess() throw Exception(ErrorCodes::LOGICAL_ERROR, "Server UUID is not initialized"); const String path = fmt::format("clickhouse_access_check_{}", toString(server_uuid)); + /// Disks created in parallel (`Context::getOrCreateDisk`) can share a backing store, and every + /// probe writes, reads and removes this one name: overlapping probes see each other's file. + static std::mutex probe_mutex; + std::lock_guard lock(probe_mutex); checkAccessImpl(path); } diff --git a/src/Disks/tests/gtest_disk_access_check.cpp b/src/Disks/tests/gtest_disk_access_check.cpp new file mode 100644 index 0000000000000..d97c7a72d59ab --- /dev/null +++ b/src/Disks/tests/gtest_disk_access_check.cpp @@ -0,0 +1,64 @@ +#include + +#include +#include + +#include +#include +#include +#include +#include + +namespace +{ + +/// Counts the access probes running at the same time. A probe waits for a second one to arrive, so +/// two unserialized probes meet inside `checkAccessImpl` and a serialized pair runs one after the other. +struct ProbeMeter +{ + std::mutex mutex; + std::condition_variable arrived; + size_t active = 0; + size_t max_active = 0; +}; + +class ProbeDisk : public DB::DiskLocal +{ +public: + ProbeDisk(const String & name_, const String & path_, ProbeMeter & meter_) : DiskLocal(name_, path_), meter(meter_) { } + +protected: + void checkAccessImpl(const String &) override + { + std::unique_lock lock(meter.mutex); + ++meter.active; + meter.max_active = std::max(meter.max_active, meter.active); + meter.arrived.notify_all(); + meter.arrived.wait_for(lock, std::chrono::milliseconds(300), [&] { return meter.active == 2; }); + --meter.active; + } + +private: + ProbeMeter & meter; +}; + +} + +/// Every disk writes the probe `clickhouse_access_check_`; disks created in parallel +/// (`Context::getOrCreateDisk`) can share a backing store, so their probes must not overlap. +TEST(DiskAccessCheck, ProbesOfDisksOverOneDirectoryDoNotOverlap) +{ + DB::ServerUUID::setRandomForUnitTests(); + auto backing = createDisk("access_check"); + ProbeMeter meter; + auto a = std::make_shared("a", backing->getPath(), meter); + auto b = std::make_shared("b", backing->getPath(), meter); + + std::thread ta([&] { a->startup(/*skip_access_check=*/false); }); + std::thread tb([&] { b->startup(/*skip_access_check=*/false); }); + ta.join(); + tb.join(); + + EXPECT_EQ(meter.max_active, 1u); + destroyDisk(backing); +}