From b9cbf40951d8aaefd7dedd1e6aa30f92ab35aa3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20L=C3=B8nner=C3=B8d=20Madsen?= Date: Mon, 12 Oct 2026 14:31:27 +0200 Subject: [PATCH] fix: cancel license lock waits on teardown and write the license atomically Instances that share a license file take turns re-validating it under a file lock, and a waiting instance blocked in flock() with no way out. Destroying the ActivationController while another instance held the lock for its own network check therefore held up the host until that check finished, which offline can take until the request times out. The lock is now taken without blocking and retried, file_license_store gains cancel_lock_waits(), and the controller calls it on teardown alongside its transport cancels. On Windows this also replaces _LK_LOCK, which gave up after about 10 seconds and left a waiting instance locked for the session. store_local_license truncated and rewrote the license in place, while readers such as an instance loading at startup read it without the lock, and load_local_license deletes a file it cannot parse. A reader that caught a write half done deleted the license. The store now writes a temporary file beside the license and renames it into place, keeping the file's permissions, following a symlinked license path, and writing in place for a hard-linked file or where no new file can be created. Tested with new store tests (a concurrent reader during repeated writes, cancelling a lock wait, permissions, symlinks, hard links, the in-place fallback) and a controller test that tears down while another store holds the lock. Each failed before the fix; the full suite passes on macOS. --- docs/core-sdk.md | 6 +- docs/juce-module.md | 3 +- include/moonbase/detail/file_lock.hpp | 72 ++++-- include/moonbase/store.hpp | 138 +++++++++++- .../juce/ActivationController.cpp | 15 +- .../juce/ActivationController.h | 5 +- modules/moonbase_licensing/juce/WorkerPool.h | 9 +- .../moonbase/detail/file_lock.hpp | 72 ++++-- modules/moonbase_licensing/moonbase/store.hpp | 138 +++++++++++- tests/juce/controller_tests.cpp | 34 +++ tests/store_tests.cpp | 210 ++++++++++++++++++ 11 files changed, 642 insertions(+), 60 deletions(-) diff --git a/docs/core-sdk.md b/docs/core-sdk.md index 367cada..9dca4c8 100644 --- a/docs/core-sdk.md +++ b/docs/core-sdk.md @@ -336,7 +336,11 @@ auto resolver = std::make_shared(); moonbase::licensing licensing(options, store, resolver); ``` -The default store is in-memory. `file_license_store` persists a JSON representation of the validated license. +The default store is in-memory. `file_license_store` persists a JSON representation of the +validated license, replacing the file in one step so a reader never finds half of one. +Instances that share the file take turns in `validate_token_online`, so one may wait for +another's network check. When shutting down, call `cancel_lock_waits()` on the store and a +waiting call throws `storage_error` instead. A custom resolver's id is compared literally, so it does not need to follow the [`mbd2_` stamp format](device-identity.md), and it gives up cross-SDK compatibility by diff --git a/docs/juce-module.md b/docs/juce-module.md index e1002e0..89c0872 100644 --- a/docs/juce-module.md +++ b/docs/juce-module.md @@ -119,7 +119,8 @@ ActivationDialog::show(config, [](bool wasActivated) { /* … */ }); `moonbase::licensing`. Network calls run on a controller-owned thread pool; all state changes and repaints happen on the message thread, gated by a generation counter so a slow request can never clobber a newer state. Destroying the controller cancels any -in-flight request and joins its workers, so nothing keeps running after teardown — you +in-flight request, stops waiting on another instance's license check, and joins its +workers, so nothing keeps running after teardown: you can call `start()` straight from the editor constructor and destroy the editor at any time (plugin scanning, pluginval, rapid open/close) without deferring or guarding it. The screens: diff --git a/include/moonbase/detail/file_lock.hpp b/include/moonbase/detail/file_lock.hpp index a70c292..7285c23 100644 --- a/include/moonbase/detail/file_lock.hpp +++ b/include/moonbase/detail/file_lock.hpp @@ -1,10 +1,14 @@ #pragma once +#include +#include #include +#include #include #include #include #include +#include #include #include "moonbase/errors.hpp" @@ -34,7 +38,7 @@ namespace moonbase::detail { // POSIX: flock(LOCK_EX) on the file's descriptor. Locks are per-open-file // description and cooperative; only callers that go through this class // observe each other. -// Windows: CRT _locking(_LK_LOCK) on a single-byte range. The range is +// Windows: CRT _locking on a single-byte range. The range is // required by the API but otherwise arbitrary; all callers using this class // lock the same range. _locking accepts ranges past EOF, so we deliberately // do not _chsize_s the file — SetEndOfFile on a sibling handle would race @@ -42,10 +46,18 @@ namespace moonbase::detail { // This intentionally avoids in public SDK headers because // store.hpp includes this detail header. // -// Throws moonbase::storage_error on unrecoverable open/lock failures. +// The lock is requested without blocking and retried every few milliseconds +// while someone else holds it, which can be for the length of their network +// check. A blocking request could not be called off, so a plugin closing +// while it waited would hold up the host until that check finished. Setting +// `give_up` ends the wait instead, with storage_error. No time limit +// otherwise: the holder lets go once its own request returns or times out. +// +// Throws moonbase::storage_error on unrecoverable open/lock failures, and +// when it gives up waiting. class file_lock { public: - explicit file_lock(const std::filesystem::path& path) + file_lock(const std::filesystem::path& path, const std::atomic& give_up) { const auto parent = path.parent_path(); if (!parent.empty()) { @@ -70,15 +82,6 @@ class file_lock { + std::generic_category().message(open_error)); } fd_ = fd; - - if (::_locking(fd_, _LK_LOCK, lock_range_size_) != 0) { - const auto err = errno; - ::_close(fd_); - fd_ = -1; - throw storage_error( - "Could not acquire license file lock: " - + std::generic_category().message(err)); - } #else fd_ = ::open(path.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0644); if (fd_ < 0) { @@ -87,16 +90,17 @@ class file_lock { "Could not open license lock file: " + std::generic_category().message(err)); } +#endif - if (::flock(fd_, LOCK_EX) != 0) { - const auto err = errno; - ::close(fd_); - fd_ = -1; - throw storage_error( - "Could not acquire license file lock: " - + std::generic_category().message(err)); + auto pause = std::chrono::milliseconds(1); + while (!try_lock()) { + if (give_up.load(std::memory_order_acquire)) { + release(); + throw storage_error("Stopped waiting for the license file lock"); + } + std::this_thread::sleep_for(pause); + pause = std::min(pause * 2, max_pause_); } -#endif } file_lock(const file_lock&) = delete; @@ -121,6 +125,33 @@ class file_lock { ~file_lock() { release(); } private: + // True once the lock is ours, false while another holder has it. Closes the + // file and throws for any other failure. + bool try_lock() + { +#if defined(_WIN32) + if (::_locking(fd_, _LK_NBLCK, lock_range_size_) == 0) { + return true; + } + const auto err = errno; + if (err == EACCES) { + return false; + } +#else + if (::flock(fd_, LOCK_EX | LOCK_NB) == 0) { + return true; + } + const auto err = errno; + if (err == EWOULDBLOCK || err == EINTR) { + return false; + } +#endif + release(); + throw storage_error( + "Could not acquire license file lock: " + + std::generic_category().message(err)); + } + void release() noexcept { #if defined(_WIN32) @@ -142,6 +173,7 @@ class file_lock { #if defined(_WIN32) static constexpr long lock_range_size_ = 1; #endif + static constexpr std::chrono::milliseconds max_pause_{20}; int fd_ = -1; }; diff --git a/include/moonbase/store.hpp b/include/moonbase/store.hpp index 4fc5c36..73f148c 100644 --- a/include/moonbase/store.hpp +++ b/include/moonbase/store.hpp @@ -1,6 +1,9 @@ #pragma once +#include +#include #include +#include #include #include #include @@ -8,6 +11,13 @@ #include #include #include +#include + +#if defined(_WIN32) +#include +#else +#include +#endif #include @@ -143,6 +153,18 @@ class file_license_store : public license_store { } } + const auto contents = nlohmann::json(value).dump(2); + + // Write the new license beside the old one and rename it into place, so + // a reader sees one or the other and never a half-written file, which + // load_local_license would delete as corrupt. That holds for readers + // that don't take the lock, such as an instance loading at startup. + if (replace_with_temp_file(contents)) { + return; + } + + // Where that fails, as in a folder that takes no new files but lets the + // license itself be rewritten, write it in place. errno = 0; std::ofstream file(path_, std::ios::trunc); if (!file) { @@ -150,7 +172,7 @@ class file_license_store : public license_store { throw storage_error("Could not open local license file for writing: " + utf8_path(path_) + errno_suffix(err)); } - file << nlohmann::json(value).dump(2); + file << contents; file.flush(); if (!file) { throw storage_error("Could not write local license file: " + utf8_path(path_)); @@ -178,10 +200,115 @@ class file_license_store : public license_store { // process tree. auto lock_path = path_; lock_path += ".lock"; - return std::make_unique(lock_path); + return std::make_unique(lock_path, *lock_waits_cancelled_); + } + + // For shutting down: from here on lock_for_update() throws storage_error + // instead of waiting for a lock that another instance or process holds, and + // a call already waiting gives up the same way. A free lock is still taken. + // Without this, an instance closing while a sibling re-validates waits for + // that check, which offline can run until its request times out. Callable + // from any thread; it cannot be undone. + void cancel_lock_waits() noexcept + { + lock_waits_cancelled_->store(true, std::memory_order_release); } private: + // Writes contents to a new file beside the license and renames it over the + // license. False, with the new file removed again, if any step fails or the + // rename would change more than the contents. + bool replace_with_temp_file(const std::string& contents) const + { + // Follow a symlinked license path to the file itself, so the link keeps + // pointing at the license instead of being replaced by a copy of it. + auto target = path_; + std::error_code link_error; + if (std::filesystem::is_symlink(path_, link_error)) { + target = std::filesystem::canonical(path_, link_error); + if (link_error) { + return false; // a dangling link: writing in place creates its target + } + } + + // No license yet is fine (status reports that as an error too). One also + // known by another name, a hard link, would keep the old contents there. + std::error_code status_error; + const auto existing = std::filesystem::status(target, status_error); + const bool exists = std::filesystem::exists(existing); + std::error_code links_error; + if (exists && std::filesystem::hard_link_count(target, links_error) != 1) { + return false; + } + + auto temp = target; + temp += temp_suffix(); + { + std::ofstream file(temp, std::ios::trunc); + if (!file) { + return false; + } + // Keep the license's permissions, so one restricted to its owner + // stays that way, and set them before the contents land. Only on + // POSIX: on Windows these are just the read-only flag. + std::error_code mode_error; +#if !defined(_WIN32) + if (exists) { + std::filesystem::permissions(temp, existing.permissions(), mode_error); + } +#endif + if (!mode_error) { + file << contents; + } + file.close(); + if (mode_error || !file) { + std::error_code ignored; + std::filesystem::remove(temp, ignored); + return false; + } + } + + // Windows won't replace a file that another handle has open, as a + // sibling reading the license does for a moment (and a virus scanner + // may for longer), so try a few times there before giving up. +#if defined(_WIN32) + constexpr int attempts = 10; +#else + constexpr int attempts = 1; +#endif + std::error_code ec; + auto pause = std::chrono::milliseconds(1); + for (int attempt = 1;; ++attempt) { + std::filesystem::rename(temp, target, ec); + if (!ec) { + return true; + } + if (attempt == attempts) { + break; + } + std::this_thread::sleep_for(pause); + pause = std::min(pause * 2, std::chrono::milliseconds(100)); + } + std::filesystem::remove(temp, ec); + return false; + } + + // Unique to one write, so writers never share a temporary file, whether in + // different processes, on different threads, or in two plugin binaries + // loaded into the same host. + static std::string temp_suffix() + { +#if defined(_WIN32) + const auto process = static_cast(::_getpid()); +#else + const auto process = static_cast(::getpid()); +#endif + static std::atomic sequence{0}; + const auto ticks = std::chrono::steady_clock::now().time_since_epoch().count(); + return "." + std::to_string(process) + "-" + std::to_string(ticks) + "-" + + std::to_string(sequence.fetch_add(1)) + ".tmp"; + } + // Best-effort OS reason for a failed std::fstream open. Stream failures don't // portably set errno, so callers reset errno to 0 first and we only append a // reason when something was actually recorded. @@ -205,13 +332,18 @@ class file_license_store : public license_store { class file_store_lock : public store_lock_guard { public: - explicit file_store_lock(const std::filesystem::path& path) : lock_(path) {} + file_store_lock(const std::filesystem::path& path, const std::atomic& give_up) + : lock_(path, give_up) + { + } private: detail::file_lock lock_; }; std::filesystem::path path_; + // Shared so the store stays copyable; copies cancel together. + std::shared_ptr> lock_waits_cancelled_ = std::make_shared>(false); }; } // namespace moonbase diff --git a/modules/moonbase_licensing/juce/ActivationController.cpp b/modules/moonbase_licensing/juce/ActivationController.cpp index 8027bf4..268b467 100644 --- a/modules/moonbase_licensing/juce/ActivationController.cpp +++ b/modules/moonbase_licensing/juce/ActivationController.cpp @@ -146,7 +146,7 @@ ActivationController::ActivationController(ActivationConfig config) try { licensing_ = std::make_shared( - config_.toLicensingOptions(), std::move(store), deviceIds, transport); + config_.toLicensingOptions(), store, deviceIds, transport); } catch (const std::exception& ex) { @@ -159,10 +159,14 @@ ActivationController::ActivationController(ActivationConfig config) inventory_.emplace(config_.toLicensingOptions(), inventoryTransport); - cancelInFlight_ = [transport, inventoryTransport] + cancelInFlight_ = [transport, inventoryTransport, store] { transport->cancel(); inventoryTransport->cancel(); + // A worker waiting its turn on the license lock while another instance + // re-validates stops waiting, rather than hold up the host until that + // check is done. + store->cancel_lock_waits(); }; // A custom resolver's device_name() is arbitrary consumer code, and this // constructor runs inside a plugin editor's constructor, where an escaping @@ -218,9 +222,10 @@ ActivationController::~ActivationController() stopTimer(); ++updateGeneration_; // drop any queued update-flow continuations updateDownload_.reset(); // cancels + joins the installer download thread - // Unblock any in-flight request, then wait for the workers to finish, so no - // detached thread keeps running module code after we (and possibly the - // plugin binary) are gone. cancelInFlight_ makes the drain near-instant. + // Unblock any in-flight request or wait for the license lock, then wait for + // the workers to finish, so no detached thread keeps running module code + // after we (and possibly the plugin binary) are gone. cancelInFlight_ makes + // the drain near-instant. if (cancelInFlight_) cancelInFlight_(); threadPool_.stop(); diff --git a/modules/moonbase_licensing/juce/ActivationController.h b/modules/moonbase_licensing/juce/ActivationController.h index 3f3a1fe..e0959b7 100644 --- a/modules/moonbase_licensing/juce/ActivationController.h +++ b/modules/moonbase_licensing/juce/ActivationController.h @@ -86,8 +86,9 @@ class ActivationController : private juce::Timer, // with fake store / transport / fingerprint. The primary constructor builds // the real dependencies from the config; this one takes them ready-made. // cancelInFlight (optional) is invoked on teardown to interrupt a blocking - // request so workers can't outlive the controller (the real path wires it to - // the HTTP transport's cancel()). + // request or lock wait so workers can't outlive the controller (the real path + // wires it to the HTTP transports' cancel() and the store's + // cancel_lock_waits()). ActivationController(ActivationConfig config, std::shared_ptr licensing, juce::String deviceName = "Test Device", diff --git a/modules/moonbase_licensing/juce/WorkerPool.h b/modules/moonbase_licensing/juce/WorkerPool.h index 1ef1be9..e639cd7 100644 --- a/modules/moonbase_licensing/juce/WorkerPool.h +++ b/modules/moonbase_licensing/juce/WorkerPool.h @@ -72,11 +72,10 @@ class WorkerPool // Drops the jobs that haven't started, then waits for the running ones and // joins the workers. Callers cancel what they can first (the controller - // cancels its HTTP requests), so this is normally instant. It never gives up - // on a worker, because one left running could still be executing module code - // after the plugin binary is unloaded. The longest it waits is a job blocked - // on the license lock while another process holds it for its own network - // check, which that process's request timeouts bound. Safe to call twice. + // cancels its HTTP requests and any wait for the license lock), so this is + // normally instant. It never gives up on a worker, because one left running + // could still be executing module code after the plugin binary is unloaded. + // Safe to call twice. void stop() { if (threads_.empty()) diff --git a/modules/moonbase_licensing/moonbase/detail/file_lock.hpp b/modules/moonbase_licensing/moonbase/detail/file_lock.hpp index a70c292..7285c23 100644 --- a/modules/moonbase_licensing/moonbase/detail/file_lock.hpp +++ b/modules/moonbase_licensing/moonbase/detail/file_lock.hpp @@ -1,10 +1,14 @@ #pragma once +#include +#include #include +#include #include #include #include #include +#include #include #include "moonbase/errors.hpp" @@ -34,7 +38,7 @@ namespace moonbase::detail { // POSIX: flock(LOCK_EX) on the file's descriptor. Locks are per-open-file // description and cooperative; only callers that go through this class // observe each other. -// Windows: CRT _locking(_LK_LOCK) on a single-byte range. The range is +// Windows: CRT _locking on a single-byte range. The range is // required by the API but otherwise arbitrary; all callers using this class // lock the same range. _locking accepts ranges past EOF, so we deliberately // do not _chsize_s the file — SetEndOfFile on a sibling handle would race @@ -42,10 +46,18 @@ namespace moonbase::detail { // This intentionally avoids in public SDK headers because // store.hpp includes this detail header. // -// Throws moonbase::storage_error on unrecoverable open/lock failures. +// The lock is requested without blocking and retried every few milliseconds +// while someone else holds it, which can be for the length of their network +// check. A blocking request could not be called off, so a plugin closing +// while it waited would hold up the host until that check finished. Setting +// `give_up` ends the wait instead, with storage_error. No time limit +// otherwise: the holder lets go once its own request returns or times out. +// +// Throws moonbase::storage_error on unrecoverable open/lock failures, and +// when it gives up waiting. class file_lock { public: - explicit file_lock(const std::filesystem::path& path) + file_lock(const std::filesystem::path& path, const std::atomic& give_up) { const auto parent = path.parent_path(); if (!parent.empty()) { @@ -70,15 +82,6 @@ class file_lock { + std::generic_category().message(open_error)); } fd_ = fd; - - if (::_locking(fd_, _LK_LOCK, lock_range_size_) != 0) { - const auto err = errno; - ::_close(fd_); - fd_ = -1; - throw storage_error( - "Could not acquire license file lock: " - + std::generic_category().message(err)); - } #else fd_ = ::open(path.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0644); if (fd_ < 0) { @@ -87,16 +90,17 @@ class file_lock { "Could not open license lock file: " + std::generic_category().message(err)); } +#endif - if (::flock(fd_, LOCK_EX) != 0) { - const auto err = errno; - ::close(fd_); - fd_ = -1; - throw storage_error( - "Could not acquire license file lock: " - + std::generic_category().message(err)); + auto pause = std::chrono::milliseconds(1); + while (!try_lock()) { + if (give_up.load(std::memory_order_acquire)) { + release(); + throw storage_error("Stopped waiting for the license file lock"); + } + std::this_thread::sleep_for(pause); + pause = std::min(pause * 2, max_pause_); } -#endif } file_lock(const file_lock&) = delete; @@ -121,6 +125,33 @@ class file_lock { ~file_lock() { release(); } private: + // True once the lock is ours, false while another holder has it. Closes the + // file and throws for any other failure. + bool try_lock() + { +#if defined(_WIN32) + if (::_locking(fd_, _LK_NBLCK, lock_range_size_) == 0) { + return true; + } + const auto err = errno; + if (err == EACCES) { + return false; + } +#else + if (::flock(fd_, LOCK_EX | LOCK_NB) == 0) { + return true; + } + const auto err = errno; + if (err == EWOULDBLOCK || err == EINTR) { + return false; + } +#endif + release(); + throw storage_error( + "Could not acquire license file lock: " + + std::generic_category().message(err)); + } + void release() noexcept { #if defined(_WIN32) @@ -142,6 +173,7 @@ class file_lock { #if defined(_WIN32) static constexpr long lock_range_size_ = 1; #endif + static constexpr std::chrono::milliseconds max_pause_{20}; int fd_ = -1; }; diff --git a/modules/moonbase_licensing/moonbase/store.hpp b/modules/moonbase_licensing/moonbase/store.hpp index 4fc5c36..73f148c 100644 --- a/modules/moonbase_licensing/moonbase/store.hpp +++ b/modules/moonbase_licensing/moonbase/store.hpp @@ -1,6 +1,9 @@ #pragma once +#include +#include #include +#include #include #include #include @@ -8,6 +11,13 @@ #include #include #include +#include + +#if defined(_WIN32) +#include +#else +#include +#endif #include @@ -143,6 +153,18 @@ class file_license_store : public license_store { } } + const auto contents = nlohmann::json(value).dump(2); + + // Write the new license beside the old one and rename it into place, so + // a reader sees one or the other and never a half-written file, which + // load_local_license would delete as corrupt. That holds for readers + // that don't take the lock, such as an instance loading at startup. + if (replace_with_temp_file(contents)) { + return; + } + + // Where that fails, as in a folder that takes no new files but lets the + // license itself be rewritten, write it in place. errno = 0; std::ofstream file(path_, std::ios::trunc); if (!file) { @@ -150,7 +172,7 @@ class file_license_store : public license_store { throw storage_error("Could not open local license file for writing: " + utf8_path(path_) + errno_suffix(err)); } - file << nlohmann::json(value).dump(2); + file << contents; file.flush(); if (!file) { throw storage_error("Could not write local license file: " + utf8_path(path_)); @@ -178,10 +200,115 @@ class file_license_store : public license_store { // process tree. auto lock_path = path_; lock_path += ".lock"; - return std::make_unique(lock_path); + return std::make_unique(lock_path, *lock_waits_cancelled_); + } + + // For shutting down: from here on lock_for_update() throws storage_error + // instead of waiting for a lock that another instance or process holds, and + // a call already waiting gives up the same way. A free lock is still taken. + // Without this, an instance closing while a sibling re-validates waits for + // that check, which offline can run until its request times out. Callable + // from any thread; it cannot be undone. + void cancel_lock_waits() noexcept + { + lock_waits_cancelled_->store(true, std::memory_order_release); } private: + // Writes contents to a new file beside the license and renames it over the + // license. False, with the new file removed again, if any step fails or the + // rename would change more than the contents. + bool replace_with_temp_file(const std::string& contents) const + { + // Follow a symlinked license path to the file itself, so the link keeps + // pointing at the license instead of being replaced by a copy of it. + auto target = path_; + std::error_code link_error; + if (std::filesystem::is_symlink(path_, link_error)) { + target = std::filesystem::canonical(path_, link_error); + if (link_error) { + return false; // a dangling link: writing in place creates its target + } + } + + // No license yet is fine (status reports that as an error too). One also + // known by another name, a hard link, would keep the old contents there. + std::error_code status_error; + const auto existing = std::filesystem::status(target, status_error); + const bool exists = std::filesystem::exists(existing); + std::error_code links_error; + if (exists && std::filesystem::hard_link_count(target, links_error) != 1) { + return false; + } + + auto temp = target; + temp += temp_suffix(); + { + std::ofstream file(temp, std::ios::trunc); + if (!file) { + return false; + } + // Keep the license's permissions, so one restricted to its owner + // stays that way, and set them before the contents land. Only on + // POSIX: on Windows these are just the read-only flag. + std::error_code mode_error; +#if !defined(_WIN32) + if (exists) { + std::filesystem::permissions(temp, existing.permissions(), mode_error); + } +#endif + if (!mode_error) { + file << contents; + } + file.close(); + if (mode_error || !file) { + std::error_code ignored; + std::filesystem::remove(temp, ignored); + return false; + } + } + + // Windows won't replace a file that another handle has open, as a + // sibling reading the license does for a moment (and a virus scanner + // may for longer), so try a few times there before giving up. +#if defined(_WIN32) + constexpr int attempts = 10; +#else + constexpr int attempts = 1; +#endif + std::error_code ec; + auto pause = std::chrono::milliseconds(1); + for (int attempt = 1;; ++attempt) { + std::filesystem::rename(temp, target, ec); + if (!ec) { + return true; + } + if (attempt == attempts) { + break; + } + std::this_thread::sleep_for(pause); + pause = std::min(pause * 2, std::chrono::milliseconds(100)); + } + std::filesystem::remove(temp, ec); + return false; + } + + // Unique to one write, so writers never share a temporary file, whether in + // different processes, on different threads, or in two plugin binaries + // loaded into the same host. + static std::string temp_suffix() + { +#if defined(_WIN32) + const auto process = static_cast(::_getpid()); +#else + const auto process = static_cast(::getpid()); +#endif + static std::atomic sequence{0}; + const auto ticks = std::chrono::steady_clock::now().time_since_epoch().count(); + return "." + std::to_string(process) + "-" + std::to_string(ticks) + "-" + + std::to_string(sequence.fetch_add(1)) + ".tmp"; + } + // Best-effort OS reason for a failed std::fstream open. Stream failures don't // portably set errno, so callers reset errno to 0 first and we only append a // reason when something was actually recorded. @@ -205,13 +332,18 @@ class file_license_store : public license_store { class file_store_lock : public store_lock_guard { public: - explicit file_store_lock(const std::filesystem::path& path) : lock_(path) {} + file_store_lock(const std::filesystem::path& path, const std::atomic& give_up) + : lock_(path, give_up) + { + } private: detail::file_lock lock_; }; std::filesystem::path path_; + // Shared so the store stays copyable; copies cancel together. + std::shared_ptr> lock_waits_cancelled_ = std::make_shared>(false); }; } // namespace moonbase diff --git a/tests/juce/controller_tests.cpp b/tests/juce/controller_tests.cpp index 56a1ffa..9ba1e23 100644 --- a/tests/juce/controller_tests.cpp +++ b/tests/juce/controller_tests.cpp @@ -2268,6 +2268,40 @@ TEST_CASE("destroying the controller mid-request cancels and joins without hangi CHECK(blocking->entered.load()); } +TEST_CASE("destroying the controller while another instance holds the license lock does not wait for it") +{ + controller_fixture fx; + auto claims = default_claims(); + claims["validated"] = now_seconds() - 3600; // past the throttle -> start() checks online, under the lock + fx.seedStored(fx.token(claims)); + fx.config.deviceIdResolver = fx.fingerprint; + + // Another instance holds the lock for its own online check, which offline + // can run until its request times out. Released after 3 s at the latest, so + // a controller that waits for it fails this test instead of hanging it. + moonbase::file_license_store sibling(compat::toFilesystemPath(fx.licenseFile)); + auto held = sibling.lock_for_update(); + juce::WaitableEvent tornDown; + std::thread releaser([&held, &tornDown] + { + tornDown.wait(3000); + held.reset(); + }); + + // The primary constructor, so the teardown runs exactly as in a plugin. + auto controller = std::make_unique(fx.config); + controller->start(); + pumpFor(300); // the worker is waiting for the lock by now + + const auto before = std::chrono::steady_clock::now(); + controller.reset(); + const auto teardown = std::chrono::steady_clock::now() - before; + tornDown.signal(); + releaser.join(); + + CHECK(teardown < std::chrono::milliseconds(1500)); +} + TEST_CASE("the worker pool runs every job it is given") { std::atomic ran{0}; diff --git a/tests/store_tests.cpp b/tests/store_tests.cpp index 1929877..528c80a 100644 --- a/tests/store_tests.cpp +++ b/tests/store_tests.cpp @@ -143,6 +143,86 @@ TEST_CASE("file_license_store removes an unparseable license file and reports no std::filesystem::remove(path, ec); } +TEST_CASE("file_license_store readers never catch the license half-written") +{ + // Readers don't take the lock (an instance loading its license at startup + // doesn't), and load_local_license deletes a file it can't parse. So a reader + // that caught a write half done would delete the license, and the customer + // would have to activate again. + const auto dir = std::filesystem::temp_directory_path() / ("moonbase-cpp-torn-" + unique_suffix()); + std::filesystem::create_directories(dir); + const auto path = dir / "license.mb"; + + auto small = sample_license(); + auto large = sample_license(); + large.token = std::string(64 * 1024, 'x'); // takes several writes, so a wide window + + file_license_store writer(path); + writer.store_local_license(small); + + std::atomic done{false}; + std::atomic reads{0}; + std::atomic misses{0}; + std::thread reading([&] { + file_license_store reader(path); + while (!done.load()) { + try { + if (!reader.load_local_license()) { + ++misses; + } + } catch (const storage_error&) { + ++misses; + } + ++reads; + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } + }); + + for (int i = 0; reads.load() < 100 && i < 100000; ++i) { + writer.store_local_license(i % 2 == 0 ? large : small); + } + done = true; + reading.join(); + + CHECK(reads.load() > 0); + CHECK(misses.load() == 0); + CHECK(std::filesystem::exists(path)); + + // Nothing left beside it but the license itself. + int entries = 0; + for (const auto& entry : std::filesystem::directory_iterator(dir)) { + (void) entry; + ++entries; + } + CHECK(entries == 1); + + std::error_code ec; + std::filesystem::remove_all(dir, ec); +} + +TEST_CASE("file_license_store keeps a hard-linked license shared") +{ + // Writing a new file and renaming it into place would replace only the name + // written through, leaving the other name on the old license. + const auto dir = std::filesystem::temp_directory_path() / ("moonbase-cpp-hardlink-" + unique_suffix()); + std::filesystem::create_directories(dir); + const auto path = dir / "license.mb"; + const auto alias = dir / "alias.mb"; + file_license_store(path).store_local_license(sample_license()); + std::filesystem::create_hard_link(path, alias); + + auto updated = sample_license(); + updated.id = "license-456"; + file_license_store(alias).store_local_license(updated); + + const auto loaded = file_license_store(path).load_local_license(); + REQUIRE(loaded.has_value()); + CHECK(loaded->id == "license-456"); + + std::error_code ec; + std::filesystem::remove_all(dir, ec); +} + TEST_CASE("file_license_store::lock_for_update serializes concurrent acquirers") { const auto path = std::filesystem::temp_directory_path() / @@ -290,6 +370,52 @@ TEST_CASE("file_license_store::lock_for_update survives delete_local_license") std::filesystem::remove(lock_path, ec); } +TEST_CASE("file_license_store::cancel_lock_waits ends a wait for a lock held elsewhere") +{ + const auto path = std::filesystem::temp_directory_path() / + ("moonbase-cpp-lock-cancel-" + unique_suffix() + ".json"); + + // Another instance holds the lock, as it does for its whole online check. + file_license_store sibling(path); + file_license_store store(path); + auto held = sibling.lock_for_update(); + + std::atomic finished{false}; + std::atomic gave_up{false}; + std::thread waiting([&] { + try { + auto guard = store.lock_for_update(); + } catch (const storage_error&) { + gave_up = true; + } + finished = true; + }); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); // waiting by now + + store.cancel_lock_waits(); + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(2); + while (!finished.load() && std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + CHECK(finished.load()); + CHECK(gave_up.load()); + held.reset(); // a waiter that ignored the cancel finishes now, rather than hang the run + waiting.join(); + + // Later calls don't wait for a lock that is taken either... + held = sibling.lock_for_update(); + CHECK_THROWS_AS((void) store.lock_for_update(), storage_error); + // ...but still take one that is free. + held.reset(); + CHECK(store.lock_for_update() != nullptr); + + std::error_code ec; + std::filesystem::remove(path, ec); + auto lock_path = path; + lock_path += ".lock"; + std::filesystem::remove(lock_path, ec); +} + TEST_CASE("memory_license_store::lock_for_update blocks concurrent mutations") { // Regression: the default store used to return nullptr from @@ -479,4 +605,88 @@ TEST_CASE("file_license_store reports a storage_error with the path when the dir std::error_code ec; std::filesystem::remove_all(dir, ec); } + +TEST_CASE("file_license_store keeps a license restricted to its owner that way") +{ + const auto dir = std::filesystem::temp_directory_path() / ("moonbase-cpp-mode-" + unique_suffix()); + std::filesystem::create_directories(dir); + const auto path = dir / "license.mb"; + + // A umask that leaves new files readable by everyone, as most do. + const auto previous_umask = ::umask(022); + file_license_store store(path); + store.store_local_license(sample_license()); + REQUIRE(::chmod(path.c_str(), 0600) == 0); + + store.store_local_license(sample_license()); + ::umask(previous_umask); + + struct stat info {}; + REQUIRE(::stat(path.c_str(), &info) == 0); + CHECK((info.st_mode & 0777) == 0600); + + std::error_code ec; + std::filesystem::remove_all(dir, ec); +} + +TEST_CASE("file_license_store writes through a symlinked license path") +{ + // The link must keep pointing at the license, not be replaced by a copy + // while what it pointed at keeps the old one. + const auto dir = std::filesystem::temp_directory_path() / ("moonbase-cpp-symlink-" + unique_suffix()); + std::filesystem::create_directories(dir); + const auto target = dir / "target.mb"; + const auto link = dir / "license.mb"; + file_license_store(target).store_local_license(sample_license()); + std::filesystem::create_symlink("target.mb", link); // relative, as links often are + + auto updated = sample_license(); + updated.id = "license-456"; + file_license_store(link).store_local_license(updated); + + CHECK(std::filesystem::is_symlink(link)); + const auto loaded = file_license_store(target).load_local_license(); + REQUIRE(loaded.has_value()); + CHECK(loaded->id == "license-456"); + + int entries = 0; + for (const auto& entry : std::filesystem::directory_iterator(dir)) { + (void) entry; + ++entries; + } + CHECK(entries == 2); // the link and its target, no leftovers + + std::error_code ec; + std::filesystem::remove_all(dir, ec); +} + +TEST_CASE("file_license_store rewrites the license in place in a folder that takes no new files") +{ + if (::geteuid() == 0) + return; + + const auto dir = std::filesystem::temp_directory_path() / + ("moonbase-cpp-fixed-dir-" + unique_suffix()); + std::filesystem::create_directories(dir); + const auto path = dir / "license.mb"; + { + file_license_store seed(path); + seed.store_local_license(sample_license()); + } + // The license stays writable, but nothing new can be created beside it, so + // there is nowhere to write a replacement first. + REQUIRE(::chmod(dir.c_str(), 0500) == 0); + + file_license_store store(path); + auto updated = sample_license(); + updated.id = "license-456"; + CHECK_NOTHROW(store.store_local_license(updated)); + const auto loaded = store.load_local_license(); + REQUIRE(loaded.has_value()); + CHECK(loaded->id == "license-456"); + + ::chmod(dir.c_str(), 0700); + std::error_code ec; + std::filesystem::remove_all(dir, ec); +} #endif // _WIN32