From 627f69589b17ff5e0aa1e6626c0ad056f68bbcd1 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 30 Jun 2026 05:25:10 -0500 Subject: [PATCH 1/3] test: consolidate TempDir/TempPath into shared test/utils helper Four download/file-writer unit tests each defined a near-identical local TempDir or TempPath RAII class, and test_helpers.h carried the CurrentPid/MakeUniqueTempPath helpers they relied on. Move all of it into a single test/utils/temp_dir.h in the fl::test namespace and have the tests use it, removing the duplication Scott flagged on #793. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../internal_api/blob_download_state_test.cc | 18 +--- .../cross_process_file_lock_test.cc | 20 +--- sdk_v2/cpp/test/internal_api/download_test.cc | 33 +----- .../cpp/test/internal_api/file_writer_test.cc | 16 +-- sdk_v2/cpp/test/internal_api/test_helpers.h | 30 +----- sdk_v2/cpp/test/utils/temp_dir.h | 101 ++++++++++++++++++ 6 files changed, 107 insertions(+), 111 deletions(-) create mode 100644 sdk_v2/cpp/test/utils/temp_dir.h diff --git a/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc b/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc index f70b9980..19c5152f 100644 --- a/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc +++ b/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc @@ -17,23 +17,7 @@ using namespace fl; namespace { -class TempDir { - public: - TempDir() { - path_ = fl::test::MakeUniqueTempPath("fl_dlstate_test_"); - fs::create_directories(path_); - } - - ~TempDir() { - std::error_code ec; - fs::remove_all(path_, ec); - } - - const fs::path& path() const { return path_; } - - private: - fs::path path_; -}; +using fl::test::TempDir; constexpr int64_t kBlobSize = 20 * 1024 * 1024; // 20 MiB constexpr int32_t kChunkSize = 2 * 1024 * 1024; // 2 MiB diff --git a/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc b/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc index f32df42d..f25df86a 100644 --- a/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc +++ b/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc @@ -27,25 +27,7 @@ using namespace fl; namespace { -/// Per-test temp directory. Auto-cleans on destruction so a flaky test never -/// leaks lock files into the system temp dir. -class TempDir { - public: - TempDir() { - path_ = fl::test::MakeUniqueTempPath("fl_lock_test_"); - fs::create_directories(path_); - } - - ~TempDir() { - std::error_code ec; - fs::remove_all(path_, ec); - } - - const fs::path& path() const { return path_; } - - private: - fs::path path_; -}; +using fl::test::TempDir; } // namespace diff --git a/sdk_v2/cpp/test/internal_api/download_test.cc b/sdk_v2/cpp/test/internal_api/download_test.cc index 96ce4f54..d0f44059 100644 --- a/sdk_v2/cpp/test/internal_api/download_test.cc +++ b/sdk_v2/cpp/test/internal_api/download_test.cc @@ -47,38 +47,7 @@ using namespace fl; namespace { -/// Create a temporary directory for test isolation. The name comes from MakeUniqueTempPath, so it is -/// unique both across the separate processes CTest launches per test and across multiple TempDirs -/// within one test. create_directory must succeed — the directory must not already exist — so a -/// residual collision (e.g. a directory leaked by an earlier process that reused this pid) advances -/// to the next name and retries instead of silently sharing an existing directory. -class TempDir { - public: - TempDir() { - while (true) { - auto candidate = fl::test::MakeUniqueTempPath("fl_test_"); - std::error_code ec; - if (fs::create_directory(candidate, ec)) { - path_ = std::move(candidate); - return; - } - if (ec) { - throw std::runtime_error("TempDir: failed to create '" + candidate.string() + "': " + - ec.message()); - } - // candidate already existed — try the next name. - } - } - ~TempDir() { - std::error_code ec; - fs::remove_all(path_, ec); - } - const fs::path& path() const { return path_; } - std::string string() const { return path_.string(); } - - private: - fs::path path_; -}; +using fl::test::TempDir; /// Read entire file contents. std::string ReadFile(const fs::path& path) { diff --git a/sdk_v2/cpp/test/internal_api/file_writer_test.cc b/sdk_v2/cpp/test/internal_api/file_writer_test.cc index 4eac3783..bc520b08 100644 --- a/sdk_v2/cpp/test/internal_api/file_writer_test.cc +++ b/sdk_v2/cpp/test/internal_api/file_writer_test.cc @@ -24,21 +24,7 @@ using namespace fl; namespace { -class TempPath { - public: - TempPath() { - path_ = fl::test::MakeUniqueTempPath("file_writer_test_"); - path_ += ".bin"; - } - ~TempPath() { - std::error_code ec; - fs::remove(path_, ec); - } - const fs::path& path() const { return path_; } - - private: - fs::path path_; -}; +using fl::test::TempPath; } // namespace diff --git a/sdk_v2/cpp/test/internal_api/test_helpers.h b/sdk_v2/cpp/test/internal_api/test_helpers.h index de4c41e2..c6bc06fd 100644 --- a/sdk_v2/cpp/test/internal_api/test_helpers.h +++ b/sdk_v2/cpp/test/internal_api/test_helpers.h @@ -8,42 +8,16 @@ #include "inferencing/model_load_manager.h" #include "logger.h" -#include +#include "utils/temp_dir.h" + #include #include #include #include #include -#ifdef _WIN32 -#include // _getpid -#else -#include // getpid -#endif - namespace fl::test { -/// Current process id. CTest (gtest_discover_tests) launches a separate process per test, so -/// temp paths must include the pid to stay unique across concurrent test processes. process.h -/// is used instead of windows.h so callers that use std::min/std::max aren't broken by its macros. -inline long CurrentPid() { -#ifdef _WIN32 - return ::_getpid(); -#else - return static_cast(::getpid()); -#endif -} - -/// Build a unique path under the system temp directory as `_`. The pid -/// separates concurrent test processes and the per-process atomic counter separates callers -/// within one process, so no two live temp paths collide — no randomness required. -inline std::filesystem::path MakeUniqueTempPath(const std::string& prefix) { - static std::atomic counter{0}; - return std::filesystem::temp_directory_path() / - (prefix + std::to_string(CurrentPid()) + "_" + - std::to_string(counter.fetch_add(1, std::memory_order_relaxed))); -} - /// EP detector that only reports CPU — used by tests that load real models /// without requiring GPU hardware. class CpuOnlyEpDetector : public IEpDetector { diff --git a/sdk_v2/cpp/test/utils/temp_dir.h b/sdk_v2/cpp/test/utils/temp_dir.h new file mode 100644 index 00000000..ed55feb8 --- /dev/null +++ b/sdk_v2/cpp/test/utils/temp_dir.h @@ -0,0 +1,101 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +// Shared temp directory / path helpers for tests. CTest (gtest_discover_tests) +// launches a separate process per test, so temp names embed the pid plus a +// per-process counter and never collide across concurrent test processes. +#pragma once + +#include +#include +#include +#include +#include +#include + +#ifdef _WIN32 +#include // _getpid +#else +#include // getpid +#endif + +namespace fl::test { + +/// Current process id. process.h is used instead of windows.h so callers that use +/// std::min/std::max aren't broken by its macros. +inline long CurrentPid() { +#ifdef _WIN32 + return ::_getpid(); +#else + return static_cast(::getpid()); +#endif +} + +/// Build a unique path under the system temp directory as `_`. The pid +/// separates concurrent test processes and the per-process atomic counter separates callers +/// within one process, so no two live temp paths collide — no randomness required. +inline std::filesystem::path MakeUniqueTempPath(const std::string& prefix) { + static std::atomic counter{0}; + return std::filesystem::temp_directory_path() / + (prefix + std::to_string(CurrentPid()) + "_" + + std::to_string(counter.fetch_add(1, std::memory_order_relaxed))); +} + +/// RAII temporary directory for test isolation. The name comes from MakeUniqueTempPath, so it is +/// unique both across the separate processes CTest launches per test and across multiple TempDirs +/// within one test. create_directory must succeed — the directory must not already exist — so a +/// residual collision (e.g. a directory leaked by an earlier process that reused this pid) advances +/// to the next name and retries instead of silently sharing an existing directory. +class TempDir { + public: + explicit TempDir(const std::string& prefix = "fl_test_") { + while (true) { + auto candidate = MakeUniqueTempPath(prefix); + std::error_code ec; + if (std::filesystem::create_directory(candidate, ec)) { + path_ = std::move(candidate); + return; + } + if (ec) { + throw std::runtime_error("TempDir: failed to create '" + candidate.string() + "': " + + ec.message()); + } + // candidate already existed — try the next name. + } + } + + ~TempDir() { + std::error_code ec; + std::filesystem::remove_all(path_, ec); + } + + TempDir(const TempDir&) = delete; + TempDir& operator=(const TempDir&) = delete; + + const std::filesystem::path& path() const { return path_; } + std::string string() const { return path_.string(); } + + private: + std::filesystem::path path_; +}; + +/// RAII unique temporary file path. The path is not created here — callers create the file — and +/// it is removed on destruction so a flaky test never leaks files into the system temp dir. +class TempPath { + public: + explicit TempPath(const std::string& prefix = "fl_test_") : path_(MakeUniqueTempPath(prefix)) {} + + ~TempPath() { + std::error_code ec; + std::filesystem::remove(path_, ec); + } + + TempPath(const TempPath&) = delete; + TempPath& operator=(const TempPath&) = delete; + + const std::filesystem::path& path() const { return path_; } + + private: + std::filesystem::path path_; +}; + +} // namespace fl::test From 022968e09392902212dd4181764e5d1ff000ce83 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 30 Jun 2026 05:26:17 -0500 Subject: [PATCH 2/3] refactor: move file I/O and lock platform code into src/platform file_writer.cc and cross_process_file_lock.cc each carried Win32/POSIX implementations behind in-file #ifdef _WIN32 branches. Follow the existing src/platform/{windows,posix}/path.cc pattern instead: - Add src/platform/file_io.h declaring fl::platform OpenWritableFile / WriteFileAt / CloseFile primitives over an integer handle, implemented per OS in src/platform/{windows,posix}/file_io.cc. FileWriter now stores an intptr_t handle and delegates, dropping its #ifdef member and branches. - Split CrossProcessFileLock: the platform-specific State, its releasing destructor, FormatProcessInfo, the constructor/destructor, and TryAcquireForDirectory move verbatim into src/platform/{windows,posix}/cross_process_file_lock.cc; the neutral src/download/cross_process_file_lock.cc keeps only the cross-platform WaitForDirectoryLock orchestration. - Wire the four new sources into FOUNDRY_LOCAL_PLATFORM_SOURCES. No behavior change; addresses Scott's #793 nit about keeping platform code in src/platform. Verified: Windows build green, 83 download/lock/file-writer/state tests pass, POSIX sources syntax-clean under g++ -Wall -Wextra. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- sdk_v2/cpp/CMakeLists.txt | 4 + .../src/download/cross_process_file_lock.cc | 189 +----------------- sdk_v2/cpp/src/download/file_writer.cc | 116 ++--------- sdk_v2/cpp/src/download/file_writer.h | 12 +- sdk_v2/cpp/src/platform/file_io.h | 34 ++++ .../platform/posix/cross_process_file_lock.cc | 134 +++++++++++++ sdk_v2/cpp/src/platform/posix/file_io.cc | 58 ++++++ .../windows/cross_process_file_lock.cc | 116 +++++++++++ sdk_v2/cpp/src/platform/windows/file_io.cc | 66 ++++++ 9 files changed, 435 insertions(+), 294 deletions(-) create mode 100644 sdk_v2/cpp/src/platform/file_io.h create mode 100644 sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc create mode 100644 sdk_v2/cpp/src/platform/posix/file_io.cc create mode 100644 sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc create mode 100644 sdk_v2/cpp/src/platform/windows/file_io.cc diff --git a/sdk_v2/cpp/CMakeLists.txt b/sdk_v2/cpp/CMakeLists.txt index f4a3c2dd..ff606ed4 100644 --- a/sdk_v2/cpp/CMakeLists.txt +++ b/sdk_v2/cpp/CMakeLists.txt @@ -102,11 +102,15 @@ if(WIN32) list(APPEND FOUNDRY_LOCAL_PLATFORM_SOURCES src/util/stacktrace_windows.cc src/platform/windows/path.cc + src/platform/windows/file_io.cc + src/platform/windows/cross_process_file_lock.cc ) else() list(APPEND FOUNDRY_LOCAL_PLATFORM_SOURCES src/util/stacktrace_posix.cc src/platform/posix/path.cc + src/platform/posix/file_io.cc + src/platform/posix/cross_process_file_lock.cc ) endif() diff --git a/sdk_v2/cpp/src/download/cross_process_file_lock.cc b/sdk_v2/cpp/src/download/cross_process_file_lock.cc index 5aa82c76..9ea5d3b8 100644 --- a/sdk_v2/cpp/src/download/cross_process_file_lock.cc +++ b/sdk_v2/cpp/src/download/cross_process_file_lock.cc @@ -1,199 +1,20 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +// Cross-platform orchestration for CrossProcessFileLock. The platform-specific +// pieces — the lock handle (State) and its releasing destructor, +// FormatProcessInfo, the CrossProcessFileLock destructor/constructor, and +// TryAcquireForDirectory — live in +// src/platform/{windows,posix}/cross_process_file_lock.cc. #include "download/cross_process_file_lock.h" #include "exception.h" -#include "logger.h" #include #include -#include -#include -#include -#include #include -#ifdef _WIN32 -#define WIN32_LEAN_AND_MEAN -#define NOMINMAX -#include -#include -#else -#include -#include -#include -#include -#include -#endif - namespace fl { -namespace { - -constexpr const char* kLockFileName = ".download.lock"; - -/// `PID:,Time:\n` -std::string FormatProcessInfo() { -#ifdef _WIN32 - auto pid = static_cast(_getpid()); -#else - auto pid = static_cast(getpid()); -#endif - auto t = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()); - std::tm tm{}; -#ifdef _WIN32 - gmtime_s(&tm, &t); -#else - gmtime_r(&t, &tm); -#endif - std::ostringstream oss; - oss << "PID:" << pid << ",Time:" << std::put_time(&tm, "%Y-%m-%dT%H:%M:%SZ") << '\n'; - return oss.str(); -} - -} // namespace - -// Platform-specific resource handle. The destructor here is the only thing -// that releases the lock; CrossProcessFileLock's destructor is defaulted. -#ifdef _WIN32 -struct CrossProcessFileLock::State { - HANDLE handle; - ~State() { - if (handle != INVALID_HANDLE_VALUE) { - // FILE_FLAG_DELETE_ON_CLOSE removes the file when the last handle closes. - CloseHandle(handle); - } - } -}; -#else -struct CrossProcessFileLock::State { - int fd; - std::filesystem::path path; - ~State() { - if (fd >= 0) { - // Unlink before close so the file disappears the instant the lock - // releases; a concurrent acquirer simply recreates it. This is the - // classic flock()+unlink() pattern, and it is safe here because every - // acquirer verifies, while holding the flock, that the inode it locked is - // still the one at `path` (see the fstat/stat check in - // TryAcquireForDirectory). An acquirer that raced in on the old inode - // between our unlink and a third party's recreate will see the inode - // mismatch and retry, so two processes never hold "the lock" at once. - // There is also no protected work between this unlink and close. - ::unlink(path.c_str()); - ::close(fd); - } - } -}; -#endif - -CrossProcessFileLock::CrossProcessFileLock(std::filesystem::path path, - std::unique_ptr state, - ILogger& logger) - : path_(std::move(path)), state_(std::move(state)), logger_(logger) {} - -CrossProcessFileLock::~CrossProcessFileLock() { - // Release the OS handle first so the "released" log message is accurate. - state_.reset(); - logger_.Log(LogLevel::Debug, "CrossProcessFileLock released: " + path_.string()); -} - -std::unique_ptr CrossProcessFileLock::TryAcquireForDirectory( - const std::filesystem::path& directory, ILogger& logger) { - std::error_code ec; - std::filesystem::create_directories(directory, ec); - // Best-effort: if create_directories failed, the platform open below will - // surface a clearer error message. - - auto lock_path = directory / kLockFileName; - std::unique_ptr state; - -#ifdef _WIN32 - // dwShareMode=0 blocks any other open (cross- and in-process) until this - // handle closes. FILE_FLAG_DELETE_ON_CLOSE pairs OPEN_ALWAYS into a - // self-cleaning lock that doesn't require unlink-then-close races. - auto wide = lock_path.wstring(); - HANDLE handle = CreateFileW(wide.c_str(), - GENERIC_READ | GENERIC_WRITE, - 0, - nullptr, - OPEN_ALWAYS, - FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, - nullptr); - if (handle == INVALID_HANDLE_VALUE) { - DWORD err = GetLastError(); - if (err == ERROR_SHARING_VIOLATION || err == ERROR_LOCK_VIOLATION || err == ERROR_ACCESS_DENIED) { - // SHARING/LOCK_VIOLATION: another handle already holds the share-none - // lock. ACCESS_DENIED: the holder is mid-release — FILE_FLAG_DELETE_ON_CLOSE - // puts the file into STATUS_DELETE_PENDING during the close window, and a - // concurrent open of a delete-pending file is reported as ACCESS_DENIED. - // All three mean "another process has it"; treat as contention so the - // caller retries. (A genuine permission error also lands here and would - // poll until timeout, but the directory was just created successfully so - // that is improbable.) - return nullptr; - } - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "CreateFileW failed for lock '" + lock_path.string() + - "' (GetLastError=" + std::to_string(err) + ")"); - } - - auto info = FormatProcessInfo(); - DWORD written = 0; - WriteFile(handle, info.data(), static_cast(info.size()), &written, nullptr); - FlushFileBuffers(handle); - - state = std::unique_ptr(new State{handle}); -#else - int fd = ::open(lock_path.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644); - if (fd < 0) { - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "open failed for lock '" + lock_path.string() + "' (errno=" + std::to_string(errno) + ")"); - } - if (::flock(fd, LOCK_EX | LOCK_NB) != 0) { - int err = errno; - ::close(fd); - if (err == EWOULDBLOCK || err == EAGAIN) { - return nullptr; - } - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "flock failed for '" + lock_path.string() + "' (errno=" + std::to_string(err) + ")"); - } - - // Robust-flock inode check. We now hold an exclusive flock on whatever inode - // `fd` refers to, but a releaser unlink()s the lock file in its destructor — - // so between our open() and flock() the path may have been unlinked and a - // third process may have recreated it. If so, we are holding a lock on an - // orphaned inode that guards nothing while the live file at `lock_path` is a - // different inode. Confirm the inode we locked is still the one at the path; - // if not, drop it and report contention so the caller retries against the - // live file. This closes the flock()+unlink() orphan-inode race, which is - // what lets two processes never both believe they hold the lock. - struct stat fd_stat {}; - struct stat path_stat {}; - if (::fstat(fd, &fd_stat) != 0 || ::stat(lock_path.c_str(), &path_stat) != 0 || - fd_stat.st_dev != path_stat.st_dev || fd_stat.st_ino != path_stat.st_ino) { - ::close(fd); // releases the flock on the stale / orphaned inode - return nullptr; - } - - auto info = FormatProcessInfo(); - // Best-effort: record this process's identity in the lock file for diagnostics. - // A failure here doesn't affect lock correctness, so it is only logged at Debug. - if (::ftruncate(fd, 0) != 0 || ::write(fd, info.data(), info.size()) < 0) { - logger.Log(LogLevel::Debug, - "CrossProcessFileLock: failed to write diagnostic process info to lock file '" + - lock_path.string() + "' (errno=" + std::to_string(errno) + ")"); - } - - state = std::unique_ptr(new State{fd, lock_path}); -#endif - - logger.Log(LogLevel::Debug, "CrossProcessFileLock acquired: " + lock_path.string()); - return std::unique_ptr( - new CrossProcessFileLock(std::move(lock_path), std::move(state), logger)); -} - std::unique_ptr CrossProcessFileLock::WaitForDirectoryLock( const std::filesystem::path& directory, const CancellationPredicate& is_cancelled, diff --git a/sdk_v2/cpp/src/download/file_writer.cc b/sdk_v2/cpp/src/download/file_writer.cc index 6ffb6dd5..cdde85d2 100644 --- a/sdk_v2/cpp/src/download/file_writer.cc +++ b/sdk_v2/cpp/src/download/file_writer.cc @@ -3,6 +3,7 @@ #include "download/file_writer.h" #include "exception.h" #include "logger.h" +#include "platform/file_io.h" #include @@ -10,18 +11,6 @@ #include #include -#ifdef _WIN32 -#ifndef NOMINMAX -#define NOMINMAX -#endif -#include -#else -#include -#include -#include -#include -#endif - namespace fl { namespace fs = std::filesystem; @@ -69,112 +58,33 @@ void EnsureFileExistsAtSize(const fs::path& path, int64_t expected_size) { FileWriter::FileWriter(ILogger& logger) : logger_(logger) {} -#ifdef _WIN32 - FileWriter::~FileWriter() { Close(); } void FileWriter::Open(const fs::path& path, int64_t expected_size) { EnsureFileExistsAtSize(path, expected_size); - // FILE_SHARE_READ | FILE_SHARE_WRITE so the lock file / other tools can peek - // at the partial file without us erroring; positional WriteFile is safe - // regardless of share mode. - HANDLE h = ::CreateFileW(path.wstring().c_str(), GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, nullptr); - if (h == INVALID_HANDLE_VALUE) { + std::string error; + handle_ = platform::OpenWritableFile(path, error); + if (handle_ == platform::kInvalidFileHandle) { FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "FileWriter open failed for " + path.string() + " (Win32 err " + - std::to_string(::GetLastError()) + ")"); + "FileWriter open failed for " + path.string() + " (" + error + ")"); } - handle_ = h; } void FileWriter::WriteAt(int64_t offset, const uint8_t* data, size_t len) { - // Concurrent WriteFile calls with distinct OVERLAPPED offsets on the same - // handle are safe for non-overlapping ranges; the kernel orders them. - while (len > 0) { - OVERLAPPED ov{}; - // Split the 64-bit file offset across the OVERLAPPED halves: the DWORD casts - // keep the low 32 bits in Offset and the high 32 bits in OffsetHigh. - ov.Offset = static_cast(static_cast(offset)); - ov.OffsetHigh = static_cast(static_cast(offset) >> 32); - DWORD to_write = static_cast(len > 0x7FFFFFFFu ? 0x7FFFFFFFu : len); - DWORD written = 0; - if (!::WriteFile(handle_, data, to_write, &written, &ov)) { - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "FileWriter write failed at offset " + std::to_string(offset) + " (Win32 err " + - std::to_string(::GetLastError()) + ")"); - } - if (written == 0) { - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "FileWriter short write at offset " + std::to_string(offset)); - } - offset += static_cast(written); - data += written; - len -= written; + std::string error; + if (!platform::WriteFileAt(handle_, offset, data, len, error)) { + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, "FileWriter " + error); } } void FileWriter::Close() { - if (handle_ != nullptr) { - if (!::CloseHandle(handle_)) { - const DWORD err = ::GetLastError(); - logger_.Log(LogLevel::Warning, - "FileWriter: CloseHandle failed (Win32 err " + std::to_string(err) + ")"); + if (handle_ != platform::kInvalidFileHandle) { + std::string error; + if (!platform::CloseFile(handle_, error)) { + logger_.Log(LogLevel::Warning, "FileWriter: " + error); } - handle_ = nullptr; - } -} - -#else // POSIX - -FileWriter::~FileWriter() { Close(); } - -void FileWriter::Open(const fs::path& path, int64_t expected_size) { - EnsureFileExistsAtSize(path, expected_size); - fd_ = ::open(path.c_str(), O_RDWR | O_CLOEXEC); - if (fd_ < 0) { - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "FileWriter open failed for " + path.string() + " (errno " + - std::to_string(errno) + ")"); + handle_ = platform::kInvalidFileHandle; } } -void FileWriter::WriteAt(int64_t offset, const uint8_t* data, size_t len) { - while (len > 0) { - ssize_t n = ::pwrite(fd_, data, len, static_cast(offset)); - if (n < 0) { - if (errno == EINTR) continue; - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "FileWriter pwrite failed at offset " + std::to_string(offset) + " (errno " + - std::to_string(errno) + ")"); - } - if (n == 0) { - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "FileWriter short pwrite at offset " + std::to_string(offset)); - } - offset += n; - data += n; - len -= static_cast(n); - } -} - -void FileWriter::Close() { - if (fd_ >= 0) { - // A failing close() can surface a deferred write error (e.g. EIO, or ENOSPC - // on delayed allocation / a networked filesystem), so the file may be - // incomplete even though every pwrite returned success. Log it for - // diagnosis. Don't retry: on Linux the descriptor is freed even when close() - // returns EINTR, so a retry could close an unrelated, since-reused fd. - if (::close(fd_) != 0) { - const int err = errno; - logger_.Log(LogLevel::Warning, - "FileWriter: close failed (errno " + std::to_string(err) + ")"); - } - fd_ = -1; - } -} - -#endif - } // namespace fl diff --git a/sdk_v2/cpp/src/download/file_writer.h b/sdk_v2/cpp/src/download/file_writer.h index 29540e2a..2b63feb7 100644 --- a/sdk_v2/cpp/src/download/file_writer.h +++ b/sdk_v2/cpp/src/download/file_writer.h @@ -15,7 +15,8 @@ class ILogger; /// Workers in a single download claim disjoint chunks, so concurrent `WriteAt` /// calls always target non-overlapping byte ranges. Backed by `pwrite` (POSIX) /// or `WriteFile` + `OVERLAPPED` (Windows): the OS arbitrates concurrent writes -/// to disjoint ranges, so no user-space lock is taken. +/// to disjoint ranges, so no user-space lock is taken. The OS-specific calls +/// live in `src/platform/file_io.*`. class FileWriter { public: explicit FileWriter(ILogger& logger); @@ -38,12 +39,9 @@ class FileWriter { private: ILogger& logger_; -#ifdef _WIN32 - // Win32 HANDLE. Holds a valid handle while open, nullptr otherwise. - void* handle_ = nullptr; -#else - int fd_ = -1; -#endif + // Native file handle (Win32 HANDLE or POSIX fd) as an integer; see + // src/platform/file_io.h. kInvalidFileHandle (-1) when not open. + std::intptr_t handle_ = -1; }; } // namespace fl diff --git a/sdk_v2/cpp/src/platform/file_io.h b/sdk_v2/cpp/src/platform/file_io.h new file mode 100644 index 00000000..49e7d89e --- /dev/null +++ b/sdk_v2/cpp/src/platform/file_io.h @@ -0,0 +1,34 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +#pragma once + +#include +#include +#include +#include + +namespace fl::platform { + +/// Native file handle (a Win32 HANDLE or a POSIX file descriptor) carried as an +/// integer so callers don't depend on platform headers. `kInvalidFileHandle` +/// matches both INVALID_HANDLE_VALUE ((HANDLE)-1) and a POSIX fd of -1. +inline constexpr std::intptr_t kInvalidFileHandle = -1; + +/// Open an existing file for positional read/write. Returns a handle that is not +/// `kInvalidFileHandle` on success; on failure returns `kInvalidFileHandle` and +/// sets `error` to a human-readable OS error (e.g. "Win32 err 5" / "errno 13"). +std::intptr_t OpenWritableFile(const std::filesystem::path& path, std::string& error); + +/// Write all `len` bytes from `data` at byte offset `offset`. Safe for concurrent +/// calls on the same handle targeting disjoint ranges. Returns true on success; +/// on failure returns false and sets `error` (e.g. "pwrite failed at offset N +/// (errno M)"). +bool WriteFileAt(std::intptr_t handle, std::int64_t offset, const std::uint8_t* data, std::size_t len, + std::string& error); + +/// Close `handle`. Returns true on success; on failure returns false and sets +/// `error`. A failing close can surface a deferred write error, so the data may +/// be incomplete even when every write succeeded. +bool CloseFile(std::intptr_t handle, std::string& error); + +} // namespace fl::platform diff --git a/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc b/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc new file mode 100644 index 00000000..443bbc9e --- /dev/null +++ b/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc @@ -0,0 +1,134 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +// POSIX implementation of CrossProcessFileLock acquisition and the +// platform-specific lock handle. Cross-platform orchestration +// (WaitForDirectoryLock) lives in src/download/cross_process_file_lock.cc. +#include "download/cross_process_file_lock.h" +#include "exception.h" +#include "logger.h" + +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace fl { + +namespace { + +constexpr const char* kLockFileName = ".download.lock"; + +/// `PID:,Time:\n` +std::string FormatProcessInfo() { + auto pid = static_cast(getpid()); + auto t = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()); + std::tm tm{}; + gmtime_r(&t, &tm); + std::ostringstream oss; + oss << "PID:" << pid << ",Time:" << std::put_time(&tm, "%Y-%m-%dT%H:%M:%SZ") << '\n'; + return oss.str(); +} + +} // namespace + +// Platform-specific resource handle. The destructor here is the only thing +// that releases the lock; CrossProcessFileLock's destructor is defaulted. +struct CrossProcessFileLock::State { + int fd; + std::filesystem::path path; + ~State() { + if (fd >= 0) { + // Unlink before close so the file disappears the instant the lock + // releases; a concurrent acquirer simply recreates it. This is the + // classic flock()+unlink() pattern, and it is safe here because every + // acquirer verifies, while holding the flock, that the inode it locked is + // still the one at `path` (see the fstat/stat check in + // TryAcquireForDirectory). An acquirer that raced in on the old inode + // between our unlink and a third party's recreate will see the inode + // mismatch and retry, so two processes never hold "the lock" at once. + // There is also no protected work between this unlink and close. + ::unlink(path.c_str()); + ::close(fd); + } + } +}; + +CrossProcessFileLock::CrossProcessFileLock(std::filesystem::path path, + std::unique_ptr state, + ILogger& logger) + : path_(std::move(path)), state_(std::move(state)), logger_(logger) {} + +CrossProcessFileLock::~CrossProcessFileLock() { + // Release the OS handle first so the "released" log message is accurate. + state_.reset(); + logger_.Log(LogLevel::Debug, "CrossProcessFileLock released: " + path_.string()); +} + +std::unique_ptr CrossProcessFileLock::TryAcquireForDirectory( + const std::filesystem::path& directory, ILogger& logger) { + std::error_code ec; + std::filesystem::create_directories(directory, ec); + // Best-effort: if create_directories failed, the platform open below will + // surface a clearer error message. + + auto lock_path = directory / kLockFileName; + std::unique_ptr state; + + int fd = ::open(lock_path.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644); + if (fd < 0) { + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, + "open failed for lock '" + lock_path.string() + "' (errno=" + std::to_string(errno) + ")"); + } + if (::flock(fd, LOCK_EX | LOCK_NB) != 0) { + int err = errno; + ::close(fd); + if (err == EWOULDBLOCK || err == EAGAIN) { + return nullptr; + } + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, + "flock failed for '" + lock_path.string() + "' (errno=" + std::to_string(err) + ")"); + } + + // Robust-flock inode check. We now hold an exclusive flock on whatever inode + // `fd` refers to, but a releaser unlink()s the lock file in its destructor — + // so between our open() and flock() the path may have been unlinked and a + // third process may have recreated it. If so, we are holding a lock on an + // orphaned inode that guards nothing while the live file at `lock_path` is a + // different inode. Confirm the inode we locked is still the one at the path; + // if not, drop it and report contention so the caller retries against the + // live file. This closes the flock()+unlink() orphan-inode race, which is + // what lets two processes never both believe they hold the lock. + struct stat fd_stat {}; + struct stat path_stat {}; + if (::fstat(fd, &fd_stat) != 0 || ::stat(lock_path.c_str(), &path_stat) != 0 || + fd_stat.st_dev != path_stat.st_dev || fd_stat.st_ino != path_stat.st_ino) { + ::close(fd); // releases the flock on the stale / orphaned inode + return nullptr; + } + + auto info = FormatProcessInfo(); + // Best-effort: record this process's identity in the lock file for diagnostics. + // A failure here doesn't affect lock correctness, so it is only logged at Debug. + if (::ftruncate(fd, 0) != 0 || ::write(fd, info.data(), info.size()) < 0) { + logger.Log(LogLevel::Debug, + "CrossProcessFileLock: failed to write diagnostic process info to lock file '" + + lock_path.string() + "' (errno=" + std::to_string(errno) + ")"); + } + + state = std::unique_ptr(new State{fd, lock_path}); + + logger.Log(LogLevel::Debug, "CrossProcessFileLock acquired: " + lock_path.string()); + return std::unique_ptr( + new CrossProcessFileLock(std::move(lock_path), std::move(state), logger)); +} + +} // namespace fl diff --git a/sdk_v2/cpp/src/platform/posix/file_io.cc b/sdk_v2/cpp/src/platform/posix/file_io.cc new file mode 100644 index 00000000..e6c8c281 --- /dev/null +++ b/sdk_v2/cpp/src/platform/posix/file_io.cc @@ -0,0 +1,58 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +#include "platform/file_io.h" + +#include +#include +#include +#include +#include + +namespace fl::platform { + +std::intptr_t OpenWritableFile(const std::filesystem::path& path, std::string& error) { + int fd = ::open(path.c_str(), O_RDWR | O_CLOEXEC); + if (fd < 0) { + error = "errno " + std::to_string(errno); + return kInvalidFileHandle; + } + return static_cast(fd); +} + +bool WriteFileAt(std::intptr_t handle, std::int64_t offset, const std::uint8_t* data, std::size_t len, + std::string& error) { + int fd = static_cast(handle); + while (len > 0) { + ssize_t n = ::pwrite(fd, data, len, static_cast(offset)); + if (n < 0) { + if (errno == EINTR) continue; + error = "pwrite failed at offset " + std::to_string(offset) + " (errno " + + std::to_string(errno) + ")"; + return false; + } + if (n == 0) { + error = "short pwrite at offset " + std::to_string(offset); + return false; + } + offset += n; + data += n; + len -= static_cast(n); + } + return true; +} + +bool CloseFile(std::intptr_t handle, std::string& error) { + int fd = static_cast(handle); + // A failing close() can surface a deferred write error (e.g. EIO, or ENOSPC on + // delayed allocation / a networked filesystem), so the file may be incomplete + // even though every pwrite returned success. Don't retry: on Linux the + // descriptor is freed even when close() returns EINTR, so a retry could close + // an unrelated, since-reused fd. + if (::close(fd) != 0) { + error = "close failed (errno " + std::to_string(errno) + ")"; + return false; + } + return true; +} + +} // namespace fl::platform diff --git a/sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc b/sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc new file mode 100644 index 00000000..c80fcc92 --- /dev/null +++ b/sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc @@ -0,0 +1,116 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +// Windows implementation of CrossProcessFileLock acquisition and the +// platform-specific lock handle. Cross-platform orchestration +// (WaitForDirectoryLock) lives in src/download/cross_process_file_lock.cc. +#include "download/cross_process_file_lock.h" +#include "exception.h" +#include "logger.h" + +#include + +#include +#include +#include +#include +#include + +#define WIN32_LEAN_AND_MEAN +#define NOMINMAX +#include +#include + +namespace fl { + +namespace { + +constexpr const char* kLockFileName = ".download.lock"; + +/// `PID:,Time:\n` +std::string FormatProcessInfo() { + auto pid = static_cast(_getpid()); + auto t = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()); + std::tm tm{}; + gmtime_s(&tm, &t); + std::ostringstream oss; + oss << "PID:" << pid << ",Time:" << std::put_time(&tm, "%Y-%m-%dT%H:%M:%SZ") << '\n'; + return oss.str(); +} + +} // namespace + +// Platform-specific resource handle. The destructor here is the only thing +// that releases the lock; CrossProcessFileLock's destructor is defaulted. +struct CrossProcessFileLock::State { + HANDLE handle; + ~State() { + if (handle != INVALID_HANDLE_VALUE) { + // FILE_FLAG_DELETE_ON_CLOSE removes the file when the last handle closes. + CloseHandle(handle); + } + } +}; + +CrossProcessFileLock::CrossProcessFileLock(std::filesystem::path path, + std::unique_ptr state, + ILogger& logger) + : path_(std::move(path)), state_(std::move(state)), logger_(logger) {} + +CrossProcessFileLock::~CrossProcessFileLock() { + // Release the OS handle first so the "released" log message is accurate. + state_.reset(); + logger_.Log(LogLevel::Debug, "CrossProcessFileLock released: " + path_.string()); +} + +std::unique_ptr CrossProcessFileLock::TryAcquireForDirectory( + const std::filesystem::path& directory, ILogger& logger) { + std::error_code ec; + std::filesystem::create_directories(directory, ec); + // Best-effort: if create_directories failed, the platform open below will + // surface a clearer error message. + + auto lock_path = directory / kLockFileName; + std::unique_ptr state; + + // dwShareMode=0 blocks any other open (cross- and in-process) until this + // handle closes. FILE_FLAG_DELETE_ON_CLOSE pairs OPEN_ALWAYS into a + // self-cleaning lock that doesn't require unlink-then-close races. + auto wide = lock_path.wstring(); + HANDLE handle = CreateFileW(wide.c_str(), + GENERIC_READ | GENERIC_WRITE, + 0, + nullptr, + OPEN_ALWAYS, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, + nullptr); + if (handle == INVALID_HANDLE_VALUE) { + DWORD err = GetLastError(); + if (err == ERROR_SHARING_VIOLATION || err == ERROR_LOCK_VIOLATION || err == ERROR_ACCESS_DENIED) { + // SHARING/LOCK_VIOLATION: another handle already holds the share-none + // lock. ACCESS_DENIED: the holder is mid-release — FILE_FLAG_DELETE_ON_CLOSE + // puts the file into STATUS_DELETE_PENDING during the close window, and a + // concurrent open of a delete-pending file is reported as ACCESS_DENIED. + // All three mean "another process has it"; treat as contention so the + // caller retries. (A genuine permission error also lands here and would + // poll until timeout, but the directory was just created successfully so + // that is improbable.) + return nullptr; + } + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, + "CreateFileW failed for lock '" + lock_path.string() + + "' (GetLastError=" + std::to_string(err) + ")"); + } + + auto info = FormatProcessInfo(); + DWORD written = 0; + WriteFile(handle, info.data(), static_cast(info.size()), &written, nullptr); + FlushFileBuffers(handle); + + state = std::unique_ptr(new State{handle}); + + logger.Log(LogLevel::Debug, "CrossProcessFileLock acquired: " + lock_path.string()); + return std::unique_ptr( + new CrossProcessFileLock(std::move(lock_path), std::move(state), logger)); +} + +} // namespace fl diff --git a/sdk_v2/cpp/src/platform/windows/file_io.cc b/sdk_v2/cpp/src/platform/windows/file_io.cc new file mode 100644 index 00000000..597ce6ad --- /dev/null +++ b/sdk_v2/cpp/src/platform/windows/file_io.cc @@ -0,0 +1,66 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +#include "platform/file_io.h" + +#ifndef NOMINMAX +#define NOMINMAX +#endif +#include + +#include + +namespace fl::platform { + +std::intptr_t OpenWritableFile(const std::filesystem::path& path, std::string& error) { + // FILE_SHARE_READ | FILE_SHARE_WRITE so the lock file / other tools can peek + // at the partial file without us erroring; positional WriteFile is safe + // regardless of share mode. + HANDLE h = ::CreateFileW(path.wstring().c_str(), GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, nullptr); + if (h == INVALID_HANDLE_VALUE) { + error = "Win32 err " + std::to_string(::GetLastError()); + return kInvalidFileHandle; + } + return reinterpret_cast(h); +} + +bool WriteFileAt(std::intptr_t handle, std::int64_t offset, const std::uint8_t* data, std::size_t len, + std::string& error) { + HANDLE h = reinterpret_cast(handle); + // Concurrent WriteFile calls with distinct OVERLAPPED offsets on the same + // handle are safe for non-overlapping ranges; the kernel orders them. + while (len > 0) { + OVERLAPPED ov{}; + // Split the 64-bit file offset across the OVERLAPPED halves: the DWORD casts + // keep the low 32 bits in Offset and the high 32 bits in OffsetHigh. + ov.Offset = static_cast(static_cast(offset)); + ov.OffsetHigh = static_cast(static_cast(offset) >> 32); + DWORD to_write = static_cast(len > 0x7FFFFFFFu ? 0x7FFFFFFFu : len); + DWORD written = 0; + if (!::WriteFile(h, data, to_write, &written, &ov)) { + error = "write failed at offset " + std::to_string(offset) + " (Win32 err " + + std::to_string(::GetLastError()) + ")"; + return false; + } + if (written == 0) { + error = "short write at offset " + std::to_string(offset); + return false; + } + offset += static_cast(written); + data += written; + len -= written; + } + return true; +} + +bool CloseFile(std::intptr_t handle, std::string& error) { + HANDLE h = reinterpret_cast(handle); + if (!::CloseHandle(h)) { + error = "CloseHandle failed (Win32 err " + std::to_string(::GetLastError()) + ")"; + return false; + } + return true; +} + +} // namespace fl::platform From 1eae3bcd71faebc486ecf47fb0f7fad2b383e1cd Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 7 Jul 2026 21:17:11 -0500 Subject: [PATCH 3/3] Address PR review: move lock to src/platform, unify temp test helper - Move cross_process_file_lock.{h,cc} from src/download into src/platform so the generic orchestration lives alongside its per-OS implementations (matching the ORT layout), instead of split across two directories. Update includes, CMakeLists source path, comments, and the plan doc. - Combine the TempDir and TempPath test helpers into a single TempPath (test/utils/temp_path.h) with a private constructor and CreateTempDir / CreateTempFile factory methods. - Guard the TempPath destructor with try/catch and a stderr diagnostic so a cleanup failure can never escape the destructor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- sdk_v2/cpp/CMakeLists.txt | 2 +- sdk_v2/cpp/docs/ResumableDownloadsPlan.md | 4 +- sdk_v2/cpp/src/download/download_manager.cc | 2 +- .../cross_process_file_lock.cc | 2 +- .../cross_process_file_lock.h | 0 .../platform/posix/cross_process_file_lock.cc | 4 +- .../windows/cross_process_file_lock.cc | 4 +- .../internal_api/blob_download_state_test.cc | 32 +++--- .../cross_process_file_lock_test.cc | 24 ++--- sdk_v2/cpp/test/internal_api/download_test.cc | 100 +++++++++--------- .../cpp/test/internal_api/file_writer_test.cc | 10 +- sdk_v2/cpp/test/internal_api/test_helpers.h | 2 +- .../test/utils/{temp_dir.h => temp_path.h} | 85 ++++++++------- 13 files changed, 138 insertions(+), 133 deletions(-) rename sdk_v2/cpp/src/{download => platform}/cross_process_file_lock.cc (97%) rename sdk_v2/cpp/src/{download => platform}/cross_process_file_lock.h (100%) rename sdk_v2/cpp/test/utils/{temp_dir.h => temp_path.h} (53%) diff --git a/sdk_v2/cpp/CMakeLists.txt b/sdk_v2/cpp/CMakeLists.txt index ff606ed4..7326d6fe 100644 --- a/sdk_v2/cpp/CMakeLists.txt +++ b/sdk_v2/cpp/CMakeLists.txt @@ -155,7 +155,7 @@ set(FOUNDRY_LOCAL_SOURCES src/configuration.cc src/download/blob_download_state.cc src/download/blob_downloader.cc - src/download/cross_process_file_lock.cc + src/platform/cross_process_file_lock.cc src/download/download_manager.cc src/download/file_writer.cc src/download/inference_model_writer.cc diff --git a/sdk_v2/cpp/docs/ResumableDownloadsPlan.md b/sdk_v2/cpp/docs/ResumableDownloadsPlan.md index f0c819ae..3d8b6f31 100644 --- a/sdk_v2/cpp/docs/ResumableDownloadsPlan.md +++ b/sdk_v2/cpp/docs/ResumableDownloadsPlan.md @@ -73,8 +73,8 @@ shippable state. **No public C ABI changes** in any increment. **Files added** -- `sdk_v2/cpp/src/download/cross_process_file_lock.h` -- `sdk_v2/cpp/src/download/cross_process_file_lock.cc` +- `sdk_v2/cpp/src/platform/cross_process_file_lock.h` +- `sdk_v2/cpp/src/platform/cross_process_file_lock.cc` **Files modified** diff --git a/sdk_v2/cpp/src/download/download_manager.cc b/sdk_v2/cpp/src/download/download_manager.cc index c4f9dc56..f7219e7e 100644 --- a/sdk_v2/cpp/src/download/download_manager.cc +++ b/sdk_v2/cpp/src/download/download_manager.cc @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. #include "download/download_manager.h" -#include "download/cross_process_file_lock.h" +#include "platform/cross_process_file_lock.h" #include "download/inference_model_writer.h" #include "exception.h" #include "log_level.h" diff --git a/sdk_v2/cpp/src/download/cross_process_file_lock.cc b/sdk_v2/cpp/src/platform/cross_process_file_lock.cc similarity index 97% rename from sdk_v2/cpp/src/download/cross_process_file_lock.cc rename to sdk_v2/cpp/src/platform/cross_process_file_lock.cc index 9ea5d3b8..1ce5a5e6 100644 --- a/sdk_v2/cpp/src/download/cross_process_file_lock.cc +++ b/sdk_v2/cpp/src/platform/cross_process_file_lock.cc @@ -5,7 +5,7 @@ // FormatProcessInfo, the CrossProcessFileLock destructor/constructor, and // TryAcquireForDirectory — live in // src/platform/{windows,posix}/cross_process_file_lock.cc. -#include "download/cross_process_file_lock.h" +#include "platform/cross_process_file_lock.h" #include "exception.h" #include diff --git a/sdk_v2/cpp/src/download/cross_process_file_lock.h b/sdk_v2/cpp/src/platform/cross_process_file_lock.h similarity index 100% rename from sdk_v2/cpp/src/download/cross_process_file_lock.h rename to sdk_v2/cpp/src/platform/cross_process_file_lock.h diff --git a/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc b/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc index 443bbc9e..4d32a69f 100644 --- a/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc +++ b/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc @@ -2,8 +2,8 @@ // Licensed under the MIT License. // POSIX implementation of CrossProcessFileLock acquisition and the // platform-specific lock handle. Cross-platform orchestration -// (WaitForDirectoryLock) lives in src/download/cross_process_file_lock.cc. -#include "download/cross_process_file_lock.h" +// (WaitForDirectoryLock) lives in src/platform/cross_process_file_lock.cc. +#include "platform/cross_process_file_lock.h" #include "exception.h" #include "logger.h" diff --git a/sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc b/sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc index c80fcc92..6f5e44cf 100644 --- a/sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc +++ b/sdk_v2/cpp/src/platform/windows/cross_process_file_lock.cc @@ -2,8 +2,8 @@ // Licensed under the MIT License. // Windows implementation of CrossProcessFileLock acquisition and the // platform-specific lock handle. Cross-platform orchestration -// (WaitForDirectoryLock) lives in src/download/cross_process_file_lock.cc. -#include "download/cross_process_file_lock.h" +// (WaitForDirectoryLock) lives in src/platform/cross_process_file_lock.cc. +#include "platform/cross_process_file_lock.h" #include "exception.h" #include "logger.h" diff --git a/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc b/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc index 19c5152f..38dfdd7c 100644 --- a/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc +++ b/sdk_v2/cpp/test/internal_api/blob_download_state_test.cc @@ -17,7 +17,7 @@ using namespace fl; namespace { -using fl::test::TempDir; +using fl::test::TempPath; constexpr int64_t kBlobSize = 20 * 1024 * 1024; // 20 MiB constexpr int32_t kChunkSize = 2 * 1024 * 1024; // 2 MiB @@ -32,7 +32,7 @@ TEST(BlobDownloadStateTest, GetStateFilePathAppendsDlstate) { } TEST(BlobDownloadStateTest, CreateNewInitializesEmptyBitmap) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); ASSERT_NE(s, nullptr); @@ -48,7 +48,7 @@ TEST(BlobDownloadStateTest, CreateNewInitializesEmptyBitmap) { } TEST(BlobDownloadStateTest, MarkChunkCompleteUpdatesBitmapAndCounter) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); s->MarkChunkComplete(3); @@ -60,7 +60,7 @@ TEST(BlobDownloadStateTest, MarkChunkCompleteUpdatesBitmapAndCounter) { } TEST(BlobDownloadStateTest, MarkChunkCompleteIsIdempotent) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); s->MarkChunkComplete(5); @@ -70,7 +70,7 @@ TEST(BlobDownloadStateTest, MarkChunkCompleteIsIdempotent) { } TEST(BlobDownloadStateTest, CalculateDownloadedSizeAccountsForPartialFinalChunk) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; constexpr int64_t kOddBlobSize = 4 * 1024 * 1024 + 17; // 3 chunks of 2 MiB; last chunk is a partial 17 bytes constexpr int32_t kOddNumChunks = 3; @@ -83,7 +83,7 @@ TEST(BlobDownloadStateTest, CalculateDownloadedSizeAccountsForPartialFinalChunk) } TEST(BlobDownloadStateTest, GetPendingChunksReturnsGaps) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); for (int32_t i : {0, 1, 2, 5, 7}) { @@ -95,7 +95,7 @@ TEST(BlobDownloadStateTest, GetPendingChunksReturnsGaps) { } TEST(BlobDownloadStateTest, SaveAndLoadRoundTrip) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; { auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); @@ -120,7 +120,7 @@ TEST(BlobDownloadStateTest, SaveAndLoadRoundTrip) { } TEST(BlobDownloadStateTest, SaveStateAdvancesBitmapByteAlignedStart) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; // Use a large enough total that whole-word advance is meaningful. constexpr int32_t kBigNumChunks = 200; @@ -154,7 +154,7 @@ TEST(BlobDownloadStateTest, SaveStateAdvancesBitmapByteAlignedStart) { // previously accumulated +64 per word onto the unaligned base and overshot by // (start % 64), silently marking never-downloaded chunks complete on reload. TEST(BlobDownloadStateTest, SaveStateFromUnalignedStartDoesNotMarkPendingComplete) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; constexpr int32_t kBigNumChunks = 200; constexpr int64_t kBigBlobSize = static_cast(kBigNumChunks) * kChunkSize; @@ -192,14 +192,14 @@ TEST(BlobDownloadStateTest, SaveStateFromUnalignedStartDoesNotMarkPendingComplet } TEST(BlobDownloadStateTest, LoadStateReturnsNullWhenFileMissing) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; auto s = BlobDownloadState::LoadState("blob", local, kBlobSize, kChunkSize, kNumChunks, fl::test::NullLog()); EXPECT_EQ(s, nullptr); } TEST(BlobDownloadStateTest, LoadStateRejectsBadMagic) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; auto sidecar = BlobDownloadState::GetStateFilePath(local); { @@ -213,7 +213,7 @@ TEST(BlobDownloadStateTest, LoadStateRejectsBadMagic) { } TEST(BlobDownloadStateTest, LoadStateRejectsBlobSizeMismatch) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; { auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); @@ -227,7 +227,7 @@ TEST(BlobDownloadStateTest, LoadStateRejectsBlobSizeMismatch) { } TEST(BlobDownloadStateTest, LoadStateRejectsChunkSizeMismatch) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; { auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); @@ -240,7 +240,7 @@ TEST(BlobDownloadStateTest, LoadStateRejectsChunkSizeMismatch) { } TEST(BlobDownloadStateTest, LoadStateRejectsTotalChunksMismatch) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; { auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); @@ -253,7 +253,7 @@ TEST(BlobDownloadStateTest, LoadStateRejectsTotalChunksMismatch) { } TEST(BlobDownloadStateTest, DeleteStateRemovesSidecar) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; { auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); @@ -268,7 +268,7 @@ TEST(BlobDownloadStateTest, DeleteStateRemovesSidecar) { } TEST(BlobDownloadStateTest, IsCompleteFlipsTrueWhenAllChunksMarked) { - TempDir d; + auto d = TempPath::CreateTempDir(); auto local = d.path() / "blob.bin"; auto s = BlobDownloadState::CreateNew("blob", local, kBlobSize, kChunkSize, kNumChunks); for (int32_t i = 0; i < kNumChunks; ++i) { diff --git a/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc b/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc index f25df86a..bc492b76 100644 --- a/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc +++ b/sdk_v2/cpp/test/internal_api/cross_process_file_lock_test.cc @@ -1,6 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -#include "download/cross_process_file_lock.h" +#include "platform/cross_process_file_lock.h" #include "test_helpers.h" #include "exception.h" @@ -27,12 +27,12 @@ using namespace fl; namespace { -using fl::test::TempDir; +using fl::test::TempPath; } // namespace TEST(CrossProcessFileLockTest, TryAcquireSucceedsForFreshDirectory) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); auto lock = CrossProcessFileLock::TryAcquireForDirectory(dir.path(), fl::test::NullLog()); @@ -43,7 +43,7 @@ TEST(CrossProcessFileLockTest, TryAcquireSucceedsForFreshDirectory) { } TEST(CrossProcessFileLockTest, ReleaseOnDestructionRemovesLockFile) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); fs::path lock_file; { @@ -59,7 +59,7 @@ TEST(CrossProcessFileLockTest, ReleaseOnDestructionRemovesLockFile) { } TEST(CrossProcessFileLockTest, SecondAcquireReturnsNullWhileFirstIsHeld) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); auto first = CrossProcessFileLock::TryAcquireForDirectory(dir.path(), fl::test::NullLog()); ASSERT_NE(first, nullptr); @@ -68,7 +68,7 @@ TEST(CrossProcessFileLockTest, SecondAcquireReturnsNullWhileFirstIsHeld) { } TEST(CrossProcessFileLockTest, ReacquireSucceedsAfterRelease) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); { auto first = CrossProcessFileLock::TryAcquireForDirectory(dir.path(), fl::test::NullLog()); ASSERT_NE(first, nullptr); @@ -78,7 +78,7 @@ TEST(CrossProcessFileLockTest, ReacquireSucceedsAfterRelease) { } TEST(CrossProcessFileLockTest, CreatesDirectoryIfMissing) { - TempDir parent; + auto parent = TempPath::CreateTempDir(); auto missing = parent.path() / "nested" / "model"; ASSERT_FALSE(fs::exists(missing)); @@ -91,7 +91,7 @@ TEST(CrossProcessFileLockTest, CreatesDirectoryIfMissing) { } TEST(CrossProcessFileLockTest, WaitForLockReturnsImmediatelyWhenAvailable) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); auto start = std::chrono::steady_clock::now(); auto lock = CrossProcessFileLock::WaitForDirectoryLock(dir.path(), []() { return false; }, fl::test::NullLog()); @@ -103,7 +103,7 @@ TEST(CrossProcessFileLockTest, WaitForLockReturnsImmediatelyWhenAvailable) { } TEST(CrossProcessFileLockTest, WaitForLockAcquiresAfterHolderReleases) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); auto holder = CrossProcessFileLock::TryAcquireForDirectory(dir.path(), fl::test::NullLog()); ASSERT_NE(holder, nullptr); @@ -126,7 +126,7 @@ TEST(CrossProcessFileLockTest, WaitForLockAcquiresAfterHolderReleases) { } TEST(CrossProcessFileLockTest, WaitForLockThrowsOnCancellation) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); auto holder = CrossProcessFileLock::TryAcquireForDirectory(dir.path(), fl::test::NullLog()); ASSERT_NE(holder, nullptr); @@ -149,7 +149,7 @@ TEST(CrossProcessFileLockTest, WaitForLockThrowsOnCancellation) { } TEST(CrossProcessFileLockTest, WaitForLockThrowsOnTimeout) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); auto holder = CrossProcessFileLock::TryAcquireForDirectory(dir.path(), fl::test::NullLog()); ASSERT_NE(holder, nullptr); @@ -174,7 +174,7 @@ TEST(CrossProcessFileLockTest, WaitForLockThrowsOnTimeout) { // in-process by SecondAcquireReturnsNullWhileFirstIsHeld (dwShareMode=0 is // enforced identically for same- and cross-process opens). TEST(CrossProcessFileLockTest, HeldAcrossProcessesAndReleasedWhenHolderExits) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); const auto acquired_signal = dir.path() / "child_acquired"; const auto release_signal = dir.path() / "parent_done"; diff --git a/sdk_v2/cpp/test/internal_api/download_test.cc b/sdk_v2/cpp/test/internal_api/download_test.cc index d0f44059..d8a5f9b5 100644 --- a/sdk_v2/cpp/test/internal_api/download_test.cc +++ b/sdk_v2/cpp/test/internal_api/download_test.cc @@ -10,7 +10,7 @@ #include "catalog/azure_catalog_models.h" #include "download/blob_download_state.h" #include "download/blob_downloader.h" -#include "download/cross_process_file_lock.h" +#include "platform/cross_process_file_lock.h" #include "download/download_manager.h" #include "download/inference_model_writer.h" #include "download/model_registry_client.h" @@ -47,7 +47,7 @@ using namespace fl; namespace { -using fl::test::TempDir; +using fl::test::TempPath; /// Read entire file contents. std::string ReadFile(const fs::path& path) { @@ -418,7 +418,7 @@ TEST(ModelRegistryClientTest, Fallback_PerCallRegionOverridesStickyRegion) { // ======================================================================== TEST(BlobDownloadTest, DownloadsAllBlobs) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"model/weights.safetensors", 1000}, @@ -433,7 +433,7 @@ TEST(BlobDownloadTest, DownloadsAllBlobs) { } TEST(BlobDownloadTest, FiltersByPathPrefix) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"variant-a/weights.safetensors", 1000}, @@ -450,7 +450,7 @@ TEST(BlobDownloadTest, FiltersByPathPrefix) { } TEST(BlobDownloadTest, FiltersOutInferenceModelJson) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"weights.safetensors", 1000}, @@ -468,7 +468,7 @@ TEST(BlobDownloadTest, FiltersOutInferenceModelJson) { } TEST(BlobDownloadTest, ReportsProgress) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"file1.bin", 500}, @@ -496,7 +496,7 @@ TEST(BlobDownloadTest, ReportsProgress) { } TEST(BlobDownloadTest, HandlesEmptyBlobList) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; // No blobs @@ -511,7 +511,7 @@ TEST(BlobDownloadTest, HandlesEmptyBlobList) { // ======================================================================== TEST(BlobDownloadTest, SkipsExistingFilesWithCorrectSize) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); // Pre-create one of the blobs at the expected size on disk. std::ofstream(tmpdir.path() / "weights.safetensors") << std::string(1000, 'X'); @@ -530,7 +530,7 @@ TEST(BlobDownloadTest, SkipsExistingFilesWithCorrectSize) { } TEST(BlobDownloadTest, RedownloadsFilesWithWrongSize) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); // Existing file is truncated relative to the expected blob size. std::ofstream(tmpdir.path() / "weights.safetensors") << std::string(500, 'X'); @@ -548,7 +548,7 @@ TEST(BlobDownloadTest, RedownloadsFilesWithWrongSize) { } TEST(BlobDownloadTest, ReportsSkippedBytesInInitialProgress) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); // 500 of 2000 bytes already on disk → initial progress should be 25%. std::ofstream(tmpdir.path() / "already.bin") << std::string(500, 'X'); @@ -575,7 +575,7 @@ TEST(BlobDownloadTest, ReportsSkippedBytesInInitialProgress) { } TEST(BlobDownloadTest, EmitsHundredPercentWhenEverythingIsCached) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); std::ofstream(tmpdir.path() / "a.bin") << std::string(100, 'A'); std::ofstream(tmpdir.path() / "b.bin") << std::string(200, 'B'); @@ -632,7 +632,7 @@ TEST(IsPathWithinDirectoryTest, RejectsSiblingPrefixCollision) { } TEST(BlobDownloadTest, RejectsPathTraversalBlobName) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"../evil.bin", 4}, @@ -645,7 +645,7 @@ TEST(BlobDownloadTest, RejectsPathTraversalBlobName) { } TEST(BlobDownloadTest, RejectsBackslashPathTraversalBlobName) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"..\\evil.bin", 4}, @@ -658,7 +658,7 @@ TEST(BlobDownloadTest, RejectsBackslashPathTraversalBlobName) { } TEST(BlobDownloadTest, RejectsNestedPathTraversalBlobName) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"good/../../evil.bin", 4}, @@ -671,7 +671,7 @@ TEST(BlobDownloadTest, RejectsNestedPathTraversalBlobName) { } TEST(BlobDownloadTest, CancellationStopsRemainingBlobs) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); CancellingMockDownloader mock; mock.blobs_to_return = { {"blob1.bin", 100}, @@ -696,7 +696,7 @@ TEST(BlobDownloadTest, CancellationStopsRemainingBlobs) { } TEST(BlobDownloadTest, CancelledFlagAbortsInFlightDownload) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); CancelCheckingMockDownloader mock; mock.blobs_to_return = { {"blob1.bin", 100}, @@ -728,7 +728,7 @@ TEST(BlobDownloadTest, CancelledFlagAbortsInFlightDownload) { // ======================================================================== TEST(InferenceModelWriterTest, WritesJsonWithPromptTemplate) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); fl::KeyValuePairs templates; templates.Add("system", "You are a helpful assistant."); templates.Add("user", "{input}"); @@ -743,7 +743,7 @@ TEST(InferenceModelWriterTest, WritesJsonWithPromptTemplate) { } TEST(InferenceModelWriterTest, WritesNullPromptTemplateWhenEmpty) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); fl::KeyValuePairs templates; WriteInferenceModelJson(tmpdir.string(), "test-model", templates); @@ -759,7 +759,7 @@ TEST(InferenceModelWriterTest, WritesNullPromptTemplateWhenEmpty) { // ======================================================================== TEST(VariantFixupTest, CopiesInferenceModelToSubdirs) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); const auto& root = tmpdir.path(); // Create inference_model.json at root @@ -782,7 +782,7 @@ TEST(VariantFixupTest, CopiesInferenceModelToSubdirs) { } TEST(VariantFixupTest, DoesNotOverwriteExisting) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); const auto& root = tmpdir.path(); { @@ -805,7 +805,7 @@ TEST(VariantFixupTest, DoesNotOverwriteExisting) { } TEST(VariantFixupTest, NoOpWhenNoRootFile) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); fs::create_directories(tmpdir.path() / "sub"); // Should not throw @@ -817,7 +817,7 @@ TEST(VariantFixupTest, PreservesRootFileWhenNoSubdirs) { // Single-variant downloads put every blob (and inference_model.json) at the root // with no variant subdirectory. The fixup must not delete the root file in that // case, otherwise IsModelCached would report false on the next check. - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); const auto& root = tmpdir.path(); { @@ -845,7 +845,7 @@ TEST(VariantFixupTest, PreservesRootFileWhenNoSubdirs) { // ======================================================================== TEST(DownloadManagerTest, FullDownloadFlow) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto manager = std::make_unique(tmpdir.string(), "eastus", 64, fl::test::NullLog()); @@ -897,7 +897,7 @@ TEST(DownloadManagerTest, FullDownloadFlow) { // Run one download and return the registry URL the manager hit. static std::string CaptureRegistryUrlForDownload(const std::string& config_region, const std::string& detected_region) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto manager = std::make_unique(tmpdir.string(), config_region, 64, fl::test::NullLog()); @@ -951,7 +951,7 @@ TEST(DownloadManagerTest, Region_FallsBackToDefaultRegistryRegionWhenNoConfigAnd } TEST(DownloadManagerTest, SkipsAlreadyCachedModel) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto manager = std::make_unique(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -977,7 +977,7 @@ TEST(DownloadManagerTest, SkipsAlreadyCachedModel) { } TEST(DownloadManagerTest, IsModelCachedReturnsFalseForMissing) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -988,7 +988,7 @@ TEST(DownloadManagerTest, IsModelCachedReturnsFalseForMissing) { } TEST(DownloadManagerTest, IsModelCachedReturnsFalseForIncomplete) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1006,7 +1006,7 @@ TEST(DownloadManagerTest, IsModelCachedReturnsFalseForIncomplete) { } TEST(DownloadManagerTest, IsModelCachedReturnsTrueForComplete) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1025,7 +1025,7 @@ TEST(DownloadManagerTest, IsModelCachedReturnsTrueForComplete) { } TEST(DownloadManagerTest, IsModelCachedReturnsFalseForEmptyDir) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1041,7 +1041,7 @@ TEST(DownloadManagerTest, IsModelCachedReturnsFalseForEmptyDir) { } TEST(DownloadManagerTest, VersionSuffixConversion) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1061,7 +1061,7 @@ TEST(DownloadManagerTest, VersionSuffixConversion) { } TEST(DownloadManagerTest, ThrowsOnEmptyUri) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1075,7 +1075,7 @@ TEST(DownloadManagerTest, ThrowsOnEmptyUri) { // thread sees the cached result rather than re-downloading. Different models still // proceed in parallel — covered by the unrelated-model test below. TEST(DownloadManagerTest, ConcurrentDownloadsOfSameModelSerialize) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); auto registry = std::make_unique( @@ -1159,7 +1159,7 @@ TEST(DownloadManagerTest, ConcurrentDownloadsOfSameModelSerialize) { // downloads running at once; correct serialization keeps that peak at 1 (the // second download can't enter until the first releases the mutex). TEST(DownloadManagerTest, ModelDownloadsSerializeUnderGlobalLock) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); auto registry = std::make_unique( @@ -1251,7 +1251,7 @@ TEST(DownloadManagerTest, ModelDownloadsSerializeUnderGlobalLock) { // AND inference_model.json is present, return the cached result via the post-lock // recheck WITHOUT re-downloading anything. TEST(DownloadManagerTest, WaitsForCrossProcessLockThenServesCachedResult) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); // Registry + downloader that must stay untouched if the post-lock recheck works. @@ -1306,7 +1306,7 @@ TEST(DownloadManagerTest, WaitsForCrossProcessLockThenServesCachedResult) { // it's asked about is not a directory (e.g. a regular file). Previously the // underlying directory_iterator would throw filesystem_error. TEST(DownloadManagerTest, IsModelCachedReturnsFalseWhenPathIsRegularFile) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1447,7 +1447,7 @@ TEST(EndToEndTest, DISABLED_LiveCatalogAndDownload) { // ======================================================================== TEST(DownloadManagerTest, RejectsParentEscapeInModelId) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1459,7 +1459,7 @@ TEST(DownloadManagerTest, RejectsParentEscapeInModelId) { } TEST(DownloadManagerTest, RejectsBackslashInPublisher) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1470,7 +1470,7 @@ TEST(DownloadManagerTest, RejectsBackslashInPublisher) { } TEST(DownloadManagerTest, RejectsForwardSlashInPublisher) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1483,7 +1483,7 @@ TEST(DownloadManagerTest, RejectsForwardSlashInPublisher) { TEST(DownloadManagerTest, RejectsColonInBareModelId) { // model_id "drive:c:1" splits as bare="drive:c", version="1"; the bare half then // contains a stray ':' that would let a Windows drive letter slip through. - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1494,7 +1494,7 @@ TEST(DownloadManagerTest, RejectsColonInBareModelId) { } TEST(DownloadManagerTest, RejectsTrailingDotInPublisher) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1505,7 +1505,7 @@ TEST(DownloadManagerTest, RejectsTrailingDotInPublisher) { } TEST(DownloadManagerTest, RejectsEmptyModelId) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1516,7 +1516,7 @@ TEST(DownloadManagerTest, RejectsEmptyModelId) { } TEST(DownloadManagerTest, AcceptsNormalModelIdAndPublisher) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); DownloadManager manager(tmpdir.string(), "eastus", 64, fl::test::NullLog()); ModelInfo info; @@ -1599,7 +1599,7 @@ class FakeChunkAzureDownloader : public AzureBlobDownloader { } // namespace TEST(AzureBlobDownloaderResumeTest, SkipsChunksAlreadyMarkedCompleteInSidecar) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "blob.bin"; constexpr int32_t kChunkSize = 2 * 1024 * 1024; @@ -1642,7 +1642,7 @@ TEST(AzureBlobDownloaderResumeTest, IgnoresSidecarWhenDataFileTruncated) { // an external cleanup) while the sidecar survived. The downloader must not trust // the sidecar — those "completed" chunks are no longer on disk — and must // re-download every chunk rather than leave them as zeros. - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "blob.bin"; constexpr int32_t kChunkSize = 2 * 1024 * 1024; @@ -1675,7 +1675,7 @@ TEST(AzureBlobDownloaderResumeTest, IgnoresSidecarWhenDataFileTruncated) { } TEST(AzureBlobDownloaderResumeTest, DownloadsAllChunksWhenSidecarMissing) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "blob.bin"; constexpr int32_t kChunkSize = 2 * 1024 * 1024; @@ -1695,7 +1695,7 @@ TEST(AzureBlobDownloaderResumeTest, DownloadsAllChunksWhenSidecarMissing) { } TEST(AzureBlobDownloaderResumeTest, PersistsSidecarOnChunkFailure) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "blob.bin"; constexpr int32_t kChunkSize = 2 * 1024 * 1024; @@ -1742,7 +1742,7 @@ TEST(AzureBlobDownloaderResumeTest, PersistsSidecarOnChunkFailure) { // next run skips — silently serving zeros. Verify a sidecar is already present // the moment the first chunk is requested. TEST(AzureBlobDownloaderResumeTest, SidecarExistsBeforeFirstChunkCompletes) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "blob.bin"; constexpr int32_t kChunkSize = 2 * 1024 * 1024; @@ -1778,7 +1778,7 @@ TEST(AzureBlobDownloaderResumeTest, SidecarExistsBeforeFirstChunkCompletes) { } TEST(AzureBlobDownloaderResumeTest, CleansUpSidecarOnEmptyBlob) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "empty.bin"; // Plant a stale sidecar. { @@ -1798,7 +1798,7 @@ TEST(AzureBlobDownloaderResumeTest, CleansUpSidecarOnEmptyBlob) { } TEST(AzureBlobDownloaderResumeTest, ChunkFailureCancelsInFlightPeersFast) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "blob.bin"; constexpr int32_t kChunkSize = 2 * 1024 * 1024; @@ -1844,7 +1844,7 @@ TEST(AzureBlobDownloaderResumeTest, ChunkFailureCancelsInFlightPeersFast) { } TEST(AzureBlobDownloaderResumeTest, UserCancelDrainsInFlightPeersFast) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); auto local = tmpdir.path() / "blob.bin"; constexpr int32_t kChunkSize = 2 * 1024 * 1024; diff --git a/sdk_v2/cpp/test/internal_api/file_writer_test.cc b/sdk_v2/cpp/test/internal_api/file_writer_test.cc index bc520b08..c9d1ff69 100644 --- a/sdk_v2/cpp/test/internal_api/file_writer_test.cc +++ b/sdk_v2/cpp/test/internal_api/file_writer_test.cc @@ -29,7 +29,7 @@ using fl::test::TempPath; } // namespace TEST(FileWriterTest, OpenCreatesFileAtRequestedSize) { - TempPath p; + auto p = TempPath::CreateTempFile(); FileWriter w(fl::test::NullLog()); w.Open(p.path(), 4096); w.Close(); @@ -38,7 +38,7 @@ TEST(FileWriterTest, OpenCreatesFileAtRequestedSize) { } TEST(FileWriterTest, OpenPreservesExistingFileAtSameSize) { - TempPath p; + auto p = TempPath::CreateTempFile(); // Pre-write a sentinel byte the writer must NOT overwrite. { std::ofstream f(p.path(), std::ios::binary); @@ -64,7 +64,7 @@ TEST(FileWriterTest, OpenPreservesExistingFileAtSameSize) { } TEST(FileWriterTest, OpenRecreatesFileWhenSizeDiffers) { - TempPath p; + auto p = TempPath::CreateTempFile(); { std::ofstream f(p.path(), std::ios::binary); f.seekp(100); @@ -79,7 +79,7 @@ TEST(FileWriterTest, OpenRecreatesFileWhenSizeDiffers) { } TEST(FileWriterTest, SingleThreadWriteAt) { - TempPath p; + auto p = TempPath::CreateTempFile(); FileWriter w(fl::test::NullLog()); w.Open(p.path(), 1024); @@ -97,7 +97,7 @@ TEST(FileWriterTest, SingleThreadWriteAt) { } TEST(FileWriterTest, ConcurrentDisjointWritesProduceCorrectFile) { - TempPath p; + auto p = TempPath::CreateTempFile(); constexpr int kThreads = 8; constexpr int kRegionSize = 256 * 1024; // 256 KB per thread constexpr int kPieceSize = 16 * 1024; // 16 KB per WriteAt diff --git a/sdk_v2/cpp/test/internal_api/test_helpers.h b/sdk_v2/cpp/test/internal_api/test_helpers.h index c6bc06fd..e5ef61ea 100644 --- a/sdk_v2/cpp/test/internal_api/test_helpers.h +++ b/sdk_v2/cpp/test/internal_api/test_helpers.h @@ -8,7 +8,7 @@ #include "inferencing/model_load_manager.h" #include "logger.h" -#include "utils/temp_dir.h" +#include "utils/temp_path.h" #include #include diff --git a/sdk_v2/cpp/test/utils/temp_dir.h b/sdk_v2/cpp/test/utils/temp_path.h similarity index 53% rename from sdk_v2/cpp/test/utils/temp_dir.h rename to sdk_v2/cpp/test/utils/temp_path.h index ed55feb8..f5e8bbb9 100644 --- a/sdk_v2/cpp/test/utils/temp_dir.h +++ b/sdk_v2/cpp/test/utils/temp_path.h @@ -7,6 +7,8 @@ #include #include +#include +#include #include #include #include @@ -40,61 +42,64 @@ inline std::filesystem::path MakeUniqueTempPath(const std::string& prefix) { std::to_string(counter.fetch_add(1, std::memory_order_relaxed))); } -/// RAII temporary directory for test isolation. The name comes from MakeUniqueTempPath, so it is -/// unique both across the separate processes CTest launches per test and across multiple TempDirs -/// within one test. create_directory must succeed — the directory must not already exist — so a -/// residual collision (e.g. a directory leaked by an earlier process that reused this pid) advances -/// to the next name and retries instead of silently sharing an existing directory. -class TempDir { +/// RAII temporary filesystem path for test isolation, removed on destruction so a flaky test +/// never leaks into the system temp dir. The name comes from MakeUniqueTempPath, so it is unique +/// both across the separate processes CTest launches per test and across multiple instances within +/// one test. Construct via the factories: +/// - CreateTempDir creates the directory up front (create_directory must succeed). +/// - CreateTempFile only reserves the path; the caller creates the file. +class TempPath { public: - explicit TempDir(const std::string& prefix = "fl_test_") { + /// Create and own a fresh temporary directory. A residual collision (e.g. a directory leaked by + /// an earlier process that reused this pid) advances to the next name and retries rather than + /// silently sharing an existing directory. + static TempPath CreateTempDir(const std::string& prefix = "fl_test_") { + return TempPath(prefix, /*create_dir=*/true); + } + + /// Reserve and own a fresh temporary file path. The file is not created here — the caller does. + static TempPath CreateTempFile(const std::string& prefix = "fl_test_") { + return TempPath(prefix, /*create_dir=*/false); + } + + ~TempPath() { + // Destructors must not throw. remove_all is used via its error_code overload, but guard the + // whole body so an unexpected exception (e.g. bad_alloc) is reported rather than terminating. + try { + std::error_code ec; + std::filesystem::remove_all(path_, ec); + } catch (const std::exception& e) { + std::fprintf(stderr, "TempPath: failed to remove '%s': %s\n", path_.string().c_str(), e.what()); + } + } + + TempPath(const TempPath&) = delete; + TempPath& operator=(const TempPath&) = delete; + + const std::filesystem::path& path() const { return path_; } + std::string string() const { return path_.string(); } + + private: + TempPath(const std::string& prefix, bool create_dir) { while (true) { auto candidate = MakeUniqueTempPath(prefix); + if (!create_dir) { + path_ = std::move(candidate); + return; + } std::error_code ec; if (std::filesystem::create_directory(candidate, ec)) { path_ = std::move(candidate); return; } if (ec) { - throw std::runtime_error("TempDir: failed to create '" + candidate.string() + "': " + + throw std::runtime_error("TempPath: failed to create '" + candidate.string() + "': " + ec.message()); } // candidate already existed — try the next name. } } - ~TempDir() { - std::error_code ec; - std::filesystem::remove_all(path_, ec); - } - - TempDir(const TempDir&) = delete; - TempDir& operator=(const TempDir&) = delete; - - const std::filesystem::path& path() const { return path_; } - std::string string() const { return path_.string(); } - - private: - std::filesystem::path path_; -}; - -/// RAII unique temporary file path. The path is not created here — callers create the file — and -/// it is removed on destruction so a flaky test never leaks files into the system temp dir. -class TempPath { - public: - explicit TempPath(const std::string& prefix = "fl_test_") : path_(MakeUniqueTempPath(prefix)) {} - - ~TempPath() { - std::error_code ec; - std::filesystem::remove(path_, ec); - } - - TempPath(const TempPath&) = delete; - TempPath& operator=(const TempPath&) = delete; - - const std::filesystem::path& path() const { return path_; } - - private: std::filesystem::path path_; };