diff --git a/sdk_v2/cpp/CMakeLists.txt b/sdk_v2/cpp/CMakeLists.txt index f4a3c2dd5..7326d6fea 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() @@ -151,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 f0c819ae5..3d8b6f31b 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 c4f9dc569..f7219e7e8 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/file_writer.cc b/sdk_v2/cpp/src/download/file_writer.cc index 6ffb6dd5e..cdde85d24 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 29540e2aa..2b63feb7f 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/cross_process_file_lock.cc b/sdk_v2/cpp/src/platform/cross_process_file_lock.cc new file mode 100644 index 000000000..1ce5a5e6f --- /dev/null +++ b/sdk_v2/cpp/src/platform/cross_process_file_lock.cc @@ -0,0 +1,48 @@ +// 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 "platform/cross_process_file_lock.h" +#include "exception.h" + +#include + +#include +#include + +namespace fl { + +std::unique_ptr CrossProcessFileLock::WaitForDirectoryLock( + const std::filesystem::path& directory, + const CancellationPredicate& is_cancelled, + ILogger& logger, + std::chrono::milliseconds poll_interval, + std::chrono::milliseconds timeout) { + auto deadline = std::chrono::steady_clock::now() + timeout; + // `is_cancelled` is the caller's progress callback, which also serves as the + // liveness heartbeat — it emits 0% on every invocation. We therefore poll it + // on a single cadence (once per `poll_interval`) rather than on a separate + // fast cancellation tick: a faster tick would spam the user callback (~10x/s) + // for the entire wait, and cancelling a multi-minute cross-process wait a + // second sooner is imperceptible. There is no separate cancellation channel + // to decouple the heartbeat from. + while (true) { + if (is_cancelled && is_cancelled()) { + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "lock acquisition cancelled"); + } + auto lock = CrossProcessFileLock::TryAcquireForDirectory(directory, logger); + if (lock) { + return lock; + } + if (std::chrono::steady_clock::now() >= deadline) { + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, + "timed out waiting for cross-process download lock on '" + directory.string() + "'"); + } + std::this_thread::sleep_for(poll_interval); + } +} + +} // namespace fl 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/file_io.h b/sdk_v2/cpp/src/platform/file_io.h new file mode 100644 index 000000000..49e7d89e8 --- /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/download/cross_process_file_lock.cc b/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc similarity index 57% rename from sdk_v2/cpp/src/download/cross_process_file_lock.cc rename to sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc index 5aa82c769..4d32a69f3 100644 --- a/sdk_v2/cpp/src/download/cross_process_file_lock.cc +++ b/sdk_v2/cpp/src/platform/posix/cross_process_file_lock.cc @@ -1,6 +1,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -#include "download/cross_process_file_lock.h" +// POSIX implementation of CrossProcessFileLock acquisition and the +// platform-specific lock handle. Cross-platform orchestration +// (WaitForDirectoryLock) lives in src/platform/cross_process_file_lock.cc. +#include "platform/cross_process_file_lock.h" #include "exception.h" #include "logger.h" @@ -11,20 +14,12 @@ #include #include #include -#include - -#ifdef _WIN32 -#define WIN32_LEAN_AND_MEAN -#define NOMINMAX -#include -#include -#else + #include #include #include #include #include -#endif namespace fl { @@ -34,18 +29,10 @@ 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(); @@ -55,17 +42,6 @@ std::string FormatProcessInfo() { // 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; @@ -85,7 +61,6 @@ struct CrossProcessFileLock::State { } } }; -#endif CrossProcessFileLock::CrossProcessFileLock(std::filesystem::path path, std::unique_ptr state, @@ -108,43 +83,6 @@ std::unique_ptr CrossProcessFileLock::TryAcquireForDirecto 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, @@ -187,41 +125,10 @@ std::unique_ptr CrossProcessFileLock::TryAcquireForDirecto } 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, - ILogger& logger, - std::chrono::milliseconds poll_interval, - std::chrono::milliseconds timeout) { - auto deadline = std::chrono::steady_clock::now() + timeout; - // `is_cancelled` is the caller's progress callback, which also serves as the - // liveness heartbeat — it emits 0% on every invocation. We therefore poll it - // on a single cadence (once per `poll_interval`) rather than on a separate - // fast cancellation tick: a faster tick would spam the user callback (~10x/s) - // for the entire wait, and cancelling a multi-minute cross-process wait a - // second sooner is imperceptible. There is no separate cancellation channel - // to decouple the heartbeat from. - while (true) { - if (is_cancelled && is_cancelled()) { - FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "lock acquisition cancelled"); - } - auto lock = CrossProcessFileLock::TryAcquireForDirectory(directory, logger); - if (lock) { - return lock; - } - if (std::chrono::steady_clock::now() >= deadline) { - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, - "timed out waiting for cross-process download lock on '" + directory.string() + "'"); - } - std::this_thread::sleep_for(poll_interval); - } -} - } // 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 000000000..e6c8c2814 --- /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 000000000..6f5e44cf2 --- /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/platform/cross_process_file_lock.cc. +#include "platform/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 000000000..597ce6ada --- /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 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 f70b99809..38dfdd7cf 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::TempPath; constexpr int64_t kBlobSize = 20 * 1024 * 1024; // 20 MiB constexpr int32_t kChunkSize = 2 * 1024 * 1024; // 2 MiB @@ -48,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); @@ -64,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); @@ -76,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); @@ -86,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; @@ -99,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}) { @@ -111,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); @@ -136,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; @@ -170,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; @@ -208,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); { @@ -229,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); @@ -243,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); @@ -256,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); @@ -269,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); @@ -284,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 f32df42dd..bc492b760 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,30 +27,12 @@ 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::TempPath; } // namespace TEST(CrossProcessFileLockTest, TryAcquireSucceedsForFreshDirectory) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); auto lock = CrossProcessFileLock::TryAcquireForDirectory(dir.path(), fl::test::NullLog()); @@ -61,7 +43,7 @@ TEST(CrossProcessFileLockTest, TryAcquireSucceedsForFreshDirectory) { } TEST(CrossProcessFileLockTest, ReleaseOnDestructionRemovesLockFile) { - TempDir dir; + auto dir = TempPath::CreateTempDir(); fs::path lock_file; { @@ -77,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); @@ -86,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); @@ -96,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)); @@ -109,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()); @@ -121,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); @@ -144,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); @@ -167,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); @@ -192,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 96ce4f543..d8a5f9b5c 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,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::TempPath; /// Read entire file contents. std::string ReadFile(const fs::path& path) { @@ -449,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}, @@ -464,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}, @@ -481,7 +450,7 @@ TEST(BlobDownloadTest, FiltersByPathPrefix) { } TEST(BlobDownloadTest, FiltersOutInferenceModelJson) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"weights.safetensors", 1000}, @@ -499,7 +468,7 @@ TEST(BlobDownloadTest, FiltersOutInferenceModelJson) { } TEST(BlobDownloadTest, ReportsProgress) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"file1.bin", 500}, @@ -527,7 +496,7 @@ TEST(BlobDownloadTest, ReportsProgress) { } TEST(BlobDownloadTest, HandlesEmptyBlobList) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; // No blobs @@ -542,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'); @@ -561,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'); @@ -579,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'); @@ -606,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'); @@ -663,7 +632,7 @@ TEST(IsPathWithinDirectoryTest, RejectsSiblingPrefixCollision) { } TEST(BlobDownloadTest, RejectsPathTraversalBlobName) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"../evil.bin", 4}, @@ -676,7 +645,7 @@ TEST(BlobDownloadTest, RejectsPathTraversalBlobName) { } TEST(BlobDownloadTest, RejectsBackslashPathTraversalBlobName) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); MockBlobDownloader mock; mock.blobs_to_return = { {"..\\evil.bin", 4}, @@ -689,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}, @@ -702,7 +671,7 @@ TEST(BlobDownloadTest, RejectsNestedPathTraversalBlobName) { } TEST(BlobDownloadTest, CancellationStopsRemainingBlobs) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); CancellingMockDownloader mock; mock.blobs_to_return = { {"blob1.bin", 100}, @@ -727,7 +696,7 @@ TEST(BlobDownloadTest, CancellationStopsRemainingBlobs) { } TEST(BlobDownloadTest, CancelledFlagAbortsInFlightDownload) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); CancelCheckingMockDownloader mock; mock.blobs_to_return = { {"blob1.bin", 100}, @@ -759,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}"); @@ -774,7 +743,7 @@ TEST(InferenceModelWriterTest, WritesJsonWithPromptTemplate) { } TEST(InferenceModelWriterTest, WritesNullPromptTemplateWhenEmpty) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); fl::KeyValuePairs templates; WriteInferenceModelJson(tmpdir.string(), "test-model", templates); @@ -790,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 @@ -813,7 +782,7 @@ TEST(VariantFixupTest, CopiesInferenceModelToSubdirs) { } TEST(VariantFixupTest, DoesNotOverwriteExisting) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); const auto& root = tmpdir.path(); { @@ -836,7 +805,7 @@ TEST(VariantFixupTest, DoesNotOverwriteExisting) { } TEST(VariantFixupTest, NoOpWhenNoRootFile) { - TempDir tmpdir; + auto tmpdir = TempPath::CreateTempDir(); fs::create_directories(tmpdir.path() / "sub"); // Should not throw @@ -848,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(); { @@ -876,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()); @@ -928,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()); @@ -982,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; @@ -1008,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; @@ -1019,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; @@ -1037,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; @@ -1056,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; @@ -1072,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; @@ -1092,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; @@ -1106,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( @@ -1190,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( @@ -1282,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. @@ -1337,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; @@ -1478,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; @@ -1490,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; @@ -1501,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; @@ -1514,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; @@ -1525,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; @@ -1536,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; @@ -1547,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; @@ -1630,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; @@ -1673,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; @@ -1706,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; @@ -1726,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; @@ -1773,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; @@ -1809,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. { @@ -1829,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; @@ -1875,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 4eac37830..c9d1ff692 100644 --- a/sdk_v2/cpp/test/internal_api/file_writer_test.cc +++ b/sdk_v2/cpp/test/internal_api/file_writer_test.cc @@ -24,26 +24,12 @@ 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 TEST(FileWriterTest, OpenCreatesFileAtRequestedSize) { - TempPath p; + auto p = TempPath::CreateTempFile(); FileWriter w(fl::test::NullLog()); w.Open(p.path(), 4096); w.Close(); @@ -52,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); @@ -78,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); @@ -93,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); @@ -111,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 de4c41e22..e5ef61ea0 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_path.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_path.h b/sdk_v2/cpp/test/utils/temp_path.h new file mode 100644 index 000000000..f5e8bbb98 --- /dev/null +++ b/sdk_v2/cpp/test/utils/temp_path.h @@ -0,0 +1,106 @@ +// 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 +#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 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: + /// 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("TempPath: failed to create '" + candidate.string() + "': " + + ec.message()); + } + // candidate already existed — try the next name. + } + } + + std::filesystem::path path_; +}; + +} // namespace fl::test