From 6b15e992165623d6185b29384a2dca2419cdf36c Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 15:27:06 -0500 Subject: [PATCH 01/20] Add checked startup config file transactions --- docs/CONFIG_FILE_TRANSACTIONS.md | 73 ++++++ mods/src/config.cc | 90 ++++--- mods/src/config.h | 16 +- mods/src/file_transaction.cc | 397 +++++++++++++++++++++++++++++++ mods/src/file_transaction.h | 30 +++ tests/file_transaction_test.cc | 126 ++++++++++ 6 files changed, 698 insertions(+), 34 deletions(-) create mode 100644 docs/CONFIG_FILE_TRANSACTIONS.md create mode 100644 mods/src/file_transaction.cc create mode 100644 mods/src/file_transaction.h create mode 100644 tests/file_transaction_test.cc diff --git a/docs/CONFIG_FILE_TRANSACTIONS.md b/docs/CONFIG_FILE_TRANSACTIONS.md new file mode 100644 index 000000000..3c0143f2f --- /dev/null +++ b/docs/CONFIG_FILE_TRANSACTIONS.md @@ -0,0 +1,73 @@ +# Checked startup config output + +`file_transaction::Write` is a synchronous local-file primitive. It is used by +Config's private `SaveStartup` for its two existing startup outputs. It is not a +runtime settings API or a substitute for a future asynchronous coordinator. + +Initial user-config creation uses CreateOnly. A concurrent creator wins without +being overwritten. Generated runtime-vars output uses ReplaceSnapshot. TOML is +serialized and parsed before file staging; the existing warning header and Windows +text-mode line endings are retained. Save failure is reported without aborting +initialization. Existing user-config documents are not rewritten by this change. + +Transactions resolve supported symlink targets, reject hard-linked/non-regular +targets, attempt a cooperative sibling `.lock` once, and stage at most4 MiB in an +exclusively created same-directory transaction folder. Lock files remain in place; +their lifetime is separate from OS lock ownership. Staging names are bounded and +exclusive, not secure by secrecy. Windows transaction folders restrict access to +owner/administrators/SYSTEM; macOS creates them with mode0700. Unexpected staging +collisions are never opened or truncated. + +Writes, flush and close are checked. Windows existing-file replacement uses +ReplaceFileW with an owned backup and without ignore-ACL-error flags; new files +use a move without replacement. macOS preserves metadata using fcopyfile and uses +same-filesystem rename, with exclusive rename for creation and a parent-directory +fsync after commit. Platform documentation is not a power-loss test. + +Results distinguish NotCommitted, Conflict, Busy, Committed, +DurabilityUnverified and RecoveryRequired. Committed means the supported local +operation completed, not guaranteed survival of every hardware/power failure. +Windows's unsupported ReplaceFile write-through flag is not used. A post-commit +directory-flush error does not pretend the write failed before commit. + +Documented Windows partial replacement failures retain the transaction folder and +its old/new files for recovery. Cleanup touches only owned filenames, never sweeps +a directory, and records cleanup errors. There is no automatic startup recovery or +rollback into a destination another writer may have changed. An interruption may +leave a private staging folder; recovery/retention coordination is required before +this primitive supports live user-document edits. + +## Boundaries + +- Callers supply trusted paths already routed by File::MakePath. This is not an + arbitrary-path sandbox or a destination registry. Windows UNC/device/alternate + stream destinations are rejected in this first local-filesystem implementation. +- Canonicalization and no-follow checks are useful validation, not exclusion of an + adversary swapping parent directories or links. Uncooperative same-permission + writers remain outside the guarantee. Only participants using the same lock are + serialized; canonical alias/case behavior still needs platform fixtures. +- No conflict-aware edits, schema merge, bounded runtime queue, UI completion, + shutdown coordinator, cloud persistence or log-writer replacement is introduced. +- Mod-state PR267 retains its existing API/implementation. Adoption of these shared + primitives is a separate explicit migration after platform validation. + +## Tests + +Run the isolated standalone fixture (never against a real game config): + +```powershell +clang++ -std=c++23 -Wall -Wextra -Werror -I mods/src tests/file_transaction_test.cc -o file_transaction_test.exe -ladvapi32 +./file_transaction_test.exe +``` + +On macOS omit `-ladvapi32` and use the native compiler. The fixture covers create +races, replacement, partial staged output, injected pre-commit failure boundaries, +post-commit uncertainty, size/path rejection and hard links. Windows additionally +tests held locks/targets and emulates the documented1177 partial-replacement +postcondition, checking that the old copy survives. This is not a naturally +triggered OS1177 or power-loss test. Symlink creation explicitly reports a skip +when unavailable. Production builds contain no fault-injection interface. + +Required follow-up evidence includes macOS native execution, permission/ACL +preservation, alias contention, interrupted-process recovery and storage behavior +under real faults. Runtime latency/queue tests belong to the asynchronous phase. diff --git a/mods/src/config.cc b/mods/src/config.cc index 25086345e..152360d4a 100644 --- a/mods/src/config.cc +++ b/mods/src/config.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -91,35 +92,55 @@ Config::Config() Load(); } -void Config::Save(const toml::table& config, const std::string_view filename, bool apply_warning) +file_transaction::Result Config::SaveStartup(const toml::table& config, const std::string_view filename, + bool apply_warning, file_transaction::Mode mode) { - std::ofstream config_file; - - auto config_path = File::MakePath(filename, true); - config_file.open(config_path); - - if (apply_warning) { - char defaultFile[255], configFile[255]; - snprintf(defaultFile, 255, "%s", File::Default()); - snprintf(configFile, 255, "%s", File::Config()); - - config_file << "#######################################################################\n"; - config_file << "#######################################################################\n"; - config_file << "#### ####\n"; - config_file << "#### NOTE: This file is not the configuration file that is used ####\n"; - config_file << "#### by the STFC Community Mod. It is provided to help ####\n"; - config_file << "#### see what configuration is being used by the runtime ####\n"; - config_file << "#### and any desired settings should be copied to the same ####\n"; - config_file << "#### section in: " << defaultFile << "\n"; - config_file << "#### ####\n"; - config_file << "#### Config in: " << configFile << "\n"; - config_file << "#### ####\n"; - config_file << "#######################################################################\n"; - config_file << "#######################################################################\n\n"; - } + using namespace file_transaction; + try { + std::ostringstream config_file; + config_file.exceptions(std::ios::badbit | std::ios::failbit); + if (apply_warning) { + char defaultFile[255], configFile[255]; + snprintf(defaultFile, 255, "%s", File::Default()); + snprintf(configFile, 255, "%s", File::Config()); + + config_file << "#######################################################################\n"; + config_file << "#######################################################################\n"; + config_file << "#### ####\n"; + config_file << "#### NOTE: This file is not the configuration file that is used ####\n"; + config_file << "#### by the STFC Community Mod. It is provided to help ####\n"; + config_file << "#### see what configuration is being used by the runtime ####\n"; + config_file << "#### and any desired settings should be copied to the same ####\n"; + config_file << "#### section in: " << defaultFile << "\n"; + config_file << "#### ####\n"; + config_file << "#### Config in: " << configFile << "\n"; + config_file << "#### ####\n"; + config_file << "#######################################################################\n"; + config_file << "#######################################################################\n\n"; + } - config_file << config; - config_file.close(); + config_file << config; + auto bytes = config_file.str(); + (void)toml::parse(bytes); +#if _WIN32 + // Match the former ofstream text-mode output, including the warning header. + std::string windows_bytes; + windows_bytes.reserve(bytes.size()); + for (char c : bytes) { + if (c == '\n') + windows_bytes += '\r'; + windows_bytes += c; + } + bytes = std::move(windows_bytes); +#endif + return Write(std::filesystem::path(File::MakePath(filename, true)), bytes, mode); + } catch (const toml::parse_error&) { + return {State::NotCommitted, Stage::Validate, std::make_error_code(std::errc::invalid_argument), {}}; + } catch (const std::filesystem::filesystem_error& error) { + return {State::NotCommitted, Stage::Resolve, error.code(), {}}; + } catch (...) { + return {State::NotCommitted, Stage::Validate, std::make_error_code(std::errc::io_error), {}}; + } } Config& Config::Get() @@ -1414,12 +1435,23 @@ void Config::Load() spdlog::debug(""); + const auto report_save = [](const char* purpose, const file_transaction::Result& result) { + using namespace file_transaction; + if (result.state == State::Committed && !result.error) + return; + spdlog::warn("[Config] {} save: {} at {} (error={}); initialization continues", purpose, + Name(result.state), Name(result.stage), result.error.value()); + if (!result.recovery_directory.empty()) + spdlog::warn("[Config] Retained transaction directory '{}' beside the destination; do not discard before recovery", + result.recovery_directory.filename().string()); + }; + if (!std::filesystem::exists(File::MakePath(File::Config()))) { message.str(""); message << "Creating " << File::Config() << " (default config file)"; spdlog::warn(message.str()); - Config::Save(parsed, File::Config(), false); + report_save("initial config", SaveStartup(parsed, File::Config(), false, file_transaction::Mode::CreateOnly)); } message.str(""); @@ -1434,7 +1466,7 @@ void Config::Load() std::filesystem::remove(FILE_DEF_PARSED); } - Config::Save(parsed, File::Vars()); + report_save("runtime vars", SaveStartup(parsed, File::Vars(), true, file_transaction::Mode::ReplaceSnapshot)); std::cout << "\n\n-----------------------------\n\n" << parsed << "\n\n-----------------------------\nVersion " diff --git a/mods/src/config.h b/mods/src/config.h index cd9d4de6d..6d3d8bca9 100644 --- a/mods/src/config.h +++ b/mods/src/config.h @@ -1,4 +1,5 @@ #pragma once +#include "file_transaction.h" #include #include @@ -144,11 +145,16 @@ class Config final [[nodiscard]] static HWND WindowHandle(); #endif - static void Save(const toml::table& config, std::string_view filename, bool apply_warning = true); - void Load(); - void AdjustUiScale(bool scaleUp); - void AdjustUiShipScale(bool scaleUp); - void AdjustUiViewerScale(bool scaleUp); +private: + // Startup-only whole-table output; runtime/user edits require the future coordinator. + static file_transaction::Result SaveStartup(const toml::table& config, std::string_view filename, bool apply_warning, + file_transaction::Mode mode); + +public: + void Load(); + void AdjustUiScale(bool scaleUp); + void AdjustUiShipScale(bool scaleUp); + void AdjustUiViewerScale(bool scaleUp); [[nodiscard]] MissionHudVisibility MissionHudButtonVisibility(std::string_view button_name) const; [[nodiscard]] bool MissionHudTweaksEnabled() const; diff --git a/mods/src/file_transaction.cc b/mods/src/file_transaction.cc new file mode 100644 index 000000000..524534ef5 --- /dev/null +++ b/mods/src/file_transaction.cc @@ -0,0 +1,397 @@ +#include "file_transaction.h" + +#include +#include +#include +#include +#include +#include +#include + +#if _WIN32 +#include +#include +#pragma comment(lib, "advapi32.lib") +#elif __APPLE__ +#include +#include +#include +#include +#include +#include +#endif + +namespace file_transaction +{ +const char* Name(State state) noexcept +{ + switch (state) { + case State::NotCommitted: + return "not-committed"; + case State::Conflict: + return "conflict"; + case State::Busy: + return "busy"; + case State::Committed: + return "committed"; + case State::DurabilityUnverified: + return "durability-unverified"; + case State::RecoveryRequired: + return "recovery-required"; + } + return "unknown"; +} +const char* Name(Stage stage) noexcept +{ + switch (stage) { + case Stage::Validate: + return "validate"; + case Stage::Resolve: + return "resolve"; + case Stage::Lock: + return "lock"; + case Stage::StageFile: + return "stage"; + case Stage::Write: + return "write"; + case Stage::Flush: + return "flush"; + case Stage::Close: + return "close"; + case Stage::Commit: + return "commit"; + case Stage::DirectoryFlush: + return "directory-flush"; + case Stage::Cleanup: + return "cleanup"; + } + return "unknown"; +} + +namespace +{ + namespace fs = std::filesystem; + constexpr std::size_t MaxBytes = 4 * 1024 * 1024; + std::atomic_uint64_t sequence{0}; + +#ifdef MOD_FILE_TRANSACTION_TESTING + extern bool InjectFailure(Stage); + extern bool InjectPartialReplace(); +#else + bool InjectFailure(Stage) + { return false; } +#endif + + struct Failure { + std::error_code error; + State state = State::NotCommitted; + }; + void Fail(std::errc error, State state = State::NotCommitted) + { throw Failure{std::make_error_code(error), state}; } + void Checkpoint(Stage stage) + { + if (InjectFailure(stage)) + Fail(std::errc::io_error); + } + +#if _WIN32 + struct Handle { + HANDLE value = INVALID_HANDLE_VALUE; + ~Handle() + { + if (value != INVALID_HANDLE_VALUE) + CloseHandle(value); + } + Handle() = default; + Handle(const Handle&) = delete; + bool valid() const + { return value != INVALID_HANDLE_VALUE; } + void Close() + { + auto old = value; + value = INVALID_HANDLE_VALUE; + if (old != INVALID_HANDLE_VALUE && !CloseHandle(old)) + throw Failure{{static_cast(GetLastError()), std::system_category()}}; + } + }; + void WinFail(State state = State::NotCommitted) + { throw Failure{{static_cast(GetLastError()), std::system_category()}, state}; } + struct PrivateSecurity { + PSECURITY_DESCRIPTOR descriptor = nullptr; + SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), nullptr, FALSE}; + PrivateSecurity() + { + // Restrict staged data to its owner, administrators and SYSTEM. + if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(L"D:P(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;OW)", + SDDL_REVISION_1, &descriptor, nullptr)) + WinFail(); + attributes.lpSecurityDescriptor = descriptor; + } + ~PrivateSecurity() + { + if (descriptor) + LocalFree(descriptor); + } + }; + void Regular(HANDLE handle) + { + BY_HANDLE_FILE_INFORMATION info{}; + if (!GetFileInformationByHandle(handle, &info)) + WinFail(); + if ((info.dwFileAttributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT)) || info.nNumberOfLinks != 1) + Fail(std::errc::operation_not_supported); + } +#elif __APPLE__ + struct Handle { + int value = -1; + ~Handle() + { + if (value >= 0) + close(value); + } + Handle() = default; + Handle(const Handle&) = delete; + bool valid() const + { return value >= 0; } + void Close() + { + auto old = value; + value = -1; + if (old >= 0 && close(old) != 0) + throw Failure{{errno, std::generic_category()}}; + } + }; + void PosixFail(State state = State::NotCommitted) + { throw Failure{{errno, std::generic_category()}, state}; } + void Regular(int descriptor) + { + struct stat info{}; + if (fstat(descriptor, &info)) + PosixFail(); + if (!S_ISREG(info.st_mode) || info.st_nlink != 1) + Fail(std::errc::operation_not_supported); + } +#endif +} // namespace + +Result Write(const fs::path& destination, std::string_view bytes, Mode mode) +{ + Result result; + fs::path transaction, payload, backup; + bool preserve = false; + // Cleanup never sweeps a directory. Only these owned names can be removed. + auto cleanup = [&] { + if (transaction.empty()) + return; + if (preserve) { + result.recovery_directory = transaction; + return; + } + std::error_code first, ec; + for (const auto& path : {payload, backup, transaction}) { + if (path.empty()) + continue; + fs::remove(path, ec); + if (ec && !first) + first = ec; + } + if (first) { + result.recovery_directory = transaction; + if (!result.error) { + result.error = first; + result.stage = Stage::Cleanup; + } + } + }; + try { + if (bytes.size() > MaxBytes || destination.empty()) + Fail(std::errc::invalid_argument); + const auto raw = destination.native(); + if (raw.find(typename fs::path::value_type{}) != decltype(raw)::npos) + Fail(std::errc::invalid_argument); + result.stage = Stage::Resolve; + auto target = fs::weakly_canonical(fs::absolute(destination)); +#if _WIN32 + // Config paths may be user-selected, but device/UNC/alternate-stream writes + // are outside this first local-filesystem transaction contract. + const auto native = target.native(); + if (native.rfind(L"\\\\", 0) == 0 || native.find(L':', 2) != std::wstring::npos) + Fail(std::errc::operation_not_supported); +#endif + if (!fs::is_directory(target.parent_path())) + Fail(std::errc::not_a_directory); + result.stage = Stage::Lock; + Checkpoint(result.stage); +#if _WIN32 || __APPLE__ + Handle lock, existing, staged; + auto lockPath = target; + lockPath += ".lock"; +#if _WIN32 + PrivateSecurity security; + lock.value = CreateFileW(lockPath.c_str(), GENERIC_READ | GENERIC_WRITE, 0, &security.attributes, OPEN_ALWAYS, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + if (!lock.valid()) { + auto error = GetLastError(); + WinFail(error == ERROR_SHARING_VIOLATION || error == ERROR_LOCK_VIOLATION ? State::Busy : State::NotCommitted); + } + Regular(lock.value); + existing.value = CreateFileW(target.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + if (!existing.valid() && GetLastError() != ERROR_FILE_NOT_FOUND) + WinFail(); + if (existing.valid()) + Regular(existing.value); +#else + lock.value = open(lockPath.c_str(), O_CREAT | O_RDWR | O_NOFOLLOW | O_CLOEXEC, 0600); + if (!lock.valid()) + PosixFail(); + Regular(lock.value); + if (flock(lock.value, LOCK_EX | LOCK_NB)) + PosixFail(errno == EWOULDBLOCK || errno == EAGAIN ? State::Busy : State::NotCommitted); + existing.value = open(target.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC); + if (!existing.valid() && errno != ENOENT) + PosixFail(); + if (existing.valid()) + Regular(existing.value); +#endif + const bool existed = existing.valid(); + if (existed && mode == Mode::CreateOnly) + Fail(std::errc::file_exists, State::Conflict); + result.stage = Stage::StageFile; + Checkpoint(result.stage); + for (int attempt = 0; attempt < 16; ++attempt) { + auto candidate = target.parent_path() / (".stfc-save-" + std::to_string(++sequence)); +#if _WIN32 + candidate += "-" + std::to_string(GetCurrentProcessId()); + if (CreateDirectoryW(candidate.c_str(), &security.attributes)) { + transaction = candidate; + break; + } + if (GetLastError() != ERROR_ALREADY_EXISTS) + WinFail(); +#else + candidate += "-" + std::to_string(getpid()); + if (mkdir(candidate.c_str(), 0700) == 0) { + transaction = candidate; + break; + } + if (errno != EEXIST) + PosixFail(); +#endif + } + if (transaction.empty()) + Fail(std::errc::file_exists); + payload = transaction / "new"; + backup = transaction / "previous"; +#if _WIN32 + staged.value = CreateFileW(payload.c_str(), GENERIC_READ | GENERIC_WRITE, 0, &security.attributes, CREATE_NEW, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + if (!staged.valid()) + WinFail(); +#else + staged.value = open(payload.c_str(), O_CREAT | O_EXCL | O_RDWR | O_NOFOLLOW | O_CLOEXEC, 0600); + if (!staged.valid()) + PosixFail(); +#endif + result.stage = Stage::Write; + // Deliberately stage a prefix before the injected failure to test cleanup of + // genuinely partial output, not merely a rejected request. + std::size_t offset = 0; + while (offset < bytes.size()) { + auto length = std::min(4096, bytes.size() - offset); +#if _WIN32 + DWORD written = 0; + if (!WriteFile(staged.value, bytes.data() + offset, static_cast(length), &written, nullptr)) + WinFail(); + if (!written) + Fail(std::errc::io_error); +#else + auto written = write(staged.value, bytes.data() + offset, length); + if (written < 0 && errno == EINTR) + continue; + if (written < 0) + PosixFail(); + if (!written) + Fail(std::errc::io_error); +#endif + offset += written; + Checkpoint(result.stage); + } +#if __APPLE__ + if (existed && fcopyfile(existing.value, staged.value, nullptr, COPYFILE_METADATA)) + PosixFail(); +#endif + result.stage = Stage::Flush; + Checkpoint(result.stage); +#if _WIN32 + if (!FlushFileBuffers(staged.value)) + WinFail(); +#else + if (fsync(staged.value)) + PosixFail(); +#endif + result.stage = Stage::Close; + staged.Close(); + Checkpoint(result.stage); + existing.Close(); + result.stage = Stage::Commit; + Checkpoint(result.stage); +#if _WIN32 + if (existed) { + preserve = true; + BOOL replaced; +#ifdef MOD_FILE_TRANSACTION_TESTING + if (InjectPartialReplace()) { + if (!MoveFileExW(target.c_str(), backup.c_str(), 0)) + WinFail(State::RecoveryRequired); + SetLastError(ERROR_UNABLE_TO_MOVE_REPLACEMENT_2); + replaced = FALSE; + } else +#endif + replaced = ReplaceFileW(target.c_str(), payload.c_str(), backup.c_str(), 0, nullptr, nullptr); + if (!replaced) { + const auto error = GetLastError(); + std::error_code inspectionError; + const bool backupExists = fs::exists(backup, inspectionError); + preserve = error == ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 || backupExists || inspectionError; + throw Failure{{static_cast(error), std::system_category()}, + preserve ? State::RecoveryRequired : State::NotCommitted}; + } + preserve = false; + } else if (!MoveFileExW(payload.c_str(), target.c_str(), MOVEFILE_WRITE_THROUGH)) { + const auto error = GetLastError(); + WinFail(error == ERROR_ALREADY_EXISTS || error == ERROR_FILE_EXISTS ? State::Conflict : State::NotCommitted); + } +#else + if (renamex_np(payload.c_str(), target.c_str(), existed ? 0 : RENAME_EXCL)) + PosixFail(errno == EEXIST ? State::Conflict : State::NotCommitted); +#endif + result.state = State::Committed; + result.stage = Stage::DirectoryFlush; +#if __APPLE__ + Handle parent; + parent.value = open(target.parent_path().c_str(), O_RDONLY | O_DIRECTORY | O_CLOEXEC); + if (!parent.valid() || fsync(parent.value)) { + result.state = State::DurabilityUnverified; + result.error = {errno, std::generic_category()}; + } +#endif + if (InjectFailure(Stage::DirectoryFlush)) { + result.state = State::DurabilityUnverified; + result.error = std::make_error_code(std::errc::io_error); + } +#else + Fail(std::errc::operation_not_supported); +#endif + } catch (const Failure& failure) { + result.state = failure.state; + result.error = failure.error; + } catch (const fs::filesystem_error& failure) { + result.error = failure.code(); + } catch (...) { + result.error = std::make_error_code(std::errc::io_error); + } + cleanup(); + return result; +} +} // namespace file_transaction diff --git a/mods/src/file_transaction.h b/mods/src/file_transaction.h new file mode 100644 index 000000000..12484cba0 --- /dev/null +++ b/mods/src/file_transaction.h @@ -0,0 +1,30 @@ +#pragma once + +#include +#include +#include + +namespace file_transaction +{ +enum class Mode { CreateOnly, ReplaceSnapshot }; +enum class State { NotCommitted, Conflict, Busy, Committed, DurabilityUnverified, RecoveryRequired }; +enum class Stage { Validate, Resolve, Lock, StageFile, Write, Flush, Close, Commit, DirectoryFlush, Cleanup }; + +struct Result { + State state = State::NotCommitted; + Stage stage = Stage::Validate; + std::error_code error; + // Nonempty only when transaction-owned files were retained for inspection/recovery. + std::filesystem::path recovery_directory; + bool committed() const noexcept + { return state == State::Committed || state == State::DurabilityUnverified; } +}; + +// Synchronous storage primitive for trusted, already-routed local destinations. +// Not a runtime/UI API, revision-aware editor, or sandbox for arbitrary paths. +// Preserves supported symlink targets; rejects hard-linked/non-regular destinations. +// Uses a cooperative sibling lock. Uncooperative external writers are not excluded. +[[nodiscard]] Result Write(const std::filesystem::path& destination, std::string_view bytes, Mode mode); +const char* Name(State state) noexcept; +const char* Name(Stage stage) noexcept; +} // namespace file_transaction diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc new file mode 100644 index 000000000..df94a18d3 --- /dev/null +++ b/tests/file_transaction_test.cc @@ -0,0 +1,126 @@ +#define MOD_FILE_TRANSACTION_TESTING +#include "file_transaction.cc" + +#include +#include +#include +#include +#include + +namespace file_transaction +{ +namespace +{ + thread_local std::optional failureStage; + thread_local bool partialReplace = false; + bool InjectFailure(Stage stage) + { return failureStage == stage; } +[[maybe_unused]] bool InjectPartialReplace() + { return partialReplace; } +} // namespace +} // namespace file_transaction + +std::string Read(const std::filesystem::path& path) +{ + std::ifstream in(path, std::ios::binary); + return {std::istreambuf_iterator(in), std::istreambuf_iterator()}; +} + +int main() +{ + namespace fs = std::filesystem; + using namespace file_transaction; + auto root = + fs::temp_directory_path() + / ("stfc-transaction-test-" + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count())); + assert(fs::create_directory(root)); + const auto path = root / "settings.toml"; + auto result = Write(path, "value = 1\n", Mode::CreateOnly); + assert(result.state == State::Committed && !result.error); + assert(Read(path) == "value = 1\n"); + assert(Write(path, "value = 2\n", Mode::CreateOnly).state == State::Conflict); + assert(Read(path) == "value = 1\n"); + + for (auto stage : {Stage::Lock, Stage::StageFile, Stage::Write, Stage::Flush, Stage::Close, Stage::Commit}) { + failureStage = stage; + result = Write(path, std::string(9000, 'x'), Mode::ReplaceSnapshot); + failureStage.reset(); + assert(result.state == State::NotCommitted && result.error && result.stage == stage); + assert(Read(path) == "value = 1\n"); + assert(result.recovery_directory.empty()); + for (const auto& entry : fs::directory_iterator(root)) + assert(entry.path().filename().string().rfind(".stfc-save-", 0) != 0); + } + + result = Write(path, "value = 2\n", Mode::ReplaceSnapshot); + assert(result.state == State::Committed && Read(path) == "value = 2\n"); + failureStage = Stage::DirectoryFlush; + result = Write(path, "value = 3\n", Mode::ReplaceSnapshot); + failureStage.reset(); + assert(result.state == State::DurabilityUnverified && result.committed()); + assert(Read(path) == "value = 3\n"); + + // Two create-only writers must never replace the winner, even when both began + // from an absent destination. Busy is an explicit rejection, not accepted work. + const auto race = root / "race.toml"; + Result a, b; + std::thread first([&] { a = Write(race, "a", Mode::CreateOnly); }); + std::thread second([&] { b = Write(race, "b", Mode::CreateOnly); }); + first.join(); + second.join(); + assert(a.committed() != b.committed()); + assert(Read(race) == (a.committed() ? "a" : "b")); + + assert(!Write(root / "missing" / "bad.toml", "x", Mode::CreateOnly).committed()); + assert(!Write(path, std::string(4 * 1024 * 1024 + 1, 'x'), Mode::ReplaceSnapshot).committed()); + assert(Read(path) == "value = 3\n"); + + // A hard link must not silently split into independent state files on replace. + const auto hard = root / "hard.toml"; + fs::create_hard_link(path, hard); + assert(!Write(hard, "bad", Mode::ReplaceSnapshot).committed()); + assert(Read(path) == "value = 3\n"); + fs::remove(hard); + +#if _WIN32 + auto lockPath = path; + lockPath += ".lock"; + auto held = CreateFileW(lockPath.c_str(), GENERIC_READ | GENERIC_WRITE, 0, nullptr, OPEN_EXISTING, 0, nullptr); + assert(held != INVALID_HANDLE_VALUE); + assert(Write(path, "bad", Mode::ReplaceSnapshot).state == State::Busy); + CloseHandle(held); + auto targetHeld = CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, 0, nullptr); + assert(targetHeld != INVALID_HANDLE_VALUE); + result = Write(path, "bad", Mode::ReplaceSnapshot); + assert(!result.committed() && Read(path) == "value = 3\n"); + CloseHandle(targetHeld); + // Emulate the documented Windows1177 postcondition, not just a pre-commit + // exception. The only old copy must survive cleanup in the recovery directory. + partialReplace = true; + result = Write(path, "new candidate", Mode::ReplaceSnapshot); + partialReplace = false; + assert(result.state == State::RecoveryRequired && !result.recovery_directory.empty()); + assert(!fs::exists(path)); + assert(Read(result.recovery_directory / "previous") == "value = 3\n"); + assert(Read(result.recovery_directory / "new") == "new candidate"); + fs::rename(result.recovery_directory / "previous", path); + fs::remove(result.recovery_directory / "new"); + fs::remove(result.recovery_directory); +#endif + // Link creation can require developer mode/privilege on Windows; explicitly + // report the skip instead of claiming the alias fixture ran. + std::error_code linkError; + const auto alias = root / "alias.toml"; + fs::create_symlink(path, alias, linkError); + if (!linkError) { + assert(Write(alias, "value = 4\n", Mode::ReplaceSnapshot).committed()); + assert(fs::is_symlink(alias) && Read(path) == "value = 4\n"); + } else { + std::cout << "SKIP symlink fixture: " << linkError.message() << '\n'; + } + // Only this exclusively created test directory is recursively removed. + assert(fs::canonical(root).parent_path() == fs::canonical(fs::temp_directory_path())); + assert(fs::canonical(root).filename() == root.filename()); + fs::remove_all(root); + std::cout << "PASS checked file transactions and pre-commit fault fixtures\n"; +} From 7f09fe0050c5471af92451fde55b889c61cd48b1 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 15:31:40 -0500 Subject: [PATCH 02/20] Reject macOS FIFO save targets without blocking --- docs/CONFIG_FILE_TRANSACTIONS.md | 2 ++ mods/src/file_transaction.cc | 5 ++-- tests/file_transaction_test.cc | 44 +++++++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/docs/CONFIG_FILE_TRANSACTIONS.md b/docs/CONFIG_FILE_TRANSACTIONS.md index 3c0143f2f..37786ac3a 100644 --- a/docs/CONFIG_FILE_TRANSACTIONS.md +++ b/docs/CONFIG_FILE_TRANSACTIONS.md @@ -67,6 +67,8 @@ tests held locks/targets and emulates the documented1177 partial-replacement postcondition, checking that the old copy survives. This is not a naturally triggered OS1177 or power-loss test. Symlink creation explicitly reports a skip when unavailable. Production builds contain no fault-injection interface. +On macOS, FIFO destination and lock fixtures run in child processes with a +three-second deadline, verifying rejection without blocking on a FIFO peer. Required follow-up evidence includes macOS native execution, permission/ACL preservation, alias contention, interrupted-process recovery and storage behavior diff --git a/mods/src/file_transaction.cc b/mods/src/file_transaction.cc index 524534ef5..d2eafe773 100644 --- a/mods/src/file_transaction.cc +++ b/mods/src/file_transaction.cc @@ -242,13 +242,14 @@ Result Write(const fs::path& destination, std::string_view bytes, Mode mode) if (existing.valid()) Regular(existing.value); #else - lock.value = open(lockPath.c_str(), O_CREAT | O_RDWR | O_NOFOLLOW | O_CLOEXEC, 0600); + // Validate descriptors without waiting for a FIFO peer or acquiring a tty. + lock.value = open(lockPath.c_str(), O_CREAT | O_RDWR | O_NOFOLLOW | O_CLOEXEC | O_NONBLOCK | O_NOCTTY, 0600); if (!lock.valid()) PosixFail(); Regular(lock.value); if (flock(lock.value, LOCK_EX | LOCK_NB)) PosixFail(errno == EWOULDBLOCK || errno == EAGAIN ? State::Busy : State::NotCommitted); - existing.value = open(target.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC); + existing.value = open(target.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC | O_NONBLOCK | O_NOCTTY); if (!existing.valid() && errno != ENOENT) PosixFail(); if (existing.valid()) diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc index df94a18d3..09e1a67e1 100644 --- a/tests/file_transaction_test.cc +++ b/tests/file_transaction_test.cc @@ -7,6 +7,11 @@ #include #include +#if __APPLE__ +#include +#include +#endif + namespace file_transaction { namespace @@ -15,7 +20,7 @@ namespace thread_local bool partialReplace = false; bool InjectFailure(Stage stage) { return failureStage == stage; } -[[maybe_unused]] bool InjectPartialReplace() + [[maybe_unused]] bool InjectPartialReplace() { return partialReplace; } } // namespace } // namespace file_transaction @@ -82,6 +87,43 @@ int main() assert(Read(path) == "value = 3\n"); fs::remove(hard); +#if __APPLE__ + // A nonregular destination (or lock) must be rejected before waiting for a + // FIFO peer. Bound the child so a regression fails instead of hanging CI. + for (bool lockFixture : {false, true}) { + const auto fifoTarget = root / (lockFixture ? "fifo-lock.toml" : "fifo.toml"); + auto fifo = fifoTarget; + if (lockFixture) + fifo += ".lock"; + assert(mkfifo(fifo.c_str(), 0600) == 0); + const auto child = fork(); + assert(child >= 0); + if (child == 0) { + const auto rejected = Write(fifoTarget, "bad", Mode::ReplaceSnapshot); + _exit(rejected.state == State::NotCommitted + && rejected.error == std::make_error_code(std::errc::operation_not_supported) + ? 0 : 1); + } + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(3); + int status = 0; + pid_t waited; + do { + waited = waitpid(child, &status, WNOHANG); + if (waited == child || (waited < 0 && errno != EINTR)) + break; + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } while (std::chrono::steady_clock::now() < deadline); + if (waited != child) { + kill(child, SIGKILL); + while (waitpid(child, &status, 0) < 0 && errno == EINTR) {} + assert(false && "FIFO rejection exceeded deadline"); + } + assert(WIFEXITED(status) && WEXITSTATUS(status) == 0); + assert(fs::is_fifo(fifo)); + fs::remove(fifo); + } +#endif + #if _WIN32 auto lockPath = path; lockPath += ".lock"; From 44b1549100bcddad7b03bb368e7d76c199de85f6 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 15:39:47 -0500 Subject: [PATCH 03/20] Add bounded snapshot save scheduling core --- docs/SNAPSHOT_SAVE_QUEUE.md | 85 +++++++++++++ mods/src/snapshot_save_queue.cc | 132 ++++++++++++++++++++ mods/src/snapshot_save_queue.h | 73 +++++++++++ tests/snapshot_save_queue_test.cc | 198 ++++++++++++++++++++++++++++++ 4 files changed, 488 insertions(+) create mode 100644 docs/SNAPSHOT_SAVE_QUEUE.md create mode 100644 mods/src/snapshot_save_queue.cc create mode 100644 mods/src/snapshot_save_queue.h create mode 100644 tests/snapshot_save_queue_test.cc diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md new file mode 100644 index 000000000..90fff6921 --- /dev/null +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -0,0 +1,85 @@ +# Bounded snapshot scheduling core + +`persistence::SnapshotSaveQueue` is an internal, single-destination scheduling +core for generated snapshots. It is not yet a runtime service and has no game +callers. User TOML editing, JSON schema updates, log appends, and cloud preferences +require separate adapters; they must not submit whole-document replacements here. + +## Admission and completion + +Trusted setup constructs one queue per canonical destination, outside gameplay. +The eventual host must enforce that unique ownership and a global budget across +destinations. Construction resolves the path and can throw. Submission takes a +revision and an owned string, never an arbitrary path or game object. + +`TrySubmit` uses a try-lock and performs no serialization, allocation, filesystem +work, or wait for capacity. A successful submission transfers the buffer; a +rejection leaves it intact. This bounds admission work, not preparation of the +snapshot: future adapters must keep serialization and large allocation off input +handlers as well. Neither this test nor a try-lock proves a frame-time budget. + +Limits are eight outstanding tickets, 4 MiB of content per request, and 8 MiB of +retained string capacity per queue (plus fixed slot/string overhead and backend +transaction allocations). In-flight payloads count until released. A string with +small content and huge reserved capacity cannot bypass the byte limit. Finished +results retain their slots until consumed, so an abandoned page cannot cause an +unbounded completion backlog or silently discard accepted outcomes. + +Admission returns Accepted(ticket), Busy, InvalidRequest, StaleRevision, or Stopping. +Accepted does not mean saved. Revision numbers are nonzero, strictly increasing +within this queue's lifetime, assigned by one adapter authority; they are snapshot +sequence numbers, not disk hashes or external-editor conflict detection. Failed +admission never advances the sequence. Retrying an accepted-but-failed snapshot +requires a fresh revision. Recreating the queue requires a new consumer session; +tickets from different instances must never be compared as global identities. + +Worker execution selects admitted tickets in order. It holds no admission lock +while executing storage or freeing payloads, and only one `RunOne` can execute at +a time. `TryTakeCompletion` moves a result to the polling owner without callbacks. +An empty poll can also mean transient lock contention. The future UI adapter must +consume results centrally and check its page/session generation before applying +any continuation; closing a page must not destroy the queue. + +## Stop and lifetime + +`RequestStop` closes admission without waiting for storage. A submission overlapping +stop may already have passed its admission check; it remains an accounted ticket +and is cancelled by the worker if it has not started. `RunOne` produces explicit +CancelledBeforeStart completions for queued requests after stop. Already selected +work is in-flight and reports its actual result, even if stop arrives meanwhile. +The host must keep pumping until queued cancellation is accounted for. + +RecoveryRequired (including an unexpected backend exception whose commit point is +unknown) closes admission and cancels later queued snapshots. Known pre-commit +failures and committed-but-durability-unverified results retain their distinct +outcomes; this layer does not retry or roll back automatically. + +This core owns no thread and registers no destructor/exit hook. Its owner must +keep it alive until producers, consumer and worker calls have ended. Destroying it +while another thread uses it is invalid. There is no implicit join, detach, timer, +poll loop, game hook, or claim of safe live unload in this patch. + +Inspection found no coordinated storage shutdown in the current host: Windows +`DllMain` has an empty process-detach branch; macOS's injection constructor has no +matching teardown handler. Existing sync threads are not a save lifecycle contract. +Before runtime wiring, choose and validate module/worker ownership, cancellation +wakeup, completion draining, and actual process-exit/unload behavior. A join under +the loader lock or detached worker is not an acceptable default. Indefinitely +stalled OS I/O prevents promising both bounded unload and guaranteed completion. + +## Isolated validation + +```powershell +clang++ -std=c++23 -Wall -Wextra -Werror -I mods/src tests/snapshot_save_queue_test.cc -o snapshot_save_queue_test.exe +./snapshot_save_queue_test.exe +``` + +The test backend never touches config files. It holds storage behind an explicit +barrier while exercising admission, polling, a second worker, and stop. It covers +count/capacity bounds, stale revisions, retained completions, retry identity, +queued cancellation versus in-flight completion, and recovery-required closure. +A concurrent producer/worker/consumer fixture checks 500 revisions through slot +reuse with exactly one ordered completion per admission. A watchdog terminates a +hung fixture. This validates scheduling logic, not native +storage durability, production frame latency, UI lifetime integration, or host +shutdown. Those remain separate gates, alongside native macOS transaction tests. diff --git a/mods/src/snapshot_save_queue.cc b/mods/src/snapshot_save_queue.cc new file mode 100644 index 000000000..9d69b6b60 --- /dev/null +++ b/mods/src/snapshot_save_queue.cc @@ -0,0 +1,132 @@ +#include "snapshot_save_queue.h" + +#include +#include +#include + +namespace persistence +{ +namespace +{ + std::filesystem::path Resolve(const std::filesystem::path& path) + { + if (path.empty() || path.native().find(std::filesystem::path::value_type{}) != path.native().npos) + throw std::invalid_argument("Invalid snapshot destination"); + return std::filesystem::weakly_canonical(std::filesystem::absolute(path)); + } +#ifdef MOD_SNAPSHOT_QUEUE_TESTING + file_transaction::Result Execute(const std::filesystem::path&, std::string_view); +#else + file_transaction::Result Execute(const std::filesystem::path& path, std::string_view bytes) + { return file_transaction::Write(path, bytes, file_transaction::Mode::ReplaceSnapshot); } +#endif +} // namespace + +SnapshotSaveQueue::SnapshotSaveQueue(const std::filesystem::path& trustedDestination) + : destination_(Resolve(trustedDestination)) +{ +} + +SnapshotSaveQueue::Submission SnapshotSaveQueue::TrySubmit(std::uint64_t revision, std::string&& bytes) +{ + if (stopping_.load()) + return {Admission::Stopping}; + if (!revision || bytes.size() > MaxPayload || bytes.capacity() > MaxRetainedBytes) + return {Admission::InvalidRequest}; + std::unique_lock lock(mutex_, std::try_to_lock); + if (!lock.owns_lock()) + return {Admission::Busy}; + if (stopping_.load()) + return {Admission::Stopping}; + if (revision <= lastRevision_) + return {Admission::StaleRevision}; + // Never wrap ticket/revision identity. A new owner/session is required. + if (nextTicket_ == std::numeric_limits::max()) + return {Admission::InvalidRequest}; + if (bytes.capacity() > MaxRetainedBytes - retainedBytes_) + return {Admission::Busy}; + for (auto& slot : slots_) { + if (slot.phase != Phase::Empty) + continue; + slot.retainedBytes = bytes.capacity(); + slot.bytes.swap(bytes); + slot.completion.ticket = nextTicket_++; + slot.completion.revision = revision; + slot.phase = Phase::Queued; + retainedBytes_ += slot.retainedBytes; + lastRevision_ = revision; + return {Admission::Accepted, slot.completion.ticket}; + } + return {Admission::Busy}; +} + +std::optional SnapshotSaveQueue::TryTakeCompletion() +{ + std::unique_lock lock(mutex_, std::try_to_lock); + if (!lock.owns_lock()) + return std::nullopt; + Slot* oldest = nullptr; + for (auto& slot : slots_) + if (slot.phase == Phase::Done && (!oldest || slot.completion.ticket < oldest->completion.ticket)) + oldest = &slot; + if (!oldest) + return std::nullopt; + auto completion = std::move(oldest->completion); + oldest->completion = {}; + oldest->phase = Phase::Empty; + return completion; +} + +void SnapshotSaveQueue::RequestStop() noexcept +{ stopping_.store(true); } + +bool SnapshotSaveQueue::RunOne() +{ + if (running_.test_and_set()) + return false; + struct Release { + std::atomic_flag& flag; + ~Release() + { flag.clear(); } + } release{running_}; + Slot* selected = nullptr; + bool cancelled; + std::string bytes; + { + std::lock_guard lock(mutex_); + for (auto& slot : slots_) + if (slot.phase == Phase::Queued && (!selected || slot.completion.ticket < selected->completion.ticket)) + selected = &slot; + if (!selected) + return false; + // Once selected, this request is in-flight; a later stop cannot claim to + // cancel an OS operation that may already have committed. + cancelled = stopping_.load(); + selected->phase = Phase::Running; + bytes.swap(selected->bytes); + } + file_transaction::Result result; + if (!cancelled) { + try { + result = Execute(destination_, bytes); + } catch (...) { + // An unexpected backend exception cannot establish whether commit occurred. + result.state = file_transaction::State::RecoveryRequired; + result.error = std::make_error_code(std::errc::io_error); + } + } + // Free potentially large buffers on the worker, outside the admission lock. + std::string{}.swap(bytes); + { + std::lock_guard lock(mutex_); + retainedBytes_ -= selected->retainedBytes; + selected->retainedBytes = 0; + selected->completion.outcome = cancelled ? Outcome::CancelledBeforeStart : Outcome::Executed; + selected->completion.result = std::move(result); + selected->phase = Phase::Done; + if (!cancelled && selected->completion.result.state == file_transaction::State::RecoveryRequired) + stopping_.store(true); + } + return true; +} +} // namespace persistence diff --git a/mods/src/snapshot_save_queue.h b/mods/src/snapshot_save_queue.h new file mode 100644 index 000000000..40e95d082 --- /dev/null +++ b/mods/src/snapshot_save_queue.h @@ -0,0 +1,73 @@ +#pragma once + +#include "file_transaction.h" + +#include +#include +#include +#include +#include +#include + +namespace persistence +{ +// Internal scheduling core for ONE trusted generated-output destination. This +// is not a user-TOML editor or a thread owner. Construct outside gameplay; the +// future host must own/join its worker before destroying this object or unloading. +class SnapshotSaveQueue final +{ +public: + static constexpr std::size_t MaxOutstanding = 8; + static constexpr std::size_t MaxPayload = 4 * 1024 * 1024; + static constexpr std::size_t MaxRetainedBytes = 8 * 1024 * 1024; + using Ticket = std::uint64_t; + enum class Admission { Accepted, Busy, InvalidRequest, StaleRevision, Stopping }; + struct Submission { + Admission state; + Ticket ticket = 0; + }; + enum class Outcome { Executed, CancelledBeforeStart }; + struct Completion { + Ticket ticket = 0; + std::uint64_t revision = 0; + Outcome outcome = Outcome::CancelledBeforeStart; + file_transaction::Result result; + }; + + explicit SnapshotSaveQueue(const std::filesystem::path& trustedDestination); + SnapshotSaveQueue(const SnapshotSaveQueue&) = delete; + SnapshotSaveQueue& operator=(const SnapshotSaveQueue&) = delete; + + // No serialization, filesystem work or mutex wait. Accepted alone moves bytes; + // rejection leaves the caller's buffer intact. Capacity counts against limits. + // Preparing a snapshot is the adapter's job and must also stay off hot paths. + [[nodiscard]] Submission TrySubmit(std::uint64_t revision, std::string&& bytes); + // Poll on the owning consumer thread. No callbacks or borrowed UI objects. + // Empty means no completion available OR transient lock contention; retry later. + [[nodiscard]] std::optional TryTakeCompletion(); + void RequestStop() noexcept; + + // Worker-only, synchronous; runs at most one transaction (or cancellation). + // Concurrent invocations do not overlap disk writes. False means no work or + // another worker is active. Stopping still requires pumping queued cancellations. + // This does not create a thread, wait for disk at teardown, or detach anything. + [[nodiscard]] bool RunOne(); + +private: + enum class Phase { Empty, Queued, Running, Done }; + struct Slot { + Phase phase = Phase::Empty; + std::string bytes; + std::size_t retainedBytes = 0; + Completion completion; + }; + const std::filesystem::path destination_; + std::array slots_; + std::mutex mutex_; + std::atomic_bool stopping_{false}; + std::atomic_flag running_ = ATOMIC_FLAG_INIT; + std::size_t retainedBytes_ = 0; + std::uint64_t lastRevision_ = 0; + Ticket nextTicket_ = 1; +}; +} // namespace persistence diff --git a/tests/snapshot_save_queue_test.cc b/tests/snapshot_save_queue_test.cc new file mode 100644 index 000000000..a62561185 --- /dev/null +++ b/tests/snapshot_save_queue_test.cc @@ -0,0 +1,198 @@ +#define MOD_SNAPSHOT_QUEUE_TESTING +#include "snapshot_save_queue.cc" + +#include +#include +#include +#include +#include +#include +#include + +namespace persistence +{ +namespace +{ + std::mutex backendMutex; + std::condition_variable backendChanged; + bool block = false, entered = false, allow = false, throwFailure = false; + file_transaction::State backendState = file_transaction::State::Committed; + std::vector writes; + + file_transaction::Result Execute(const std::filesystem::path&, std::string_view bytes) + { + std::unique_lock lock(backendMutex); + entered = true; + backendChanged.notify_all(); + if (block) + backendChanged.wait(lock, [] { return allow; }); + if (throwFailure) + throw std::runtime_error("backend failure"); + writes.emplace_back(bytes); + file_transaction::Result result; + result.state = backendState; + return result; + } +} // namespace +} // namespace persistence + +int main() +{ + // A blocking admission regression must fail the fixture, not hang CI forever. + std::promise finished; + std::thread watchdog([future = finished.get_future()] { + if (future.wait_for(std::chrono::seconds(15)) == std::future_status::timeout) + std::abort(); + }); + using namespace persistence; + using Queue = SnapshotSaveQueue; + using Admission = Queue::Admission; + // Backend is isolated in memory: no fixture writes to a real config. + const auto destination = std::filesystem::temp_directory_path() / "stfc-queue-fixture.vars"; + Queue queue(destination); + std::string invalid = "keep"; + assert(queue.TrySubmit(0, std::move(invalid)).state == Admission::InvalidRequest && invalid == "keep"); + std::string oversized(Queue::MaxPayload + 1, 'x'); + assert(queue.TrySubmit(1, std::move(oversized)).state == Admission::InvalidRequest); + std::string excessiveCapacity = "small"; + excessiveCapacity.reserve(Queue::MaxRetainedBytes + 1); + assert(queue.TrySubmit(1, std::move(excessiveCapacity)).state == Admission::InvalidRequest); + + // Hold storage indefinitely until the test releases it; admission/polling and + // stop must finish while the worker is still inside that backend. + auto first = queue.TrySubmit(1, std::string("first")); + assert(first.state == Admission::Accepted && first.ticket); + { + std::lock_guard lock(backendMutex); + block = true; + } + std::thread worker([&] { assert(queue.RunOne()); }); + { + std::unique_lock lock(backendMutex); + assert(backendChanged.wait_for(lock, std::chrono::seconds(3), [] { return entered; })); + } + assert(!queue.RunOne()); // No second disk transaction while the first is active. + assert(!queue.TryTakeCompletion()); + auto second = queue.TrySubmit(2, std::string("second")); + assert(second.state == Admission::Accepted); + std::string stale = "stale"; + assert(queue.TrySubmit(1, std::move(stale)).state == Admission::StaleRevision && stale == "stale"); + for (std::uint64_t revision = 3; revision <= Queue::MaxOutstanding; ++revision) + assert(queue.TrySubmit(revision, std::string("queued")).state == Admission::Accepted); + std::string rejected = "retry"; + assert(queue.TrySubmit(9, std::move(rejected)).state == Admission::Busy && rejected == "retry"); + queue.RequestStop(); + assert(queue.TrySubmit(9, std::move(rejected)).state == Admission::Stopping); + { + std::lock_guard lock(backendMutex); + allow = true; + backendChanged.notify_all(); + } + worker.join(); + auto completion = queue.TryTakeCompletion(); + assert(completion && completion->ticket == first.ticket && completion->result.committed()); + assert(completion->outcome == Queue::Outcome::Executed); + for (std::uint64_t revision = 2; revision <= Queue::MaxOutstanding; ++revision) { + assert(queue.RunOne()); + completion = queue.TryTakeCompletion(); + assert(completion && completion->revision == revision); + assert(completion->outcome == Queue::Outcome::CancelledBeforeStart); + } + assert(!queue.RunOne() && !queue.TryTakeCompletion()); + assert(writes == std::vector{"first"}); + block = false; + + // Finished-but-unconsumed results reserve their slot: closing a UI cannot + // silently discard an accepted write result or create an unbounded mailbox. + Queue mailbox(destination); + for (std::uint64_t revision = 1; revision <= Queue::MaxOutstanding; ++revision) { + assert(mailbox.TrySubmit(revision, std::string("done")).state == Admission::Accepted); + assert(mailbox.RunOne()); + } + assert(mailbox.TrySubmit(9, std::string("retry")).state == Admission::Busy); + assert(mailbox.TryTakeCompletion()->revision == 1); + assert(mailbox.TrySubmit(9, std::string("retry")).state == Admission::Accepted); + assert(mailbox.RunOne()); + for (std::uint64_t revision = 2; revision <= 9; ++revision) + assert(mailbox.TryTakeCompletion()->revision == revision); + + Queue byteBound(destination); + std::string large(Queue::MaxPayload, 'x'); + auto capacity = large.capacity(); + assert(byteBound.TrySubmit(1, std::move(large)).state == Admission::Accepted); + std::string another(Queue::MaxPayload, 'y'); + if (another.capacity() <= Queue::MaxRetainedBytes - capacity) { + assert(byteBound.TrySubmit(2, std::move(another)).state == Admission::Accepted); + assert(byteBound.TrySubmit(3, std::string("overflow")).state == Admission::Busy); + } else { + assert(byteBound.TrySubmit(2, std::move(another)).state == Admission::Busy); + } + assert(byteBound.RunOne()); + assert(byteBound.TrySubmit(3, std::string("fits now")).state == Admission::Accepted); + byteBound.RequestStop(); + while (byteBound.RunOne()) {} + while (byteBound.TryTakeCompletion()) {} + + // An uncertain backend failure stops later snapshots from replacing files + // whose recovery state needs inspection. Known failures remain retryable with + // a fresh revision; admission high-water never silently regresses. + Queue failures(destination); + backendState = file_transaction::State::NotCommitted; + assert(failures.TrySubmit(10, std::string("failed")).state == Admission::Accepted); + assert(failures.RunOne()); + assert(failures.TryTakeCompletion()->result.state == backendState); + assert(failures.TrySubmit(10, std::string("retry")).state == Admission::StaleRevision); + assert(failures.TrySubmit(11, std::string("retry")).state == Admission::Accepted); + assert(failures.TrySubmit(12, std::string("later")).state == Admission::Accepted); + throwFailure = true; + assert(failures.RunOne()); + assert(failures.TryTakeCompletion()->result.state == file_transaction::State::RecoveryRequired); + assert(failures.TrySubmit(13, std::string("blocked")).state == Admission::Stopping); + assert(failures.RunOne()); + assert(failures.TryTakeCompletion()->outcome == Queue::Outcome::CancelledBeforeStart); + assert(!failures.RunOne()); + + // Exercise slot reuse while producer, storage worker and completion consumer + // run concurrently. Every accepted revision must finish exactly once in order. + throwFailure = false; + backendState = file_transaction::State::Committed; + writes.clear(); + Queue concurrent(destination); + constexpr std::uint64_t count = 500; + std::atomic_bool consumed{false}; + std::thread storage([&] { + while (!consumed.load()) + if (!concurrent.RunOne()) + std::this_thread::yield(); + }); + std::thread consumer([&] { + for (std::uint64_t revision = 1; revision <= count;) { + auto done = concurrent.TryTakeCompletion(); + if (!done) { + std::this_thread::yield(); + continue; + } + assert(done->revision == revision && done->ticket == revision && done->result.committed()); + ++revision; + } + consumed.store(true); + }); + for (std::uint64_t revision = 1; revision <= count; ++revision) { + std::string payload = std::to_string(revision); + for (;;) { + auto admitted = concurrent.TrySubmit(revision, std::move(payload)); + if (admitted.state == Admission::Accepted) + break; + assert(admitted.state == Admission::Busy && payload == std::to_string(revision)); + std::this_thread::yield(); + } + } + consumer.join(); + storage.join(); + assert(writes.size() == count); + for (std::uint64_t revision = 1; revision <= count; ++revision) + assert(writes[revision - 1] == std::to_string(revision)); + finished.set_value(); + watchdog.join(); + std::cout << "PASS bounded snapshot admission, ordering, slow storage, completion retention and stop\n"; +} From 06d7dccf9fe2a710c011af49eaa78ddfa5167115 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 15:47:04 -0500 Subject: [PATCH 04/20] Add explicit snapshot worker lifecycle and native fixture CI --- .github/workflows/persistence-tests.yaml | 41 ++++++ docs/SNAPSHOT_SAVE_QUEUE.md | 51 +++++++ mods/src/snapshot_save_queue.cc | 7 + mods/src/snapshot_save_worker.cc | 78 +++++++++++ mods/src/snapshot_save_worker.h | 43 ++++++ tests/snapshot_save_worker_test.cc | 163 +++++++++++++++++++++++ 6 files changed, 383 insertions(+) create mode 100644 .github/workflows/persistence-tests.yaml create mode 100644 mods/src/snapshot_save_worker.cc create mode 100644 mods/src/snapshot_save_worker.h create mode 100644 tests/snapshot_save_worker_test.cc diff --git a/.github/workflows/persistence-tests.yaml b/.github/workflows/persistence-tests.yaml new file mode 100644 index 000000000..e9fb4d4a3 --- /dev/null +++ b/.github/workflows/persistence-tests.yaml @@ -0,0 +1,41 @@ +name: Persistence fixtures + +on: + pull_request: + push: + branches: [main, dev] + workflow_dispatch: + +permissions: + contents: read + +jobs: + native-fixtures: + strategy: + fail-fast: false + matrix: + runner: [windows-2025-vs2026, macos-26, macos-26-intel] + runs-on: ${{ matrix.runner }} + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7 + - name: Windows fixtures + if: runner.os == 'Windows' + shell: pwsh + run: | + foreach ($fixture in @('file_transaction', 'snapshot_save_queue', 'snapshot_save_worker')) { + clang++ -std=c++23 -Wall -Wextra -Werror -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test.exe" -ladvapi32 + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + & "./${fixture}_test.exe" + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + } + - name: macOS fixtures + if: runner.os == 'macOS' + shell: bash + env: + MACOSX_DEPLOYMENT_TARGET: '13.5' + run: | + for fixture in file_transaction snapshot_save_queue snapshot_save_worker; do + clang++ -std=c++23 -Wall -Wextra -Werror -pthread -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test" + "./${fixture}_test" + done diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index 90fff6921..b8612b3a6 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -83,3 +83,54 @@ reuse with exactly one ordered completion per admission. A watchdog terminates a hung fixture. This validates scheduling logic, not native storage durability, production frame latency, UI lifetime integration, or host shutdown. Those remain separate gates, alongside native macOS transaction tests. + +## Explicit worker owner + +`SnapshotSaveWorker` owns one queue and one joinable thread. It is an internal +component, not an application singleton; no game path constructs it yet. Trusted +construction resolves the destination and starts the worker, and may throw before +any request is accepted. Start it outside loader callbacks and input handling. +The host must limit owner instances and enforce unique destination enrollment; +this component does not create a registry or a globally bounded worker pool. + +Accepted admission wakes the worker with a coalesced atomic notification. There +is no idle polling or unbounded notification counter. The worker clears the wake +flag before draining, retaining a notification that arrives during or after the +drain. Only the worker waits. Producer admission uses try-locks; snapshot +preparation and actual platform notification latency still require measurement. + +`RequestStop` immediately closes both owner admission and queue selection, then +wakes an idle worker. The worker synchronizes with any submission already inside +admission before its final cancellation drain, preventing a late accepted ticket +from being left behind after worker exit. In-flight storage finishes normally; +pending work is cancelled, and all completions remain available for central +consumption after join. A settings page closing does not stop this owner. + +The sole lifecycle owner calls `StopAndJoin` explicitly outside gameplay and +loader callbacks. It may wait indefinitely for an in-flight OS call, and must not +run concurrently or on the worker. `WorkEnded` means queue execution ended, not +that the native thread has fully exited: it never authorizes destruction or +unload. Joining and quiescing all producer/consumer calls are required first. +Like `std::thread`, destroying an unjoined owner terminates; the destructor does +not silently detach or hide a blocking join. It must not be installed as a +page-local or static-destructor-managed service. + +The host integration gate remains open: identify a pre-teardown supervisor that +can join off the game thread while keeping the mod loaded, drain outcomes, and +validate process exit and explicit unload separately. A forced process exit is +an interruption with potentially retained staging, not a successful final flush. +Do not wire this owner into startup until that contract is satisfied on each +enabled platform. Windows process detach is too late for this coordination, as +other threads may already have been terminated. See [Microsoft's DllMain +contract](https://learn.microsoft.com/en-us/windows/win32/dlls/dllmain) and the +[C++ thread lifetime contract](https://eel.is/c%2B%2Bdraft/thread.thread.class). + +`tests/snapshot_save_worker_test.cc` exercises actual worker startup, stalled +storage and non-waiting stop, retained completions after join, 500 wake cycles, +and admission racing with stop. A test-only barrier also pauses admission after +its final stop check, proving a late accepted ticket is cancelled before join +returns. It uses an isolated backend and a watchdog, not +game files. Compile with the same standalone command as the queue fixture using +the worker test filename. The Persistence fixtures workflow runs all three +fixtures on native Windows, macOS ARM and macOS Intel; adding the workflow does +not itself count as a successful CI run. diff --git a/mods/src/snapshot_save_queue.cc b/mods/src/snapshot_save_queue.cc index 9d69b6b60..38b532625 100644 --- a/mods/src/snapshot_save_queue.cc +++ b/mods/src/snapshot_save_queue.cc @@ -8,6 +8,9 @@ namespace persistence { namespace { +#ifdef MOD_SNAPSHOT_QUEUE_ADMISSION_TESTING + void BeforeSnapshotEnqueue(); +#endif std::filesystem::path Resolve(const std::filesystem::path& path) { if (path.empty() || path.native().find(std::filesystem::path::value_type{}) != path.native().npos) @@ -38,6 +41,10 @@ SnapshotSaveQueue::Submission SnapshotSaveQueue::TrySubmit(std::uint64_t revisio return {Admission::Busy}; if (stopping_.load()) return {Admission::Stopping}; +#ifdef MOD_SNAPSHOT_QUEUE_ADMISSION_TESTING + // Pause after the final stop check to exercise admission overlapping shutdown. + BeforeSnapshotEnqueue(); +#endif if (revision <= lastRevision_) return {Admission::StaleRevision}; // Never wrap ticket/revision identity. A new owner/session is required. diff --git a/mods/src/snapshot_save_worker.cc b/mods/src/snapshot_save_worker.cc new file mode 100644 index 000000000..e1df659dd --- /dev/null +++ b/mods/src/snapshot_save_worker.cc @@ -0,0 +1,78 @@ +#include "snapshot_save_worker.h" + +#include + +namespace persistence +{ +SnapshotSaveWorker::SnapshotSaveWorker(const std::filesystem::path& trustedDestination) + : queue_(trustedDestination) + , worker_([this] { Run(); }) +{ +} + +SnapshotSaveQueue::Submission SnapshotSaveWorker::TrySubmit(std::uint64_t revision, std::string&& bytes) +{ + if (stopping_.load()) + return {SnapshotSaveQueue::Admission::Stopping}; + std::unique_lock lock(admission_, std::try_to_lock); + if (!lock.owns_lock()) + return {SnapshotSaveQueue::Admission::Busy}; + if (stopping_.load()) + return {SnapshotSaveQueue::Admission::Stopping}; + auto submitted = queue_.TrySubmit(revision, std::move(bytes)); + if (submitted.state == SnapshotSaveQueue::Admission::Accepted) + Wake(); + return submitted; +} + +std::optional SnapshotSaveWorker::TryTakeCompletion() +{ return queue_.TryTakeCompletion(); } + +void SnapshotSaveWorker::Wake() noexcept +{ + // Coalesced wakeups have no counter to overflow. The worker clears this BEFORE + // draining, so a submission during/after draining cannot lose its notification. + wake_.store(true); + wake_.notify_one(); +} + +void SnapshotSaveWorker::RequestStop() noexcept +{ + stopping_.store(true); + // Close queue selection immediately: queued work must not start merely because + // the active backend has not yet returned to the worker's outer loop. + queue_.RequestStop(); + Wake(); +} + +bool SnapshotSaveWorker::WorkEnded() const noexcept +{ return workEnded_.load(); } + +void SnapshotSaveWorker::StopAndJoin() +{ + RequestStop(); + if (worker_.joinable()) + worker_.join(); +} + +void SnapshotSaveWorker::Run() +{ + for (;;) { + wake_.wait(false); + wake_.store(false); + if (stopping_.load()) { + // Synchronize with a producer that passed its stop check before stop was + // requested. After this barrier no accepted ticket can appear behind the + // final cancellation drain. Only the worker waits for this short lock. + { + std::lock_guard lock(admission_); + queue_.RequestStop(); + } + while (queue_.RunOne()) {} + workEnded_.store(true); + return; + } + while (queue_.RunOne()) {} + } +} +} // namespace persistence diff --git a/mods/src/snapshot_save_worker.h b/mods/src/snapshot_save_worker.h new file mode 100644 index 000000000..3bf846ec6 --- /dev/null +++ b/mods/src/snapshot_save_worker.h @@ -0,0 +1,43 @@ +#pragma once + +#include "snapshot_save_queue.h" + +#include + +namespace persistence +{ +// Internal lifecycle owner, not a game singleton or settings-page member. +// Construct outside the loader lock. The sole lifecycle owner MUST call +// StopAndJoin outside gameplay/loader callbacks before destruction or unload. +// Destruction without joining terminates, matching std::thread; it never hides +// an unbounded join or detaches a worker that could outlive the loaded module. +class SnapshotSaveWorker final +{ +public: + explicit SnapshotSaveWorker(const std::filesystem::path& trustedDestination); + ~SnapshotSaveWorker() = default; + SnapshotSaveWorker(const SnapshotSaveWorker&) = delete; + SnapshotSaveWorker& operator=(const SnapshotSaveWorker&) = delete; + + [[nodiscard]] SnapshotSaveQueue::Submission TrySubmit(std::uint64_t revision, std::string&& bytes); + [[nodiscard]] std::optional TryTakeCompletion(); + void RequestStop() noexcept; + // Informational: true means queue execution has ended, NOT that the native + // thread has exited or that destroying this object is safe. Join is mandatory. + [[nodiscard]] bool WorkEnded() const noexcept; + // Sole lifecycle owner only; may wait indefinitely for an in-flight OS call. + // Idempotent after join. Never call concurrently or from the worker itself. + void StopAndJoin(); + +private: + void Wake() noexcept; + void Run(); + SnapshotSaveQueue queue_; + std::mutex admission_; + std::atomic_bool stopping_{false}; + std::atomic_bool wake_{false}; + std::atomic_bool workEnded_{false}; + // Last member: everything used by Run is initialized before thread launch. + std::thread worker_; +}; +} // namespace persistence diff --git a/tests/snapshot_save_worker_test.cc b/tests/snapshot_save_worker_test.cc new file mode 100644 index 000000000..2187b553b --- /dev/null +++ b/tests/snapshot_save_worker_test.cc @@ -0,0 +1,163 @@ +#define MOD_SNAPSHOT_QUEUE_TESTING +#define MOD_SNAPSHOT_QUEUE_ADMISSION_TESTING +#include "snapshot_save_queue.cc" +#include "snapshot_save_worker.cc" + +#include +#include +#include +#include +#include +#include + +namespace persistence +{ +namespace +{ + std::mutex admissionTestMutex; + std::condition_variable admissionChanged; + bool pauseAdmission = false, admissionEntered = false, allowAdmission = false; + void BeforeSnapshotEnqueue() + { + std::unique_lock lock(admissionTestMutex); + if (!pauseAdmission) + return; + admissionEntered = true; + admissionChanged.notify_all(); + admissionChanged.wait(lock, [] { return allowAdmission; }); + } + std::mutex backendMutex; + std::condition_variable backendChanged; + bool entered = false, allow = false, block = true; + std::vector writes; + file_transaction::Result Execute(const std::filesystem::path&, std::string_view bytes) + { + std::unique_lock lock(backendMutex); + entered = true; + backendChanged.notify_all(); + if (block) + backendChanged.wait(lock, [] { return allow; }); + writes.emplace_back(bytes); + file_transaction::Result result; + result.state = file_transaction::State::Committed; + return result; + } +} // namespace +} // namespace persistence + +int main() +{ + using namespace persistence; + using Admission = SnapshotSaveQueue::Admission; + using Outcome = SnapshotSaveQueue::Outcome; + std::promise finished; + std::thread watchdog([future = finished.get_future()] { + if (future.wait_for(std::chrono::seconds(15)) == std::future_status::timeout) + std::abort(); + }); + const auto destination = std::filesystem::temp_directory_path() / "stfc-worker-fixture.vars"; + { + SnapshotSaveWorker owner(destination); + auto first = owner.TrySubmit(1, std::string("first")); + assert(first.state == Admission::Accepted); + { + std::unique_lock lock(backendMutex); + assert(backendChanged.wait_for(lock, std::chrono::seconds(3), [] { return entered; })); + } + auto second = owner.TrySubmit(2, std::string("cancel")); + assert(second.state == Admission::Accepted); + owner.RequestStop(); + assert(!owner.WorkEnded()); + assert(owner.TrySubmit(3, std::string("late")).state == Admission::Stopping); + assert(!owner.TryTakeCompletion()); + { + std::lock_guard lock(backendMutex); + allow = true; + backendChanged.notify_all(); + } + owner.StopAndJoin(); + assert(owner.WorkEnded()); + auto done = owner.TryTakeCompletion(); + assert(done && done->ticket == first.ticket && done->outcome == Outcome::Executed && done->result.committed()); + done = owner.TryTakeCompletion(); + assert(done && done->ticket == second.ticket && done->outcome == Outcome::CancelledBeforeStart); + assert(!owner.TryTakeCompletion()); + owner.StopAndJoin(); // Explicit lifecycle operation is idempotent. + } + assert(writes == std::vector{"first"}); + block = false; + { + // Repeated empty-to-nonempty transitions exercise wake coalescing; no sleep + // is needed on the producer to make a notification visible to the worker. + SnapshotSaveWorker owner(destination); + for (std::uint64_t revision = 1; revision <= 500; ++revision) { + std::string bytes = std::to_string(revision); + for (;;) { + auto accepted = owner.TrySubmit(revision, std::move(bytes)); + if (accepted.state == Admission::Accepted) + break; + assert(accepted.state == Admission::Busy); + std::this_thread::yield(); + } + std::optional done; + while (!(done = owner.TryTakeCompletion())) + std::this_thread::yield(); + assert(done->revision == revision && done->result.committed()); + } + owner.StopAndJoin(); // Wakes an idle worker too. + assert(owner.WorkEnded()); + } + // Deterministically admit a ticket AFTER stop, by holding the producer just + // past the queue's final stop check. The worker must not exit ahead of it. + { + SnapshotSaveWorker owner(destination); + { + std::lock_guard lock(admissionTestMutex); + pauseAdmission = true; + } + SnapshotSaveQueue::Submission submitted{Admission::Busy}; + std::thread producer([&] { submitted = owner.TrySubmit(1, std::string("late ticket")); }); + { + std::unique_lock lock(admissionTestMutex); + assert(admissionChanged.wait_for(lock, std::chrono::seconds(3), [] { return admissionEntered; })); + } + owner.RequestStop(); + assert(!owner.WorkEnded()); + { + std::lock_guard lock(admissionTestMutex); + allowAdmission = true; + admissionChanged.notify_all(); + } + producer.join(); + owner.StopAndJoin(); + assert(submitted.state == Admission::Accepted); + auto done = owner.TryTakeCompletion(); + assert(done && done->ticket == submitted.ticket && done->outcome == Outcome::CancelledBeforeStart); + assert(!owner.TryTakeCompletion()); + pauseAdmission = false; + } + // Also exercise unforced scheduler races; all admitted work is accounted for. + for (int attempt = 0; attempt < 50; ++attempt) { + SnapshotSaveWorker owner(destination); + SnapshotSaveQueue::Submission submitted{Admission::Busy}; + std::atomic_bool go{false}; + std::thread producer([&] { + while (!go.load()) + std::this_thread::yield(); + submitted = owner.TrySubmit(1, std::string("racing")); + }); + go.store(true); + owner.RequestStop(); + producer.join(); + owner.StopAndJoin(); + auto done = owner.TryTakeCompletion(); + if (submitted.state == Admission::Accepted) + assert(done && done->ticket == submitted.ticket); + else + assert(!done && (submitted.state == Admission::Stopping || submitted.state == Admission::Busy)); + assert(!owner.TryTakeCompletion()); + } + finished.set_value(); + watchdog.join(); + std::cout << "PASS worker wakeup, stalled stop, cancellation, retained results and admission/stop races\n"; +} From 136e3a5597304df94c2240bca2de751cf4cfe074 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 15:53:37 -0500 Subject: [PATCH 05/20] Verify transaction permissions and canonical alias locking --- docs/CONFIG_FILE_TRANSACTIONS.md | 9 ++-- tests/file_transaction_test.cc | 80 ++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/docs/CONFIG_FILE_TRANSACTIONS.md b/docs/CONFIG_FILE_TRANSACTIONS.md index 37786ac3a..276358c87 100644 --- a/docs/CONFIG_FILE_TRANSACTIONS.md +++ b/docs/CONFIG_FILE_TRANSACTIONS.md @@ -70,6 +70,9 @@ when unavailable. Production builds contain no fault-injection interface. On macOS, FIFO destination and lock fixtures run in child processes with a three-second deadline, verifying rejection without blocking on a FIFO peer. -Required follow-up evidence includes macOS native execution, permission/ACL -preservation, alias contention, interrupted-process recovery and storage behavior -under real faults. Runtime latency/queue tests belong to the asynchronous phase. +Native fixture CI runs Windows and macOS ARM/Intel. Fixtures check preservation of +a restrictive Windows DACL and macOS mode/owner/group/extended attribute, plus +contention through a symlink while the canonical lock is held. These are specific +cases, not exhaustive ACL or filesystem coverage. Interrupted-process recovery +and storage behavior under real faults still require follow-up evidence. Runtime +latency/queue tests belong to the asynchronous phase. diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc index 09e1a67e1..510289527 100644 --- a/tests/file_transaction_test.cc +++ b/tests/file_transaction_test.cc @@ -10,6 +10,7 @@ #if __APPLE__ #include #include +#include #endif namespace file_transaction @@ -31,6 +32,30 @@ std::string Read(const std::filesystem::path& path) return {std::istreambuf_iterator(in), std::istreambuf_iterator()}; } +#if _WIN32 +std::pair Dacl(const std::filesystem::path& path) +{ + DWORD needed = 0; + GetFileSecurityW(path.c_str(), DACL_SECURITY_INFORMATION, nullptr, 0, &needed); + assert(needed); + std::vector descriptor(needed); + assert(GetFileSecurityW(path.c_str(), DACL_SECURITY_INFORMATION, descriptor.data(), needed, &needed)); + LPWSTR text = nullptr; + assert(ConvertSecurityDescriptorToStringSecurityDescriptorW(descriptor.data(), SDDL_REVISION_1, + DACL_SECURITY_INFORMATION, &text, nullptr)); + std::wstring result(text); + LocalFree(text); + SECURITY_DESCRIPTOR_CONTROL control{}; + DWORD revision = 0; + assert(GetSecurityDescriptorControl(descriptor.data(), &control, &revision)); + const auto entries = result.find(L'('); + assert(entries != std::wstring::npos); + // ReplaceFile may add the informational AUTO_INHERITED marker. Compare the + // actual ordered ACEs and protected-inheritance bit, not that history marker. + return {(control & SE_DACL_PROTECTED) != 0, result.substr(entries)}; +} +#endif + int main() { namespace fs = std::filesystem; @@ -87,12 +112,41 @@ int main() assert(Read(path) == "value = 3\n"); fs::remove(hard); + // Replace preserves deliberately restrictive destination metadata, rather + // than widening it to the staging file's defaults. + const auto metadata = root / "metadata.toml"; + assert(Write(metadata, "old", Mode::CreateOnly).committed()); +#if _WIN32 + PSECURITY_DESCRIPTOR restricted = nullptr; + assert( + ConvertStringSecurityDescriptorToSecurityDescriptorW(L"D:P(A;;FA;;;OW)", SDDL_REVISION_1, &restricted, nullptr)); + assert( + SetFileSecurityW(metadata.c_str(), DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, restricted)); + LocalFree(restricted); + const auto beforeDacl = Dacl(metadata); + assert(Write(metadata, "new", Mode::ReplaceSnapshot).committed()); + assert(Dacl(metadata) == beforeDacl && Read(metadata) == "new"); +#elif __APPLE__ + assert(chmod(metadata.c_str(), 0640) == 0); + constexpr const char* attribute = "com.stfc-mod.fixture"; + assert(setxattr(metadata.c_str(), attribute, "kept", 4, 0, 0) == 0); + struct stat beforeMetadata{}, afterMetadata{}; + assert(stat(metadata.c_str(), &beforeMetadata) == 0); + assert(Write(metadata, "new", Mode::ReplaceSnapshot).committed()); + assert(stat(metadata.c_str(), &afterMetadata) == 0); + assert(beforeMetadata.st_mode == afterMetadata.st_mode && beforeMetadata.st_uid == afterMetadata.st_uid + && beforeMetadata.st_gid == afterMetadata.st_gid); + char attributeValue[4]{}; + assert(getxattr(metadata.c_str(), attribute, attributeValue, sizeof(attributeValue), 0, 0) == 4); + assert(std::string_view(attributeValue, 4) == "kept" && Read(metadata) == "new"); +#endif + #if __APPLE__ // A nonregular destination (or lock) must be rejected before waiting for a // FIFO peer. Bound the child so a regression fails instead of hanging CI. for (bool lockFixture : {false, true}) { const auto fifoTarget = root / (lockFixture ? "fifo-lock.toml" : "fifo.toml"); - auto fifo = fifoTarget; + auto fifo = fifoTarget; if (lockFixture) fifo += ".lock"; assert(mkfifo(fifo.c_str(), 0600) == 0); @@ -102,11 +156,12 @@ int main() const auto rejected = Write(fifoTarget, "bad", Mode::ReplaceSnapshot); _exit(rejected.state == State::NotCommitted && rejected.error == std::make_error_code(std::errc::operation_not_supported) - ? 0 : 1); + ? 0 + : 1); } const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(3); - int status = 0; - pid_t waited; + int status = 0; + pid_t waited; do { waited = waitpid(child, &status, WNOHANG); if (waited == child || (waited < 0 && errno != EINTR)) @@ -155,6 +210,23 @@ int main() const auto alias = root / "alias.toml"; fs::create_symlink(path, alias, linkError); if (!linkError) { + auto canonicalLock = path; + canonicalLock += ".lock"; +#if _WIN32 + auto aliasLock = + CreateFileW(canonicalLock.c_str(), GENERIC_READ | GENERIC_WRITE, 0, nullptr, OPEN_EXISTING, 0, nullptr); + assert(aliasLock != INVALID_HANDLE_VALUE); +#elif __APPLE__ + auto aliasLock = open(canonicalLock.c_str(), O_RDWR | O_CLOEXEC); + assert(aliasLock >= 0 && flock(aliasLock, LOCK_EX | LOCK_NB) == 0); +#endif + assert(Write(alias, "must not write", Mode::ReplaceSnapshot).state == State::Busy); + assert(Read(path) == "value = 3\n"); +#if _WIN32 + CloseHandle(aliasLock); +#elif __APPLE__ + close(aliasLock); +#endif assert(Write(alias, "value = 4\n", Mode::ReplaceSnapshot).committed()); assert(fs::is_symlink(alias) && Read(path) == "value = 4\n"); } else { From acf08910ca0f5c053cc79d8e8e8f30caf6d1e124 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 16:23:42 -0500 Subject: [PATCH 06/20] Add explicit drain-accepted shutdown mode --- docs/SNAPSHOT_SAVE_QUEUE.md | 27 +++++++++--- mods/src/snapshot_save_queue.cc | 12 ++++-- mods/src/snapshot_save_queue.h | 7 ++- mods/src/snapshot_save_worker.cc | 16 +++---- mods/src/snapshot_save_worker.h | 5 ++- tests/snapshot_save_worker_test.cc | 68 +++++++++++++++++++++++++----- 6 files changed, 103 insertions(+), 32 deletions(-) diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index b8612b3a6..52a9f368b 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -42,13 +42,20 @@ any continuation; closing a page must not destroy the queue. ## Stop and lifetime -`RequestStop` closes admission without waiting for storage. A submission overlapping +`RequestStop(CancelQueued)` (the default) closes admission without waiting for storage. A submission overlapping stop may already have passed its admission check; it remains an accounted ticket and is cancelled by the worker if it has not started. `RunOne` produces explicit CancelledBeforeStart completions for queued requests after stop. Already selected work is in-flight and reports its actual result, even if stop arrives meanwhile. The host must keep pumping until queued cancellation is accounted for. +`RequestStop(DrainAccepted)` instead closes admission while finishing accepted +snapshots in ticket order. Every accepted ticket still gets its actual completion; +drain is not a promise that all writes succeed. Cancellation is sticky: an explicit +CancelQueued request, before or after a drain request, cancels work not yet selected. +A later drain request cannot re-enable those writes. RecoveryRequired also forces +cancellation, even during a drain; already selected storage reports its result. + RecoveryRequired (including an unexpected backend exception whose commit point is unknown) closes admission and cancels later queued snapshots. Known pre-commit failures and committed-but-durability-unverified results retain their distinct @@ -99,16 +106,17 @@ flag before draining, retaining a notification that arrives during or after the drain. Only the worker waits. Producer admission uses try-locks; snapshot preparation and actual platform notification latency still require measurement. -`RequestStop` immediately closes both owner admission and queue selection, then -wakes an idle worker. The worker synchronizes with any submission already inside +`RequestStop(mode)` immediately closes owner admission and, for CancelQueued, +queue selection, then wakes an idle worker. The worker synchronizes with any submission already inside admission before its final cancellation drain, preventing a late accepted ticket from being left behind after worker exit. In-flight storage finishes normally; -pending work is cancelled, and all completions remain available for central +pending work follows the requested drain/cancel mode, and all completions remain available for central consumption after join. A settings page closing does not stop this owner. -The sole lifecycle owner calls `StopAndJoin` explicitly outside gameplay and +The sole lifecycle owner calls `StopAndJoin(mode)` explicitly outside gameplay and loader callbacks. It may wait indefinitely for an in-flight OS call, and must not -run concurrently or on the worker. `WorkEnded` means queue execution ended, not +run concurrently or on the worker. The mode argument is required so joining an +earlier drain cannot silently select default cancellation. `WorkEnded` means queue execution ended, not that the native thread has fully exited: it never authorizes destruction or unload. Joining and quiescing all producer/consumer calls are required first. Like `std::thread`, destroying an unjoined owner terminates; the destructor does @@ -134,3 +142,10 @@ game files. Compile with the same standalone command as the queue fixture using the worker test filename. The Persistence fixtures workflow runs all three fixtures on native Windows, macOS ARM and macOS Intel; adding the workflow does not itself count as a successful CI run. + +Drain fixtures additionally hold the first write behind a barrier and verify that +the second accepted write finishes, that cancellation before/after drain wins, +and that known failure, durability uncertainty and recovery-required results keep +their distinct meaning. Late admission across the final stop check is tested in +both modes. This supplies a worker policy for future orderly quit integration; +it does not change F10, defer Unity quitting, or wire any game callbacks. diff --git a/mods/src/snapshot_save_queue.cc b/mods/src/snapshot_save_queue.cc index 38b532625..3f358c586 100644 --- a/mods/src/snapshot_save_queue.cc +++ b/mods/src/snapshot_save_queue.cc @@ -84,8 +84,12 @@ std::optional SnapshotSaveQueue::TryTakeCompletio return completion; } -void SnapshotSaveQueue::RequestStop() noexcept -{ stopping_.store(true); } +void SnapshotSaveQueue::RequestStop(StopMode mode) noexcept +{ + if (mode == StopMode::CancelQueued) + cancelQueued_.store(true); + stopping_.store(true); +} bool SnapshotSaveQueue::RunOne() { @@ -108,7 +112,7 @@ bool SnapshotSaveQueue::RunOne() return false; // Once selected, this request is in-flight; a later stop cannot claim to // cancel an OS operation that may already have committed. - cancelled = stopping_.load(); + cancelled = cancelQueued_.load(); selected->phase = Phase::Running; bytes.swap(selected->bytes); } @@ -132,7 +136,7 @@ bool SnapshotSaveQueue::RunOne() selected->completion.result = std::move(result); selected->phase = Phase::Done; if (!cancelled && selected->completion.result.state == file_transaction::State::RecoveryRequired) - stopping_.store(true); + RequestStop(StopMode::CancelQueued); } return true; } diff --git a/mods/src/snapshot_save_queue.h b/mods/src/snapshot_save_queue.h index 40e95d082..305873de3 100644 --- a/mods/src/snapshot_save_queue.h +++ b/mods/src/snapshot_save_queue.h @@ -11,6 +11,7 @@ namespace persistence { +enum class StopMode { DrainAccepted, CancelQueued }; // Internal scheduling core for ONE trusted generated-output destination. This // is not a user-TOML editor or a thread owner. Construct outside gameplay; the // future host must own/join its worker before destroying this object or unloading. @@ -45,11 +46,12 @@ class SnapshotSaveQueue final // Poll on the owning consumer thread. No callbacks or borrowed UI objects. // Empty means no completion available OR transient lock contention; retry later. [[nodiscard]] std::optional TryTakeCompletion(); - void RequestStop() noexcept; + // Cancellation is sticky and wins over concurrent drain requests. + void RequestStop(StopMode mode = StopMode::CancelQueued) noexcept; // Worker-only, synchronous; runs at most one transaction (or cancellation). // Concurrent invocations do not overlap disk writes. False means no work or - // another worker is active. Stopping still requires pumping queued cancellations. + // another worker is active. Stopping still requires pumping pending outcomes. // This does not create a thread, wait for disk at teardown, or detach anything. [[nodiscard]] bool RunOne(); @@ -65,6 +67,7 @@ class SnapshotSaveQueue final std::array slots_; std::mutex mutex_; std::atomic_bool stopping_{false}; + std::atomic_bool cancelQueued_{false}; std::atomic_flag running_ = ATOMIC_FLAG_INIT; std::size_t retainedBytes_ = 0; std::uint64_t lastRevision_ = 0; diff --git a/mods/src/snapshot_save_worker.cc b/mods/src/snapshot_save_worker.cc index e1df659dd..218678598 100644 --- a/mods/src/snapshot_save_worker.cc +++ b/mods/src/snapshot_save_worker.cc @@ -36,21 +36,20 @@ void SnapshotSaveWorker::Wake() noexcept wake_.notify_one(); } -void SnapshotSaveWorker::RequestStop() noexcept +void SnapshotSaveWorker::RequestStop(StopMode mode) noexcept { stopping_.store(true); - // Close queue selection immediately: queued work must not start merely because - // the active backend has not yet returned to the worker's outer loop. - queue_.RequestStop(); + // Drain closes admission; cancellation additionally closes queued selection. + queue_.RequestStop(mode); Wake(); } bool SnapshotSaveWorker::WorkEnded() const noexcept { return workEnded_.load(); } -void SnapshotSaveWorker::StopAndJoin() +void SnapshotSaveWorker::StopAndJoin(StopMode mode) { - RequestStop(); + RequestStop(mode); if (worker_.joinable()) worker_.join(); } @@ -63,10 +62,11 @@ void SnapshotSaveWorker::Run() if (stopping_.load()) { // Synchronize with a producer that passed its stop check before stop was // requested. After this barrier no accepted ticket can appear behind the - // final cancellation drain. Only the worker waits for this short lock. + // final stop drain. Only the worker waits for this short lock. { std::lock_guard lock(admission_); - queue_.RequestStop(); + // Preserve draining; concurrent cancellation remains sticky in the queue. + queue_.RequestStop(StopMode::DrainAccepted); } while (queue_.RunOne()) {} workEnded_.store(true); diff --git a/mods/src/snapshot_save_worker.h b/mods/src/snapshot_save_worker.h index 3bf846ec6..af2997404 100644 --- a/mods/src/snapshot_save_worker.h +++ b/mods/src/snapshot_save_worker.h @@ -21,13 +21,14 @@ class SnapshotSaveWorker final [[nodiscard]] SnapshotSaveQueue::Submission TrySubmit(std::uint64_t revision, std::string&& bytes); [[nodiscard]] std::optional TryTakeCompletion(); - void RequestStop() noexcept; + void RequestStop(StopMode mode = StopMode::CancelQueued) noexcept; // Informational: true means queue execution has ended, NOT that the native // thread has exited or that destroying this object is safe. Join is mandatory. [[nodiscard]] bool WorkEnded() const noexcept; // Sole lifecycle owner only; may wait indefinitely for an in-flight OS call. // Idempotent after join. Never call concurrently or from the worker itself. - void StopAndJoin(); + // Require mode explicitly so a join cannot accidentally cancel an earlier drain. + void StopAndJoin(StopMode mode); private: void Wake() noexcept; diff --git a/tests/snapshot_save_worker_test.cc b/tests/snapshot_save_worker_test.cc index 2187b553b..5d742a4a9 100644 --- a/tests/snapshot_save_worker_test.cc +++ b/tests/snapshot_save_worker_test.cc @@ -30,6 +30,7 @@ namespace std::condition_variable backendChanged; bool entered = false, allow = false, block = true; std::vector writes; + file_transaction::State backendState = file_transaction::State::Committed; file_transaction::Result Execute(const std::filesystem::path&, std::string_view bytes) { std::unique_lock lock(backendMutex); @@ -39,7 +40,7 @@ namespace backendChanged.wait(lock, [] { return allow; }); writes.emplace_back(bytes); file_transaction::Result result; - result.state = file_transaction::State::Committed; + result.state = backendState; return result; } } // namespace @@ -75,17 +76,62 @@ int main() allow = true; backendChanged.notify_all(); } - owner.StopAndJoin(); + owner.StopAndJoin(StopMode::CancelQueued); assert(owner.WorkEnded()); auto done = owner.TryTakeCompletion(); assert(done && done->ticket == first.ticket && done->outcome == Outcome::Executed && done->result.committed()); done = owner.TryTakeCompletion(); assert(done && done->ticket == second.ticket && done->outcome == Outcome::CancelledBeforeStart); assert(!owner.TryTakeCompletion()); - owner.StopAndJoin(); // Explicit lifecycle operation is idempotent. + owner.StopAndJoin(StopMode::CancelQueued); // Explicit lifecycle operation is idempotent. } assert(writes == std::vector{"first"}); block = false; + // A drain keeps accepted work executable while closing admission. Explicit + // cancellation (before OR after drain) and uncertain storage failures override + // that policy. Ordinary failure/durability outcomes must not become "saved". + for (auto state : {file_transaction::State::Committed, file_transaction::State::NotCommitted, + file_transaction::State::DurabilityUnverified, file_transaction::State::RecoveryRequired}) { + for (int cancelOrder : {0, 1, 2}) { + entered = allow = false; + block = true; + backendState = state; + writes.clear(); + SnapshotSaveWorker owner(destination); + assert(owner.TrySubmit(1, std::string("active")).state == Admission::Accepted); + { + std::unique_lock lock(backendMutex); + assert(backendChanged.wait_for(lock, std::chrono::seconds(3), [] { return entered; })); + } + assert(owner.TrySubmit(2, std::string("accepted")).state == Admission::Accepted); + if (cancelOrder == 1) + owner.RequestStop(StopMode::CancelQueued); + owner.RequestStop(StopMode::DrainAccepted); + if (cancelOrder == 2) + owner.RequestStop(StopMode::CancelQueued); + owner.RequestStop(StopMode::DrainAccepted); // Cannot undo cancellation. + assert(!owner.WorkEnded()); + assert(owner.TrySubmit(3, std::string("late")).state == Admission::Stopping); + { + std::lock_guard lock(backendMutex); + allow = true; + backendChanged.notify_all(); + } + owner.StopAndJoin(StopMode::DrainAccepted); + auto active = owner.TryTakeCompletion(); + auto queued = owner.TryTakeCompletion(); + assert(active && active->outcome == Outcome::Executed && active->result.state == state); + const bool cancelled = cancelOrder != 0 || state == file_transaction::State::RecoveryRequired; + assert(queued && queued->revision == 2); + assert(queued->outcome == (cancelled ? Outcome::CancelledBeforeStart : Outcome::Executed)); + if (!cancelled) + assert(queued->result.state == state); + assert(writes.size() == (cancelled ? 1u : 2u)); + assert(!owner.TryTakeCompletion()); + } + } + block = false; + backendState = file_transaction::State::Committed; { // Repeated empty-to-nonempty transitions exercise wake coalescing; no sleep // is needed on the producer to make a notification visible to the worker. @@ -104,16 +150,17 @@ int main() std::this_thread::yield(); assert(done->revision == revision && done->result.committed()); } - owner.StopAndJoin(); // Wakes an idle worker too. + owner.StopAndJoin(StopMode::CancelQueued); // Wakes an idle worker too. assert(owner.WorkEnded()); } // Deterministically admit a ticket AFTER stop, by holding the producer just // past the queue's final stop check. The worker must not exit ahead of it. - { + for (auto mode : {StopMode::CancelQueued, StopMode::DrainAccepted}) { SnapshotSaveWorker owner(destination); { std::lock_guard lock(admissionTestMutex); - pauseAdmission = true; + pauseAdmission = true; + admissionEntered = allowAdmission = false; } SnapshotSaveQueue::Submission submitted{Admission::Busy}; std::thread producer([&] { submitted = owner.TrySubmit(1, std::string("late ticket")); }); @@ -121,7 +168,7 @@ int main() std::unique_lock lock(admissionTestMutex); assert(admissionChanged.wait_for(lock, std::chrono::seconds(3), [] { return admissionEntered; })); } - owner.RequestStop(); + owner.RequestStop(mode); assert(!owner.WorkEnded()); { std::lock_guard lock(admissionTestMutex); @@ -129,10 +176,11 @@ int main() admissionChanged.notify_all(); } producer.join(); - owner.StopAndJoin(); + owner.StopAndJoin(mode); assert(submitted.state == Admission::Accepted); auto done = owner.TryTakeCompletion(); - assert(done && done->ticket == submitted.ticket && done->outcome == Outcome::CancelledBeforeStart); + assert(done && done->ticket == submitted.ticket); + assert(done->outcome == (mode == StopMode::CancelQueued ? Outcome::CancelledBeforeStart : Outcome::Executed)); assert(!owner.TryTakeCompletion()); pauseAdmission = false; } @@ -149,7 +197,7 @@ int main() go.store(true); owner.RequestStop(); producer.join(); - owner.StopAndJoin(); + owner.StopAndJoin(StopMode::CancelQueued); auto done = owner.TryTakeCompletion(); if (submitted.state == Admission::Accepted) assert(done && done->ticket == submitted.ticket); From fb39259a219a02aaefd1a887a5a84d2042e68a8d Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 17:11:24 -0500 Subject: [PATCH 07/20] Add bounded process-owned snapshot save service --- .github/workflows/persistence-tests.yaml | 4 +- docs/SNAPSHOT_SAVE_QUEUE.md | 65 +++++++++ mods/src/snapshot_save_service.cc | 169 +++++++++++++++++++++++ mods/src/snapshot_save_service.h | 46 ++++++ tests/snapshot_save_service_test.cc | 135 ++++++++++++++++++ 5 files changed, 417 insertions(+), 2 deletions(-) create mode 100644 mods/src/snapshot_save_service.cc create mode 100644 mods/src/snapshot_save_service.h create mode 100644 tests/snapshot_save_service_test.cc diff --git a/.github/workflows/persistence-tests.yaml b/.github/workflows/persistence-tests.yaml index e9fb4d4a3..8dc1cd766 100644 --- a/.github/workflows/persistence-tests.yaml +++ b/.github/workflows/persistence-tests.yaml @@ -23,7 +23,7 @@ jobs: if: runner.os == 'Windows' shell: pwsh run: | - foreach ($fixture in @('file_transaction', 'snapshot_save_queue', 'snapshot_save_worker')) { + foreach ($fixture in @('file_transaction', 'snapshot_save_queue', 'snapshot_save_worker', 'snapshot_save_service')) { clang++ -std=c++23 -Wall -Wextra -Werror -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test.exe" -ladvapi32 if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } & "./${fixture}_test.exe" @@ -35,7 +35,7 @@ jobs: env: MACOSX_DEPLOYMENT_TARGET: '13.5' run: | - for fixture in file_transaction snapshot_save_queue snapshot_save_worker; do + for fixture in file_transaction snapshot_save_queue snapshot_save_worker snapshot_save_service; do clang++ -std=c++23 -Wall -Wextra -Werror -pthread -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test" "./${fixture}_test" done diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index 52a9f368b..0925f1a43 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -149,3 +149,68 @@ and that known failure, durability uncertainty and recovery-required results kee their distinct meaning. Late admission across the final stop check is tested in both modes. This supplies a worker policy for future orderly quit integration; it does not change F10, defer Unity quitting, or wire any game callbacks. + +## Bounded registered service + +Enrollment reserves each destination together with its derived `.lock` identity. +A destination cannot replace another destination's lock file, even when declared +in reverse order or with an equivalent spelling. Replacing a held lock could +otherwise let another process lock a new inode while the old inode remains held. + +`SnapshotSaveService` owns one process-wide lease and up to four registered +generated-snapshot destinations. Construction validates every destination before +starting any worker. A second live service is refused, including after the first +service joins but before it is destroyed. Partial startup failure joins already +created workers before releasing the lease. Construction and explicit teardown +belong to the supervisor/startup scope, never a settings/input or loader callback. + +Each destination has one worker and its existing eight-ticket/eight-MiB capacity +limit. The process service therefore permits at most four worker threads, +32 outstanding tickets and32MiB of retained payload capacity. A stalled destination +does not stop other destinations from progressing; each file remains serialized. +These limits cover service-owned requests, not buffers callers prepare before +submission. Idle workers sleep. Existing low-level queue/worker types remain +internal implementation components, not alternate feature-facing save APIs. + +Trusted startup supplies the path list once. Callers receive an opaque destination +handle and submit only revisions and owned bytes; submission never resolves paths +or serializes config. A monotonically assigned session prevents handles from a +destroyed service being accepted by its replacement. Invalid handles preserve the +input buffer. Ticket numbers are scoped to the destination and service session, +not globally unique. Consumers must retain that identity with their requests. + +Enrollment requires existing parents, canonicalizes supported aliases and rejects +duplicate/ambiguous destinations and existing nonregular or hardlinked files. +Windows compares leaf names ordinally without case. Other platforms conservatively +reject case-equivalent ASCII names, and multiple names containing non-ASCII bytes +in the same parent, rather than guess volume case/normalization rules for absent +files. A single Unicode destination is supported. This may reject distinct files +on a case-sensitive volume. The native transaction still performs write-time path +validation; this registry is not protection against uncooperative directory/link +replacement or malicious native code bypassing the internal API. + +Windows also rejects components ending in dots/spaces before and after +canonicalization, because Win32 target normalization could otherwise diverge +from the sibling lock's identity. An absent leaf containing a tilde is rejected +conservatively: creating another destination on an 8.3-enabled volume could turn +it into that file's short alias. Existing aliases remain subject to filesystem +equivalence checks. The rule does not depend on permission to query volume policy. + +Stopping first closes service admission, then requests every worker's stop before +joining any worker. Completions remain readable after join. The host must quiesce +callers before destruction; a destination handle does not extend service lifetime. +Destruction without explicit joining still terminates rather than hiding a blocking +join or allowing a detached thread to outlive the module. + +Shutdown policy: distinguish the attempt's result from whether worker shutdown +finished. A failed save remains failed; once workers terminate, save failure alone +must not veto process exit. In-flight OS I/O cannot safely be cancelled by a queue +flag. Preserve transaction-owned recovery artifacts when an outcome is uncertain. +Host shutdown/F10 routing, result presentation and targeted user-TOML editing remain +separate integration work; no game path constructs this service yet. + +The service fixture exercises exclusive ownership, constructor rollback after a +worker has started, destination ambiguity, stale handles across replacement, +bounded retained tickets, independent progress under stalled storage, and admission +closure with retained outcomes after join. It uses an isolated backend and the +native CI matrix; it is not game runtime or filesystem interruption evidence. diff --git a/mods/src/snapshot_save_service.cc b/mods/src/snapshot_save_service.cc new file mode 100644 index 000000000..8ea00bc77 --- /dev/null +++ b/mods/src/snapshot_save_service.cc @@ -0,0 +1,169 @@ +#include "snapshot_save_service.h" +#include +#include +#if defined(_WIN32) +#include +#endif + +namespace persistence +{ +namespace +{ +std::atomic_flag serviceOwned = ATOMIC_FLAG_INIT; +std::atomic lastSession{0}; +#if defined(MOD_SNAPSHOT_SERVICE_TESTING) +void BeforeServiceWorkerStart(std::size_t index); +#endif +std::uint64_t NewSession() +{ + auto previous = lastSession.load(); + do { + if (previous == (std::numeric_limits::max)()) + throw std::overflow_error("snapshot service session exhausted"); + } while (!lastSession.compare_exchange_weak(previous, previous + 1)); + return previous + 1; +} + +bool AmbiguousNames(const std::filesystem::path& first, const std::filesystem::path& second) +{ +#if defined(_WIN32) + const auto a = first.native(), b = second.native(); + if (a.size() > INT_MAX || b.size() > INT_MAX) + return true; + const int comparison = CompareStringOrdinal(a.data(), static_cast(a.size()), b.data(), + static_cast(b.size()), TRUE); + return comparison == 0 || comparison == CSTR_EQUAL; +#else + // Conservative for case/normalization-sensitive volume differences. Existing + // aliases also use filesystem equivalence. For absent Unicode leaves in the + // same directory, refuse ambiguity rather than guess a filesystem collation. + auto a = first.native(), b = second.native(); + for (auto* text : {&a, &b}) { + for (char& c : *text) { + if (static_cast(c) >= 128) + return true; + if (c >= 'A' && c <= 'Z') c = static_cast(c + ('a' - 'A')); + } + } + return a == b; +#endif +} +void ValidateSpelling(const std::filesystem::path& path) +{ +#if defined(_WIN32) + // Win32 strips these suffixes on target access but not when a suffix such as + // .lock follows them. Reject before canonicalization can hide that spelling. + for (const auto& component : path) { + const auto& text = component.native(); + if (text != L"." && text != L".." && !text.empty() && (text.back() == L'.' || text.back() == L' ')) + throw std::invalid_argument("ambiguous Windows snapshot path spelling"); + } +#else + (void)path; +#endif +} +bool SameIdentity(const std::filesystem::path& first, const std::filesystem::path& second) +{ + return (std::filesystem::equivalent(first.parent_path(), second.parent_path()) && + AmbiguousNames(first.filename(), second.filename())) || + (std::filesystem::exists(first) && std::filesystem::exists(second) && + std::filesystem::equivalent(first, second)); +} +} + +SnapshotSaveService::Lease::Lease() +{ + if (serviceOwned.test_and_set()) + throw std::logic_error("snapshot service already owned"); +} +SnapshotSaveService::Lease::~Lease() { serviceOwned.clear(); } + +SnapshotSaveService::SnapshotSaveService(std::span paths) + : session_(NewSession()), count_(paths.size()) +{ + if (count_ == 0 || count_ > MaxDestinations) + throw std::invalid_argument("snapshot destination count out of bounds"); + std::array resolved; + // Finish filesystem enrollment before any thread starts. Existing parents are + // required; this registry creates no paths or config files. + for (std::size_t i = 0; i < count_; ++i) { + if (paths[i].empty() || paths[i].native().find(std::filesystem::path::value_type{}) != + std::filesystem::path::string_type::npos) + throw std::invalid_argument("invalid snapshot destination"); + ValidateSpelling(paths[i]); + resolved[i] = std::filesystem::weakly_canonical(std::filesystem::absolute(paths[i])); + ValidateSpelling(resolved[i]); + if (!std::filesystem::is_directory(resolved[i].parent_path()) || resolved[i].filename().empty()) + throw std::invalid_argument("snapshot parent must exist"); + const bool exists = std::filesystem::exists(resolved[i]); +#if defined(_WIN32) + // On 8.3-enabled volumes another destination's creation can turn an absent + // tilde leaf into its short alias. Reject conservatively without depending + // on the current volume policy. Existing aliases use equivalence below. + if (!exists && resolved[i].filename().native().find(L'~') != std::wstring::npos) + throw std::invalid_argument("ambiguous absent Windows short-name destination"); +#endif + if (exists && (!std::filesystem::is_regular_file(resolved[i]) || std::filesystem::hard_link_count(resolved[i]) != 1)) + throw std::invalid_argument("snapshot destination must be a single regular file"); + for (std::size_t j = 0; j < i; ++j) { + auto lockI = resolved[i], lockJ = resolved[j]; + lockI += ".lock"; + lockJ += ".lock"; + // A destination must never replace another transaction's lock inode. + // Reserve the complete target/lock identity pair, in either order. + if (SameIdentity(resolved[i], resolved[j]) || SameIdentity(resolved[i], lockJ) || + SameIdentity(lockI, resolved[j]) || SameIdentity(lockI, lockJ)) + throw std::invalid_argument("duplicate or ambiguous snapshot destination or lock"); + } + } + try { + for (std::size_t i = 0; i < count_; ++i) { +#if defined(MOD_SNAPSHOT_SERVICE_TESTING) + BeforeServiceWorkerStart(i); +#endif + workers_[i] = std::make_unique(resolved[i]); + } + } catch (...) { + // Construction is supervisor/startup-only. Partial thread creation must not + // destroy a joinable worker during stack unwinding. + for (auto& worker : workers_) + if (worker) worker->StopAndJoin(StopMode::CancelQueued); + throw; + } +} + +bool SnapshotSaveService::Valid(Destination destination) const noexcept +{ return destination.session_ == session_ && destination.index_ < count_; } + +SnapshotSaveService::Destination SnapshotSaveService::GetDestination(std::size_t index) const +{ + if (index >= count_) throw std::out_of_range("snapshot destination index"); + Destination result; + result.session_ = session_; + result.index_ = index; + return result; +} + +SnapshotSaveQueue::Submission SnapshotSaveService::TrySubmit(Destination destination, std::uint64_t revision, + std::string&& bytes) +{ + if (!Valid(destination)) return {SnapshotSaveQueue::Admission::InvalidRequest}; + if (stopping_.load()) return {SnapshotSaveQueue::Admission::Stopping}; + return workers_[destination.index_]->TrySubmit(revision, std::move(bytes)); +} +std::optional SnapshotSaveService::TryTakeCompletion(Destination destination) +{ + if (!Valid(destination)) return std::nullopt; + return workers_[destination.index_]->TryTakeCompletion(); +} +void SnapshotSaveService::RequestStop(StopMode mode) noexcept +{ + stopping_.store(true); + for (std::size_t i = 0; i < count_; ++i) workers_[i]->RequestStop(mode); +} +void SnapshotSaveService::StopAndJoin(StopMode mode) +{ + RequestStop(mode); // Close every destination before joining any stalled one. + for (std::size_t i = 0; i < count_; ++i) workers_[i]->StopAndJoin(mode); +} +} diff --git a/mods/src/snapshot_save_service.h b/mods/src/snapshot_save_service.h new file mode 100644 index 000000000..db0978d8c --- /dev/null +++ b/mods/src/snapshot_save_service.h @@ -0,0 +1,46 @@ +#pragma once +#include "snapshot_save_worker.h" +#include +#include +#include + +namespace persistence +{ +// Internal process-wide owner of registered generated-snapshot destinations. +// Construct off gameplay/loader callbacks. Quiesce callers and explicitly join +// before destruction. No singleton destructor or implicit detach/join. +class SnapshotSaveService final +{ +public: + static constexpr std::size_t MaxDestinations = 4; + class Destination { + public: + Destination() = default; + private: + friend class SnapshotSaveService; + std::uint64_t session_ = 0; + std::size_t index_ = 0; + }; + explicit SnapshotSaveService(std::span trustedPaths); + ~SnapshotSaveService() = default; + SnapshotSaveService(const SnapshotSaveService&) = delete; + SnapshotSaveService& operator=(const SnapshotSaveService&) = delete; + [[nodiscard]] Destination GetDestination(std::size_t index) const; + [[nodiscard]] SnapshotSaveQueue::Submission TrySubmit(Destination destination, std::uint64_t revision, + std::string&& bytes); + [[nodiscard]] std::optional TryTakeCompletion(Destination destination); + void RequestStop(StopMode mode) noexcept; + void StopAndJoin(StopMode mode); +private: + // Declared first, destroyed last: ownership is never released before workers. + struct Lease { + Lease(); + ~Lease(); + } lease_; + const std::uint64_t session_; + const std::size_t count_; + std::atomic_bool stopping_{false}; + std::array, MaxDestinations> workers_; + bool Valid(Destination destination) const noexcept; +}; +} diff --git a/tests/snapshot_save_service_test.cc b/tests/snapshot_save_service_test.cc new file mode 100644 index 000000000..4d742a02c --- /dev/null +++ b/tests/snapshot_save_service_test.cc @@ -0,0 +1,135 @@ +#define MOD_SNAPSHOT_QUEUE_TESTING +#define MOD_SNAPSHOT_SERVICE_TESTING +#include "snapshot_save_queue.cc" +#include "snapshot_save_worker.cc" +#include "snapshot_save_service.cc" +#include +#include +#include +#include + +namespace persistence +{ +namespace +{ +std::mutex backendMutex; +std::condition_variable changed; +bool entered = false, release = false; +int finishedB = 0; +bool failSecondStart = false; +void BeforeServiceWorkerStart(std::size_t index) +{ + if (failSecondStart && index == 1) throw std::runtime_error("injected thread-start failure"); +} +file_transaction::Result Execute(const std::filesystem::path& path, std::string_view) +{ + std::unique_lock lock(backendMutex); + if (path.filename() == "a.vars") { + entered = true; + changed.notify_all(); + changed.wait(lock, [] { return release; }); + } else { + ++finishedB; + changed.notify_all(); + } + file_transaction::Result result; + result.state = file_transaction::State::Committed; + return result; +} +} +} + +int main() +{ + using namespace persistence; + using Admission = SnapshotSaveQueue::Admission; + std::promise done; + std::thread watchdog([future = done.get_future()] { + if (future.wait_for(std::chrono::seconds(15)) == std::future_status::timeout) std::abort(); + }); + const auto root = std::filesystem::temp_directory_path(); + const std::array paths{root / "a.vars", root / "b.vars"}; + auto rejects = [](auto&& construct) { + bool rejected = false; + try { construct(); } catch (const std::exception&) { rejected = true; } + assert(rejected); + }; + rejects([&] { SnapshotSaveService invalid(std::span{}); }); + const std::array tooMany{paths[0], paths[1], root/"c.vars", root/"d.vars", root/"e.vars"}; + rejects([&] { SnapshotSaveService invalid(tooMany); }); + const std::array aliases{paths[0], root / "." / "A.vars"}; + rejects([&] { SnapshotSaveService invalid(aliases); }); + const std::array lockCollision{root / "snapshot.vars", root / "snapshot.vars.lock"}; + const std::array reverseLockCollision{lockCollision[1], lockCollision[0]}; + const std::array caseLockCollision{root / "snapshot.vars", root / "SNAPSHOT.VARS.LOCK"}; + rejects([&] { SnapshotSaveService invalid(lockCollision); }); + rejects([&] { SnapshotSaveService invalid(reverseLockCollision); }); + rejects([&] { SnapshotSaveService invalid(caseLockCollision); }); +#if defined(_WIN32) + const std::array dotAliases{root / "stfc-service-absent.vars", root / "stfc-service-absent.vars."}; + const std::array spaceAliases{root / "stfc-service-absent.vars", root / "stfc-service-absent.vars "}; + const std::array shortAliases{root / "stfc-service-long-absent.vars", root / "STFCSE~1.VAR"}; + rejects([&] { SnapshotSaveService invalid(dotAliases); }); + rejects([&] { SnapshotSaveService invalid(spaceAliases); }); + rejects([&] { SnapshotSaveService invalid(shortAliases); }); + const auto parentWithoutSeparator = root.filename().empty() ? root.parent_path() : root; + const std::array parentSpelling{std::filesystem::path(parentWithoutSeparator.native() + L".") / "a.vars"}; + rejects([&] { SnapshotSaveService invalid(parentSpelling); }); +#endif + failSecondStart = true; + rejects([&] { SnapshotSaveService partial(paths); }); + failSecondStart = false; // A prior worker must have joined and released its lease. + SnapshotSaveService::Destination old; + { + SnapshotSaveService service(paths); + old = service.GetDestination(0); + const auto b = service.GetDestination(1); + // A failed second lease must not release the first one's process claim. + rejects([&] { SnapshotSaveService duplicate(paths); }); + rejects([&] { SnapshotSaveService duplicate(paths); }); + std::string invalidBytes = "untouched"; + assert(service.TrySubmit({}, 1, std::move(invalidBytes)).state == Admission::InvalidRequest); + assert(invalidBytes == "untouched"); + assert(service.TrySubmit(old, 1, std::string("first")).state == Admission::Accepted); + { + std::unique_lock lock(backendMutex); + changed.wait(lock, [] { return entered; }); + } + // Complete a different destination while A's backend remains stalled. + assert(service.TrySubmit(b, 1, std::string("other")).state == Admission::Accepted); + { + std::unique_lock lock(backendMutex); + changed.wait(lock, [] { return finishedB == 1; }); + } + // Count bound includes A's in-flight ticket, with rejection retaining input. + for (std::uint64_t revision = 2; revision <= SnapshotSaveQueue::MaxOutstanding; ++revision) + assert(service.TrySubmit(old, revision, std::string("queued")).state == Admission::Accepted); + std::string extra = "retained"; + assert(service.TrySubmit(old, 9, std::move(extra)).state == Admission::Busy && extra == "retained"); + service.RequestStop(StopMode::DrainAccepted); + assert(service.TrySubmit(b, 2, std::string("late")).state == Admission::Stopping); + auto joining = std::async(std::launch::async, [&] { service.StopAndJoin(StopMode::DrainAccepted); }); + { + std::lock_guard lock(backendMutex); + release = true; + } + changed.notify_all(); + joining.get(); + int completions = 0; + while (auto completion = service.TryTakeCompletion(old)) { + assert(completion->result.state == file_transaction::State::Committed); + ++completions; + } + assert(completions == 8 && service.TryTakeCompletion(b).has_value()); + } + { + SnapshotSaveService replacement(paths); + std::string stale = "old session"; + assert(replacement.TrySubmit(old, 10, std::move(stale)).state == Admission::InvalidRequest); + assert(stale == "old session" && !replacement.TryTakeCompletion(old)); + replacement.StopAndJoin(StopMode::DrainAccepted); + } + done.set_value(); + watchdog.join(); + std::cout << "snapshot service tests passed\n"; +} From 3822d455da34271e84adb424f387d5b3e7479106 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 17:28:56 -0500 Subject: [PATCH 08/20] Route Windows quit shortcut through the game lifecycle --- docs/SNAPSHOT_SAVE_QUEUE.md | 8 ++++++-- mods/src/patches/parts/hotkeys.cc | 13 +++++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index 0925f1a43..fe4a06608 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -206,8 +206,12 @@ Shutdown policy: distinguish the attempt's result from whether worker shutdown finished. A failed save remains failed; once workers terminate, save failure alone must not veto process exit. In-flight OS I/O cannot safely be cancelled by a queue flag. Preserve transaction-owned recovery artifacts when an outcome is uncertain. -Host shutdown/F10 routing, result presentation and targeted user-TOML editing remain -separate integration work; no game path constructs this service yet. +The mod Quit shortcut now calls `PrimeApp::Quit` on Windows as it already did on +macOS. Windows no longer uses `TerminateProcess`, which bypassed Unity quit +subscribers. An unavailable app or method does not trigger a force-kill fallback. +This routing change alone does not drain saves: host ownership/shutdown, result +presentation and targeted user-TOML editing remain separate integration work; +no game path constructs this service yet. No new quit detour is installed here. The service fixture exercises exclusive ownership, constructor rollback after a worker has started, destination ambiguity, stale handles across replacement, diff --git a/mods/src/patches/parts/hotkeys.cc b/mods/src/patches/parts/hotkeys.cc index 8428a9da0..7f40d43b3 100644 --- a/mods/src/patches/parts/hotkeys.cc +++ b/mods/src/patches/parts/hotkeys.cc @@ -494,13 +494,14 @@ void ScreenManager_Update_Hook(auto original, ScreenManager* _this) return; } -#ifdef _WIN32 - if (MapKey::IsDown(GameFunction::Quit)) { - TerminateProcess(GetCurrentProcess(), 1); - } -#elif defined(__APPLE__) +#if defined(_WIN32) || defined(__APPLE__) if (MapKey::IsDown(GameFunction::Quit)) { - Hub::get_App()->Quit(); + // Use the normal game lifecycle on both platforms. Forced termination + // bypasses quit subscribers and any pending-save shutdown coordination. + // If the app/method is unavailable, do not fall back to killing the process. + if (auto* app = Hub::get_App()) { + app->Quit(); + } return; } #endif From 6a3abc1ba269e38b6d3eac3c8591fe8ec5809eae Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:03:28 -0500 Subject: [PATCH 09/20] Add lazy snapshot supervisor and native quit drain adapter --- .github/workflows/persistence-tests.yaml | 4 +- docs/SNAPSHOT_SAVE_QUEUE.md | 61 ++++- .../patches/parts/runtime_snapshot_host.cc | 149 +++++++++++++ mods/src/patches/patches.cc | 3 + mods/src/patches/runtime_snapshot_host.h | 19 ++ mods/src/quit_drain_gate.h | 35 +++ mods/src/snapshot_save_host.cc | 160 +++++++++++++ mods/src/snapshot_save_host.h | 55 +++++ tests/snapshot_save_host_test.cc | 210 ++++++++++++++++++ 9 files changed, 691 insertions(+), 5 deletions(-) create mode 100644 mods/src/patches/parts/runtime_snapshot_host.cc create mode 100644 mods/src/patches/runtime_snapshot_host.h create mode 100644 mods/src/quit_drain_gate.h create mode 100644 mods/src/snapshot_save_host.cc create mode 100644 mods/src/snapshot_save_host.h create mode 100644 tests/snapshot_save_host_test.cc diff --git a/.github/workflows/persistence-tests.yaml b/.github/workflows/persistence-tests.yaml index 8dc1cd766..0796ebd57 100644 --- a/.github/workflows/persistence-tests.yaml +++ b/.github/workflows/persistence-tests.yaml @@ -23,7 +23,7 @@ jobs: if: runner.os == 'Windows' shell: pwsh run: | - foreach ($fixture in @('file_transaction', 'snapshot_save_queue', 'snapshot_save_worker', 'snapshot_save_service')) { + foreach ($fixture in @('file_transaction', 'snapshot_save_queue', 'snapshot_save_worker', 'snapshot_save_service', 'snapshot_save_host')) { clang++ -std=c++23 -Wall -Wextra -Werror -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test.exe" -ladvapi32 if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } & "./${fixture}_test.exe" @@ -35,7 +35,7 @@ jobs: env: MACOSX_DEPLOYMENT_TARGET: '13.5' run: | - for fixture in file_transaction snapshot_save_queue snapshot_save_worker snapshot_save_service; do + for fixture in file_transaction snapshot_save_queue snapshot_save_worker snapshot_save_service snapshot_save_host; do clang++ -std=c++23 -Wall -Wextra -Werror -pthread -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test" "./${fixture}_test" done diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index fe4a06608..8cb8e8c68 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -209,12 +209,67 @@ flag. Preserve transaction-owned recovery artifacts when an outcome is uncertain The mod Quit shortcut now calls `PrimeApp::Quit` on Windows as it already did on macOS. Windows no longer uses `TerminateProcess`, which bypassed Unity quit subscribers. An unavailable app or method does not trigger a force-kill fallback. -This routing change alone does not drain saves: host ownership/shutdown, result -presentation and targeted user-TOML editing remain separate integration work; -no game path constructs this service yet. No new quit detour is installed here. +This routing change alone does not drain saves. The lazy host described below +supplies shutdown coordination when a runtime consumer starts it. Result presentation +and targeted user-TOML editing remain separate integration work; no current feature +registers a runtime destination or submits runtime saves. The service fixture exercises exclusive ownership, constructor rollback after a worker has started, destination ambiguity, stale handles across replacement, bounded retained tickets, independent progress under stalled storage, and admission closure with retained outcomes after join. It uses an isolated backend and the native CI matrix; it is not game runtime or filesystem interruption evidence. + +## Lazy game host and shutdown + +`runtime_snapshots` is the internal game-facing registration/submission adapter. +Trusted registration takes up to four paths on the observed Update thread. It +launches a native supervisor; the supervisor enrolls paths and constructs the +existing service and workers. Status remains Starting until enrollment completes. +Registration is one-shot, including failed attempts. Ordinary consumers receive +destination handles and never supply a path with a save request. Preparing bytes +is still an adapter responsibility; this is not a preserving user-TOML editor. + +Initialization runs after `il2cpp_init`, outside `DllMain`. It registers with the +existing ScreenManager.Update owner but starts no thread. When inactive, its +callback does no logging, timing capture or filesystem work. The quit detour is +installed only on the first real registration attempt. Admission requires an +observed Update callback, the build261 Windows x64 method RVA, full instruction +fingerprint, and native unwind extent. Mismatch leaves runtime saves unavailable. +macOS and other architectures reject registration until their shutdown seam is +validated; this does not disable the synchronous checked startup outputs. + +An allowed game quit vote closes admission and wakes the supervisor. The game +callback returns without waiting on storage. The supervisor removes borrowed +service access under its mutex, drains accepted work, joins every worker, retains +unconsumed completions, and destroys the service. Producer and completion access +use try-lock operations; consumers cannot race service destruction. Retained +completions preserve their destination index and ticket, including failed saves. +No game object or callback crosses onto the supervisor or workers. + +The Update callback polls the native supervisor handle with a zero timeout during +shutdown. A published Stopped state alone cannot authorize quitting. After actual +native thread exit it closes the handle, releases the temporary loader reference, +and requests Unity quit once. The game's real subscriber vote is preserved, and +a later genuine veto cancels automatic resumption; a resumed veto does not cause +an automatic retry loop. The stopped service remains +unavailable for new requests if the game vetoes exit; it is not automatically restarted. +Failed saves do not veto exit once workers have terminated. Indefinitely stalled +OS I/O can still delay normal exit: no timer pretends to cancel an in-flight write. + +The supervisor uses an ordinary scoped module reference, not a permanent module +PIN. The small adapter control block and detour have process lifetime; there is no +static thread destructor, detached worker, or loader-lock join. Hot module unloading +is unsupported, as it is for the installed game hooks. Force termination/crash can +bypass this lifecycle; no power-loss or guaranteed shutdown-duration claim is made. + +The Windows host fixture verifies stop during startup, concurrent admission versus +shutdown, retained failure/success outcomes, rejection after stop, and the interval +between service destruction and native supervisor exit. Other platform fixtures +verify the quit gate and unsupported-host rejection only. These are isolated native +tests, not evidence of exact-artifact F10/window-X runtime behavior. + +Native lifecycle references: Microsoft documents the retained caller-owned handle +and automatic CRT cleanup for [_beginthreadex](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex), +zero-timeout [WaitForSingleObject](https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobject), +and scoped versus pinned [module references](https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulehandleexw). diff --git a/mods/src/patches/parts/runtime_snapshot_host.cc b/mods/src/patches/parts/runtime_snapshot_host.cc new file mode 100644 index 000000000..9bd379a93 --- /dev/null +++ b/mods/src/patches/parts/runtime_snapshot_host.cc @@ -0,0 +1,149 @@ +#include "patches/runtime_snapshot_host.h" + +#if defined(_WIN32) && defined(_M_X64) +#include "patches/screen_update_hook.h" +#include "quit_drain_gate.h" +#include +#include +#include +#include + +namespace +{ +using Host = persistence::SnapshotSaveHost; +// Fixed process-lifetime control block. The supervisor owns the service; its +// workers, paths and native handles are reclaimed on shutdown. Installed detours +// have process lifetime, so live module unloading is not supported. +Host* host = nullptr; +std::atomic_bool started{false}; +persistence::QuitDrainGate gate; +std::atomic updateThread{0}; +const MethodInfo* wantsMethod = nullptr; +void (*requestQuit)(int) = nullptr; +std::atomic_bool available{false}, attempted{false}; +thread_local unsigned wantsDepth = 0; + +// Exact build261 Windows x64 discovery: native extent411 bytes, SPUD overwrite24. +// Require runtime unwind extent AND the complete29-byte instruction fingerprint. +constexpr unsigned char wantsBytes[]{0x48, 0x89, 0x5c, 0x24, 0x08, 0x56, 0x57, 0x41, 0x56, 0x48, + 0x83, 0xec, 0x40, 0x80, 0x3d, 0xad, 0xd2, 0x8d, 0x01, 0x00, + 0x75, 0x29, 0x48, 0x8d, 0x0d, 0x9b, 0xdf, 0x63, 0x01}; +bool MatchesQuitMethod() +{ + const auto base = reinterpret_cast(GetModuleHandleW(L"GameAssembly.dll")); + if (!base || !wantsMethod || reinterpret_cast(wantsMethod->methodPointer) != base + 0x43548c0) + return false; + DWORD64 imageBase = 0; + const auto* extent = RtlLookupFunctionEntry(base + 0x43548c0, &imageBase, nullptr); + return extent && imageBase == base && extent->BeginAddress == 0x43548c0 && extent->EndAddress == 0x4354a5b && + std::memcmp(wantsMethod->methodPointer, wantsBytes, sizeof(wantsBytes)) == 0; +} + +bool WantsQuit(auto original) +{ + struct Depth { Depth() { ++wantsDepth; } ~Depth() { --wantsDepth; } } depth; + const bool gameAllows = original(); + // Preserve the game's vote. Save failures are separate from whether native + // code is still executing. No join, filesystem call, logging or timer here. + const bool result = gate.Vote(gameAllows, started.load()); + if (gate.DrainRequested() && !gate.Stopped()) host->RequestStop(); + return result; +} + +void UpdateHost() +{ + DWORD unset = 0; + updateThread.compare_exchange_strong(unset, GetCurrentThreadId()); + if (updateThread.load() != GetCurrentThreadId() || wantsDepth != 0 || !started.load()) return; + if (!gate.Stopped()) { + if (!gate.DrainRequested() && host->Status() != Host::State::Unavailable) return; + if (!host->PollStopped()) return; + gate.ObserveStopped(); + } + // Consume before calling into Unity: nested callbacks or a genuine subscriber + // veto must not create an automatic quit loop. Failed saves still reach here. + if (gate.TakeResumeRequest()) requestQuit(0); +} +} // namespace +#endif + +void InstallRuntimeSnapshotHost() +{ +#if defined(_WIN32) && defined(_M_X64) + if (available) return; + auto helper = il2cpp_get_class_helper("UnityEngine.CoreModule", "UnityEngine", "Application"); + if (!helper.isValidHelper()) return; + wantsMethod = helper.GetMethodInfo("Internal_ApplicationWantsToQuit", 0); + const auto* quit = helper.GetMethodInfo("Quit", 1); + const auto base = reinterpret_cast(GetModuleHandleW(L"GameAssembly.dll")); + if (!MatchesQuitMethod() || !quit || reinterpret_cast(quit->methodPointer) != base + 0x4351c00) return; + requestQuit = reinterpret_cast(quit->methodPointer); + // Existing single Update owner; no additional Update detour. Observing its + // callback is a prerequisite for Start, not just trusting an install return. + if (install_screen_manager_update_hook() && register_screen_manager_update_callback(UpdateHost)) available = true; +#endif +} + +namespace runtime_snapshots +{ +bool Start(std::vector&& paths) +{ +#if defined(_WIN32) && defined(_M_X64) + if (!available || updateThread.load() != GetCurrentThreadId() || attempted || wantsDepth != 0) return false; + attempted = true; + try { + // Install only when a real consumer enrolls, and recheck for client/other-hook + // drift immediately beforehand. A rejected seam cannot launch any worker. + if (!MatchesQuitMethod() || !SPUD_STATIC_DETOUR(wantsMethod->methodPointer, WantsQuit)) return false; + host = new Host; + // Publish stable control ownership BEFORE native launch. An off-thread quit + // during launch must be held too. Keep the control block even if launch fails; + // a concurrent callback may already reference it, and Update reaps/resumes. + started.store(true); + return host->Start(std::move(paths)); + } catch (...) { + return false; + } +#else + (void)paths; + return false; +#endif +} + +persistence::SnapshotSaveHost::State Status() noexcept +{ +#if defined(_WIN32) && defined(_M_X64) + if (started.load()) return host->Status(); + if (available && !attempted) return persistence::SnapshotSaveHost::State::Idle; +#endif + return persistence::SnapshotSaveHost::State::Unavailable; +} +std::optional TryGetDestination(std::size_t index) +{ +#if defined(_WIN32) && defined(_M_X64) + if (started.load()) return host->TryGetDestination(index); +#else + (void)index; +#endif + return std::nullopt; +} +persistence::SnapshotSaveQueue::Submission TrySubmit(persistence::SnapshotSaveService::Destination destination, + std::uint64_t revision, std::string&& bytes) +{ +#if defined(_WIN32) && defined(_M_X64) + if (started.load()) return host->TrySubmit(destination, revision, std::move(bytes)); +#else + (void)destination; (void)revision; (void)bytes; +#endif + return {persistence::SnapshotSaveQueue::Admission::InvalidRequest}; +} +std::optional TryTakeCompletion(std::size_t index) +{ +#if defined(_WIN32) && defined(_M_X64) + if (started.load()) return host->TryTakeCompletion(index); +#else + (void)index; +#endif + return std::nullopt; +} +} // namespace runtime_snapshots diff --git a/mods/src/patches/patches.cc b/mods/src/patches/patches.cc index 9579b9692..039368138 100644 --- a/mods/src/patches/patches.cc +++ b/mods/src/patches/patches.cc @@ -1,6 +1,7 @@ #include "patches.h" #include "file.h" #include "version.h" +#include "patches/runtime_snapshot_host.h" #include @@ -171,6 +172,8 @@ __int64 il2cpp_init_hook(auto original, const char* domain_name) } } + InstallRuntimeSnapshotHost(); + spdlog::info(""); #if VERSION_PATCH diff --git a/mods/src/patches/runtime_snapshot_host.h b/mods/src/patches/runtime_snapshot_host.h new file mode 100644 index 000000000..9c8bbae45 --- /dev/null +++ b/mods/src/patches/runtime_snapshot_host.h @@ -0,0 +1,19 @@ +#pragma once + +#include "snapshot_save_host.h" + +// Install after il2cpp_init, outside DllMain. No worker is launched here. +void InstallRuntimeSnapshotHost(); + +namespace runtime_snapshots +{ +// Internal trusted registration, called on the observed game Update thread after +// installation. False leaves input owned by the caller. One attempt per process. +// Unsupported clients/platforms cannot start a worker through this adapter. +[[nodiscard]] bool Start(std::vector&& trustedPaths); +[[nodiscard]] persistence::SnapshotSaveHost::State Status() noexcept; +[[nodiscard]] std::optional TryGetDestination(std::size_t index); +[[nodiscard]] persistence::SnapshotSaveQueue::Submission TrySubmit( + persistence::SnapshotSaveService::Destination destination, std::uint64_t revision, std::string&& bytes); +[[nodiscard]] std::optional TryTakeCompletion(std::size_t index); +} // namespace runtime_snapshots diff --git a/mods/src/quit_drain_gate.h b/mods/src/quit_drain_gate.h new file mode 100644 index 000000000..3b79e560c --- /dev/null +++ b/mods/src/quit_drain_gate.h @@ -0,0 +1,35 @@ +#pragma once +#include + +namespace persistence +{ +// Receives the real game's vote; never turns a veto into permission to quit. +// The owner must ObserveStopped only after native supervisor termination. +class QuitDrainGate final +{ +public: + bool Vote(bool gameAllows, bool active) noexcept + { + if (!active) return gameAllows; + if (!gameAllows) { + resume_.store(false); + return false; + } + if (stopped_.load()) return true; + requested_.store(true); + resume_.store(true); + return false; + } + bool DrainRequested() const noexcept { return requested_.load(); } + void ObserveStopped() noexcept { stopped_.store(true); } + bool Stopped() const noexcept { return stopped_.load(); } + bool TakeResumeRequest() noexcept + { + // Consume before reentering Unity. A subsequent genuine veto cannot cause + // repeated automatic quit requests once the supervisor is gone. + return stopped_.load() && resume_.exchange(false); + } +private: + std::atomic_bool requested_{false}, resume_{false}, stopped_{false}; +}; +} diff --git a/mods/src/snapshot_save_host.cc b/mods/src/snapshot_save_host.cc new file mode 100644 index 000000000..e05e80b1f --- /dev/null +++ b/mods/src/snapshot_save_host.cc @@ -0,0 +1,160 @@ +#include "snapshot_save_host.h" + +#if defined(_WIN32) +#include +#include +#endif + +namespace persistence +{ +#if defined(MOD_SNAPSHOT_HOST_TESTING) && defined(_WIN32) +namespace { void BeforeHostThreadReturn(); } +#endif +SnapshotSaveHost::~SnapshotSaveHost() +{ + // Never hide a blocking destructor or permit code to outlive its module. + if (!PollStopped()) std::terminate(); +} + +bool SnapshotSaveHost::Start(std::vector&& paths) +{ + if (state_.load() != State::Idle) return false; + state_.store(State::Unavailable); + if (stop_.load() || paths.empty() || paths.size() > SnapshotSaveService::MaxDestinations || + paths.capacity() > SnapshotSaveService::MaxDestinations) return false; + for (const auto& path : paths) + if (path.empty() || path.native().capacity() > 32767) return false; +#if defined(_WIN32) + HMODULE module = nullptr; + // Ordinary scoped loader reference, not GET_MODULE_HANDLE_EX_FLAG_PIN. Keep + // code loaded until native supervisor termination, then release on the owner. + // Live unloading of the game's installed hooks is still unsupported. + if (!GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, + reinterpret_cast(&SnapshotSaveHost::Entry), &module)) return false; + module_ = module; + count_ = paths.size(); + paths_ = std::move(paths); + state_.store(State::Starting); + thread_ = reinterpret_cast(_beginthreadex(nullptr, 0, Entry, this, 0, nullptr)); + if (!thread_) { + paths = std::move(paths_); + state_.store(State::Unavailable); + FreeLibrary(module); + module_ = nullptr; + return false; + } + return true; +#else + return false; +#endif +} + +std::optional SnapshotSaveHost::TryGetDestination(std::size_t index) +{ + std::unique_lock lock(access_, std::try_to_lock); + if (!lock || stop_.load() || state_.load() != State::Ready || !service_ || index >= count_) return std::nullopt; + return service_->GetDestination(index); +} + +SnapshotSaveQueue::Submission SnapshotSaveHost::TrySubmit(SnapshotSaveService::Destination destination, + std::uint64_t revision, std::string&& bytes) +{ + if (stop_.load()) return {SnapshotSaveQueue::Admission::Stopping}; + std::unique_lock lock(access_, std::try_to_lock); + if (!lock) return {SnapshotSaveQueue::Admission::Busy}; + if (stop_.load()) return {SnapshotSaveQueue::Admission::Stopping}; + if (state_.load() != State::Ready || !service_) return {SnapshotSaveQueue::Admission::InvalidRequest}; + return service_->TrySubmit(destination, revision, std::move(bytes)); +} + +std::optional SnapshotSaveHost::TryTakeCompletion(std::size_t index) +{ + std::unique_lock lock(access_, std::try_to_lock); + if (!lock || index >= count_) return std::nullopt; + if (service_) return service_->TryTakeCompletion(service_->GetDestination(index)); + for (auto& result : retained_[index]) { + if (!result) continue; + auto taken = std::move(result); + result.reset(); + return taken; + } + return std::nullopt; +} + +void SnapshotSaveHost::RequestStop() noexcept +{ + stop_.store(true); + stop_.notify_all(); +} + +bool SnapshotSaveHost::PollStopped() noexcept +{ +#if defined(_WIN32) + if (thread_) { + if (WaitForSingleObject(static_cast(thread_), 0) != WAIT_OBJECT_0) return false; + CloseHandle(static_cast(thread_)); + thread_ = nullptr; + FreeLibrary(static_cast(module_)); + module_ = nullptr; + } +#endif + return true; +} + +#if defined(_WIN32) +unsigned __stdcall SnapshotSaveHost::Entry(void* context) +{ + static_cast(context)->Run(); +#if defined(MOD_SNAPSHOT_HOST_TESTING) + BeforeHostThreadReturn(); +#endif + return 0; // _beginthreadex wrapper performs CRT thread cleanup before signaling. +} +#endif + +void SnapshotSaveHost::Run() noexcept +{ + // This function and all service filesystem/thread lifecycle operations run on + // the native supervisor. Remove producer access BEFORE draining/destruction. + std::unique_ptr service; + try { + service = std::make_unique(paths_); + paths_.clear(); + { + std::lock_guard lock(access_); + service_ = service.get(); + state_.store(State::Ready); + } + while (!stop_.load()) stop_.wait(false); + { + std::lock_guard lock(access_); + state_.store(State::Stopping); + service_ = nullptr; + } + service->StopAndJoin(StopMode::DrainAccepted); + { + std::lock_guard lock(access_); + for (std::size_t i = 0; i < count_; ++i) + for (auto& slot : retained_[i]) { + slot = service->TryTakeCompletion(service->GetDestination(i)); + if (!slot) break; + } + } + service.reset(); + state_.store(State::Stopped); + } catch (...) { + { + std::lock_guard lock(access_); + service_ = nullptr; + } + // Constructor rollback already joins partial workers. If a later operation + // throws, still finish accepted work before allowing native thread exit. + if (service) { + try { service->StopAndJoin(StopMode::DrainAccepted); } + catch (...) { std::terminate(); } // Never destroy possibly joinable workers. + service.reset(); + } + state_.store(State::Unavailable); + } +} +} // namespace persistence diff --git a/mods/src/snapshot_save_host.h b/mods/src/snapshot_save_host.h new file mode 100644 index 000000000..4f017a364 --- /dev/null +++ b/mods/src/snapshot_save_host.h @@ -0,0 +1,55 @@ +#pragma once + +#include "snapshot_save_service.h" +#include + +namespace persistence +{ +// One-shot native supervisor. Start/PollStopped/destruction belong to one owner +// thread, outside the loader lock. Start must return before consumers begin; +// other methods are safe for concurrent callers until destruction is quiesced. +// Windows only until an equivalent native-thread exit observation is validated. +// No implicit join, detach, filesystem work, or worker construction on the owner. +class SnapshotSaveHost final +{ +public: + enum class State { Idle, Starting, Ready, Stopping, Stopped, Unavailable }; + SnapshotSaveHost() = default; + ~SnapshotSaveHost(); + SnapshotSaveHost(const SnapshotSaveHost&) = delete; + SnapshotSaveHost& operator=(const SnapshotSaveHost&) = delete; + + // Trusted enrollment only. Takes ownership after successful launch; max four + // paths, each bounded to 32767 native code units (including retained capacity). + // An unsuccessful start is terminal. No retry/new session during shutdown. + [[nodiscard]] bool Start(std::vector&& paths); + [[nodiscard]] State Status() const noexcept { return state_.load(); } + [[nodiscard]] std::optional TryGetDestination(std::size_t index); + [[nodiscard]] SnapshotSaveQueue::Submission TrySubmit(SnapshotSaveService::Destination destination, + std::uint64_t revision, std::string&& bytes); + [[nodiscard]] std::optional TryTakeCompletion(std::size_t index); + void RequestStop() noexcept; + // Zero-timeout native thread observation, NOT WorkEnded or a std::thread join. + // Reclaims native handle/reference only after the supervisor actually exits. + // False includes a failed native wait; never authorize exit on an uncertain wait. + [[nodiscard]] bool PollStopped() noexcept; + +private: + void Run() noexcept; +#if defined(_WIN32) + static unsigned __stdcall Entry(void* context); +#endif + std::atomic state_{State::Idle}; + std::atomic_bool stop_{false}; + std::mutex access_; + SnapshotSaveService* service_ = nullptr; // borrowed only while access_ is held + std::vector paths_; + std::size_t count_ = 0; + std::array, SnapshotSaveQueue::MaxOutstanding>, + SnapshotSaveService::MaxDestinations> retained_; +#if defined(_WIN32) + void* thread_ = nullptr; + void* module_ = nullptr; +#endif +}; +} // namespace persistence diff --git a/tests/snapshot_save_host_test.cc b/tests/snapshot_save_host_test.cc new file mode 100644 index 000000000..6db1947b0 --- /dev/null +++ b/tests/snapshot_save_host_test.cc @@ -0,0 +1,210 @@ +#define MOD_SNAPSHOT_QUEUE_TESTING +#define MOD_SNAPSHOT_SERVICE_TESTING +#define MOD_SNAPSHOT_HOST_TESTING +#include "snapshot_save_queue.cc" +#include "snapshot_save_worker.cc" +#include "snapshot_save_service.cc" +#include "snapshot_save_host.cc" +#include "quit_drain_gate.h" +#include +#include +#include +#include + +namespace persistence +{ +namespace +{ +std::mutex testMutex; +std::condition_variable changed; +bool backendEntered = false, releaseBackend = false; +#if defined(_WIN32) +bool returnEntered = false, releaseReturn = false; +#endif +bool blockConstruction = false, constructionEntered = false, releaseConstruction = false; +void BeforeServiceWorkerStart(std::size_t) +{ + std::unique_lock lock(testMutex); + if (!blockConstruction) return; + constructionEntered = true; + changed.notify_all(); + changed.wait(lock, [] { return releaseConstruction; }); +} +file_transaction::Result Execute(const std::filesystem::path&, std::string_view bytes) +{ + std::unique_lock lock(testMutex); + backendEntered = true; + changed.notify_all(); + changed.wait(lock, [] { return releaseBackend; }); + file_transaction::Result result; + result.state = bytes == "fail" ? file_transaction::State::NotCommitted : file_transaction::State::Committed; + return result; +} +#if defined(_WIN32) +void BeforeHostThreadReturn() +{ + std::unique_lock lock(testMutex); + returnEntered = true; + changed.notify_all(); + changed.wait(lock, [] { return releaseReturn; }); +} +#endif +} // namespace +} // namespace persistence + +int main() +{ + using namespace persistence; + using Admission = SnapshotSaveQueue::Admission; + using State = SnapshotSaveHost::State; + std::promise done; + std::thread watchdog([future = done.get_future()] { + if (future.wait_for(std::chrono::seconds(15)) == std::future_status::timeout) std::abort(); + }); + QuitDrainGate gate; + assert(gate.Vote(true, false)); + assert(!gate.Vote(false, true) && !gate.DrainRequested()); + assert(!gate.TakeResumeRequest()); + { + QuitDrainGate cancelled; + assert(!cancelled.Vote(true, true) && cancelled.DrainRequested()); + assert(!cancelled.Vote(false, true)); + cancelled.ObserveStopped(); + assert(!cancelled.TakeResumeRequest()); // A later genuine veto cancels resume. + assert(cancelled.DrainRequested()); // It does not resurrect stopped workers. + } + const auto destination = std::filesystem::temp_directory_path() / "stfc-host-fixture.vars"; +#if defined(_WIN32) + { + SnapshotSaveHost host; + assert(host.PollStopped()); + assert(host.Start({destination})); + while (host.Status() == State::Starting) std::this_thread::yield(); + assert(host.Status() == State::Ready); + auto handle = host.TryGetDestination(0); + while (!handle) handle = host.TryGetDestination(0); + assert(host.TrySubmit(*handle, 1, std::string("fail")).state == Admission::Accepted); + { + std::unique_lock lock(testMutex); + changed.wait(lock, [] { return backendEntered; }); + } + assert(host.TrySubmit(*handle, 2, std::string("success")).state == Admission::Accepted); + assert(!gate.Vote(true, true) && gate.DrainRequested()); + host.RequestStop(); + std::string late = "unchanged"; + assert(host.TrySubmit(*handle, 3, std::move(late)).state == Admission::Stopping && late == "unchanged"); + assert(!host.PollStopped() && !gate.TakeResumeRequest()); + { + std::lock_guard lock(testMutex); + releaseBackend = true; + changed.notify_all(); + } + { + std::unique_lock lock(testMutex); + changed.wait(lock, [] { return returnEntered; }); + } + // Service and worker destruction have completed, but the native supervisor + // is deliberately still running. A published Stopped state is insufficient. + assert(host.Status() == State::Stopped && !host.PollStopped()); + assert(!gate.TakeResumeRequest()); + int failures = 0, commits = 0; + for (int i = 0; i != 2; ++i) { + auto completion = host.TryTakeCompletion(0); + assert(completion && completion->outcome == SnapshotSaveQueue::Outcome::Executed); + if (completion->result.committed()) ++commits; + else ++failures; + } + assert(failures == 1 && commits == 1 && !host.TryTakeCompletion(0)); + { + std::lock_guard lock(testMutex); + releaseReturn = true; + changed.notify_all(); + } + while (!host.PollStopped()) std::this_thread::yield(); + gate.ObserveStopped(); + assert(gate.TakeResumeRequest() && !gate.TakeResumeRequest()); + // Save failure cannot strand shutdown, but a real subscriber veto survives. + assert(!gate.Vote(false, true) && !gate.TakeResumeRequest()); + assert(gate.Vote(true, true)); + assert(!host.Start({destination})); + } + { + SnapshotSaveHost failed; + // Existing directory is invalid as a snapshot target. Failure is asynchronous + // and does not poison the process lease for the next independently owned host. + assert(failed.Start({std::filesystem::temp_directory_path()})); + while (!failed.PollStopped()) std::this_thread::yield(); + assert(failed.Status() == State::Unavailable); + } + { + { + std::lock_guard lock(testMutex); + blockConstruction = true; + } + SnapshotSaveHost starting; + assert(starting.Start({destination})); + { + std::unique_lock lock(testMutex); + changed.wait(lock, [] { return constructionEntered; }); + } + starting.RequestStop(); + assert(!starting.TryGetDestination(0) && !starting.PollStopped()); + { + std::lock_guard lock(testMutex); + releaseConstruction = true; + changed.notify_all(); + } + while (!starting.PollStopped()) std::this_thread::yield(); + assert(starting.Status() == State::Stopped); + } + { + SnapshotSaveHost concurrent; + assert(concurrent.Start({destination})); + while (concurrent.Status() == State::Starting) std::this_thread::yield(); + auto handle = concurrent.TryGetDestination(0); + while (!handle) handle = concurrent.TryGetDestination(0); + std::atomic_int accepted{0}; + std::thread producer([&] { + for (std::uint64_t revision = 1;; ++revision) { + std::string bytes = "success"; + const auto result = concurrent.TrySubmit(*handle, revision, std::move(bytes)); + if (result.state == Admission::Accepted) ++accepted; + else assert(bytes == "success"); + if (result.state == Admission::Stopping) break; + std::this_thread::yield(); + } + }); + while (accepted.load() == 0) std::this_thread::yield(); + concurrent.RequestStop(); + producer.join(); + while (!concurrent.PollStopped()) std::this_thread::yield(); + int completed = 0; + while (auto result = concurrent.TryTakeCompletion(0)) { + assert(result->result.committed()); + ++completed; + } + assert(completed == accepted.load()); + } + { + SnapshotSaveHost cancelled; + cancelled.RequestStop(); + std::vector paths{destination}; + assert(!cancelled.Start(std::move(paths)) && paths.size() == 1); + } + std::cout << "native supervisor, retained failures, launch-stop and quit gate passed\n"; +#else + SnapshotSaveHost unsupported; + std::vector paths{destination}; + assert(!unsupported.Start(std::move(paths)) && paths.size() == 1); + assert(unsupported.Status() == State::Unavailable && unsupported.PollStopped()); + std::string bytes = "untouched"; + assert(unsupported.TrySubmit({}, 1, std::move(bytes)).state == Admission::InvalidRequest && bytes == "untouched"); + assert(!gate.Vote(true, true)); + gate.ObserveStopped(); + assert(gate.TakeResumeRequest() && !gate.TakeResumeRequest()); + assert(!gate.Vote(false, true) && gate.Vote(true, true)); + std::cout << "quit gate and unsupported native host rejection passed (no macOS lifecycle claim)\n"; +#endif + done.set_value(); + watchdog.join(); +} From f5c5e316dd668eadbc78870f74bab7d51904c56f Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:09:52 -0500 Subject: [PATCH 10/20] Serialize quit votes with host activation and resume consumption --- docs/SNAPSHOT_SAVE_QUEUE.md | 8 +- .../patches/parts/runtime_snapshot_host.cc | 35 +++++--- mods/src/quit_drain_gate.h | 84 +++++++++++++++---- mods/src/snapshot_save_host.cc | 2 +- mods/src/snapshot_save_host.h | 2 +- tests/snapshot_save_host_test.cc | 54 +++++++++--- 6 files changed, 143 insertions(+), 42 deletions(-) diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index 8cb8e8c68..75aeb4ba9 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -233,12 +233,18 @@ is still an adapter responsibility; this is not a preserving user-TOML editor. Initialization runs after `il2cpp_init`, outside `DllMain`. It registers with the existing ScreenManager.Update owner but starts no thread. When inactive, its callback does no logging, timing capture or filesystem work. The quit detour is -installed only on the first real registration attempt. Admission requires an +installed during initialization so it can close registration even when a quit +request arrives before the first consumer. Admission requires an observed Update callback, the build261 Windows x64 method RVA, full instruction fingerprint, and native unwind extent. Mismatch leaves runtime saves unavailable. macOS and other architectures reject registration until their shutdown seam is validated; this does not disable the synchronous checked startup outputs. +Registration, quit votes, native-stop observation and automatic resume consumption +share one atomic lifecycle state. Registration either claims ownership before quit +is permitted or is rejected permanently. A delayed vote cannot re-arm an automatic +resume that has already been consumed. + An allowed game quit vote closes admission and wakes the supervisor. The game callback returns without waiting on storage. The supervisor removes borrowed service access under its mutex, drains accepted work, joins every worker, retains diff --git a/mods/src/patches/parts/runtime_snapshot_host.cc b/mods/src/patches/parts/runtime_snapshot_host.cc index 9bd379a93..6566319b6 100644 --- a/mods/src/patches/parts/runtime_snapshot_host.cc +++ b/mods/src/patches/parts/runtime_snapshot_host.cc @@ -45,7 +45,7 @@ bool WantsQuit(auto original) const bool gameAllows = original(); // Preserve the game's vote. Save failures are separate from whether native // code is still executing. No join, filesystem call, logging or timer here. - const bool result = gate.Vote(gameAllows, started.load()); + const bool result = gate.Vote(gameAllows); if (gate.DrainRequested() && !gate.Stopped()) host->RequestStop(); return result; } @@ -70,7 +70,10 @@ void UpdateHost() void InstallRuntimeSnapshotHost() { #if defined(_WIN32) && defined(_M_X64) - if (available) return; + static bool installedAttempt = false; + if (installedAttempt) return; + installedAttempt = true; + try { auto helper = il2cpp_get_class_helper("UnityEngine.CoreModule", "UnityEngine", "Application"); if (!helper.isValidHelper()) return; wantsMethod = helper.GetMethodInfo("Internal_ApplicationWantsToQuit", 0); @@ -80,7 +83,11 @@ void InstallRuntimeSnapshotHost() requestQuit = reinterpret_cast(quit->methodPointer); // Existing single Update owner; no additional Update detour. Observing its // callback is a prerequisite for Start, not just trusting an install return. - if (install_screen_manager_update_hook() && register_screen_manager_update_callback(UpdateHost)) available = true; + if (install_screen_manager_update_hook() && register_screen_manager_update_callback(UpdateHost) && + SPUD_STATIC_DETOUR(wantsMethod->methodPointer, WantsQuit)) available = true; + } catch (...) { + available.store(false); + } #endif } @@ -92,15 +99,21 @@ bool Start(std::vector&& paths) if (!available || updateThread.load() != GetCurrentThreadId() || attempted || wantsDepth != 0) return false; attempted = true; try { - // Install only when a real consumer enrolls, and recheck for client/other-hook - // drift immediately beforehand. A rejected seam cannot launch any worker. - if (!MatchesQuitMethod() || !SPUD_STATIC_DETOUR(wantsMethod->methodPointer, WantsQuit)) return false; + // The startup-installed quit gate serializes registration against a permitted + // quit, even before any consumer exists. Claiming after quit has begun fails. host = new Host; - // Publish stable control ownership BEFORE native launch. An off-thread quit - // during launch must be held too. Keep the control block even if launch fails; - // a concurrent callback may already reference it, and Update reaps/resumes. + if (!gate.TryActivate()) { + delete host; + host = nullptr; + return false; + } + // The gate publishes control ownership before launch, permitting only atomic + // RequestStop during Start. Publish consumer access AFTER Start returns so + // initialization cannot race completion readers. Keep failed-launch control + // too: a quit callback may reference it, and Update must reap/resume. + const bool launched = host->Start(std::move(paths)); started.store(true); - return host->Start(std::move(paths)); + return launched; } catch (...) { return false; } @@ -114,7 +127,7 @@ persistence::SnapshotSaveHost::State Status() noexcept { #if defined(_WIN32) && defined(_M_X64) if (started.load()) return host->Status(); - if (available && !attempted) return persistence::SnapshotSaveHost::State::Idle; + if (available && !attempted && !gate.Stopped()) return persistence::SnapshotSaveHost::State::Idle; #endif return persistence::SnapshotSaveHost::State::Unavailable; } diff --git a/mods/src/quit_drain_gate.h b/mods/src/quit_drain_gate.h index 3b79e560c..5c09bce58 100644 --- a/mods/src/quit_drain_gate.h +++ b/mods/src/quit_drain_gate.h @@ -3,33 +3,81 @@ namespace persistence { -// Receives the real game's vote; never turns a veto into permission to quit. -// The owner must ObserveStopped only after native supervisor termination. +#if defined(MOD_QUIT_DRAIN_GATE_TESTING) +namespace { void BeforeQuitVoteCompareExchange(); } +#endif +// Registration, real game votes and resume consumption share one atomic state. +// A permitted quit before registration permanently closes registration. After +// activation, only native supervisor termination can authorize normal exit. class QuitDrainGate final { + enum class Phase { Dormant, Active, DrainResume, DrainVeto, StoppedResume, StoppedVeto, Done }; public: - bool Vote(bool gameAllows, bool active) noexcept + bool TryActivate() noexcept { - if (!active) return gameAllows; - if (!gameAllows) { - resume_.store(false); - return false; + auto expected = Phase::Dormant; + return phase_.compare_exchange_strong(expected, Phase::Active); + } + bool Vote(bool gameAllows) noexcept + { + auto before = phase_.load(); + for (;;) { + auto after = before; + bool allow = false; + switch (before) { + case Phase::Dormant: + if (gameAllows) after = Phase::Done; + allow = gameAllows; + break; + case Phase::Active: + if (gameAllows) after = Phase::DrainResume; + break; + case Phase::DrainResume: + case Phase::DrainVeto: + after = gameAllows ? Phase::DrainResume : Phase::DrainVeto; + break; + case Phase::StoppedResume: + case Phase::StoppedVeto: + after = gameAllows ? Phase::Done : Phase::StoppedVeto; + allow = gameAllows; + break; + case Phase::Done: + return gameAllows; // Terminal: no stale vote can re-arm resumption. + } +#if defined(MOD_QUIT_DRAIN_GATE_TESTING) + BeforeQuitVoteCompareExchange(); +#endif + if (phase_.compare_exchange_weak(before, after)) return allow; } - if (stopped_.load()) return true; - requested_.store(true); - resume_.store(true); - return false; } - bool DrainRequested() const noexcept { return requested_.load(); } - void ObserveStopped() noexcept { stopped_.store(true); } - bool Stopped() const noexcept { return stopped_.load(); } + bool DrainRequested() const noexcept + { + const auto phase = phase_.load(); + return phase == Phase::DrainResume || phase == Phase::DrainVeto; + } + // Owner only, AFTER observing native supervisor termination (or failed launch). + void ObserveStopped() noexcept + { + auto before = phase_.load(); + for (;;) { + Phase after; + if (before == Phase::DrainResume) after = Phase::StoppedResume; + else if (before == Phase::Active || before == Phase::DrainVeto) after = Phase::StoppedVeto; + else return; + if (phase_.compare_exchange_weak(before, after)) return; + } + } + bool Stopped() const noexcept + { + const auto phase = phase_.load(); + return phase == Phase::StoppedResume || phase == Phase::StoppedVeto || phase == Phase::Done; + } bool TakeResumeRequest() noexcept { - // Consume before reentering Unity. A subsequent genuine veto cannot cause - // repeated automatic quit requests once the supervisor is gone. - return stopped_.load() && resume_.exchange(false); + auto expected = Phase::StoppedResume; + return phase_.compare_exchange_strong(expected, Phase::Done); } private: - std::atomic_bool requested_{false}, resume_{false}, stopped_{false}; + std::atomic phase_{Phase::Dormant}; }; } diff --git a/mods/src/snapshot_save_host.cc b/mods/src/snapshot_save_host.cc index e05e80b1f..5ad794f77 100644 --- a/mods/src/snapshot_save_host.cc +++ b/mods/src/snapshot_save_host.cc @@ -16,7 +16,7 @@ SnapshotSaveHost::~SnapshotSaveHost() if (!PollStopped()) std::terminate(); } -bool SnapshotSaveHost::Start(std::vector&& paths) +bool SnapshotSaveHost::Start(std::vector&& paths) noexcept { if (state_.load() != State::Idle) return false; state_.store(State::Unavailable); diff --git a/mods/src/snapshot_save_host.h b/mods/src/snapshot_save_host.h index 4f017a364..ab484c57f 100644 --- a/mods/src/snapshot_save_host.h +++ b/mods/src/snapshot_save_host.h @@ -22,7 +22,7 @@ class SnapshotSaveHost final // Trusted enrollment only. Takes ownership after successful launch; max four // paths, each bounded to 32767 native code units (including retained capacity). // An unsuccessful start is terminal. No retry/new session during shutdown. - [[nodiscard]] bool Start(std::vector&& paths); + [[nodiscard]] bool Start(std::vector&& paths) noexcept; [[nodiscard]] State Status() const noexcept { return state_.load(); } [[nodiscard]] std::optional TryGetDestination(std::size_t index); [[nodiscard]] SnapshotSaveQueue::Submission TrySubmit(SnapshotSaveService::Destination destination, diff --git a/tests/snapshot_save_host_test.cc b/tests/snapshot_save_host_test.cc index 6db1947b0..3dba46d16 100644 --- a/tests/snapshot_save_host_test.cc +++ b/tests/snapshot_save_host_test.cc @@ -5,6 +5,7 @@ #include "snapshot_save_worker.cc" #include "snapshot_save_service.cc" #include "snapshot_save_host.cc" +#define MOD_QUIT_DRAIN_GATE_TESTING #include "quit_drain_gate.h" #include #include @@ -22,6 +23,16 @@ bool backendEntered = false, releaseBackend = false; bool returnEntered = false, releaseReturn = false; #endif bool blockConstruction = false, constructionEntered = false, releaseConstruction = false; +std::atomic_bool blockVote{false}; +bool voteEntered = false, releaseVote = false; +void BeforeQuitVoteCompareExchange() +{ + if (!blockVote.exchange(false)) return; + std::unique_lock lock(testMutex); + voteEntered = true; + changed.notify_all(); + changed.wait(lock, [] { return releaseVote; }); +} void BeforeServiceWorkerStart(std::size_t) { std::unique_lock lock(testMutex); @@ -62,16 +73,39 @@ int main() if (future.wait_for(std::chrono::seconds(15)) == std::future_status::timeout) std::abort(); }); QuitDrainGate gate; - assert(gate.Vote(true, false)); - assert(!gate.Vote(false, true) && !gate.DrainRequested()); + QuitDrainGate dormant; + assert(dormant.Vote(true)); + assert(!dormant.TryActivate()); // A quit granted before launch forbids launch. + assert(gate.TryActivate()); + assert(!gate.Vote(false) && !gate.DrainRequested()); assert(!gate.TakeResumeRequest()); { QuitDrainGate cancelled; - assert(!cancelled.Vote(true, true) && cancelled.DrainRequested()); - assert(!cancelled.Vote(false, true)); + assert(cancelled.TryActivate()); + assert(!cancelled.Vote(true) && cancelled.DrainRequested()); + assert(!cancelled.Vote(false)); cancelled.ObserveStopped(); assert(!cancelled.TakeResumeRequest()); // A later genuine veto cancels resume. - assert(cancelled.DrainRequested()); // It does not resurrect stopped workers. + assert(!cancelled.TryActivate()); // It does not resurrect stopped workers. + } + { + QuitDrainGate racing; + assert(racing.TryActivate() && !racing.Vote(true)); + blockVote.store(true); + std::thread staleVote([&] { assert(racing.Vote(true)); }); + { + std::unique_lock lock(testMutex); + changed.wait(lock, [] { return voteEntered; }); + } + racing.ObserveStopped(); + assert(racing.TakeResumeRequest()); + { + std::lock_guard lock(testMutex); + releaseVote = true; + changed.notify_all(); + } + staleVote.join(); + assert(!racing.TakeResumeRequest()); // Stale CAS cannot re-arm a consumed resume. } const auto destination = std::filesystem::temp_directory_path() / "stfc-host-fixture.vars"; #if defined(_WIN32) @@ -89,7 +123,7 @@ int main() changed.wait(lock, [] { return backendEntered; }); } assert(host.TrySubmit(*handle, 2, std::string("success")).state == Admission::Accepted); - assert(!gate.Vote(true, true) && gate.DrainRequested()); + assert(!gate.Vote(true) && gate.DrainRequested()); host.RequestStop(); std::string late = "unchanged"; assert(host.TrySubmit(*handle, 3, std::move(late)).state == Admission::Stopping && late == "unchanged"); @@ -124,8 +158,8 @@ int main() gate.ObserveStopped(); assert(gate.TakeResumeRequest() && !gate.TakeResumeRequest()); // Save failure cannot strand shutdown, but a real subscriber veto survives. - assert(!gate.Vote(false, true) && !gate.TakeResumeRequest()); - assert(gate.Vote(true, true)); + assert(!gate.Vote(false) && !gate.TakeResumeRequest()); + assert(gate.Vote(true)); assert(!host.Start({destination})); } { @@ -199,10 +233,10 @@ int main() assert(unsupported.Status() == State::Unavailable && unsupported.PollStopped()); std::string bytes = "untouched"; assert(unsupported.TrySubmit({}, 1, std::move(bytes)).state == Admission::InvalidRequest && bytes == "untouched"); - assert(!gate.Vote(true, true)); + assert(!gate.Vote(true)); gate.ObserveStopped(); assert(gate.TakeResumeRequest() && !gate.TakeResumeRequest()); - assert(!gate.Vote(false, true) && gate.Vote(true, true)); + assert(!gate.Vote(false) && gate.Vote(true)); std::cout << "quit gate and unsupported native host rejection passed (no macOS lifecycle claim)\n"; #endif done.set_value(); From c586060fedc1e1da07c42cd951fc9bd1d99273e5 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:38:38 -0500 Subject: [PATCH 11/20] Preserve inherited Windows ACLs through private staging --- docs/CONFIG_FILE_TRANSACTIONS.md | 15 ++++++++ mods/src/file_transaction.cc | 61 +++++++++++++++++++++++++++++++- tests/file_transaction_test.cc | 33 +++++++++++++---- 3 files changed, 101 insertions(+), 8 deletions(-) diff --git a/docs/CONFIG_FILE_TRANSACTIONS.md b/docs/CONFIG_FILE_TRANSACTIONS.md index 276358c87..4622a4679 100644 --- a/docs/CONFIG_FILE_TRANSACTIONS.md +++ b/docs/CONFIG_FILE_TRANSACTIONS.md @@ -76,3 +76,18 @@ contention through a symlink while the canonical lock is held. These are specifi cases, not exhaustive ACL or filesystem coverage. Interrupted-process recovery and storage behavior under real faults still require follow-up evidence. Runtime latency/queue tests belong to the asynchronous phase. +# Windows inherited-permission correction + +Windows `ReplaceFileW` can rebuild inherited entries against the replacement's +parent directory. A private staging container with no inheritable entries caused +an ordinary inherited-only target ACL to become empty after a successful replace. +The writer now gives that container inherit-only copies of the target's inherited +file allow/deny entries. These entries grant no access to the container, and the +payload retains its protected private ACL until replacement. The resulting target +retains the original explicit/inherited permission policy, including inherited denies. + +Unknown inherited ACE forms/propagation flags, oversized ACLs and null target DACLs +are rejected before staging; the writer does not guess their inheritance behavior. +The Windows regression fixture creates an external file with inherited allow/deny +permissions and verifies two consecutive replacements preserve both content access +and the ordered ACL. Existing protected explicit-ACL coverage remains in place. diff --git a/mods/src/file_transaction.cc b/mods/src/file_transaction.cc index d2eafe773..0e26fccda 100644 --- a/mods/src/file_transaction.cc +++ b/mods/src/file_transaction.cc @@ -4,12 +4,14 @@ #include #include #include +#include #include #include #include #if _WIN32 #include +#include #include #pragma comment(lib, "advapi32.lib") #elif __APPLE__ @@ -133,6 +135,60 @@ namespace LocalFree(descriptor); } }; + struct StagingDirectorySecurity { + SECURITY_DESCRIPTOR descriptor{}; + std::vector aclBytes; + SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), &descriptor, FALSE}; + StagingDirectorySecurity(HANDLE original, PrivateSecurity& privateSecurity) + { + // ReplaceFile merges inherited ACEs using the replacement's parent. A + // private staging directory with no inheritable ACEs otherwise turns an + // inherited-only target DACL into an empty DACL. Mirror only the target's + // inherited file ACEs as inherit-only entries on our PRIVATE container. + // They grant no access to the container; staged payload stays protected. + PACL privateAcl = nullptr, originalAcl = nullptr; + BOOL present = FALSE, defaulted = FALSE; + if (!GetSecurityDescriptorDacl(privateSecurity.descriptor, &present, &privateAcl, &defaulted) || !privateAcl) + WinFail(); + struct Descriptor { + PSECURITY_DESCRIPTOR value = nullptr; + ~Descriptor() { if (value) LocalFree(value); } + } source; + if (original != INVALID_HANDLE_VALUE) { + const auto error = GetSecurityInfo(original, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, + &originalAcl, nullptr, &source.value); + if (error != ERROR_SUCCESS) throw Failure{{static_cast(error), std::system_category()}}; + if (!originalAcl) Fail(std::errc::operation_not_supported); + } + const size_t capacity = privateAcl->AclSize + (originalAcl ? originalAcl->AclSize : 0); + if (capacity > 65535) Fail(std::errc::operation_not_supported); + aclBytes.resize(capacity); + auto* acl = reinterpret_cast(aclBytes.data()); + if (!InitializeAcl(acl, static_cast(capacity), ACL_REVISION_DS)) WinFail(); + for (DWORD i = 0; i < privateAcl->AceCount; ++i) { + void* ace = nullptr; + if (!GetAce(privateAcl, i, &ace) || + !AddAce(acl, ACL_REVISION_DS, MAXDWORD, ace, static_cast(ace)->AceSize)) WinFail(); + } + if (originalAcl) for (DWORD i = 0; i < originalAcl->AceCount; ++i) { + void* raw = nullptr; + if (!GetAce(originalAcl, i, &raw)) WinFail(); + const auto* ace = static_cast(raw); + if (!(ace->AceFlags & INHERITED_ACE)) continue; + // Do not guess semantics of propagating/object/conditional file ACEs. + if (ace->AceFlags != INHERITED_ACE || + (ace->AceType != ACCESS_ALLOWED_ACE_TYPE && ace->AceType != ACCESS_DENIED_ACE_TYPE)) + Fail(std::errc::operation_not_supported); + std::vector copy(ace->AceSize); + std::memcpy(copy.data(), raw, copy.size()); + reinterpret_cast(copy.data())->AceFlags = INHERIT_ONLY_ACE | OBJECT_INHERIT_ACE; + if (!AddAce(acl, ACL_REVISION_DS, MAXDWORD, copy.data(), static_cast(copy.size()))) WinFail(); + } + if (!InitializeSecurityDescriptor(&descriptor, SECURITY_DESCRIPTOR_REVISION) || + !SetSecurityDescriptorDacl(&descriptor, TRUE, acl, FALSE) || + !SetSecurityDescriptorControl(&descriptor, SE_DACL_PROTECTED, SE_DACL_PROTECTED)) WinFail(); + } + }; void Regular(HANDLE handle) { BY_HANDLE_FILE_INFORMATION info{}; @@ -260,11 +316,14 @@ Result Write(const fs::path& destination, std::string_view bytes, Mode mode) Fail(std::errc::file_exists, State::Conflict); result.stage = Stage::StageFile; Checkpoint(result.stage); +#if _WIN32 + StagingDirectorySecurity directorySecurity(existing.value, security); +#endif for (int attempt = 0; attempt < 16; ++attempt) { auto candidate = target.parent_path() / (".stfc-save-" + std::to_string(++sequence)); #if _WIN32 candidate += "-" + std::to_string(GetCurrentProcessId()); - if (CreateDirectoryW(candidate.c_str(), &security.attributes)) { + if (CreateDirectoryW(candidate.c_str(), &directorySecurity.attributes)) { transaction = candidate; break; } diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc index 510289527..27393862b 100644 --- a/tests/file_transaction_test.cc +++ b/tests/file_transaction_test.cc @@ -35,19 +35,21 @@ std::string Read(const std::filesystem::path& path) #if _WIN32 std::pair Dacl(const std::filesystem::path& path) { - DWORD needed = 0; - GetFileSecurityW(path.c_str(), DACL_SECURITY_INFORMATION, nullptr, 0, &needed); - assert(needed); - std::vector descriptor(needed); - assert(GetFileSecurityW(path.c_str(), DACL_SECURITY_INFORMATION, descriptor.data(), needed, &needed)); + // Use the same ACL API family as the writer: querying a legacy descriptor can + // canonicalize its auto-inheritance flags before the operation being tested. + PSECURITY_DESCRIPTOR descriptor = nullptr; + auto name = path.native(); + assert(GetNamedSecurityInfoW(name.data(), SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, + nullptr, nullptr, &descriptor) == ERROR_SUCCESS); LPWSTR text = nullptr; - assert(ConvertSecurityDescriptorToStringSecurityDescriptorW(descriptor.data(), SDDL_REVISION_1, + assert(ConvertSecurityDescriptorToStringSecurityDescriptorW(descriptor, SDDL_REVISION_1, DACL_SECURITY_INFORMATION, &text, nullptr)); std::wstring result(text); LocalFree(text); SECURITY_DESCRIPTOR_CONTROL control{}; DWORD revision = 0; - assert(GetSecurityDescriptorControl(descriptor.data(), &control, &revision)); + assert(GetSecurityDescriptorControl(descriptor, &control, &revision)); + LocalFree(descriptor); const auto entries = result.find(L'('); assert(entries != std::wstring::npos); // ReplaceFile may add the informational AUTO_INHERITED marker. Compare the @@ -117,6 +119,23 @@ int main() const auto metadata = root / "metadata.toml"; assert(Write(metadata, "old", Mode::CreateOnly).committed()); #if _WIN32 + // Ordinary externally created files can have only inherited permissions. + // Replacing through a private subdirectory must not erase those entries. + PSECURITY_DESCRIPTOR inheritedSecurity = nullptr; + assert(ConvertStringSecurityDescriptorToSecurityDescriptorW( + L"D:PAI(D;OICI;FW;;;BG)(A;OICI;FA;;;OW)(A;OICI;FR;;;BU)", SDDL_REVISION_1, + &inheritedSecurity, nullptr)); + SECURITY_ATTRIBUTES inheritedAttributes{sizeof(SECURITY_ATTRIBUTES), inheritedSecurity, FALSE}; + const auto inheritedRoot = root / "inherited"; + assert(CreateDirectoryW(inheritedRoot.c_str(), &inheritedAttributes)); + LocalFree(inheritedSecurity); + const auto inheritedFile = inheritedRoot / "existing.toml"; + { std::ofstream out(inheritedFile); out << "old"; assert(out.good()); } + const auto inheritedDacl = Dacl(inheritedFile); + assert(Write(inheritedFile, "first", Mode::ReplaceSnapshot).committed()); + assert(Read(inheritedFile) == "first" && Dacl(inheritedFile) == inheritedDacl); + assert(Write(inheritedFile, "second", Mode::ReplaceSnapshot).committed()); + assert(Read(inheritedFile) == "second" && Dacl(inheritedFile) == inheritedDacl); PSECURITY_DESCRIPTOR restricted = nullptr; assert( ConvertStringSecurityDescriptorToSecurityDescriptorW(L"D:P(A;;FA;;;OW)", SDDL_REVISION_1, &restricted, nullptr)); From 0b8238cd49c17bd65f0c30a8913a4073b5a13634 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:43:46 -0500 Subject: [PATCH 12/20] test: distinguish inherited ACL regression failures --- tests/file_transaction_test.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc index 27393862b..d8d5e39c1 100644 --- a/tests/file_transaction_test.cc +++ b/tests/file_transaction_test.cc @@ -133,7 +133,15 @@ int main() { std::ofstream out(inheritedFile); out << "old"; assert(out.good()); } const auto inheritedDacl = Dacl(inheritedFile); assert(Write(inheritedFile, "first", Mode::ReplaceSnapshot).committed()); - assert(Read(inheritedFile) == "first" && Dacl(inheritedFile) == inheritedDacl); + const auto firstContent = Read(inheritedFile); + const auto firstDacl = Dacl(inheritedFile); + if (firstContent != "first" || firstDacl != inheritedDacl) { + std::cerr << "Inherited replacement content matches: " << (firstContent == "first") << '\n'; + std::wcerr << L"Before protected=" << inheritedDacl.first << L" " << inheritedDacl.second << L'\n' + << L"After protected=" << firstDacl.first << L" " << firstDacl.second << std::endl; + } + assert(firstContent == "first"); + assert(firstDacl == inheritedDacl); assert(Write(inheritedFile, "second", Mode::ReplaceSnapshot).committed()); assert(Read(inheritedFile) == "second" && Dacl(inheritedFile) == inheritedDacl); PSECURITY_DESCRIPTOR restricted = nullptr; From 408c93e92c658c263ccf17fc4694a252804b5122 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:46:18 -0500 Subject: [PATCH 13/20] fix: initialize private ACLs with modern inheritance control --- mods/src/file_transaction.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mods/src/file_transaction.cc b/mods/src/file_transaction.cc index 0e26fccda..0302633bc 100644 --- a/mods/src/file_transaction.cc +++ b/mods/src/file_transaction.cc @@ -124,7 +124,7 @@ namespace PrivateSecurity() { // Restrict staged data to its owner, administrators and SYSTEM. - if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(L"D:P(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;OW)", + if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(L"D:PAI(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;OW)", SDDL_REVISION_1, &descriptor, nullptr)) WinFail(); attributes.lpSecurityDescriptor = descriptor; @@ -186,7 +186,8 @@ namespace } if (!InitializeSecurityDescriptor(&descriptor, SECURITY_DESCRIPTOR_REVISION) || !SetSecurityDescriptorDacl(&descriptor, TRUE, acl, FALSE) || - !SetSecurityDescriptorControl(&descriptor, SE_DACL_PROTECTED, SE_DACL_PROTECTED)) WinFail(); + !SetSecurityDescriptorControl(&descriptor, SE_DACL_PROTECTED | SE_DACL_AUTO_INHERITED, + SE_DACL_PROTECTED | SE_DACL_AUTO_INHERITED)) WinFail(); } }; void Regular(HANDLE handle) From bc94fe4980d4ee33fba4a23b915905232e05f536 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:48:13 -0500 Subject: [PATCH 14/20] test: capture native inheritance flags in ACL fixture --- tests/file_transaction_test.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc index d8d5e39c1..8aa6dd138 100644 --- a/tests/file_transaction_test.cc +++ b/tests/file_transaction_test.cc @@ -33,6 +33,19 @@ std::string Read(const std::filesystem::path& path) } #if _WIN32 +void PrintNativeDacl(const std::filesystem::path& path) +{ + DWORD bytes = 0; + GetFileSecurityW(path.c_str(), DACL_SECURITY_INFORMATION, nullptr, 0, &bytes); + assert(bytes); + std::vector descriptor(bytes); + assert(GetFileSecurityW(path.c_str(), DACL_SECURITY_INFORMATION, descriptor.data(), bytes, &bytes)); + LPWSTR text = nullptr; + assert(ConvertSecurityDescriptorToStringSecurityDescriptorW(descriptor.data(), SDDL_REVISION_1, + DACL_SECURITY_INFORMATION, &text, nullptr)); + std::wcerr << L"Native DACL: " << text << std::endl; + LocalFree(text); +} std::pair Dacl(const std::filesystem::path& path) { // Use the same ACL API family as the writer: querying a legacy descriptor can @@ -131,8 +144,12 @@ int main() LocalFree(inheritedSecurity); const auto inheritedFile = inheritedRoot / "existing.toml"; { std::ofstream out(inheritedFile); out << "old"; assert(out.good()); } + PrintNativeDacl(inheritedRoot); + PrintNativeDacl(inheritedFile); const auto inheritedDacl = Dacl(inheritedFile); + PrintNativeDacl(inheritedFile); assert(Write(inheritedFile, "first", Mode::ReplaceSnapshot).committed()); + PrintNativeDacl(inheritedFile); const auto firstContent = Read(inheritedFile); const auto firstDacl = Dacl(inheritedFile); if (firstContent != "first" || firstDacl != inheritedDacl) { From 6bfd08e09ab684604633bdd97109e90c23defee2 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:49:32 -0500 Subject: [PATCH 15/20] fix: reject ambiguous legacy inheritance before replacing files --- docs/CONFIG_FILE_TRANSACTIONS.md | 6 ++++++ mods/src/file_transaction.cc | 20 +++++++++++++++--- tests/file_transaction_test.cc | 35 ++++++++++++++++++++++++++------ 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/docs/CONFIG_FILE_TRANSACTIONS.md b/docs/CONFIG_FILE_TRANSACTIONS.md index 4622a4679..878bc0748 100644 --- a/docs/CONFIG_FILE_TRANSACTIONS.md +++ b/docs/CONFIG_FILE_TRANSACTIONS.md @@ -86,6 +86,12 @@ file allow/deny entries. These entries grant no access to the container, and the payload retains its protected private ACL until replacement. The resulting target retains the original explicit/inherited permission policy, including inherited denies. +Unprotected legacy descriptors without native `SE_DACL_AUTO_INHERITED` are also +rejected before staging. `GetSecurityInfo` can synthesize inherited entries for +such a descriptor without updating the file; Windows client and server replacement +behavior differs for this case. The writer checks the native descriptor using +`GetKernelObjectSecurity` and does not migrate the original's permissions. + Unknown inherited ACE forms/propagation flags, oversized ACLs and null target DACLs are rejected before staging; the writer does not guess their inheritance behavior. The Windows regression fixture creates an external file with inherited allow/deny diff --git a/mods/src/file_transaction.cc b/mods/src/file_transaction.cc index 0302633bc..47a17196b 100644 --- a/mods/src/file_transaction.cc +++ b/mods/src/file_transaction.cc @@ -124,7 +124,7 @@ namespace PrivateSecurity() { // Restrict staged data to its owner, administrators and SYSTEM. - if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(L"D:PAI(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;OW)", + if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(L"D:P(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;OW)", SDDL_REVISION_1, &descriptor, nullptr)) WinFail(); attributes.lpSecurityDescriptor = descriptor; @@ -155,6 +155,21 @@ namespace ~Descriptor() { if (value) LocalFree(value); } } source; if (original != INVALID_HANDLE_VALUE) { + // GetSecurityInfo can synthesize modern inherited flags from a legacy + // descriptor without changing the file. ReplaceFile does not interpret + // that legacy policy consistently across Windows versions. Reject it + // before staging rather than convert permissions on the original file. + DWORD required = 0; + GetKernelObjectSecurity(original, DACL_SECURITY_INFORMATION, nullptr, 0, &required); + if (!required) WinFail(); + std::vector nativeDescriptor(required); + if (!GetKernelObjectSecurity(original, DACL_SECURITY_INFORMATION, nativeDescriptor.data(), required, + &required)) WinFail(); + SECURITY_DESCRIPTOR_CONTROL control{}; + DWORD revision = 0; + if (!GetSecurityDescriptorControl(nativeDescriptor.data(), &control, &revision)) WinFail(); + if (!(control & (SE_DACL_PROTECTED | SE_DACL_AUTO_INHERITED))) + Fail(std::errc::operation_not_supported); const auto error = GetSecurityInfo(original, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &originalAcl, nullptr, &source.value); if (error != ERROR_SUCCESS) throw Failure{{static_cast(error), std::system_category()}}; @@ -186,8 +201,7 @@ namespace } if (!InitializeSecurityDescriptor(&descriptor, SECURITY_DESCRIPTOR_REVISION) || !SetSecurityDescriptorDacl(&descriptor, TRUE, acl, FALSE) || - !SetSecurityDescriptorControl(&descriptor, SE_DACL_PROTECTED | SE_DACL_AUTO_INHERITED, - SE_DACL_PROTECTED | SE_DACL_AUTO_INHERITED)) WinFail(); + !SetSecurityDescriptorControl(&descriptor, SE_DACL_PROTECTED, SE_DACL_PROTECTED)) WinFail(); } }; void Regular(HANDLE handle) diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc index 8aa6dd138..5aa4eb0a5 100644 --- a/tests/file_transaction_test.cc +++ b/tests/file_transaction_test.cc @@ -33,7 +33,7 @@ std::string Read(const std::filesystem::path& path) } #if _WIN32 -void PrintNativeDacl(const std::filesystem::path& path) +std::wstring NativeDacl(const std::filesystem::path& path) { DWORD bytes = 0; GetFileSecurityW(path.c_str(), DACL_SECURITY_INFORMATION, nullptr, 0, &bytes); @@ -43,8 +43,9 @@ void PrintNativeDacl(const std::filesystem::path& path) LPWSTR text = nullptr; assert(ConvertSecurityDescriptorToStringSecurityDescriptorW(descriptor.data(), SDDL_REVISION_1, DACL_SECURITY_INFORMATION, &text, nullptr)); - std::wcerr << L"Native DACL: " << text << std::endl; + std::wstring result(text); LocalFree(text); + return result; } std::pair Dacl(const std::filesystem::path& path) { @@ -132,6 +133,13 @@ int main() const auto metadata = root / "metadata.toml"; assert(Write(metadata, "old", Mode::CreateOnly).committed()); #if _WIN32 + const auto ordinary = root / "ordinary.toml"; + { std::ofstream out(ordinary); out << "old"; assert(out.good()); } + const auto ordinaryDacl = Dacl(ordinary); + assert(Write(ordinary, "first", Mode::ReplaceSnapshot).committed()); + assert(Read(ordinary) == "first" && Dacl(ordinary) == ordinaryDacl); + assert(Write(ordinary, "second", Mode::ReplaceSnapshot).committed()); + assert(Read(ordinary) == "second" && Dacl(ordinary) == ordinaryDacl); // Ordinary externally created files can have only inherited permissions. // Replacing through a private subdirectory must not erase those entries. PSECURITY_DESCRIPTOR inheritedSecurity = nullptr; @@ -144,12 +152,27 @@ int main() LocalFree(inheritedSecurity); const auto inheritedFile = inheritedRoot / "existing.toml"; { std::ofstream out(inheritedFile); out << "old"; assert(out.good()); } - PrintNativeDacl(inheritedRoot); - PrintNativeDacl(inheritedFile); + // The create APIs above produce a legacy inheritance descriptor. The modern + // query API can synthesize inherited flags that ReplaceFile interprets + // differently on Windows client/server. Such originals must stay untouched. + const auto legacyDacl = NativeDacl(inheritedFile); + const auto legacyResult = Write(inheritedFile, "rejected", Mode::ReplaceSnapshot); + assert(legacyResult.state == State::NotCommitted && legacyResult.stage == Stage::StageFile); + assert(legacyResult.error == std::make_error_code(std::errc::operation_not_supported)); + assert(Read(inheritedFile) == "old" && NativeDacl(inheritedFile) == legacyDacl); + // Explicit fixture setup opts the parent into modern inheritance. Production + // never migrates an existing user's security policy as a side effect of save. + auto parentName = inheritedRoot.native(); + PSECURITY_DESCRIPTOR parentDescriptor = nullptr; + PACL parentDacl = nullptr; + assert(GetNamedSecurityInfoW(parentName.data(), SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, + nullptr, nullptr, &parentDacl, nullptr, &parentDescriptor) == ERROR_SUCCESS); + assert(SetNamedSecurityInfoW(parentName.data(), SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, + nullptr, nullptr, parentDacl, nullptr) == ERROR_SUCCESS); + LocalFree(parentDescriptor); const auto inheritedDacl = Dacl(inheritedFile); - PrintNativeDacl(inheritedFile); assert(Write(inheritedFile, "first", Mode::ReplaceSnapshot).committed()); - PrintNativeDacl(inheritedFile); const auto firstContent = Read(inheritedFile); const auto firstDacl = Dacl(inheritedFile); if (firstContent != "first" || firstDacl != inheritedDacl) { From c4095c7489f5bfeb83f1ede52c79222c90b24620 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 18:51:15 -0500 Subject: [PATCH 16/20] test: verify legacy refusal under platform temp defaults --- tests/file_transaction_test.cc | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/file_transaction_test.cc b/tests/file_transaction_test.cc index 5aa4eb0a5..0665537b6 100644 --- a/tests/file_transaction_test.cc +++ b/tests/file_transaction_test.cc @@ -136,10 +136,21 @@ int main() const auto ordinary = root / "ordinary.toml"; { std::ofstream out(ordinary); out << "old"; assert(out.good()); } const auto ordinaryDacl = Dacl(ordinary); - assert(Write(ordinary, "first", Mode::ReplaceSnapshot).committed()); - assert(Read(ordinary) == "first" && Dacl(ordinary) == ordinaryDacl); - assert(Write(ordinary, "second", Mode::ReplaceSnapshot).committed()); - assert(Read(ordinary) == "second" && Dacl(ordinary) == ordinaryDacl); + const auto ordinaryNativeDacl = NativeDacl(ordinary); + const auto ordinaryResult = Write(ordinary, "first", Mode::ReplaceSnapshot); + if (ordinaryNativeDacl.find(L"AI") == std::wstring::npos && + ordinaryNativeDacl.substr(0, ordinaryNativeDacl.find(L'(')).find(L'P') == std::wstring::npos) { + // Some Windows installations create legacy descriptors even under their + // normal temp directory. Verify the precise supported rejection there. + assert(ordinaryResult.state == State::NotCommitted && ordinaryResult.stage == Stage::StageFile); + assert(ordinaryResult.error == std::make_error_code(std::errc::operation_not_supported)); + assert(Read(ordinary) == "old" && NativeDacl(ordinary) == ordinaryNativeDacl); + } else { + assert(ordinaryResult.committed()); + assert(Read(ordinary) == "first" && Dacl(ordinary) == ordinaryDacl); + assert(Write(ordinary, "second", Mode::ReplaceSnapshot).committed()); + assert(Read(ordinary) == "second" && Dacl(ordinary) == ordinaryDacl); + } // Ordinary externally created files can have only inherited permissions. // Replacing through a private subdirectory must not erase those entries. PSECURITY_DESCRIPTOR inheritedSecurity = nullptr; From 87008dacd24f9aec92dcbafe8c43c794255d2c18 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 19:15:45 -0500 Subject: [PATCH 17/20] Restore Windows force-close quit shortcut --- docs/SNAPSHOT_SAVE_QUEUE.md | 8 +++----- mods/src/patches/parts/hotkeys.cc | 11 +++++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index 75aeb4ba9..52df7336c 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -206,11 +206,9 @@ Shutdown policy: distinguish the attempt's result from whether worker shutdown finished. A failed save remains failed; once workers terminate, save failure alone must not veto process exit. In-flight OS I/O cannot safely be cancelled by a queue flag. Preserve transaction-owned recovery artifacts when an outcome is uncertain. -The mod Quit shortcut now calls `PrimeApp::Quit` on Windows as it already did on -macOS. Windows no longer uses `TerminateProcess`, which bypassed Unity quit -subscribers. An unavailable app or method does not trigger a force-kill fallback. -This routing change alone does not drain saves. The lazy host described below -supplies shutdown coordination when a runtime consumer starts it. Result presentation +The Windows mod Quit shortcut (F10 by default) retains its existing immediate +`TerminateProcess` behavior. It bypasses the normal quit/drain lifecycle described +below. The macOS shortcut continues to call `PrimeApp::Quit`. Result presentation and targeted user-TOML editing remain separate integration work; no current feature registers a runtime destination or submits runtime saves. diff --git a/mods/src/patches/parts/hotkeys.cc b/mods/src/patches/parts/hotkeys.cc index 7f40d43b3..3c6043d72 100644 --- a/mods/src/patches/parts/hotkeys.cc +++ b/mods/src/patches/parts/hotkeys.cc @@ -494,11 +494,14 @@ void ScreenManager_Update_Hook(auto original, ScreenManager* _this) return; } -#if defined(_WIN32) || defined(__APPLE__) +#if defined(_WIN32) + if (MapKey::IsDown(GameFunction::Quit)) { + // Keep the existing emergency force-close shortcut independent of Unity quit. + TerminateProcess(GetCurrentProcess(), 1); + return; + } +#elif defined(__APPLE__) if (MapKey::IsDown(GameFunction::Quit)) { - // Use the normal game lifecycle on both platforms. Forced termination - // bypasses quit subscribers and any pending-save shutdown coordination. - // If the app/method is unavailable, do not fall back to killing the process. if (auto* app = Hub::get_App()) { app->Quit(); } From fdc5350dfbe22d0cc3e1e7f0ae8da4933c49d13f Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 19:22:41 -0500 Subject: [PATCH 18/20] Give force-close a 500ms native save cancellation grace period --- .github/workflows/persistence-tests.yaml | 4 +- docs/SNAPSHOT_SAVE_QUEUE.md | 21 ++++- mods/src/force_close.cc | 57 +++++++++++ mods/src/force_close.h | 11 +++ mods/src/patches/parts/hotkeys.cc | 4 +- .../patches/parts/runtime_snapshot_host.cc | 20 +++- mods/src/patches/runtime_snapshot_host.h | 5 + mods/src/snapshot_save_host.cc | 27 +++++- mods/src/snapshot_save_host.h | 9 +- tests/force_close_test.cc | 94 +++++++++++++++++++ 10 files changed, 240 insertions(+), 12 deletions(-) create mode 100644 mods/src/force_close.cc create mode 100644 mods/src/force_close.h create mode 100644 tests/force_close_test.cc diff --git a/.github/workflows/persistence-tests.yaml b/.github/workflows/persistence-tests.yaml index 0796ebd57..0d01b90da 100644 --- a/.github/workflows/persistence-tests.yaml +++ b/.github/workflows/persistence-tests.yaml @@ -23,7 +23,7 @@ jobs: if: runner.os == 'Windows' shell: pwsh run: | - foreach ($fixture in @('file_transaction', 'snapshot_save_queue', 'snapshot_save_worker', 'snapshot_save_service', 'snapshot_save_host')) { + foreach ($fixture in @('file_transaction', 'snapshot_save_queue', 'snapshot_save_worker', 'snapshot_save_service', 'snapshot_save_host', 'force_close')) { clang++ -std=c++23 -Wall -Wextra -Werror -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test.exe" -ladvapi32 if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } & "./${fixture}_test.exe" @@ -35,7 +35,7 @@ jobs: env: MACOSX_DEPLOYMENT_TARGET: '13.5' run: | - for fixture in file_transaction snapshot_save_queue snapshot_save_worker snapshot_save_service snapshot_save_host; do + for fixture in file_transaction snapshot_save_queue snapshot_save_worker snapshot_save_service snapshot_save_host force_close; do clang++ -std=c++23 -Wall -Wextra -Werror -pthread -I mods/src "tests/${fixture}_test.cc" -o "${fixture}_test" "./${fixture}_test" done diff --git a/docs/SNAPSHOT_SAVE_QUEUE.md b/docs/SNAPSHOT_SAVE_QUEUE.md index 52df7336c..dfd8a506a 100644 --- a/docs/SNAPSHOT_SAVE_QUEUE.md +++ b/docs/SNAPSHOT_SAVE_QUEUE.md @@ -206,9 +206,17 @@ Shutdown policy: distinguish the attempt's result from whether worker shutdown finished. A failed save remains failed; once workers terminate, save failure alone must not veto process exit. In-flight OS I/O cannot safely be cancelled by a queue flag. Preserve transaction-owned recovery artifacts when an outcome is uncertain. -The Windows mod Quit shortcut (F10 by default) retains its existing immediate -`TerminateProcess` behavior. It bypasses the normal quit/drain lifecycle described -below. The macOS shortcut continues to call `PrimeApp::Quit`. Result presentation +The Windows mod Quit shortcut (F10 by default) force-closes independently of Unity +after up to 500 ms of best-effort save cleanup. With no active supervisor it exits +immediately. Otherwise a native deadline thread requests cancellation of queued +writes and waits on a duplicated supervisor handle. It terminates the process as +soon as that handle signals or the grace period expires; it never joins a writer +or needs another Update callback. Cancellation can escalate an existing normal +drain. Active writes finish normally during the grace period. Thread/handle setup +failure falls back to immediate termination. The deadline begins when the shortcut +is received; OS scheduling still affects when termination actually executes. +F10 detection itself remains in the existing game input handler. +The macOS shortcut continues to call `PrimeApp::Quit`. Result presentation and targeted user-TOML editing remain separate integration work; no current feature registers a runtime destination or submits runtime saves. @@ -273,6 +281,13 @@ between service destruction and native supervisor exit. Other platform fixtures verify the quit gate and unsupported-host rejection only. These are isolated native tests, not evidence of exact-artifact F10/window-X runtime behavior. +The Windows force-close fixture launches isolated child processes with no further +owner updates after F10. It checks idle exit, an indefinitely blocked writer, an +active writer that finishes, and escalation of normal draining to queued-write +cancellation. Children must terminate through the force-close path; the queued +second write must never execute. This is native process evidence, not a live-game +input or forced-write durability test. + Native lifecycle references: Microsoft documents the retained caller-owned handle and automatic CRT cleanup for [_beginthreadex](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex), zero-timeout [WaitForSingleObject](https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobject), diff --git a/mods/src/force_close.cc b/mods/src/force_close.cc new file mode 100644 index 000000000..6c1e823c8 --- /dev/null +++ b/mods/src/force_close.cc @@ -0,0 +1,57 @@ +#include "force_close.h" +#if defined(_WIN32) +#include +#include + +namespace persistence +{ +namespace +{ +constexpr ULONGLONG GraceMs = 500; +struct Request { + SnapshotSaveHost* host; + HANDLE supervisor; + ULONGLONG began; +}; +Request request{}; // Single process-exit request; no heap or static destructor. +std::atomic_bool requested{false}; +unsigned __stdcall Finish(void*) +{ + // This thread never joins a writer, takes a blocking host lock, calls Unity, + // or reaps the supervisor. Its own deadline remains independent of storage. + for (;;) { + const auto elapsed = GetTickCount64() - request.began; + if (elapsed >= GraceMs) break; + request.host->RequestStop(StopMode::CancelQueued); + const auto result = WaitForSingleObject(request.supervisor, 1); + if (result != WAIT_TIMEOUT) break; // Native completion or failed observation. + } + TerminateProcess(GetCurrentProcess(), 1); + return 0; +} +} + +void ForceClose(SnapshotSaveHost* host) noexcept +{ + if (requested.exchange(true)) return; + request.began = GetTickCount64(); + request.host = host; + void* duplicate = nullptr; + if (!host || !host->DuplicateThread(duplicate) || !duplicate) { + TerminateProcess(GetCurrentProcess(), 1); + return; + } + request.supervisor = static_cast(duplicate); + host->RequestStop(StopMode::CancelQueued); + const auto thread = _beginthreadex(nullptr, 0, Finish, nullptr, 0, nullptr); + if (!thread) { + CloseHandle(request.supervisor); + TerminateProcess(GetCurrentProcess(), 1); + return; + } + // The process is exiting; no detach/join destructor or callback owns this + // thread. Its handles and the fixed request live only until termination. + CloseHandle(reinterpret_cast(thread)); +} +} +#endif diff --git a/mods/src/force_close.h b/mods/src/force_close.h new file mode 100644 index 000000000..3f6cf43e5 --- /dev/null +++ b/mods/src/force_close.h @@ -0,0 +1,11 @@ +#pragma once +#include "snapshot_save_host.h" + +namespace persistence +{ +#if defined(_WIN32) +// Owner thread only, after Start returns. Host must outlive the process. Once +// called, no other caller may start/reap the host. Does not call Unity. +void ForceClose(SnapshotSaveHost* host) noexcept; +#endif +} diff --git a/mods/src/patches/parts/hotkeys.cc b/mods/src/patches/parts/hotkeys.cc index 3c6043d72..a583a7ed2 100644 --- a/mods/src/patches/parts/hotkeys.cc +++ b/mods/src/patches/parts/hotkeys.cc @@ -1,4 +1,5 @@ #include "config.h" +#include "patches/runtime_snapshot_host.h" #include @@ -496,8 +497,7 @@ void ScreenManager_Update_Hook(auto original, ScreenManager* _this) #if defined(_WIN32) if (MapKey::IsDown(GameFunction::Quit)) { - // Keep the existing emergency force-close shortcut independent of Unity quit. - TerminateProcess(GetCurrentProcess(), 1); + runtime_snapshots::ForceClose(); return; } #elif defined(__APPLE__) diff --git a/mods/src/patches/parts/runtime_snapshot_host.cc b/mods/src/patches/parts/runtime_snapshot_host.cc index 6566319b6..de89cea3f 100644 --- a/mods/src/patches/parts/runtime_snapshot_host.cc +++ b/mods/src/patches/parts/runtime_snapshot_host.cc @@ -1,4 +1,5 @@ #include "patches/runtime_snapshot_host.h" +#include "force_close.h" #if defined(_WIN32) && defined(_M_X64) #include "patches/screen_update_hook.h" @@ -21,6 +22,7 @@ std::atomic updateThread{0}; const MethodInfo* wantsMethod = nullptr; void (*requestQuit)(int) = nullptr; std::atomic_bool available{false}, attempted{false}; +std::atomic_bool forceClosing{false}; thread_local unsigned wantsDepth = 0; // Exact build261 Windows x64 discovery: native extent411 bytes, SPUD overwrite24. @@ -52,6 +54,7 @@ bool WantsQuit(auto original) void UpdateHost() { + if (forceClosing.load()) return; DWORD unset = 0; updateThread.compare_exchange_strong(unset, GetCurrentThreadId()); if (updateThread.load() != GetCurrentThreadId() || wantsDepth != 0 || !started.load()) return; @@ -93,10 +96,25 @@ void InstallRuntimeSnapshotHost() namespace runtime_snapshots { +#if defined(_WIN32) +void ForceClose() noexcept +{ +#if defined(_M_X64) + // Start and handle reaping belong to this same observed owner. Never borrow + // its native handle from an unknown/concurrent callback. + if (updateThread.load() == GetCurrentThreadId() && wantsDepth == 0) { + forceClosing.store(true); + persistence::ForceClose(started.load() ? host : nullptr); + return; + } +#endif + persistence::ForceClose(nullptr); +} +#endif bool Start(std::vector&& paths) { #if defined(_WIN32) && defined(_M_X64) - if (!available || updateThread.load() != GetCurrentThreadId() || attempted || wantsDepth != 0) return false; + if (forceClosing.load() || !available || updateThread.load() != GetCurrentThreadId() || attempted || wantsDepth != 0) return false; attempted = true; try { // The startup-installed quit gate serializes registration against a permitted diff --git a/mods/src/patches/runtime_snapshot_host.h b/mods/src/patches/runtime_snapshot_host.h index 9c8bbae45..c198c1497 100644 --- a/mods/src/patches/runtime_snapshot_host.h +++ b/mods/src/patches/runtime_snapshot_host.h @@ -7,6 +7,11 @@ void InstallRuntimeSnapshotHost(); namespace runtime_snapshots { +#if defined(_WIN32) +// F10 owner callback: best-effort native cancellation, then force close <=500ms +// of grace without requiring another Unity callback. +void ForceClose() noexcept; +#endif // Internal trusted registration, called on the observed game Update thread after // installation. False leaves input owned by the caller. One attempt per process. // Unsupported clients/platforms cannot start a worker through this adapter. diff --git a/mods/src/snapshot_save_host.cc b/mods/src/snapshot_save_host.cc index 5ad794f77..68bff4179 100644 --- a/mods/src/snapshot_save_host.cc +++ b/mods/src/snapshot_save_host.cc @@ -81,12 +81,29 @@ std::optional SnapshotSaveHost::TryTakeCompletion return std::nullopt; } -void SnapshotSaveHost::RequestStop() noexcept +void SnapshotSaveHost::RequestStop(StopMode mode) noexcept { + if (mode == StopMode::CancelQueued) cancelQueued_.store(true); stop_.store(true); stop_.notify_all(); + if (cancelQueued_.load()) { + std::unique_lock lock(access_, std::try_to_lock); + if (lock) { + auto* target = service_ ? service_ : draining_; + if (target) target->RequestStop(StopMode::CancelQueued); + } + } } +#if defined(_WIN32) +bool SnapshotSaveHost::DuplicateThread(void*& duplicate) const noexcept +{ + duplicate = nullptr; + return !thread_ || DuplicateHandle(GetCurrentProcess(), static_cast(thread_), GetCurrentProcess(), + reinterpret_cast(&duplicate), SYNCHRONIZE, FALSE, 0); +} +#endif + bool SnapshotSaveHost::PollStopped() noexcept { #if defined(_WIN32) @@ -130,10 +147,12 @@ void SnapshotSaveHost::Run() noexcept std::lock_guard lock(access_); state_.store(State::Stopping); service_ = nullptr; + draining_ = service.get(); } - service->StopAndJoin(StopMode::DrainAccepted); + service->StopAndJoin(cancelQueued_.load() ? StopMode::CancelQueued : StopMode::DrainAccepted); { std::lock_guard lock(access_); + draining_ = nullptr; for (std::size_t i = 0; i < count_; ++i) for (auto& slot : retained_[i]) { slot = service->TryTakeCompletion(service->GetDestination(i)); @@ -146,12 +165,14 @@ void SnapshotSaveHost::Run() noexcept { std::lock_guard lock(access_); service_ = nullptr; + draining_ = service.get(); } // Constructor rollback already joins partial workers. If a later operation // throws, still finish accepted work before allowing native thread exit. if (service) { - try { service->StopAndJoin(StopMode::DrainAccepted); } + try { service->StopAndJoin(cancelQueued_.load() ? StopMode::CancelQueued : StopMode::DrainAccepted); } catch (...) { std::terminate(); } // Never destroy possibly joinable workers. + { std::lock_guard lock(access_); draining_ = nullptr; } service.reset(); } state_.store(State::Unavailable); diff --git a/mods/src/snapshot_save_host.h b/mods/src/snapshot_save_host.h index ab484c57f..f928567ff 100644 --- a/mods/src/snapshot_save_host.h +++ b/mods/src/snapshot_save_host.h @@ -28,7 +28,12 @@ class SnapshotSaveHost final [[nodiscard]] SnapshotSaveQueue::Submission TrySubmit(SnapshotSaveService::Destination destination, std::uint64_t revision, std::string&& bytes); [[nodiscard]] std::optional TryTakeCompletion(std::size_t index); - void RequestStop() noexcept; + void RequestStop(StopMode mode = StopMode::DrainAccepted) noexcept; +#if defined(_WIN32) + // Owner-thread snapshot of the native supervisor handle. Caller closes the + // duplicate; observing it needs no owner Update callback or host lock. + [[nodiscard]] bool DuplicateThread(void*& duplicate) const noexcept; +#endif // Zero-timeout native thread observation, NOT WorkEnded or a std::thread join. // Reclaims native handle/reference only after the supervisor actually exits. // False includes a failed native wait; never authorize exit on an uncertain wait. @@ -41,8 +46,10 @@ class SnapshotSaveHost final #endif std::atomic state_{State::Idle}; std::atomic_bool stop_{false}; + std::atomic_bool cancelQueued_{false}; std::mutex access_; SnapshotSaveService* service_ = nullptr; // borrowed only while access_ is held + SnapshotSaveService* draining_ = nullptr; // cancellation only, same lock/lifetime std::vector paths_; std::size_t count_ = 0; std::array, SnapshotSaveQueue::MaxOutstanding>, diff --git a/tests/force_close_test.cc b/tests/force_close_test.cc new file mode 100644 index 000000000..0877752d0 --- /dev/null +++ b/tests/force_close_test.cc @@ -0,0 +1,94 @@ +#define MOD_SNAPSHOT_QUEUE_TESTING +#include "snapshot_save_queue.cc" +#include "snapshot_save_worker.cc" +#include "snapshot_save_service.cc" +#include "snapshot_save_host.cc" +#include "force_close.cc" +#include +#include + +#if defined(_WIN32) +struct Capture { volatile LONG calls; ULONGLONG began; }; +Capture* capture; +int mode; +namespace persistence +{ +namespace +{ +file_transaction::Result Execute(const std::filesystem::path&, std::string_view) +{ + InterlockedIncrement(&capture->calls); + if (mode == 1) Sleep(INFINITE); // Storage never returns; deadline must still win. + Sleep(80); + file_transaction::Result result; + result.state = file_transaction::State::Committed; + return result; +} +} +} + +int wmain(int argc, wchar_t** argv) +{ + const bool child = argc == 4; + const auto owner = child ? std::stoul(argv[3]) : GetCurrentProcessId(); + const auto name = L"Local\\STFCForceCloseTest_" + std::to_wstring(owner); + const auto mapping = child ? OpenFileMappingW(FILE_MAP_ALL_ACCESS, FALSE, name.c_str()) : + CreateFileMappingW(INVALID_HANDLE_VALUE, nullptr, PAGE_READWRITE, 0, sizeof(Capture), name.c_str()); + assert(mapping); + capture = static_cast(MapViewOfFile(mapping, FILE_MAP_ALL_ACCESS, 0, 0, sizeof(Capture))); + assert(capture); + if (child) { + mode = std::stoi(argv[2]); + persistence::SnapshotSaveHost* host = nullptr; + if (mode) { + host = new persistence::SnapshotSaveHost; + std::vector paths; + paths.push_back(std::filesystem::temp_directory_path() / (L"force-close-" + std::to_wstring(owner))); + assert(host->Start(std::move(paths))); + std::optional destination; + while (!(destination = host->TryGetDestination(0))) Sleep(1); + using Admission = persistence::SnapshotSaveQueue::Admission; + while (host->TrySubmit(*destination, 1, "first").state != Admission::Accepted) Sleep(1); + while (!InterlockedCompareExchange(&capture->calls, 0, 0)) Sleep(1); + while (host->TrySubmit(*destination, 2, "queued").state != Admission::Accepted) Sleep(1); + if (mode == 3) host->RequestStop(); // Escalate an already requested normal drain. + } + capture->began = GetTickCount64(); + persistence::ForceClose(host); + Sleep(INFINITE); // Simulate no further Unity/owner updates. + return 99; + } + wchar_t executable[32768]{}; + assert(GetModuleFileNameW(nullptr, executable, 32768)); + for (int scenario = 0; scenario != 4; ++scenario) { + capture->calls = 0; + capture->began = 0; + auto command = L"\"" + std::wstring(executable) + L"\" --child " + std::to_wstring(scenario) + + L" " + std::to_wstring(owner); + STARTUPINFOW startup{}; + startup.cb = sizeof(startup); + PROCESS_INFORMATION process{}; + assert(CreateProcessW(nullptr, command.data(), nullptr, nullptr, FALSE, CREATE_NO_WINDOW, + nullptr, nullptr, &startup, &process)); + const auto waited = WaitForSingleObject(process.hProcess, 5000); + if (waited != WAIT_OBJECT_0) TerminateProcess(process.hProcess, 98); + assert(waited == WAIT_OBJECT_0); + DWORD code = 0; + assert(GetExitCodeProcess(process.hProcess, &code) && code == 1); + const auto elapsed = GetTickCount64() - capture->began; + assert(capture->began && elapsed < 2500); // Scheduling tolerance, not a 2.5s policy. + if (scenario == 1) assert(elapsed >= 450); + assert(capture->calls == (scenario ? 1 : 0)); // Queued second write never executes. + CloseHandle(process.hThread); + CloseHandle(process.hProcess); + std::cout << "force-close scenario " << scenario << ": " << elapsed << "ms\n"; + } + UnmapViewOfFile(capture); + CloseHandle(mapping); +} +#else +namespace persistence { namespace { +file_transaction::Result Execute(const std::filesystem::path&, std::string_view) { return {}; } +} } +int main() { std::cout << "Windows-only force-close fixture skipped\n"; } +#endif From a1e84d670546af874aea550cceacd444ed322a06 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 19:25:38 -0500 Subject: [PATCH 19/20] Make force-close cancellation visible directly at queue selection --- mods/src/snapshot_save_host.cc | 15 +++------------ mods/src/snapshot_save_host.h | 4 +++- mods/src/snapshot_save_queue.cc | 7 ++++--- mods/src/snapshot_save_queue.h | 5 ++++- mods/src/snapshot_save_service.cc | 5 +++-- mods/src/snapshot_save_service.h | 4 +++- mods/src/snapshot_save_worker.cc | 5 +++-- mods/src/snapshot_save_worker.h | 4 +++- tests/force_close_test.cc | 24 ++++++++++++++++++++++-- 9 files changed, 48 insertions(+), 25 deletions(-) diff --git a/mods/src/snapshot_save_host.cc b/mods/src/snapshot_save_host.cc index 68bff4179..991e6ae50 100644 --- a/mods/src/snapshot_save_host.cc +++ b/mods/src/snapshot_save_host.cc @@ -86,13 +86,6 @@ void SnapshotSaveHost::RequestStop(StopMode mode) noexcept if (mode == StopMode::CancelQueued) cancelQueued_.store(true); stop_.store(true); stop_.notify_all(); - if (cancelQueued_.load()) { - std::unique_lock lock(access_, std::try_to_lock); - if (lock) { - auto* target = service_ ? service_ : draining_; - if (target) target->RequestStop(StopMode::CancelQueued); - } - } } #if defined(_WIN32) @@ -135,7 +128,9 @@ void SnapshotSaveHost::Run() noexcept // the native supervisor. Remove producer access BEFORE draining/destruction. std::unique_ptr service; try { - service = std::make_unique(paths_); + // Host lifetime extends through native supervisor termination. Queues read + // this sticky flag at selection, including during a previously started drain. + service = std::make_unique(paths_, &cancelQueued_); paths_.clear(); { std::lock_guard lock(access_); @@ -147,12 +142,10 @@ void SnapshotSaveHost::Run() noexcept std::lock_guard lock(access_); state_.store(State::Stopping); service_ = nullptr; - draining_ = service.get(); } service->StopAndJoin(cancelQueued_.load() ? StopMode::CancelQueued : StopMode::DrainAccepted); { std::lock_guard lock(access_); - draining_ = nullptr; for (std::size_t i = 0; i < count_; ++i) for (auto& slot : retained_[i]) { slot = service->TryTakeCompletion(service->GetDestination(i)); @@ -165,14 +158,12 @@ void SnapshotSaveHost::Run() noexcept { std::lock_guard lock(access_); service_ = nullptr; - draining_ = service.get(); } // Constructor rollback already joins partial workers. If a later operation // throws, still finish accepted work before allowing native thread exit. if (service) { try { service->StopAndJoin(cancelQueued_.load() ? StopMode::CancelQueued : StopMode::DrainAccepted); } catch (...) { std::terminate(); } // Never destroy possibly joinable workers. - { std::lock_guard lock(access_); draining_ = nullptr; } service.reset(); } state_.store(State::Unavailable); diff --git a/mods/src/snapshot_save_host.h b/mods/src/snapshot_save_host.h index f928567ff..2b4327442 100644 --- a/mods/src/snapshot_save_host.h +++ b/mods/src/snapshot_save_host.h @@ -40,6 +40,9 @@ class SnapshotSaveHost final [[nodiscard]] bool PollStopped() noexcept; private: +#if defined(MOD_SNAPSHOT_HOST_TESTING) + friend struct SnapshotHostTestAccess; +#endif void Run() noexcept; #if defined(_WIN32) static unsigned __stdcall Entry(void* context); @@ -49,7 +52,6 @@ class SnapshotSaveHost final std::atomic_bool cancelQueued_{false}; std::mutex access_; SnapshotSaveService* service_ = nullptr; // borrowed only while access_ is held - SnapshotSaveService* draining_ = nullptr; // cancellation only, same lock/lifetime std::vector paths_; std::size_t count_ = 0; std::array, SnapshotSaveQueue::MaxOutstanding>, diff --git a/mods/src/snapshot_save_queue.cc b/mods/src/snapshot_save_queue.cc index 3f358c586..a9b2b1634 100644 --- a/mods/src/snapshot_save_queue.cc +++ b/mods/src/snapshot_save_queue.cc @@ -25,8 +25,9 @@ namespace #endif } // namespace -SnapshotSaveQueue::SnapshotSaveQueue(const std::filesystem::path& trustedDestination) - : destination_(Resolve(trustedDestination)) +SnapshotSaveQueue::SnapshotSaveQueue(const std::filesystem::path& trustedDestination, + const std::atomic_bool* hostCancellation) + : destination_(Resolve(trustedDestination)), hostCancellation_(hostCancellation) { } @@ -112,7 +113,7 @@ bool SnapshotSaveQueue::RunOne() return false; // Once selected, this request is in-flight; a later stop cannot claim to // cancel an OS operation that may already have committed. - cancelled = cancelQueued_.load(); + cancelled = cancelQueued_.load() || (hostCancellation_ && hostCancellation_->load()); selected->phase = Phase::Running; bytes.swap(selected->bytes); } diff --git a/mods/src/snapshot_save_queue.h b/mods/src/snapshot_save_queue.h index 305873de3..5a232e308 100644 --- a/mods/src/snapshot_save_queue.h +++ b/mods/src/snapshot_save_queue.h @@ -35,7 +35,9 @@ class SnapshotSaveQueue final file_transaction::Result result; }; - explicit SnapshotSaveQueue(const std::filesystem::path& trustedDestination); + // Optional host cancellation must outlive this queue and only transition to true. + explicit SnapshotSaveQueue(const std::filesystem::path& trustedDestination, + const std::atomic_bool* hostCancellation = nullptr); SnapshotSaveQueue(const SnapshotSaveQueue&) = delete; SnapshotSaveQueue& operator=(const SnapshotSaveQueue&) = delete; @@ -64,6 +66,7 @@ class SnapshotSaveQueue final Completion completion; }; const std::filesystem::path destination_; + const std::atomic_bool* const hostCancellation_; std::array slots_; std::mutex mutex_; std::atomic_bool stopping_{false}; diff --git a/mods/src/snapshot_save_service.cc b/mods/src/snapshot_save_service.cc index 8ea00bc77..8ddb52264 100644 --- a/mods/src/snapshot_save_service.cc +++ b/mods/src/snapshot_save_service.cc @@ -78,7 +78,8 @@ SnapshotSaveService::Lease::Lease() } SnapshotSaveService::Lease::~Lease() { serviceOwned.clear(); } -SnapshotSaveService::SnapshotSaveService(std::span paths) +SnapshotSaveService::SnapshotSaveService(std::span paths, + const std::atomic_bool* hostCancellation) : session_(NewSession()), count_(paths.size()) { if (count_ == 0 || count_ > MaxDestinations) @@ -121,7 +122,7 @@ SnapshotSaveService::SnapshotSaveService(std::span #if defined(MOD_SNAPSHOT_SERVICE_TESTING) BeforeServiceWorkerStart(i); #endif - workers_[i] = std::make_unique(resolved[i]); + workers_[i] = std::make_unique(resolved[i], hostCancellation); } } catch (...) { // Construction is supervisor/startup-only. Partial thread creation must not diff --git a/mods/src/snapshot_save_service.h b/mods/src/snapshot_save_service.h index db0978d8c..31bda6f8c 100644 --- a/mods/src/snapshot_save_service.h +++ b/mods/src/snapshot_save_service.h @@ -21,7 +21,9 @@ class SnapshotSaveService final std::uint64_t session_ = 0; std::size_t index_ = 0; }; - explicit SnapshotSaveService(std::span trustedPaths); + // Borrowed cancellation flag must outlive the service and all joined workers. + explicit SnapshotSaveService(std::span trustedPaths, + const std::atomic_bool* hostCancellation = nullptr); ~SnapshotSaveService() = default; SnapshotSaveService(const SnapshotSaveService&) = delete; SnapshotSaveService& operator=(const SnapshotSaveService&) = delete; diff --git a/mods/src/snapshot_save_worker.cc b/mods/src/snapshot_save_worker.cc index 218678598..6c1b36737 100644 --- a/mods/src/snapshot_save_worker.cc +++ b/mods/src/snapshot_save_worker.cc @@ -4,8 +4,9 @@ namespace persistence { -SnapshotSaveWorker::SnapshotSaveWorker(const std::filesystem::path& trustedDestination) - : queue_(trustedDestination) +SnapshotSaveWorker::SnapshotSaveWorker(const std::filesystem::path& trustedDestination, + const std::atomic_bool* hostCancellation) + : queue_(trustedDestination, hostCancellation) , worker_([this] { Run(); }) { } diff --git a/mods/src/snapshot_save_worker.h b/mods/src/snapshot_save_worker.h index af2997404..b78f8c1c9 100644 --- a/mods/src/snapshot_save_worker.h +++ b/mods/src/snapshot_save_worker.h @@ -14,7 +14,9 @@ namespace persistence class SnapshotSaveWorker final { public: - explicit SnapshotSaveWorker(const std::filesystem::path& trustedDestination); + // hostCancellation, when supplied, must outlive the worker and its joined thread. + explicit SnapshotSaveWorker(const std::filesystem::path& trustedDestination, + const std::atomic_bool* hostCancellation = nullptr); ~SnapshotSaveWorker() = default; SnapshotSaveWorker(const SnapshotSaveWorker&) = delete; SnapshotSaveWorker& operator=(const SnapshotSaveWorker&) = delete; diff --git a/tests/force_close_test.cc b/tests/force_close_test.cc index 0877752d0..d8cfdf3b4 100644 --- a/tests/force_close_test.cc +++ b/tests/force_close_test.cc @@ -1,4 +1,5 @@ #define MOD_SNAPSHOT_QUEUE_TESTING +#define MOD_SNAPSHOT_HOST_TESTING #include "snapshot_save_queue.cc" #include "snapshot_save_worker.cc" #include "snapshot_save_service.cc" @@ -11,14 +12,20 @@ struct Capture { volatile LONG calls; ULONGLONG began; }; Capture* capture; int mode; +std::atomic_bool releaseActive{false}, accessHeld{false}; namespace persistence { +struct SnapshotHostTestAccess { + static std::mutex& Access(SnapshotSaveHost& host) { return host.access_; } +}; namespace { +void BeforeHostThreadReturn() {} file_transaction::Result Execute(const std::filesystem::path&, std::string_view) { InterlockedIncrement(&capture->calls); if (mode == 1) Sleep(INFINITE); // Storage never returns; deadline must still win. + if (mode == 4) while (!releaseActive.load()) Sleep(1); Sleep(80); file_transaction::Result result; result.state = file_transaction::State::Committed; @@ -51,16 +58,29 @@ int wmain(int argc, wchar_t** argv) while (host->TrySubmit(*destination, 1, "first").state != Admission::Accepted) Sleep(1); while (!InterlockedCompareExchange(&capture->calls, 0, 0)) Sleep(1); while (host->TrySubmit(*destination, 2, "queued").state != Admission::Accepted) Sleep(1); - if (mode == 3) host->RequestStop(); // Escalate an already requested normal drain. + if (mode == 3 || mode == 4) host->RequestStop(); + if (mode == 4) { + while (host->Status() != persistence::SnapshotSaveHost::State::Stopping) Sleep(1); + // Hold producer access across cancellation and active-write completion. + // Neither the deadline nor queued cancellation may depend on this lock. + new std::thread([host] { + std::lock_guard lock(persistence::SnapshotHostTestAccess::Access(*host)); + accessHeld.store(true); + while (!releaseActive.load()) Sleep(1); + Sleep(200); + }); + while (!accessHeld.load()) Sleep(1); + } } capture->began = GetTickCount64(); persistence::ForceClose(host); + releaseActive.store(true); Sleep(INFINITE); // Simulate no further Unity/owner updates. return 99; } wchar_t executable[32768]{}; assert(GetModuleFileNameW(nullptr, executable, 32768)); - for (int scenario = 0; scenario != 4; ++scenario) { + for (int scenario = 0; scenario != 5; ++scenario) { capture->calls = 0; capture->began = 0; auto command = L"\"" + std::wstring(executable) + L"\" --child " + std::to_wstring(scenario) + From a5ccdd8ca80333bc5e91e7eda8609de919c7172d Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 19:27:05 -0500 Subject: [PATCH 20/20] Make cancellation contention fixture wait for joined workers --- mods/src/snapshot_save_host.cc | 6 ++++++ tests/force_close_test.cc | 7 +++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/mods/src/snapshot_save_host.cc b/mods/src/snapshot_save_host.cc index 991e6ae50..8adbf3106 100644 --- a/mods/src/snapshot_save_host.cc +++ b/mods/src/snapshot_save_host.cc @@ -10,6 +10,9 @@ namespace persistence #if defined(MOD_SNAPSHOT_HOST_TESTING) && defined(_WIN32) namespace { void BeforeHostThreadReturn(); } #endif +#if defined(MOD_SNAPSHOT_FORCE_CLOSE_TESTING) +namespace { void AfterHostWorkersJoined(); } +#endif SnapshotSaveHost::~SnapshotSaveHost() { // Never hide a blocking destructor or permit code to outlive its module. @@ -144,6 +147,9 @@ void SnapshotSaveHost::Run() noexcept service_ = nullptr; } service->StopAndJoin(cancelQueued_.load() ? StopMode::CancelQueued : StopMode::DrainAccepted); +#if defined(MOD_SNAPSHOT_FORCE_CLOSE_TESTING) + AfterHostWorkersJoined(); +#endif { std::lock_guard lock(access_); for (std::size_t i = 0; i < count_; ++i) diff --git a/tests/force_close_test.cc b/tests/force_close_test.cc index d8cfdf3b4..ed6b084dc 100644 --- a/tests/force_close_test.cc +++ b/tests/force_close_test.cc @@ -1,5 +1,6 @@ #define MOD_SNAPSHOT_QUEUE_TESTING #define MOD_SNAPSHOT_HOST_TESTING +#define MOD_SNAPSHOT_FORCE_CLOSE_TESTING #include "snapshot_save_queue.cc" #include "snapshot_save_worker.cc" #include "snapshot_save_service.cc" @@ -12,7 +13,7 @@ struct Capture { volatile LONG calls; ULONGLONG began; }; Capture* capture; int mode; -std::atomic_bool releaseActive{false}, accessHeld{false}; +std::atomic_bool releaseActive{false}, accessHeld{false}, workersJoined{false}; namespace persistence { struct SnapshotHostTestAccess { @@ -21,6 +22,7 @@ struct SnapshotHostTestAccess { namespace { void BeforeHostThreadReturn() {} +void AfterHostWorkersJoined() { workersJoined.store(true); } file_transaction::Result Execute(const std::filesystem::path&, std::string_view) { InterlockedIncrement(&capture->calls); @@ -67,7 +69,7 @@ int wmain(int argc, wchar_t** argv) std::lock_guard lock(persistence::SnapshotHostTestAccess::Access(*host)); accessHeld.store(true); while (!releaseActive.load()) Sleep(1); - Sleep(200); + while (!workersJoined.load()) Sleep(1); }); while (!accessHeld.load()) Sleep(1); } @@ -108,6 +110,7 @@ int wmain(int argc, wchar_t** argv) } #else namespace persistence { namespace { +void AfterHostWorkersJoined() {} file_transaction::Result Execute(const std::filesystem::path&, std::string_view) { return {}; } } } int main() { std::cout << "Windows-only force-close fixture skipped\n"; }