From 41a3016e634bd5fb1a954226dd3e743ae9c48bc5 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 07:36:25 -0400 Subject: [PATCH 1/4] Land the package filesystem shell: streaming atomic package_io, journaled rollback carve-out, asymmetric platform pickers --- CMakeLists.txt | 1 + src/shell/package/CLAUDE.md | 61 +++++++ src/shell/package/CMakeLists.txt | 20 +++ src/shell/package/package_io.cpp | 170 ++++++++++++++++++ src/shell/package/package_io.h | 112 ++++++++++++ src/shell/package/package_pickers.cpp | 93 ++++++++++ src/shell/package/package_pickers.h | 21 +++ src/shell/package/package_rollback.cpp | 40 +++++ src/shell/package/package_rollback.h | 45 +++++ tests/test_package_io.cpp | 228 +++++++++++++++++++++++++ tests/test_package_rollback.cpp | 123 +++++++++++++ 11 files changed, 914 insertions(+) create mode 100644 src/shell/package/CLAUDE.md create mode 100644 src/shell/package/CMakeLists.txt create mode 100644 src/shell/package/package_io.cpp create mode 100644 src/shell/package/package_io.h create mode 100644 src/shell/package/package_pickers.cpp create mode 100644 src/shell/package/package_pickers.h create mode 100644 src/shell/package/package_rollback.cpp create mode 100644 src/shell/package/package_rollback.h create mode 100644 tests/test_package_io.cpp create mode 100644 tests/test_package_rollback.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index f783871..431b569 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -92,3 +92,4 @@ enable_testing() add_subdirectory(src/core) add_subdirectory(src/app) add_subdirectory(src/shell/instrument) +add_subdirectory(src/shell/package) diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md new file mode 100644 index 0000000..6456fe9 --- /dev/null +++ b/src/shell/package/CLAUDE.md @@ -0,0 +1,61 @@ +# src/shell/package — package filesystem + dialog seam + +## Scope + +The filesystem and dialog acts behind bank-package export/import: streaming package +file I/O (`package_io`), the landed-file journal and its rollback delete +(`package_rollback`), and the platform file pickers (`package_pickers`). This seam +is bytes-only — the package format (magic, manifest, entry layout) is +`core/package`'s business, and the export/import verbs that orchestrate both do not +live here yet. No REAPER project state is touched in this directory: no ext-state +read or write, no undo block, no generation bump — those belong to the verbs. + +## Invariants + +- **Atomic write.** A package accumulates in a `.rsbanktmp` sibling in the + destination directory and reaches the destination only through `commit()`'s + rename (the mono-collapse temp+rename precedent). A failed, aborted, or abandoned + write leaves the destination absent or holding its prior contents — never a + partial `.rsbank`. +- **Streaming, both ways — at most ONE entry's payload in memory.** Writes append + one payload at a time; reads seek and materialize one range at a time. The claim + is structural, not aspirational: every payload crosses this seam as a move-only + `PayloadBuffer`, and `PayloadBuffer::alive()` is the seam counter the tests + assert against. There is no read-whole-package or write-whole-package entry + point; do not add one. +- **The rollback delete is prune's ONE carve-out, cited not restated.** The + citation and the discriminator live at `package_rollback.cpp`'s header. The + journal makes the discriminator structural: only paths its own `writeLandedFile` + successfully created are recorded, and `rollback()` consumes only the record — a + path this import did not write cannot be handed to it. +- **No overwrite of a bank-folder file, ever.** `writeLandedFile` refuses an + existing destination outright; collision handling (auto-rename) is the import + plan's job upstream. The package writer itself DOES replace an existing + destination — the export save dialog's own overwrite confirm is the consent — + and that asymmetry is deliberate. +- **The two pickers are asymmetric, and the asymmetry is real.** Import rides + REAPER's own `GetUserFileNameForRead` (both platforms); export goes native — + Win32 `GetSaveFileNameW` / SWELL `BrowseForSaveFile` — because the always-present + REAPER surface offers no save picker. Do not symmetrize; the newer + `GetUserFileName(mode=0)` alternative and why it is not used are recorded in + `package_pickers.cpp`'s header. + +## Modules + +- `package_io` — the streaming filesystem seam: `PayloadBuffer` (move-only payload + the `alive()` seam counter), `PackageFileWriter` (append-only temp+atomic-rename writer), `PackageFileReader` (seek-and-read one range per call, range-checked against the real file size), `readFilePayload` (one source file as one entry's payload), and `listFolderFileNames` (bare names, sorted, non-recursive, non-throwing). REAPER-free; tested without a DAW. +- `package_rollback` — `LandedFileJournal`: `writeLandedFile` (temp+rename land, recorded on success only, refuses an existing destination and an empty payload) and `rollback` (deletes exactly the recorded set, hard unlink — nothing ever referenced these bytes — tolerating a vanished file). REAPER-free; tested without a DAW. +- `package_pickers` — the two pickers in one platform TU (`#ifdef _WIN32` / `#else swell/swell.h`, the `draw_kit`/`prune_fs` split): `pickPackageForImport` (REAPER read picker) and `pickPackageSavePath` (native save dialog, UTF-8 in/out on Windows). Compile-only until the verbs land; nothing here can be exercised in a unit test. + +## Gotchas + +- A crash mid-write strands the `.rsbanktmp` sibling. It is not a `.rsbank` (no + picker filter matches it), and a later export to the same destination truncates + it — but one stranded in the BANK folder by a mid-import crash is a foreign file + to prune (not owned, so never an orphan) until removed by hand. `[verify — DAW]` + whether the import verb should pre-clean stale `.rsbanktmp` names when it lands. +- The picker `defext`/filter strings are spelled to the Win32 `lpstrDefExt` + convention (no dot) but are `[verify — DAW]` on all three platforms — neither + picker is exercised outside a live REAPER session. +- `readRange(_, 0)` returns an empty buffer — indistinguishable from failure, by + design (the one "nothing to work with" branch). A genuinely zero-length entry + cannot round-trip through this seam; the format layer must not emit one. diff --git a/src/shell/package/CMakeLists.txt b/src/shell/package/CMakeLists.txt new file mode 100644 index 0000000..ad716e4 --- /dev/null +++ b/src/shell/package/CMakeLists.txt @@ -0,0 +1,20 @@ +# The filesystem + dialog seam for bank packages. package_io / package_rollback are +# REAPER-free (standard filesystem only), so the pure-library/test helpers fit and +# their tests run without a DAW. The export/import verbs that drive all three targets +# are not in this directory yet. + +reasampler_pure_library(package_io SOURCES package_io.cpp LINK PUBLIC file_bytes) +reasampler_test(package_io LINK package_io) + +reasampler_pure_library(package_rollback SOURCES package_rollback.cpp LINK PUBLIC package_io) +reasampler_test(package_rollback LINK package_rollback) + +# The platform pickers touch the REAPER API and the native save dialog, so no test +# target can exercise them; declared as a library so both picker paths stay compiled. +add_library(package_pickers STATIC package_pickers.cpp) +target_include_directories(package_pickers PUBLIC ${REASAMPLER_SRC_DIR}) +target_include_directories(package_pickers PRIVATE ${SDK_INC} ${WDL_INC}) +if(NOT WIN32) + # Match the loadable modules: SWELL is provided by the host REAPER at runtime. + target_compile_definitions(package_pickers PRIVATE SWELL_PROVIDED_BY_APP) +endif() diff --git a/src/shell/package/package_io.cpp b/src/shell/package/package_io.cpp new file mode 100644 index 0000000..92f645b --- /dev/null +++ b/src/shell/package/package_io.cpp @@ -0,0 +1,170 @@ +// package_io.cpp — see package_io.h for the seam's contract. Non-throwing at the +// boundary: every filesystem call uses the error_code form so no filesystem_error +// crosses into a REAPER action body. + +#include "shell/package/package_io.h" + +#include +#include +#include +#include + +#include "core/util/file_bytes.h" + +namespace reasampler { + +namespace fs = std::filesystem; + +namespace { +std::atomic g_alivePayloads{0}; +} + +// --------------------------------------------------------------------------- +// PayloadBuffer + +PayloadBuffer::PayloadBuffer(std::vector bytes) + : bytes_(std::move(bytes)), counted_(!bytes_.empty()) { + if (counted_) g_alivePayloads.fetch_add(1, std::memory_order_relaxed); +} + +PayloadBuffer::~PayloadBuffer() { release(); } + +PayloadBuffer::PayloadBuffer(PayloadBuffer&& other) noexcept + : bytes_(std::move(other.bytes_)), counted_(other.counted_) { + // The count transfers with the bytes — a move must never double-count. + other.bytes_.clear(); + other.counted_ = false; +} + +PayloadBuffer& PayloadBuffer::operator=(PayloadBuffer&& other) noexcept { + if (this != &other) { + release(); + bytes_ = std::move(other.bytes_); + counted_ = other.counted_; + other.bytes_.clear(); + other.counted_ = false; + } + return *this; +} + +int PayloadBuffer::alive() { return g_alivePayloads.load(std::memory_order_relaxed); } + +void PayloadBuffer::release() { + if (counted_) g_alivePayloads.fetch_sub(1, std::memory_order_relaxed); + counted_ = false; + bytes_.clear(); +} + +// --------------------------------------------------------------------------- +// PackageFileWriter + +PackageFileWriter::PackageFileWriter(std::string destAbsPath) + : destPath_(std::move(destAbsPath)), tempPath_(destPath_ + ".rsbanktmp") { + out_.open(tempPath_, std::ios::binary | std::ios::trunc); + ok_ = static_cast(out_); +} + +PackageFileWriter::~PackageFileWriter() { + if (!done_) abort(); +} + +bool PackageFileWriter::appendRaw(const std::uint8_t* data, std::size_t len) { + if (!ok_ || done_) return false; + if (len == 0) return true; + out_.write(reinterpret_cast(data), + static_cast(len)); + ok_ = static_cast(out_); + return ok_; +} + +bool PackageFileWriter::appendPayload(const PayloadBuffer& payload) { + return appendRaw(payload.data(), payload.size()); +} + +bool PackageFileWriter::commit() { + if (done_) return false; + if (ok_) { + out_.flush(); + ok_ = static_cast(out_); + } + out_.close(); + if (!ok_) { + abort(); + return false; + } + // rename() replaces the destination in one step (the mono-collapse precedent): + // prior contents survive until the replacement is known-complete, and a failed + // rename self-cleans the temp rather than littering it. + std::error_code ec; + fs::rename(tempPath_, destPath_, ec); + if (ec) { + fs::remove(tempPath_, ec); + done_ = true; + ok_ = false; + return false; + } + done_ = true; + return true; +} + +void PackageFileWriter::abort() { + if (done_) return; + out_.close(); + std::error_code ec; + fs::remove(tempPath_, ec); + done_ = true; + ok_ = false; +} + +// --------------------------------------------------------------------------- +// PackageFileReader + +PackageFileReader::PackageFileReader(const std::string& srcAbsPath) { + std::error_code ec; + const std::uintmax_t sz = fs::file_size(srcAbsPath, ec); + if (ec) return; + in_.open(srcAbsPath, std::ios::binary); + if (!in_) return; + size_ = static_cast(sz); + ok_ = true; +} + +PayloadBuffer PackageFileReader::readRange(std::uint64_t offset, std::uint64_t length) { + // Overflow-safe range check: length is capped by the real file size before any + // allocation happens, so a hostile offset/length pair cannot demand the moon. + if (!ok_ || length == 0 || length > size_ || offset > size_ - length) { + return PayloadBuffer{}; + } + in_.clear(); // a prior failed read must not poison this one + in_.seekg(static_cast(offset)); + if (!in_) return PayloadBuffer{}; + std::vector bytes(static_cast(length)); + in_.read(reinterpret_cast(bytes.data()), + static_cast(length)); + if (static_cast(in_.gcount()) != length) return PayloadBuffer{}; + return PayloadBuffer(std::move(bytes)); +} + +// --------------------------------------------------------------------------- + +PayloadBuffer readFilePayload(const std::string& absPath) { + return PayloadBuffer(util::readFileBytes(absPath)); +} + +std::vector listFolderFileNames(const std::string& dirAbsPath) { + std::vector names; + std::error_code ec; + // Manual iterator form (it.increment(ec)) keeps the loop non-throwing on a + // mid-iteration failure, matching prune_fs's enumerate. + fs::directory_iterator it(dirAbsPath, ec); + for (; !ec && it != fs::directory_iterator{}; it.increment(ec)) { + const auto& entry = *it; + std::error_code reg_ec; + if (!entry.is_regular_file(reg_ec)) continue; // skip subdirs / specials + names.push_back(entry.path().filename().string()); + } + std::sort(names.begin(), names.end()); + return names; +} + +} // namespace reasampler diff --git a/src/shell/package/package_io.h b/src/shell/package/package_io.h new file mode 100644 index 0000000..6f36d2d --- /dev/null +++ b/src/shell/package/package_io.h @@ -0,0 +1,112 @@ +// shell/package/package_io — streaming filesystem seam for bank packages: append one +// payload at a time through a temp-file + atomic-rename writer, seek and read one +// payload at a time back out. Bytes only: what a package contains is core/package's +// business, never this seam's. Blocking I/O — UI-thread actions only, never the +// audio thread. + +#pragma once + +#include +#include +#include +#include + +namespace reasampler { + +// One entry's payload, and the seam counter that makes "never more than one entry in +// memory" assertable: alive() counts every buffer currently holding bytes, so the +// streaming claim is a test CHECK against this counter rather than a memory +// measurement. Move-only — copying a payload would silently double the held bytes. +class PayloadBuffer { +public: + PayloadBuffer() = default; + explicit PayloadBuffer(std::vector bytes); + ~PayloadBuffer(); + PayloadBuffer(PayloadBuffer&& other) noexcept; + PayloadBuffer& operator=(PayloadBuffer&& other) noexcept; + PayloadBuffer(const PayloadBuffer&) = delete; + PayloadBuffer& operator=(const PayloadBuffer&) = delete; + + const std::uint8_t* data() const { return bytes_.data(); } + std::size_t size() const { return bytes_.size(); } + bool empty() const { return bytes_.empty(); } + const std::vector& bytes() const { return bytes_; } + + // Buffers currently holding at least one byte, process-wide. + static int alive(); + +private: + void release(); + + std::vector bytes_; + bool counted_ = false; +}; + +// Streaming atomic writer. Bytes accumulate in ".rsbanktmp" beside the +// destination (same directory, so the final rename never crosses a volume); the +// destination itself is touched only by commit()'s rename, so a failed, aborted, or +// abandoned write leaves it absent or holding its prior contents — never a partial +// file. Destruction without commit() aborts and removes the temp. commit() REPLACES +// an existing destination: the export save dialog's own overwrite confirm is the +// consent (the never-overwrite rule for bank-folder files lives in +// LandedFileJournal, upstream of this writer). Neither copyable nor movable, and +// append-only — there is deliberately no way to hand it a whole package at once. +class PackageFileWriter { +public: + explicit PackageFileWriter(std::string destAbsPath); + ~PackageFileWriter(); + PackageFileWriter(const PackageFileWriter&) = delete; + PackageFileWriter& operator=(const PackageFileWriter&) = delete; + + bool ok() const { return ok_; } + // Framing/header bytes. False on a failed or already-finished writer. + bool appendRaw(const std::uint8_t* data, std::size_t len); + // One entry's bytes. Same contract as appendRaw. + bool appendPayload(const PayloadBuffer& payload); + // Flush, close, rename over the destination. False (and self-cleaning: the temp + // is removed, the destination untouched) on any failure or on a second call. + bool commit(); + // Close and remove the temp; the destination is never touched. Idempotent. + void abort(); + + const std::string& destPath() const { return destPath_; } + const std::string& tempPath() const { return tempPath_; } + +private: + std::string destPath_; + std::string tempPath_; + std::ofstream out_; + bool ok_ = false; + bool done_ = false; +}; + +// Seek-and-read reader: exactly one payload is materialized per readRange call, and +// there is deliberately no read-whole-file entry point. Empty buffer on ANY failure +// — unopenable file, zero length, out of range, short read — so the caller has one +// "nothing to work with" branch (file_bytes' contract). +class PackageFileReader { +public: + explicit PackageFileReader(const std::string& srcAbsPath); + + bool ok() const { return ok_; } + std::uint64_t fileSize() const { return size_; } + // Bytes [offset, offset+length). Range-checked against the real file size, so a + // hostile layout can never demand an allocation past the file's end. + PayloadBuffer readRange(std::uint64_t offset, std::uint64_t length); + +private: + std::ifstream in_; + std::uint64_t size_ = 0; + bool ok_ = false; +}; + +// One source file read whole as one entry's payload — a bank file IS the streaming +// unit, so whole-file here is one entry, released before the next is read. Empty on +// any failure, per readFileBytes. +PayloadBuffer readFilePayload(const std::string& absPath); + +// Bare file names (regular files only, never a path) in dirAbsPath, sorted so +// callers see a deterministic order; empty on a missing or unreadable folder. +std::vector listFolderFileNames(const std::string& dirAbsPath); + +} // namespace reasampler diff --git a/src/shell/package/package_pickers.cpp b/src/shell/package/package_pickers.cpp new file mode 100644 index 0000000..b122bd9 --- /dev/null +++ b/src/shell/package/package_pickers.cpp @@ -0,0 +1,93 @@ +// package_pickers.cpp — see package_pickers.h. Export is native rather than REAPER +// API: the SDK's newer GetUserFileName(mode=0) could save, but it resolves to null +// on older REAPER builds this extension still loads in, while GetSaveFileNameW / +// BrowseForSaveFile are always present. main.cpp owns the REAPER API pointers; this +// TU gets them extern. + +#include "shell/package/package_pickers.h" + +#ifdef _WIN32 +#include +#include +#else +#include "swell/swell.h" +#endif + +#define REAPERAPI_MINIMAL +#define REAPERAPI_WANT_GetUserFileNameForRead +#define REAPERAPI_WANT_GetMainHwnd +#include "reaper_plugin_functions.h" + +namespace reasampler { + +bool pickPackageForImport(std::string& outAbsPath) { + outAbsPath.clear(); + if (!GetUserFileNameForRead) return false; + char buf[4096]; + buf[0] = '\0'; + // defext spelled without the dot, matching Win32 lpstrDefExt. [verify — DAW] + // whether REAPER's picker applies it to the shown filter. + if (!GetUserFileNameForRead(buf, "Import bank package", "rsbank")) return false; + outAbsPath = buf; + return !outAbsPath.empty(); +} + +#ifdef _WIN32 + +bool pickPackageSavePath(const std::string& suggestedFileName, std::string& outAbsPath) { + outAbsPath.clear(); + + wchar_t file[4096]; + file[0] = L'\0'; + if (!suggestedFileName.empty()) { + const int wrote = MultiByteToWideChar(CP_UTF8, 0, suggestedFileName.c_str(), + -1, file, 4096); + if (wrote <= 0) file[0] = L'\0'; // unconvertible suggestion -> empty box + } + + OPENFILENAMEW ofn{}; + ofn.lStructSize = sizeof(ofn); + ofn.hwndOwner = GetMainHwnd ? GetMainHwnd() : nullptr; + ofn.lpstrFilter = + L"ReaSampler bank package (*.rsbank)\0*.rsbank\0All files (*.*)\0*.*\0"; + ofn.lpstrFile = file; + ofn.nMaxFile = 4096; + ofn.lpstrTitle = L"Export bank package"; + ofn.lpstrDefExt = L"rsbank"; + // NOCHANGEDIR: REAPER's process-wide working directory is not ours to move. + ofn.Flags = OFN_OVERWRITEPROMPT | OFN_PATHMUSTEXIST | OFN_NOCHANGEDIR | + OFN_HIDEREADONLY; + if (!GetSaveFileNameW(&ofn)) return false; + + const int need = + WideCharToMultiByte(CP_UTF8, 0, file, -1, nullptr, 0, nullptr, nullptr); + if (need <= 0) return false; + std::string utf8(static_cast(need), '\0'); + WideCharToMultiByte(CP_UTF8, 0, file, -1, &utf8[0], need, nullptr, nullptr); + if (!utf8.empty() && utf8.back() == '\0') utf8.pop_back(); + outAbsPath = std::move(utf8); + return !outAbsPath.empty(); +} + +#else // SWELL (macOS / Linux) + +bool pickPackageSavePath(const std::string& suggestedFileName, std::string& outAbsPath) { + outAbsPath.clear(); + // GetOpenFileName-style pair list; the literal's implicit terminator supplies the + // closing double-NUL. [verify — DAW] the exact filter strings SWELL accepts. + static const char kExtList[] = "ReaSampler bank package (*.rsbank)\0*.rsbank\0"; + char fn[4096]; + fn[0] = '\0'; + if (!BrowseForSaveFile("Export bank package", nullptr, + suggestedFileName.empty() ? nullptr + : suggestedFileName.c_str(), + kExtList, fn, static_cast(sizeof(fn)))) { + return false; + } + outAbsPath = fn; + return !outAbsPath.empty(); +} + +#endif + +} // namespace reasampler diff --git a/src/shell/package/package_pickers.h b/src/shell/package/package_pickers.h new file mode 100644 index 0000000..cdfbc1b --- /dev/null +++ b/src/shell/package/package_pickers.h @@ -0,0 +1,21 @@ +// shell/package/package_pickers — the two package file pickers, and they are +// deliberately asymmetric: import rides REAPER's own read picker +// (GetUserFileNameForRead, both platforms); export goes native — Win32 +// GetSaveFileNameW / SWELL BrowseForSaveFile — because the always-present REAPER +// surface offers no save picker. Do not symmetrize; the rationale is in the TU. + +#pragma once + +#include + +namespace reasampler { + +// REAPER's read picker. True with outAbsPath set iff the user chose a file. +bool pickPackageForImport(std::string& outAbsPath); + +// Native save picker, pre-filled with suggestedFileName (a bare name, e.g. +// "MyBank.rsbank"). True with outAbsPath set iff the user chose a destination; the +// dialog's own overwrite confirm has already run by then. +bool pickPackageSavePath(const std::string& suggestedFileName, std::string& outAbsPath); + +} // namespace reasampler diff --git a/src/shell/package/package_rollback.cpp b/src/shell/package/package_rollback.cpp new file mode 100644 index 0000000..ddface7 --- /dev/null +++ b/src/shell/package/package_rollback.cpp @@ -0,0 +1,40 @@ +// package_rollback.cpp — see package_rollback.h. The rollback delete below runs +// under the ONE carve-out from prune's exclusive file-deletion authority, stated at +// src/shell/persist/prune_fs.cpp:5-11; it satisfies that discriminator by +// construction — every recorded path was created by this journal's own +// writeLandedFile, and the abandoned index mutation means nothing ever referenced it. + +#include "shell/package/package_rollback.h" + +#include + +namespace reasampler { + +namespace fs = std::filesystem; + +bool LandedFileJournal::writeLandedFile(const std::string& absPath, + const PayloadBuffer& payload) { + if (payload.empty()) return false; + std::error_code ec; + if (fs::exists(absPath, ec) || ec) return false; + PackageFileWriter writer(absPath); + if (!writer.appendPayload(payload)) return false; // dtor aborts; temp removed + if (!writer.commit()) return false; + paths_.push_back(absPath); + return true; +} + +RollbackResult LandedFileJournal::rollback() { + RollbackResult result; + for (const std::string& path : paths_) { + std::error_code ec; + const bool removed = fs::remove(path, ec); + if (removed) ++result.deletedCount; + else if (ec) ++result.failedCount; + else ++result.alreadyAbsentCount; // no error, nothing there + } + paths_.clear(); + return result; +} + +} // namespace reasampler diff --git a/src/shell/package/package_rollback.h b/src/shell/package/package_rollback.h new file mode 100644 index 0000000..a72f89c --- /dev/null +++ b/src/shell/package/package_rollback.h @@ -0,0 +1,45 @@ +// shell/package/package_rollback — the files ONE import call has landed, as a +// journal: writes record themselves on success, and rollback() deletes exactly what +// is recorded — a path this import did not write is structurally impossible to hand +// it. The deletion carve-out this satisfies is cited at package_rollback.cpp's +// header. + +#pragma once + +#include +#include + +#include "shell/package/package_io.h" + +namespace reasampler { + +struct RollbackResult { + int deletedCount = 0; + int alreadyAbsentCount = 0; // vanished between land and rollback — not a failure + int failedCount = 0; // locked / permission — recorded, never thrown +}; + +// The evidence for the rollback discriminator: only paths this journal's own +// writeLandedFile successfully created are recorded, so rollback() can never touch a +// byte this import did not write. +class LandedFileJournal { +public: + // Lands one payload at absPath through the atomic temp+rename writer and records + // the path on success. REFUSES an existing destination — a bank-folder file is + // never overwritten; collision handling is the import plan's job, upstream. An + // empty payload is refused too: it signals an upstream read failure, never a + // real entry. + bool writeLandedFile(const std::string& absPath, const PayloadBuffer& payload); + + // Deletes exactly the recorded files and clears the journal, so a second call is + // a no-op. Hard unlink, not trash: nothing ever referenced these bytes. + RollbackResult rollback(); + + const std::vector& landedPaths() const { return paths_; } + bool empty() const { return paths_.empty(); } + +private: + std::vector paths_; +}; + +} // namespace reasampler diff --git a/tests/test_package_io.cpp b/tests/test_package_io.cpp new file mode 100644 index 0000000..7f10b73 --- /dev/null +++ b/tests/test_package_io.cpp @@ -0,0 +1,228 @@ +// Standalone tests for shell/package/package_io — no REAPER, no framework. Pins the +// two properties the seam exists for: an interrupted or failed write leaves the +// destination absent or holding its prior contents (failure injected at the writer +// seam — abandonment, open failure, rename failure), and a multi-entry round trip +// holds at most one entry's payload, asserted against PayloadBuffer::alive() — the +// seam counter — rather than a memory measurement. + +#include "../src/shell/package/package_io.h" + +#include +#include +#include +#include +#include +#include + +using namespace reasampler; +namespace fs = std::filesystem; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +static std::vector patternBytes(std::size_t n, std::uint8_t seed) { + std::vector v(n); + for (std::size_t i = 0; i < n; ++i) + v[i] = static_cast(seed + i * 7u); + return v; +} + +static void writeScratchFile(const std::string& path, + const std::vector& bytes) { + std::ofstream f(path, std::ios::binary | std::ios::trunc); + f.write(reinterpret_cast(bytes.data()), + static_cast(bytes.size())); +} + +static std::vector readAll(const std::string& path) { + std::ifstream f(path, std::ios::binary); + return std::vector(std::istreambuf_iterator(f), + std::istreambuf_iterator()); +} + +static void testPayloadCounterTracksMovesNotCopies() { + CHECK(PayloadBuffer::alive() == 0); + { + PayloadBuffer a(patternBytes(4, 1)); + CHECK(PayloadBuffer::alive() == 1); + PayloadBuffer b = std::move(a); + CHECK(PayloadBuffer::alive() == 1); // the count moved with the bytes + PayloadBuffer c; + c = std::move(b); + CHECK(PayloadBuffer::alive() == 1); + CHECK(c.size() == 4); + } + CHECK(PayloadBuffer::alive() == 0); + { + PayloadBuffer empty; + CHECK(PayloadBuffer::alive() == 0); // holding nothing counts as nothing + } +} + +static void testStreamingRoundTripHoldsOnePayload() { + const std::string dest = "pkg_io_scratch.rsbank"; + const std::vector header = patternBytes(16, 0xA0); + const std::vector> entries = { + patternBytes(1000, 1), patternBytes(500, 2), patternBytes(1, 3)}; + + std::vector srcs; + for (std::size_t i = 0; i < entries.size(); ++i) { + srcs.push_back("pkg_io_src" + std::to_string(i) + ".bin"); + writeScratchFile(srcs[i], entries[i]); + } + + CHECK(PayloadBuffer::alive() == 0); + std::vector> layout; // offset, length + { + PackageFileWriter writer(dest); + CHECK(writer.ok()); + CHECK(writer.appendRaw(header.data(), header.size())); + std::uint64_t offset = header.size(); + for (std::size_t i = 0; i < entries.size(); ++i) { + PayloadBuffer p = readFilePayload(srcs[i]); + CHECK(p.bytes() == entries[i]); + CHECK(PayloadBuffer::alive() == 1); // exactly one entry in memory + CHECK(writer.appendPayload(p)); + layout.emplace_back(offset, p.size()); + offset += p.size(); + } + CHECK(PayloadBuffer::alive() == 0); // each released before the next + CHECK(writer.commit()); + CHECK(!writer.commit()); // a second commit is refused + } + CHECK(!fs::exists(dest + ".rsbanktmp")); + CHECK(fs::exists(dest)); + + { + // Scoped: the reader holds the file open, and Windows refuses to delete an + // open file — cleanup below needs it closed first. + PackageFileReader reader(dest); + CHECK(reader.ok()); + CHECK(reader.fileSize() == header.size() + 1000 + 500 + 1); + for (std::size_t i = 0; i < entries.size(); ++i) { + PayloadBuffer p = reader.readRange(layout[i].first, layout[i].second); + CHECK(PayloadBuffer::alive() == 1); // one entry per readRange, no more + CHECK(p.bytes() == entries[i]); + } + CHECK(PayloadBuffer::alive() == 0); + } + + std::error_code ec; + for (const std::string& s : srcs) fs::remove(s, ec); + fs::remove(dest, ec); +} + +static void testAbandonedWriteLeavesNoDestination() { + const std::string dest = "pkg_io_abandon.rsbank"; + { + PackageFileWriter writer(dest); + const std::vector some = patternBytes(64, 9); + CHECK(writer.appendRaw(some.data(), some.size())); + // no commit — destruction is the injected interruption + } + CHECK(!fs::exists(dest)); + CHECK(!fs::exists(dest + ".rsbanktmp")); +} + +static void testAbortPreservesPriorContents() { + const std::string dest = "pkg_io_prior.rsbank"; + const std::vector prior = patternBytes(32, 0x40); + writeScratchFile(dest, prior); + { + PackageFileWriter writer(dest); + const std::vector some = patternBytes(64, 9); + CHECK(writer.appendRaw(some.data(), some.size())); + writer.abort(); + CHECK(!writer.appendRaw(some.data(), some.size())); // dead after abort + CHECK(!writer.commit()); + } + CHECK(readAll(dest) == prior); + CHECK(!fs::exists(dest + ".rsbanktmp")); + std::error_code ec; + fs::remove(dest, ec); +} + +static void testOpenFailureIsInert() { + const std::string dest = "pkg_io_no_such_dir/x.rsbank"; + PackageFileWriter writer(dest); + CHECK(!writer.ok()); + const std::vector some = patternBytes(8, 1); + CHECK(!writer.appendRaw(some.data(), some.size())); + CHECK(!writer.commit()); + CHECK(!fs::exists("pkg_io_no_such_dir")); +} + +static void testCommitRenameFailureSelfCleans() { + // A directory squatting on the destination makes the final rename fail — a real + // injected commit failure, not a simulated one. + const std::string dest = "pkg_io_dir.rsbank"; + std::error_code ec; + fs::create_directory(dest, ec); + CHECK(!ec); + { + PackageFileWriter writer(dest); + CHECK(writer.ok()); + const std::vector some = patternBytes(8, 1); + CHECK(writer.appendRaw(some.data(), some.size())); + CHECK(!writer.commit()); + } + CHECK(fs::is_directory(dest)); // prior state intact + CHECK(!fs::exists(dest + ".rsbanktmp")); + fs::remove(dest, ec); +} + +static void testReaderEdges() { + PackageFileReader missing("pkg_io_no_such_file.rsbank"); + CHECK(!missing.ok()); + CHECK(missing.readRange(0, 1).empty()); + + const std::string path = "pkg_io_edges.bin"; + const std::vector bytes = patternBytes(10, 5); + writeScratchFile(path, bytes); + { + // Scoped: the reader must be closed before the cleanup remove below. + PackageFileReader reader(path); + CHECK(reader.ok()); + CHECK(reader.fileSize() == 10); + CHECK(reader.readRange(5, 10).empty()); // past the end + CHECK(reader.readRange(10, 1).empty()); // starts at the end + CHECK(reader.readRange(0, 0).empty()); // zero length is failure, one branch + const PayloadBuffer slice = reader.readRange(2, 3); + CHECK(slice.bytes() == + std::vector(bytes.begin() + 2, bytes.begin() + 5)); + } + std::error_code ec; + fs::remove(path, ec); +} + +static void testListFolderFileNames() { + const std::string dir = "pkg_io_listdir"; + std::error_code ec; + fs::create_directory(dir, ec); + writeScratchFile(dir + "/b.bin", patternBytes(2, 1)); + writeScratchFile(dir + "/a.bin", patternBytes(2, 2)); + fs::create_directory(dir + "/sub", ec); + writeScratchFile(dir + "/sub/c.bin", patternBytes(2, 3)); + + const std::vector names = listFolderFileNames(dir); + CHECK(names == (std::vector{"a.bin", "b.bin"})); // sorted, bare, non-recursive + CHECK(listFolderFileNames("pkg_io_no_such_dir").empty()); + + fs::remove_all(dir, ec); +} + +int main() { + testPayloadCounterTracksMovesNotCopies(); + testStreamingRoundTripHoldsOnePayload(); + testAbandonedWriteLeavesNoDestination(); + testAbortPreservesPriorContents(); + testOpenFailureIsInert(); + testCommitRenameFailureSelfCleans(); + testReaderEdges(); + testListFolderFileNames(); + + if (g_fail == 0) std::printf("package_io: all tests passed\n"); + else std::printf("package_io: %d CHECK(s) FAILED\n", g_fail); + return g_fail == 0 ? 0 : 1; +} diff --git a/tests/test_package_rollback.cpp b/tests/test_package_rollback.cpp new file mode 100644 index 0000000..6c28bfa --- /dev/null +++ b/tests/test_package_rollback.cpp @@ -0,0 +1,123 @@ +// Standalone tests for shell/package/package_rollback — no REAPER, no framework. +// Pins the discriminator's mechanics: a file is recorded only when this journal's +// own write landed it, a pre-existing destination is refused untouched, and +// rollback deletes exactly the recorded set — a bystander file beside them stays, +// and a vanished file is tolerated rather than failed. + +#include "../src/shell/package/package_rollback.h" + +#include +#include +#include +#include +#include + +using namespace reasampler; +namespace fs = std::filesystem; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +static std::vector patternBytes(std::size_t n, std::uint8_t seed) { + std::vector v(n); + for (std::size_t i = 0; i < n; ++i) + v[i] = static_cast(seed + i * 7u); + return v; +} + +static void writeScratchFile(const std::string& path, + const std::vector& bytes) { + std::ofstream f(path, std::ios::binary | std::ios::trunc); + f.write(reinterpret_cast(bytes.data()), + static_cast(bytes.size())); +} + +static std::vector readAll(const std::string& path) { + std::ifstream f(path, std::ios::binary); + return std::vector(std::istreambuf_iterator(f), + std::istreambuf_iterator()); +} + +static void testLandRecordsOnSuccessOnly() { + LandedFileJournal journal; + const std::string path = "rb_land.bin"; + const std::vector bytes = patternBytes(32, 1); + CHECK(journal.writeLandedFile(path, PayloadBuffer(bytes))); + CHECK(readAll(path) == bytes); + CHECK(journal.landedPaths() == (std::vector{path})); + CHECK(!fs::exists(path + ".rsbanktmp")); + journal.rollback(); + CHECK(!fs::exists(path)); +} + +static void testExistingDestinationRefusedUntouched() { + LandedFileJournal journal; + const std::string path = "rb_existing.bin"; + const std::vector original = patternBytes(16, 0x60); + writeScratchFile(path, original); + + CHECK(!journal.writeLandedFile(path, PayloadBuffer(patternBytes(8, 1)))); + CHECK(readAll(path) == original); // never overwritten + CHECK(journal.empty()); // a refused write is not recorded + + const RollbackResult result = journal.rollback(); + CHECK(result.deletedCount == 0); + CHECK(fs::exists(path)); // rollback cannot touch a file it did not write + std::error_code ec; + fs::remove(path, ec); +} + +static void testEmptyPayloadRefused() { + LandedFileJournal journal; + CHECK(!journal.writeLandedFile("rb_empty.bin", PayloadBuffer{})); + CHECK(!fs::exists("rb_empty.bin")); + CHECK(journal.empty()); +} + +static void testRollbackDeletesExactlyTheRecordedSet() { + LandedFileJournal journal; + CHECK(journal.writeLandedFile("rb_a.bin", PayloadBuffer(patternBytes(8, 1)))); + CHECK(journal.writeLandedFile("rb_c.bin", PayloadBuffer(patternBytes(8, 2)))); + writeScratchFile("rb_bystander.bin", patternBytes(8, 3)); // not journal-written + + const RollbackResult result = journal.rollback(); + CHECK(result.deletedCount == 2); + CHECK(result.alreadyAbsentCount == 0); + CHECK(result.failedCount == 0); + CHECK(!fs::exists("rb_a.bin")); + CHECK(!fs::exists("rb_c.bin")); + CHECK(fs::exists("rb_bystander.bin")); // exactly the given files, nothing else + CHECK(journal.empty()); + + const RollbackResult second = journal.rollback(); // cleared: a no-op + CHECK(second.deletedCount == 0); + CHECK(fs::exists("rb_bystander.bin")); + std::error_code ec; + fs::remove("rb_bystander.bin", ec); +} + +static void testVanishedFileIsToleratedNotFailed() { + LandedFileJournal journal; + CHECK(journal.writeLandedFile("rb_gone.bin", PayloadBuffer(patternBytes(8, 1)))); + std::error_code ec; + fs::remove("rb_gone.bin", ec); // vanished between land and rollback + CHECK(!ec); + + const RollbackResult result = journal.rollback(); + CHECK(result.deletedCount == 0); + CHECK(result.alreadyAbsentCount == 1); + CHECK(result.failedCount == 0); +} + +int main() { + testLandRecordsOnSuccessOnly(); + testExistingDestinationRefusedUntouched(); + testEmptyPayloadRefused(); + testRollbackDeletesExactlyTheRecordedSet(); + testVanishedFileIsToleratedNotFailed(); + + if (g_fail == 0) std::printf("package_rollback: all tests passed\n"); + else std::printf("package_rollback: %d CHECK(s) FAILED\n", g_fail); + return g_fail == 0 ? 0 : 1; +} From edfd7ead4d4c90cde982e5ff29a696ecf7ab69ed Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:15:12 -0400 Subject: [PATCH 2/4] Fix the package fs seam: UTF-8 paths, GetUserFileName pickers, exclusive-create landing, rollback arm/disarm Both pickers now ride GetUserFileName (mode 0/1); the "no save picker" premise was false. Landing uses O_EXCL so the create is the existence check, not a TOCTOU pair. --- src/shell/package/CLAUDE.md | 110 ++++++++++------ src/shell/package/CMakeLists.txt | 6 +- src/shell/package/package_io.cpp | 100 ++++++++++++-- src/shell/package/package_io.h | 75 ++++++----- src/shell/package/package_path.h | 19 +++ src/shell/package/package_pickers.cpp | 96 ++++---------- src/shell/package/package_pickers.h | 20 +-- src/shell/package/package_rollback.cpp | 35 +++-- src/shell/package/package_rollback.h | 34 +++-- tests/test_package_io.cpp | 173 ++++++++++++++++++++----- tests/test_package_rollback.cpp | 117 ++++++++++++----- 11 files changed, 532 insertions(+), 253 deletions(-) create mode 100644 src/shell/package/package_path.h diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index 6456fe9..0caf183 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -3,59 +3,89 @@ ## Scope The filesystem and dialog acts behind bank-package export/import: streaming package -file I/O (`package_io`), the landed-file journal and its rollback delete -(`package_rollback`), and the platform file pickers (`package_pickers`). This seam -is bytes-only — the package format (magic, manifest, entry layout) is -`core/package`'s business, and the export/import verbs that orchestrate both do not -live here yet. No REAPER project state is touched in this directory: no ext-state -read or write, no undo block, no generation bump — those belong to the verbs. +file I/O plus the file-status and exclusive-create acts (`package_io`), the +UTF-8 path conversion every one of them goes through (`package_path`), the landed-file +journal and its rollback delete (`package_rollback`), and the two file pickers +(`package_pickers`). This seam is bytes-only — the package format (magic, manifest, +entry layout) is `core/package`'s business, and the export/import verbs that +orchestrate both do not live here yet. No REAPER project state is touched in this +directory: no ext-state read or write, no undo block, no generation bump — those +belong to the verbs. ## Invariants -- **Atomic write.** A package accumulates in a `.rsbanktmp` sibling in the - destination directory and reaches the destination only through `commit()`'s - rename (the mono-collapse temp+rename precedent). A failed, aborted, or abandoned - write leaves the destination absent or holding its prior contents — never a - partial `.rsbank`. +- **Paths cross this seam as UTF-8 narrow strings and are converted through + `utf8Path()` before ANY filesystem call.** This is not decoration: on Windows + `std::filesystem` decodes a narrow path through the runtime ANSI code page (measured + `GetACP() == 1252`), so a bare `fs::path(std::string)` turns `café.rsbank` into + `café.rsbank` or fails to open it. Every path a verb hands in or gets back — + including `listFolderFileNames`' results, which use `u8string()` and never + `string()` — is UTF-8. `core/util/file_bytes` has the un-converted shape, which is + why `readFilePayload` reads through this module's own `PackageFileReader` instead. +- **Atomic package write, to the limit of a rename.** A package accumulates in a + `.rsbanktmp` sibling in the destination directory and reaches the destination only + through `commit()`'s rename (the mono-collapse temp+rename precedent). A failed, + aborted, or abandoned write leaves the destination absent or holding its prior + contents. This is process-crash atomic, NOT power-loss atomic: `commit()` flushes + and closes but does not `fsync`/`FlushFileBuffers`, so a power cut can still leave a + renamed-but-unflushed file. Deliberate — an fsync over a whole sample bank is a real + stall, and the failure this design targets is a refused or interrupted export. - **Streaming, both ways — at most ONE entry's payload in memory.** Writes append one payload at a time; reads seek and materialize one range at a time. The claim is structural, not aspirational: every payload crosses this seam as a move-only `PayloadBuffer`, and `PayloadBuffer::alive()` is the seam counter the tests assert against. There is no read-whole-package or write-whole-package entry point; do not add one. -- **The rollback delete is prune's ONE carve-out, cited not restated.** The - citation and the discriminator live at `package_rollback.cpp`'s header. The - journal makes the discriminator structural: only paths its own `writeLandedFile` - successfully created are recorded, and `rollback()` consumes only the record — a - path this import did not write cannot be handed to it. -- **No overwrite of a bank-folder file, ever.** `writeLandedFile` refuses an - existing destination outright; collision handling (auto-rename) is the import - plan's job upstream. The package writer itself DOES replace an existing - destination — the export save dialog's own overwrite confirm is the consent — - and that asymmetry is deliberate. -- **The two pickers are asymmetric, and the asymmetry is real.** Import rides - REAPER's own `GetUserFileNameForRead` (both platforms); export goes native — - Win32 `GetSaveFileNameW` / SWELL `BrowseForSaveFile` — because the always-present - REAPER surface offers no save picker. Do not symmetrize; the newer - `GetUserFileName(mode=0)` alternative and why it is not used are recorded in - `package_pickers.cpp`'s header. +- **An empty `PayloadBuffer` is a failure signal, never an entry.** It is the seam's + one "nothing to work with" branch, so both `PackageFileWriter::appendPayload` and + `writeFileExclusive` refuse it — appending it would let a verb commit framing that + claims bytes nobody wrote. `appendRaw(ptr, 0)` stays tolerated: framing has + legitimate zero-length edges. +- **No overwrite of a bank-folder file, ever — and the create is the check.** + `writeLandedFile` lands through `writeFileExclusive` (`O_EXCL` / `_O_EXCL`), so the + refusal of an occupied path is one atomic act rather than an `exists()` a concurrent + writer could win the race against. Collision handling (auto-rename) remains the + import plan's job upstream. The package writer itself DOES replace an existing + destination — the export save dialog's own overwrite confirm is the consent — and + that asymmetry is deliberate. +- **The rollback delete is prune's ONE carve-out, and only HALF of it is structural.** + The citation and the full discriminator live at `package_rollback.cpp`'s header. + "Did this call create it" is structural: only exclusively-created paths are + recorded, resolved absolute at record time so a later CWD change cannot re-aim the + delete. "Did anything ever reference it" is a **contract the import verb must + honour**: it MUST call `markIndexCommitted()` at the moment it commits the index, + after which `rollback()` refuses and `writeLandedFile` refuses. +- **Both pickers ride `GetUserFileName`** — mode 1 for import, mode 0 for export. + There is no platform split and no fallback: `main.cpp` defines `REAPERAPI_IMPLEMENT` + without `REAPERAPI_MINIMAL` and aborts the extension load if any single name fails + to resolve, so a build that can load us cannot lack it. ## Modules -- `package_io` — the streaming filesystem seam: `PayloadBuffer` (move-only payload + the `alive()` seam counter), `PackageFileWriter` (append-only temp+atomic-rename writer), `PackageFileReader` (seek-and-read one range per call, range-checked against the real file size), `readFilePayload` (one source file as one entry's payload), and `listFolderFileNames` (bare names, sorted, non-recursive, non-throwing). REAPER-free; tested without a DAW. -- `package_rollback` — `LandedFileJournal`: `writeLandedFile` (temp+rename land, recorded on success only, refuses an existing destination and an empty payload) and `rollback` (deletes exactly the recorded set, hard unlink — nothing ever referenced these bytes — tolerating a vanished file). REAPER-free; tested without a DAW. -- `package_pickers` — the two pickers in one platform TU (`#ifdef _WIN32` / `#else swell/swell.h`, the `draw_kit`/`prune_fs` split): `pickPackageForImport` (REAPER read picker) and `pickPackageSavePath` (native save dialog, UTF-8 in/out on Windows). Compile-only until the verbs land; nothing here can be exercised in a unit test. +- `package_path` — header-only; the ONE UTF-8-narrow → `fs::path` conversion, so the encoding contract has a single enforcement point. +- `package_io` — every filesystem act the verbs need: `PayloadBuffer` (move-only payload + the `alive()` seam counter), `PackageFileWriter` (append-only temp+atomic-rename writer), `PackageFileReader` (seek-and-read one range per call, range-checked against the real file size), `readFilePayload` (one source file as one entry's payload), `fileStatus` (Present/Absent/Unreadable — export's refusal message must distinguish the last two, and an empty payload cannot), `writeFileExclusive` (exclusive create + write, self-cleaning on a partial write), and `listFolderFileNames` (bare UTF-8 names, sorted, non-recursive, non-throwing). REAPER-free; tested without a DAW. +- `package_rollback` — `LandedFileJournal`: `writeLandedFile` (exclusive-create land, path resolved absolute, recorded on success only), `markIndexCommitted` (disarms the journal), and `rollback` (deletes exactly the recorded set, hard unlink, tolerating a vanished file; refuses once disarmed). REAPER-free; tested without a DAW. +- `package_pickers` — `pickPackageForImport` and `pickPackageSavePath`, both `GetUserFileName`. Compile-only until the verbs land; nothing here can be exercised in a unit test. ## Gotchas -- A crash mid-write strands the `.rsbanktmp` sibling. It is not a `.rsbank` (no - picker filter matches it), and a later export to the same destination truncates - it — but one stranded in the BANK folder by a mid-import crash is a foreign file - to prune (not owned, so never an orphan) until removed by hand. `[verify — DAW]` - whether the import verb should pre-clean stale `.rsbanktmp` names when it lands. -- The picker `defext`/filter strings are spelled to the Win32 `lpstrDefExt` - convention (no dot) but are `[verify — DAW]` on all three platforms — neither - picker is exercised outside a live REAPER session. +- A crash mid-export strands the `.rsbanktmp` sibling. It is not a `.rsbank` (no + picker filter matches it), and a later export to the same destination truncates it. + A crash mid-import strands a partial bank file under its real name instead — the + land is a direct exclusive create, not temp+rename. Either way the debris was never + recorded in the tracking ledger, so prune sees a foreign file (not owned, never an + orphan) and will not touch it; removal is by hand. `[verify — DAW]` whether the + import verb should pre-clean stale debris when it lands. +- The picker filter and mode arguments are spelled to `GetUserFileName`'s documented + pair format but are `[verify — DAW]` on all three platforms — neither picker is + exercised outside a live REAPER session. `GetUserFileName` also takes no owner + window, so dialog parenting is REAPER's to do; the superseded Win32 path passed + `GetMainHwnd()` explicitly. +- `pickPackageSavePath`'s `suggestedPath` doubles as the dialog's starting directory + when it is a full path. The verbs should seed it from the project directory — + passing a bare name leaves the dialog on REAPER's process working directory, which + is its install or resource path. - `readRange(_, 0)` returns an empty buffer — indistinguishable from failure, by - design (the one "nothing to work with" branch). A genuinely zero-length entry - cannot round-trip through this seam; the format layer must not emit one. + design (the one "nothing to work with" branch). **Cross-track contract, not a local + rule:** a genuinely zero-length entry cannot round-trip through this seam, so + `core/package`'s format layer must not emit one. diff --git a/src/shell/package/CMakeLists.txt b/src/shell/package/CMakeLists.txt index ad716e4..aff4414 100644 --- a/src/shell/package/CMakeLists.txt +++ b/src/shell/package/CMakeLists.txt @@ -3,14 +3,14 @@ # their tests run without a DAW. The export/import verbs that drive all three targets # are not in this directory yet. -reasampler_pure_library(package_io SOURCES package_io.cpp LINK PUBLIC file_bytes) +reasampler_pure_library(package_io SOURCES package_io.cpp) reasampler_test(package_io LINK package_io) reasampler_pure_library(package_rollback SOURCES package_rollback.cpp LINK PUBLIC package_io) reasampler_test(package_rollback LINK package_rollback) -# The platform pickers touch the REAPER API and the native save dialog, so no test -# target can exercise them; declared as a library so both picker paths stay compiled. +# The pickers call the REAPER API, so no test target can exercise them; declared as a +# library so the TU stays compiled. reaper_plugin.h pulls SWELL in on non-Windows. add_library(package_pickers STATIC package_pickers.cpp) target_include_directories(package_pickers PUBLIC ${REASAMPLER_SRC_DIR}) target_include_directories(package_pickers PRIVATE ${SDK_INC} ${WDL_INC}) diff --git a/src/shell/package/package_io.cpp b/src/shell/package/package_io.cpp index 92f645b..901dc3f 100644 --- a/src/shell/package/package_io.cpp +++ b/src/shell/package/package_io.cpp @@ -1,15 +1,24 @@ // package_io.cpp — see package_io.h for the seam's contract. Non-throwing at the -// boundary: every filesystem call uses the error_code form so no filesystem_error +// boundary: every filesystem call uses the error_code form, so no filesystem_error // crosses into a REAPER action body. #include "shell/package/package_io.h" #include #include -#include #include -#include "core/util/file_bytes.h" +#include "shell/package/package_path.h" + +#ifdef _WIN32 +#include +#include +#include +#include +#else +#include +#include +#endif namespace reasampler { @@ -58,8 +67,9 @@ void PayloadBuffer::release() { // --------------------------------------------------------------------------- // PackageFileWriter -PackageFileWriter::PackageFileWriter(std::string destAbsPath) - : destPath_(std::move(destAbsPath)), tempPath_(destPath_ + ".rsbanktmp") { +PackageFileWriter::PackageFileWriter(const std::string& destAbsPath) + : destPath_(utf8Path(destAbsPath)), tempPath_(destPath_) { + tempPath_ += ".rsbanktmp"; // += concatenates; / would make it a child out_.open(tempPath_, std::ios::binary | std::ios::trunc); ok_ = static_cast(out_); } @@ -78,6 +88,10 @@ bool PackageFileWriter::appendRaw(const std::uint8_t* data, std::size_t len) { } bool PackageFileWriter::appendPayload(const PayloadBuffer& payload) { + if (payload.empty()) { + ok_ = false; // the stream is now short of what the framing will claim + return false; + } return appendRaw(payload.data(), payload.size()); } @@ -120,10 +134,11 @@ void PackageFileWriter::abort() { // PackageFileReader PackageFileReader::PackageFileReader(const std::string& srcAbsPath) { + const fs::path path = utf8Path(srcAbsPath); std::error_code ec; - const std::uintmax_t sz = fs::file_size(srcAbsPath, ec); + const std::uintmax_t sz = fs::file_size(path, ec); if (ec) return; - in_.open(srcAbsPath, std::ios::binary); + in_.open(path, std::ios::binary); if (!in_) return; size_ = static_cast(sz); ok_ = true; @@ -147,8 +162,73 @@ PayloadBuffer PackageFileReader::readRange(std::uint64_t offset, std::uint64_t l // --------------------------------------------------------------------------- +// Deliberately NOT core/util/file_bytes: that loader takes an unconverted narrow path +// (see package_path.h), and this seam's own reader already goes through utf8Path. PayloadBuffer readFilePayload(const std::string& absPath) { - return PayloadBuffer(util::readFileBytes(absPath)); + PackageFileReader reader(absPath); + return reader.readRange(0, reader.fileSize()); +} + +FileStatus fileStatus(const std::string& absPath) { + const fs::path path = utf8Path(absPath); + std::error_code ec; + const fs::file_status st = fs::status(path, ec); + // status() reports not_found through the type AND sets ec, so the type is the + // discriminator; an ec with any other type is a real access failure. + if (st.type() == fs::file_type::not_found) return FileStatus::Absent; + if (ec || !fs::is_regular_file(st)) return FileStatus::Unreadable; + std::ifstream probe(path, std::ios::binary); + return probe ? FileStatus::Present : FileStatus::Unreadable; +} + +bool writeFileExclusive(const std::string& absPath, const PayloadBuffer& payload) { + if (payload.empty()) return false; + const fs::path path = utf8Path(absPath); + + int fd = -1; +#ifdef _WIN32 + if (_wsopen_s(&fd, path.wstring().c_str(), + _O_WRONLY | _O_CREAT | _O_EXCL | _O_BINARY, _SH_DENYNO, + _S_IREAD | _S_IWRITE) != 0) { + return false; + } +#else + fd = ::open(path.c_str(), O_WRONLY | O_CREAT | O_EXCL, 0644); +#endif + if (fd < 0) return false; + + bool ok = true; + std::size_t written = 0; + while (written < payload.size()) { + // Chunked because the Windows _write count is an unsigned int, not size_t. + const std::size_t chunk = + std::min(payload.size() - written, 1u << 20); +#ifdef _WIN32 + const int n = _write(fd, payload.data() + written, + static_cast(chunk)); +#else + const ssize_t n = ::write(fd, payload.data() + written, chunk); +#endif + if (n <= 0) { + ok = false; + break; + } + written += static_cast(n); + } + +#ifdef _WIN32 + _close(fd); +#else + ::close(fd); +#endif + + if (!ok) { + // Self-cleanup, not deletion authority: this call created the file moments + // ago and nothing has ever referenced it (prune_fs.cpp's carve-out). + std::error_code ec; + fs::remove(path, ec); + } + return ok; } std::vector listFolderFileNames(const std::string& dirAbsPath) { @@ -156,12 +236,12 @@ std::vector listFolderFileNames(const std::string& dirAbsPath) { std::error_code ec; // Manual iterator form (it.increment(ec)) keeps the loop non-throwing on a // mid-iteration failure, matching prune_fs's enumerate. - fs::directory_iterator it(dirAbsPath, ec); + fs::directory_iterator it(utf8Path(dirAbsPath), ec); for (; !ec && it != fs::directory_iterator{}; it.increment(ec)) { const auto& entry = *it; std::error_code reg_ec; if (!entry.is_regular_file(reg_ec)) continue; // skip subdirs / specials - names.push_back(entry.path().filename().string()); + names.push_back(entry.path().filename().u8string()); // never .string(): ANSI } std::sort(names.begin(), names.end()); return names; diff --git a/src/shell/package/package_io.h b/src/shell/package/package_io.h index 6f36d2d..332a5f1 100644 --- a/src/shell/package/package_io.h +++ b/src/shell/package/package_io.h @@ -1,22 +1,22 @@ -// shell/package/package_io — streaming filesystem seam for bank packages: append one -// payload at a time through a temp-file + atomic-rename writer, seek and read one -// payload at a time back out. Bytes only: what a package contains is core/package's -// business, never this seam's. Blocking I/O — UI-thread actions only, never the -// audio thread. +// shell/package/package_io — every filesystem act the export/import verbs need: +// streaming package read/write, whole-file payload read, folder listing, file status, +// and the exclusive create that lands one bank file. Bytes only — what a package +// contains is core/package's business. Blocking I/O: UI-thread actions only, never +// the audio thread. #pragma once #include +#include #include #include #include namespace reasampler { -// One entry's payload, and the seam counter that makes "never more than one entry in -// memory" assertable: alive() counts every buffer currently holding bytes, so the -// streaming claim is a test CHECK against this counter rather than a memory -// measurement. Move-only — copying a payload would silently double the held bytes. +// One entry's payload. Move-only, because a copy would silently double the bytes the +// seam promises to hold at most one of; alive() is the counter that makes that +// promise assertable instead of aspirational. class PayloadBuffer { public: PayloadBuffer() = default; @@ -30,7 +30,6 @@ public: const std::uint8_t* data() const { return bytes_.data(); } std::size_t size() const { return bytes_.size(); } bool empty() const { return bytes_.empty(); } - const std::vector& bytes() const { return bytes_; } // Buffers currently holding at least one byte, process-wide. static int alive(); @@ -42,26 +41,25 @@ private: bool counted_ = false; }; -// Streaming atomic writer. Bytes accumulate in ".rsbanktmp" beside the -// destination (same directory, so the final rename never crosses a volume); the -// destination itself is touched only by commit()'s rename, so a failed, aborted, or -// abandoned write leaves it absent or holding its prior contents — never a partial -// file. Destruction without commit() aborts and removes the temp. commit() REPLACES -// an existing destination: the export save dialog's own overwrite confirm is the -// consent (the never-overwrite rule for bank-folder files lives in -// LandedFileJournal, upstream of this writer). Neither copyable nor movable, and -// append-only — there is deliberately no way to hand it a whole package at once. +// Streaming atomic writer; paths cross this seam as UTF-8 narrow strings and are held +// as fs::path internally. The temp sibling is created in the DESTINATION's own +// directory so commit()'s rename never crosses a volume — a cross-device rename +// degrades to a copy and stops being atomic. commit() REPLACES an existing +// destination (the deliberate asymmetry against LandedFileJournal; see CLAUDE.md). class PackageFileWriter { public: - explicit PackageFileWriter(std::string destAbsPath); + explicit PackageFileWriter(const std::string& destAbsPath); ~PackageFileWriter(); PackageFileWriter(const PackageFileWriter&) = delete; PackageFileWriter& operator=(const PackageFileWriter&) = delete; bool ok() const { return ok_; } - // Framing/header bytes. False on a failed or already-finished writer. + // Framing/header bytes. False on a failed or already-finished writer. A zero + // length is accepted — framing has legitimate zero-length edges. bool appendRaw(const std::uint8_t* data, std::size_t len); - // One entry's bytes. Same contract as appendRaw. + // One entry's bytes. Also false — and the writer poisoned — on an EMPTY payload: + // empty is this seam's one "nothing to work with" signal, so accepting it would + // let a verb commit a package whose framing claims bytes nobody wrote. bool appendPayload(const PayloadBuffer& payload); // Flush, close, rename over the destination. False (and self-cleaning: the temp // is removed, the destination untouched) on any failure or on a second call. @@ -69,21 +67,21 @@ public: // Close and remove the temp; the destination is never touched. Idempotent. void abort(); - const std::string& destPath() const { return destPath_; } - const std::string& tempPath() const { return tempPath_; } + const std::filesystem::path& destPath() const { return destPath_; } + const std::filesystem::path& tempPath() const { return tempPath_; } private: - std::string destPath_; - std::string tempPath_; + std::filesystem::path destPath_; + std::filesystem::path tempPath_; std::ofstream out_; bool ok_ = false; bool done_ = false; }; // Seek-and-read reader: exactly one payload is materialized per readRange call, and -// there is deliberately no read-whole-file entry point. Empty buffer on ANY failure -// — unopenable file, zero length, out of range, short read — so the caller has one -// "nothing to work with" branch (file_bytes' contract). +// there is deliberately no read-whole-file entry point. Empty buffer on ANY failure — +// unopenable file, zero length, out of range, short read — so the caller has one +// "nothing to work with" branch. Use fileStatus() when the two must be told apart. class PackageFileReader { public: explicit PackageFileReader(const std::string& srcAbsPath); @@ -101,11 +99,24 @@ private: }; // One source file read whole as one entry's payload — a bank file IS the streaming -// unit, so whole-file here is one entry, released before the next is read. Empty on -// any failure, per readFileBytes. +// unit. Empty on any failure, per PackageFileReader. PayloadBuffer readFilePayload(const std::string& absPath); -// Bare file names (regular files only, never a path) in dirAbsPath, sorted so +// Export must tell a missing indexed file from an unreadable one in its refusal +// message; readFilePayload deliberately cannot, since both fail to an empty buffer. +enum class FileStatus { Present, Absent, Unreadable }; +FileStatus fileStatus(const std::string& absPath); + +// Creates absPath and writes the payload, failing if ANYTHING already occupies the +// path. The create IS the existence check (O_EXCL / CREATE_NEW), so nothing can slip +// in between: an exists()-then-write pair would let a file created in that window be +// overwritten and then deleted by a rollback that believes it wrote it. Refuses an +// empty payload, and removes its own partial file on a mid-write failure. Not +// temp+rename — an exclusive rename has no portable spelling, and the debris a crash +// leaves here is unrecorded and unindexed either way. +bool writeFileExclusive(const std::string& absPath, const PayloadBuffer& payload); + +// Bare file names (regular files only, never a path) in dirAbsPath, UTF-8, sorted so // callers see a deterministic order; empty on a missing or unreadable folder. std::vector listFolderFileNames(const std::string& dirAbsPath); diff --git a/src/shell/package/package_path.h b/src/shell/package/package_path.h new file mode 100644 index 0000000..9982496 --- /dev/null +++ b/src/shell/package/package_path.h @@ -0,0 +1,19 @@ +// shell/package/package_path — the ONE narrow-string -> fs::path conversion for this +// seam. std::filesystem decodes a narrow path through the RUNTIME ANSI code page on +// Windows (measured: GetACP() == 1252 here), never UTF-8, so a bare +// fs::path(std::string) turns every non-ASCII path this repo's UTF-8 convention +// produces into mojibake. u8path is the C++17 spelling; it is deprecated in C++20, so +// a standard bump replaces the body here rather than at every call site. + +#pragma once + +#include +#include + +namespace reasampler { + +inline std::filesystem::path utf8Path(const std::string& utf8) { + return std::filesystem::u8path(utf8); +} + +} // namespace reasampler diff --git a/src/shell/package/package_pickers.cpp b/src/shell/package/package_pickers.cpp index b122bd9..75af2a0 100644 --- a/src/shell/package/package_pickers.cpp +++ b/src/shell/package/package_pickers.cpp @@ -1,93 +1,45 @@ -// package_pickers.cpp — see package_pickers.h. Export is native rather than REAPER -// API: the SDK's newer GetUserFileName(mode=0) could save, but it resolves to null -// on older REAPER builds this extension still loads in, while GetSaveFileNameW / -// BrowseForSaveFile are always present. main.cpp owns the REAPER API pointers; this -// TU gets them extern. +// package_pickers.cpp — see package_pickers.h. GetUserFileName cannot be null here: +// main.cpp defines REAPERAPI_IMPLEMENT without REAPERAPI_MINIMAL, so the generated +// resolver walks the FULL table, and it aborts the extension load if any single name +// fails to resolve. A fallback picker would be unreachable code. #include "shell/package/package_pickers.h" -#ifdef _WIN32 -#include -#include -#else -#include "swell/swell.h" -#endif - #define REAPERAPI_MINIMAL -#define REAPERAPI_WANT_GetUserFileNameForRead -#define REAPERAPI_WANT_GetMainHwnd +#define REAPERAPI_WANT_GetUserFileName #include "reaper_plugin_functions.h" namespace reasampler { -bool pickPackageForImport(std::string& outAbsPath) { +namespace { + +// GetUserFileName's documented pair format: label|pattern|label|pattern. +const char kExtList[] = + "ReaSampler bank package (*.rsbank)|*.rsbank|All files (*.*)|*.*"; + +bool runPicker(int mode, const char* caption, const char* initial, + std::string& outAbsPath) { outAbsPath.clear(); - if (!GetUserFileNameForRead) return false; char buf[4096]; buf[0] = '\0'; - // defext spelled without the dot, matching Win32 lpstrDefExt. [verify — DAW] - // whether REAPER's picker applies it to the shown filter. - if (!GetUserFileNameForRead(buf, "Import bank package", "rsbank")) return false; + if (!GetUserFileName(mode, caption, initial, kExtList, buf, + static_cast(sizeof(buf)))) { + return false; + } outAbsPath = buf; return !outAbsPath.empty(); } -#ifdef _WIN32 +} // namespace -bool pickPackageSavePath(const std::string& suggestedFileName, std::string& outAbsPath) { - outAbsPath.clear(); - - wchar_t file[4096]; - file[0] = L'\0'; - if (!suggestedFileName.empty()) { - const int wrote = MultiByteToWideChar(CP_UTF8, 0, suggestedFileName.c_str(), - -1, file, 4096); - if (wrote <= 0) file[0] = L'\0'; // unconvertible suggestion -> empty box - } - - OPENFILENAMEW ofn{}; - ofn.lStructSize = sizeof(ofn); - ofn.hwndOwner = GetMainHwnd ? GetMainHwnd() : nullptr; - ofn.lpstrFilter = - L"ReaSampler bank package (*.rsbank)\0*.rsbank\0All files (*.*)\0*.*\0"; - ofn.lpstrFile = file; - ofn.nMaxFile = 4096; - ofn.lpstrTitle = L"Export bank package"; - ofn.lpstrDefExt = L"rsbank"; - // NOCHANGEDIR: REAPER's process-wide working directory is not ours to move. - ofn.Flags = OFN_OVERWRITEPROMPT | OFN_PATHMUSTEXIST | OFN_NOCHANGEDIR | - OFN_HIDEREADONLY; - if (!GetSaveFileNameW(&ofn)) return false; - - const int need = - WideCharToMultiByte(CP_UTF8, 0, file, -1, nullptr, 0, nullptr, nullptr); - if (need <= 0) return false; - std::string utf8(static_cast(need), '\0'); - WideCharToMultiByte(CP_UTF8, 0, file, -1, &utf8[0], need, nullptr, nullptr); - if (!utf8.empty() && utf8.back() == '\0') utf8.pop_back(); - outAbsPath = std::move(utf8); - return !outAbsPath.empty(); +bool pickPackageForImport(std::string& outAbsPath) { + return runPicker(1, "Import bank package", "", outAbsPath); } -#else // SWELL (macOS / Linux) - -bool pickPackageSavePath(const std::string& suggestedFileName, std::string& outAbsPath) { - outAbsPath.clear(); - // GetOpenFileName-style pair list; the literal's implicit terminator supplies the - // closing double-NUL. [verify — DAW] the exact filter strings SWELL accepts. - static const char kExtList[] = "ReaSampler bank package (*.rsbank)\0*.rsbank\0"; - char fn[4096]; - fn[0] = '\0'; - if (!BrowseForSaveFile("Export bank package", nullptr, - suggestedFileName.empty() ? nullptr - : suggestedFileName.c_str(), - kExtList, fn, static_cast(sizeof(fn)))) { - return false; - } - outAbsPath = fn; - return !outAbsPath.empty(); +bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath) { + // [verify — DAW] GetUserFileName takes no owner window, so the dialog's parenting + // is REAPER's to do; the previous Win32 path passed GetMainHwnd() explicitly. + return runPicker(0, "Export bank package", suggestedPath.c_str(), outAbsPath); } -#endif - } // namespace reasampler diff --git a/src/shell/package/package_pickers.h b/src/shell/package/package_pickers.h index cdfbc1b..2cd8cf7 100644 --- a/src/shell/package/package_pickers.h +++ b/src/shell/package/package_pickers.h @@ -1,8 +1,7 @@ -// shell/package/package_pickers — the two package file pickers, and they are -// deliberately asymmetric: import rides REAPER's own read picker -// (GetUserFileNameForRead, both platforms); export goes native — Win32 -// GetSaveFileNameW / SWELL BrowseForSaveFile — because the always-present REAPER -// surface offers no save picker. Do not symmetrize; the rationale is in the TU. +// shell/package/package_pickers — the two package file pickers, both on REAPER's own +// GetUserFileName (mode 1 = existing file, mode 0 = new file). No native/platform +// split: the SDK's save mode is not optional on any build that can load this +// extension. Paths in and out are UTF-8, per the REAPER API contract. #pragma once @@ -10,12 +9,13 @@ namespace reasampler { -// REAPER's read picker. True with outAbsPath set iff the user chose a file. +// True with outAbsPath set iff the user chose a file. bool pickPackageForImport(std::string& outAbsPath); -// Native save picker, pre-filled with suggestedFileName (a bare name, e.g. -// "MyBank.rsbank"). True with outAbsPath set iff the user chose a destination; the -// dialog's own overwrite confirm has already run by then. -bool pickPackageSavePath(const std::string& suggestedFileName, std::string& outAbsPath); +// suggestedPath is a bare file name ("MyBank.rsbank") or a full path — a full one +// also seeds the dialog's starting directory, which is how a caller keeps the picker +// off REAPER's process working directory. True with outAbsPath set iff the user chose +// a destination; the dialog's own overwrite confirm has already run by then. +bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath); } // namespace reasampler diff --git a/src/shell/package/package_rollback.cpp b/src/shell/package/package_rollback.cpp index ddface7..1219292 100644 --- a/src/shell/package/package_rollback.cpp +++ b/src/shell/package/package_rollback.cpp @@ -1,34 +1,45 @@ -// package_rollback.cpp — see package_rollback.h. The rollback delete below runs -// under the ONE carve-out from prune's exclusive file-deletion authority, stated at -// src/shell/persist/prune_fs.cpp:5-11; it satisfies that discriminator by -// construction — every recorded path was created by this journal's own -// writeLandedFile, and the abandoned index mutation means nothing ever referenced it. +// package_rollback.cpp — see package_rollback.h. The rollback delete below runs under +// the ONE carve-out from prune's exclusive file-deletion authority, stated at +// src/shell/persist/prune_fs.cpp:5-11. That discriminator has two clauses and this +// journal makes only the FIRST structural: "did this call create it" is guaranteed by +// recording exclusively-created paths, but "did anything ever reference it" is a +// claim about the caller's ordering — hence markIndexCommitted(), which the import +// verb must fire at the index commit so a later rollback() refuses instead of +// deleting indexed files. #include "shell/package/package_rollback.h" #include +#include "shell/package/package_path.h" + namespace reasampler { namespace fs = std::filesystem; -bool LandedFileJournal::writeLandedFile(const std::string& absPath, +bool LandedFileJournal::writeLandedFile(const std::string& destPath, const PayloadBuffer& payload) { - if (payload.empty()) return false; + if (indexCommitted_) return false; + std::error_code ec; - if (fs::exists(absPath, ec) || ec) return false; - PackageFileWriter writer(absPath); - if (!writer.appendPayload(payload)) return false; // dtor aborts; temp removed - if (!writer.commit()) return false; + const fs::path resolved = fs::absolute(utf8Path(destPath), ec); + if (ec) return false; + const std::string absPath = resolved.u8string(); + + if (!writeFileExclusive(absPath, payload)) return false; paths_.push_back(absPath); return true; } RollbackResult LandedFileJournal::rollback() { RollbackResult result; + if (indexCommitted_) { + result.refused = true; + return result; + } for (const std::string& path : paths_) { std::error_code ec; - const bool removed = fs::remove(path, ec); + const bool removed = fs::remove(utf8Path(path), ec); if (removed) ++result.deletedCount; else if (ec) ++result.failedCount; else ++result.alreadyAbsentCount; // no error, nothing there diff --git a/src/shell/package/package_rollback.h b/src/shell/package/package_rollback.h index a72f89c..cfc663d 100644 --- a/src/shell/package/package_rollback.h +++ b/src/shell/package/package_rollback.h @@ -1,8 +1,7 @@ // shell/package/package_rollback — the files ONE import call has landed, as a // journal: writes record themselves on success, and rollback() deletes exactly what -// is recorded — a path this import did not write is structurally impossible to hand -// it. The deletion carve-out this satisfies is cited at package_rollback.cpp's -// header. +// is recorded. The deletion carve-out this satisfies, and the half of it the caller +// still owns, are at package_rollback.cpp's header. #pragma once @@ -17,22 +16,30 @@ struct RollbackResult { int deletedCount = 0; int alreadyAbsentCount = 0; // vanished between land and rollback — not a failure int failedCount = 0; // locked / permission — recorded, never thrown + bool refused = false; // markIndexCommitted() ran: nothing was deleted }; -// The evidence for the rollback discriminator: only paths this journal's own -// writeLandedFile successfully created are recorded, so rollback() can never touch a -// byte this import did not write. class LandedFileJournal { public: - // Lands one payload at absPath through the atomic temp+rename writer and records - // the path on success. REFUSES an existing destination — a bank-folder file is - // never overwritten; collision handling is the import plan's job, upstream. An - // empty payload is refused too: it signals an upstream read failure, never a - // real entry. - bool writeLandedFile(const std::string& absPath, const PayloadBuffer& payload); + // Lands one payload at destPath through the exclusive create (which refuses an + // occupied path outright — a bank-folder file is never overwritten, and collision + // handling is the import plan's job upstream) and records it on success. An empty + // payload is refused, per writeFileExclusive. Relative paths are resolved against + // the process CWD before the write, so the journal's record is always absolute + // and a later CWD change cannot re-aim the delete. Refused once + // markIndexCommitted() has run. + bool writeLandedFile(const std::string& destPath, const PayloadBuffer& payload); + + // Disarms the journal: the index mutation these files back is committed, so they + // are now referenced bytes and the carve-out no longer covers them. This is the + // half of prune's discriminator the journal cannot make structural on its own — + // the import verb MUST call it at the moment the index is committed. + void markIndexCommitted() { indexCommitted_ = true; } + bool indexCommitted() const { return indexCommitted_; } // Deletes exactly the recorded files and clears the journal, so a second call is - // a no-op. Hard unlink, not trash: nothing ever referenced these bytes. + // a no-op. Hard unlink, not trash: nothing ever referenced these bytes. Refuses + // (deleting nothing, keeping the record) once markIndexCommitted() has run. RollbackResult rollback(); const std::vector& landedPaths() const { return paths_; } @@ -40,6 +47,7 @@ public: private: std::vector paths_; + bool indexCommitted_ = false; }; } // namespace reasampler diff --git a/tests/test_package_io.cpp b/tests/test_package_io.cpp index 7f10b73..a533c72 100644 --- a/tests/test_package_io.cpp +++ b/tests/test_package_io.cpp @@ -1,12 +1,11 @@ -// Standalone tests for shell/package/package_io — no REAPER, no framework. Pins the -// two properties the seam exists for: an interrupted or failed write leaves the -// destination absent or holding its prior contents (failure injected at the writer -// seam — abandonment, open failure, rename failure), and a multi-entry round trip -// holds at most one entry's payload, asserted against PayloadBuffer::alive() — the -// seam counter — rather than a memory measurement. +// Standalone tests for shell/package/package_io — no REAPER, no framework. Failure is +// injected at the writer seam (abandonment, open failure, rename failure) rather than +// simulated, and the streaming claim is asserted against PayloadBuffer::alive(). #include "../src/shell/package/package_io.h" +#include "../src/shell/package/package_path.h" +#include #include #include #include @@ -28,19 +27,31 @@ static std::vector patternBytes(std::size_t n, std::uint8_t seed) return v; } +static bool sameBytes(const PayloadBuffer& p, const std::vector& want) { + return p.size() == want.size() && + (want.empty() || std::equal(want.begin(), want.end(), p.data())); +} + static void writeScratchFile(const std::string& path, const std::vector& bytes) { - std::ofstream f(path, std::ios::binary | std::ios::trunc); + std::ofstream f(utf8Path(path), std::ios::binary | std::ios::trunc); f.write(reinterpret_cast(bytes.data()), static_cast(bytes.size())); } static std::vector readAll(const std::string& path) { - std::ifstream f(path, std::ios::binary); + std::ifstream f(utf8Path(path), std::ios::binary); return std::vector(std::istreambuf_iterator(f), std::istreambuf_iterator()); } +static bool exists(const std::string& path) { return fs::exists(utf8Path(path)); } + +static void removeQuietly(const std::string& path) { + std::error_code ec; + fs::remove(utf8Path(path), ec); +} + static void testPayloadCounterTracksMovesNotCopies() { CHECK(PayloadBuffer::alive() == 0); { @@ -81,7 +92,7 @@ static void testStreamingRoundTripHoldsOnePayload() { std::uint64_t offset = header.size(); for (std::size_t i = 0; i < entries.size(); ++i) { PayloadBuffer p = readFilePayload(srcs[i]); - CHECK(p.bytes() == entries[i]); + CHECK(sameBytes(p, entries[i])); CHECK(PayloadBuffer::alive() == 1); // exactly one entry in memory CHECK(writer.appendPayload(p)); layout.emplace_back(offset, p.size()); @@ -91,8 +102,8 @@ static void testStreamingRoundTripHoldsOnePayload() { CHECK(writer.commit()); CHECK(!writer.commit()); // a second commit is refused } - CHECK(!fs::exists(dest + ".rsbanktmp")); - CHECK(fs::exists(dest)); + CHECK(!exists(dest + ".rsbanktmp")); + CHECK(exists(dest)); { // Scoped: the reader holds the file open, and Windows refuses to delete an @@ -103,14 +114,36 @@ static void testStreamingRoundTripHoldsOnePayload() { for (std::size_t i = 0; i < entries.size(); ++i) { PayloadBuffer p = reader.readRange(layout[i].first, layout[i].second); CHECK(PayloadBuffer::alive() == 1); // one entry per readRange, no more - CHECK(p.bytes() == entries[i]); + CHECK(sameBytes(p, entries[i])); } CHECK(PayloadBuffer::alive() == 0); } - std::error_code ec; - for (const std::string& s : srcs) fs::remove(s, ec); - fs::remove(dest, ec); + for (const std::string& s : srcs) removeQuietly(s); + removeQuietly(dest); +} + +static void testEmptyPayloadIsRefusedAndPoisonsTheWriter() { + // The failure this guards: an unreadable source yields an empty payload, and a + // verb that trusted a `true` here would commit framing claiming bytes nobody wrote. + const std::string dest = "pkg_io_emptypayload.rsbank"; + { + PackageFileWriter writer(dest); + const std::vector head = patternBytes(4, 1); + CHECK(writer.appendRaw(head.data(), head.size())); + CHECK(!writer.appendPayload(PayloadBuffer{})); + CHECK(!writer.ok()); + CHECK(!writer.commit()); // the short stream can never reach the destination + } + CHECK(!exists(dest)); + CHECK(!exists(dest + ".rsbanktmp")); + // A zero-length appendRaw stays tolerated — framing has legitimate empty edges. + { + PackageFileWriter writer(dest); + CHECK(writer.appendRaw(nullptr, 0)); + CHECK(writer.ok()); + writer.abort(); + } } static void testAbandonedWriteLeavesNoDestination() { @@ -121,8 +154,8 @@ static void testAbandonedWriteLeavesNoDestination() { CHECK(writer.appendRaw(some.data(), some.size())); // no commit — destruction is the injected interruption } - CHECK(!fs::exists(dest)); - CHECK(!fs::exists(dest + ".rsbanktmp")); + CHECK(!exists(dest)); + CHECK(!exists(dest + ".rsbanktmp")); } static void testAbortPreservesPriorContents() { @@ -138,9 +171,26 @@ static void testAbortPreservesPriorContents() { CHECK(!writer.commit()); } CHECK(readAll(dest) == prior); - CHECK(!fs::exists(dest + ".rsbanktmp")); - std::error_code ec; - fs::remove(dest, ec); + CHECK(!exists(dest + ".rsbanktmp")); + removeQuietly(dest); +} + +static void testCommitReplacesAnExistingFile() { + // The module's one deliberate asymmetry against LandedFileJournal's never-overwrite + // rule: the save dialog's overwrite confirm is the consent, so commit() replaces. + const std::string dest = "pkg_io_replace.rsbank"; + const std::vector prior = patternBytes(40, 0x11); + const std::vector fresh = patternBytes(7, 0x22); + writeScratchFile(dest, prior); + { + PackageFileWriter writer(dest); + CHECK(writer.ok()); + CHECK(writer.appendRaw(fresh.data(), fresh.size())); + CHECK(writer.commit()); + } + CHECK(readAll(dest) == fresh); + CHECK(!exists(dest + ".rsbanktmp")); + removeQuietly(dest); } static void testOpenFailureIsInert() { @@ -150,7 +200,7 @@ static void testOpenFailureIsInert() { const std::vector some = patternBytes(8, 1); CHECK(!writer.appendRaw(some.data(), some.size())); CHECK(!writer.commit()); - CHECK(!fs::exists("pkg_io_no_such_dir")); + CHECK(!exists("pkg_io_no_such_dir")); } static void testCommitRenameFailureSelfCleans() { @@ -158,7 +208,7 @@ static void testCommitRenameFailureSelfCleans() { // injected commit failure, not a simulated one. const std::string dest = "pkg_io_dir.rsbank"; std::error_code ec; - fs::create_directory(dest, ec); + fs::create_directory(utf8Path(dest), ec); CHECK(!ec); { PackageFileWriter writer(dest); @@ -167,9 +217,9 @@ static void testCommitRenameFailureSelfCleans() { CHECK(writer.appendRaw(some.data(), some.size())); CHECK(!writer.commit()); } - CHECK(fs::is_directory(dest)); // prior state intact - CHECK(!fs::exists(dest + ".rsbanktmp")); - fs::remove(dest, ec); + CHECK(fs::is_directory(utf8Path(dest))); // prior state intact + CHECK(!exists(dest + ".rsbanktmp")); + fs::remove(utf8Path(dest), ec); } static void testReaderEdges() { @@ -188,38 +238,97 @@ static void testReaderEdges() { CHECK(reader.readRange(5, 10).empty()); // past the end CHECK(reader.readRange(10, 1).empty()); // starts at the end CHECK(reader.readRange(0, 0).empty()); // zero length is failure, one branch - const PayloadBuffer slice = reader.readRange(2, 3); - CHECK(slice.bytes() == - std::vector(bytes.begin() + 2, bytes.begin() + 5)); + CHECK(sameBytes(reader.readRange(2, 3), + std::vector(bytes.begin() + 2, bytes.begin() + 5))); } + removeQuietly(path); +} + +static void testFileStatusSeparatesAbsentFromUnreadable() { + CHECK(fileStatus("pkg_io_no_such_file.bin") == FileStatus::Absent); + + const std::string path = "pkg_io_status.bin"; + writeScratchFile(path, patternBytes(4, 1)); + CHECK(fileStatus(path) == FileStatus::Present); + removeQuietly(path); + + // A directory in a file's place is not a readable file — the export message must + // not report it as simply missing. + const std::string dir = "pkg_io_status_dir"; std::error_code ec; - fs::remove(path, ec); + fs::create_directory(utf8Path(dir), ec); + CHECK(fileStatus(dir) == FileStatus::Unreadable); + fs::remove(utf8Path(dir), ec); +} + +static void testNonAsciiPathsRoundTripAsUtf8() { + // "café" in UTF-8. Windows decodes a narrow std::filesystem path through the ANSI + // code page, so an unconverted seam lands "café" or fails outright. + const std::string dir = "pkg_io_caf\xC3\xA9_dir"; + const std::string name = "caf\xC3\xA9.rsbank"; + std::error_code ec; + fs::create_directory(utf8Path(dir), ec); + CHECK(!ec); + + const std::string dest = dir + "/" + name; + const std::vector bytes = patternBytes(24, 0x5A); + { + PackageFileWriter writer(dest); + CHECK(writer.ok()); + CHECK(writer.appendRaw(bytes.data(), bytes.size())); + CHECK(writer.commit()); + } + CHECK(exists(dest)); + CHECK(fileStatus(dest) == FileStatus::Present); + CHECK(sameBytes(readFilePayload(dest), bytes)); + // The listing must hand the name back in the same encoding it was given. + CHECK(listFolderFileNames(dir) == (std::vector{name})); + + fs::remove_all(utf8Path(dir), ec); +} + +static void testWriteFileExclusiveRefusesAnOccupiedPath() { + const std::string path = "pkg_io_excl.bin"; + const std::vector mine = patternBytes(12, 3); + CHECK(writeFileExclusive(path, PayloadBuffer(mine))); + CHECK(readAll(path) == mine); + // The create IS the check: a second call cannot replace the first file's bytes. + CHECK(!writeFileExclusive(path, PayloadBuffer(patternBytes(9, 8)))); + CHECK(readAll(path) == mine); + CHECK(!writeFileExclusive("pkg_io_excl_empty.bin", PayloadBuffer{})); + CHECK(!exists("pkg_io_excl_empty.bin")); + removeQuietly(path); } static void testListFolderFileNames() { const std::string dir = "pkg_io_listdir"; std::error_code ec; - fs::create_directory(dir, ec); + fs::create_directory(utf8Path(dir), ec); writeScratchFile(dir + "/b.bin", patternBytes(2, 1)); writeScratchFile(dir + "/a.bin", patternBytes(2, 2)); - fs::create_directory(dir + "/sub", ec); + fs::create_directory(utf8Path(dir + "/sub"), ec); writeScratchFile(dir + "/sub/c.bin", patternBytes(2, 3)); const std::vector names = listFolderFileNames(dir); CHECK(names == (std::vector{"a.bin", "b.bin"})); // sorted, bare, non-recursive CHECK(listFolderFileNames("pkg_io_no_such_dir").empty()); - fs::remove_all(dir, ec); + fs::remove_all(utf8Path(dir), ec); } int main() { testPayloadCounterTracksMovesNotCopies(); testStreamingRoundTripHoldsOnePayload(); + testEmptyPayloadIsRefusedAndPoisonsTheWriter(); testAbandonedWriteLeavesNoDestination(); testAbortPreservesPriorContents(); + testCommitReplacesAnExistingFile(); testOpenFailureIsInert(); testCommitRenameFailureSelfCleans(); testReaderEdges(); + testFileStatusSeparatesAbsentFromUnreadable(); + testNonAsciiPathsRoundTripAsUtf8(); + testWriteFileExclusiveRefusesAnOccupiedPath(); testListFolderFileNames(); if (g_fail == 0) std::printf("package_io: all tests passed\n"); diff --git a/tests/test_package_rollback.cpp b/tests/test_package_rollback.cpp index 6c28bfa..8ddeb32 100644 --- a/tests/test_package_rollback.cpp +++ b/tests/test_package_rollback.cpp @@ -1,11 +1,12 @@ // Standalone tests for shell/package/package_rollback — no REAPER, no framework. -// Pins the discriminator's mechanics: a file is recorded only when this journal's -// own write landed it, a pre-existing destination is refused untouched, and -// rollback deletes exactly the recorded set — a bystander file beside them stays, -// and a vanished file is tolerated rather than failed. +// Pins both halves of the deletion discriminator: the structural half (only an +// exclusively-created path is recorded, and the record is absolute so a CWD change +// cannot re-aim it) and the contract half (markIndexCommitted disarms rollback). #include "../src/shell/package/package_rollback.h" +#include "../src/shell/package/package_path.h" +#include #include #include #include @@ -19,6 +20,11 @@ static int g_fail = 0; #define CHECK(cond) do { if(!(cond)) { \ std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) +// The journal records absolute paths, so every expectation is built the same way. +static std::string scratch(const std::string& name) { + return (fs::current_path() / utf8Path(name)).u8string(); +} + static std::vector patternBytes(std::size_t n, std::uint8_t seed) { std::vector v(n); for (std::size_t i = 0; i < n; ++i) @@ -28,32 +34,55 @@ static std::vector patternBytes(std::size_t n, std::uint8_t seed) static void writeScratchFile(const std::string& path, const std::vector& bytes) { - std::ofstream f(path, std::ios::binary | std::ios::trunc); + std::ofstream f(utf8Path(path), std::ios::binary | std::ios::trunc); f.write(reinterpret_cast(bytes.data()), static_cast(bytes.size())); } static std::vector readAll(const std::string& path) { - std::ifstream f(path, std::ios::binary); + std::ifstream f(utf8Path(path), std::ios::binary); return std::vector(std::istreambuf_iterator(f), std::istreambuf_iterator()); } +static bool exists(const std::string& path) { return fs::exists(utf8Path(path)); } + +static void removeQuietly(const std::string& path) { + std::error_code ec; + fs::remove(utf8Path(path), ec); +} + static void testLandRecordsOnSuccessOnly() { LandedFileJournal journal; - const std::string path = "rb_land.bin"; + const std::string path = scratch("rb_land.bin"); const std::vector bytes = patternBytes(32, 1); CHECK(journal.writeLandedFile(path, PayloadBuffer(bytes))); CHECK(readAll(path) == bytes); - CHECK(journal.landedPaths() == (std::vector{path})); - CHECK(!fs::exists(path + ".rsbanktmp")); + CHECK(journal.landedPaths().size() == 1); + CHECK(!exists(path + ".rsbanktmp")); // the land is a direct exclusive create journal.rollback(); - CHECK(!fs::exists(path)); + CHECK(!exists(path)); // the recorded path denoted the file we asked for +} + +static void testRelativeInputIsRecordedAbsolute() { + // The hazard: a bare name recorded verbatim, then a CWD change, and rollback + // unlinks whatever now sits at that name in the new directory. + LandedFileJournal journal; + CHECK(journal.writeLandedFile("rb_relative.bin", PayloadBuffer(patternBytes(8, 4)))); + CHECK(journal.landedPaths().size() == 1); + const std::string recorded = journal.landedPaths().front(); + CHECK(utf8Path(recorded).is_absolute()); + std::error_code ec; + // Absolute AND still the same file — a spelling check alone would not prove that. + CHECK(fs::equivalent(utf8Path(recorded), utf8Path(scratch("rb_relative.bin")), ec)); + CHECK(!ec); + journal.rollback(); + CHECK(!exists(scratch("rb_relative.bin"))); } static void testExistingDestinationRefusedUntouched() { LandedFileJournal journal; - const std::string path = "rb_existing.bin"; + const std::string path = scratch("rb_existing.bin"); const std::vector original = patternBytes(16, 0x60); writeScratchFile(path, original); @@ -63,46 +92,49 @@ static void testExistingDestinationRefusedUntouched() { const RollbackResult result = journal.rollback(); CHECK(result.deletedCount == 0); - CHECK(fs::exists(path)); // rollback cannot touch a file it did not write - std::error_code ec; - fs::remove(path, ec); + CHECK(exists(path)); // rollback cannot touch a file it did not write + removeQuietly(path); } static void testEmptyPayloadRefused() { LandedFileJournal journal; - CHECK(!journal.writeLandedFile("rb_empty.bin", PayloadBuffer{})); - CHECK(!fs::exists("rb_empty.bin")); + const std::string path = scratch("rb_empty.bin"); + CHECK(!journal.writeLandedFile(path, PayloadBuffer{})); + CHECK(!exists(path)); CHECK(journal.empty()); } static void testRollbackDeletesExactlyTheRecordedSet() { LandedFileJournal journal; - CHECK(journal.writeLandedFile("rb_a.bin", PayloadBuffer(patternBytes(8, 1)))); - CHECK(journal.writeLandedFile("rb_c.bin", PayloadBuffer(patternBytes(8, 2)))); - writeScratchFile("rb_bystander.bin", patternBytes(8, 3)); // not journal-written + const std::string a = scratch("rb_a.bin"); + const std::string c = scratch("rb_c.bin"); + const std::string bystander = scratch("rb_bystander.bin"); + CHECK(journal.writeLandedFile(a, PayloadBuffer(patternBytes(8, 1)))); + CHECK(journal.writeLandedFile(c, PayloadBuffer(patternBytes(8, 2)))); + writeScratchFile(bystander, patternBytes(8, 3)); // not journal-written const RollbackResult result = journal.rollback(); CHECK(result.deletedCount == 2); CHECK(result.alreadyAbsentCount == 0); CHECK(result.failedCount == 0); - CHECK(!fs::exists("rb_a.bin")); - CHECK(!fs::exists("rb_c.bin")); - CHECK(fs::exists("rb_bystander.bin")); // exactly the given files, nothing else + CHECK(!result.refused); + CHECK(!exists(a)); + CHECK(!exists(c)); + CHECK(exists(bystander)); // exactly the given files, nothing else CHECK(journal.empty()); const RollbackResult second = journal.rollback(); // cleared: a no-op CHECK(second.deletedCount == 0); - CHECK(fs::exists("rb_bystander.bin")); - std::error_code ec; - fs::remove("rb_bystander.bin", ec); + CHECK(exists(bystander)); + removeQuietly(bystander); } static void testVanishedFileIsToleratedNotFailed() { LandedFileJournal journal; - CHECK(journal.writeLandedFile("rb_gone.bin", PayloadBuffer(patternBytes(8, 1)))); - std::error_code ec; - fs::remove("rb_gone.bin", ec); // vanished between land and rollback - CHECK(!ec); + const std::string path = scratch("rb_gone.bin"); + CHECK(journal.writeLandedFile(path, PayloadBuffer(patternBytes(8, 1)))); + removeQuietly(path); // vanished between land and rollback + CHECK(!exists(path)); const RollbackResult result = journal.rollback(); CHECK(result.deletedCount == 0); @@ -110,12 +142,39 @@ static void testVanishedFileIsToleratedNotFailed() { CHECK(result.failedCount == 0); } +static void testIndexCommitDisarmsRollback() { + // Once the index references these files the carve-out no longer covers them, so a + // late failure in the verb must not be able to delete indexed bytes. + LandedFileJournal journal; + const std::string path = scratch("rb_committed.bin"); + const std::vector bytes = patternBytes(8, 1); + CHECK(journal.writeLandedFile(path, PayloadBuffer(bytes))); + + journal.markIndexCommitted(); + CHECK(journal.indexCommitted()); + + const RollbackResult result = journal.rollback(); + CHECK(result.refused); + CHECK(result.deletedCount == 0); + CHECK(readAll(path) == bytes); // untouched + CHECK(!journal.empty()); // the record survives the refusal + + // Landing more files after the commit would produce unrollbackable state. + const std::string late = scratch("rb_late.bin"); + CHECK(!journal.writeLandedFile(late, PayloadBuffer(patternBytes(8, 2)))); + CHECK(!exists(late)); + + removeQuietly(path); +} + int main() { testLandRecordsOnSuccessOnly(); + testRelativeInputIsRecordedAbsolute(); testExistingDestinationRefusedUntouched(); testEmptyPayloadRefused(); testRollbackDeletesExactlyTheRecordedSet(); testVanishedFileIsToleratedNotFailed(); + testIndexCommitDisarmsRollback(); if (g_fail == 0) std::printf("package_rollback: all tests passed\n"); else std::printf("package_rollback: %d CHECK(s) FAILED\n", g_fail); From 655159ceac471e112bd2d329fb9e4dfebe2e3d61 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:44:29 -0400 Subject: [PATCH 3/4] Close package fs review findings: readRange bounds, picker ext, non-ASCII tests Cap readRange's allocation and reject size_t overflow instead of truncating; re-append .rsbank when the export picker omits it; add cafe coverage for writeFileExclusive and writeLandedFile; loop write() on EINTR. --- src/shell/package/CLAUDE.md | 14 +++++++++++--- src/shell/package/package_io.cpp | 20 ++++++++++++++++---- src/shell/package/package_io.h | 4 +++- src/shell/package/package_path.h | 15 +++++++++++---- src/shell/package/package_pickers.cpp | 20 +++++++++++++++++++- src/shell/package/package_pickers.h | 5 ++++- src/shell/package/package_rollback.cpp | 2 +- src/shell/package/package_rollback.h | 8 +++++++- tests/test_package_io.cpp | 11 +++++++++++ tests/test_package_rollback.cpp | 15 ++++++++++++++- 10 files changed, 97 insertions(+), 17 deletions(-) diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index 0caf183..7835e65 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -53,8 +53,12 @@ belong to the verbs. "Did this call create it" is structural: only exclusively-created paths are recorded, resolved absolute at record time so a later CWD change cannot re-aim the delete. "Did anything ever reference it" is a **contract the import verb must - honour**: it MUST call `markIndexCommitted()` at the moment it commits the index, - after which `rollback()` refuses and `writeLandedFile` refuses. + honour**: it MUST call `markIndexCommitted()` only AFTER the index write has + returned success — calling it before, then having that write fail, strands the + landed files with no index entry and a journal that now refuses to roll them + back — after which `rollback()` refuses and `writeLandedFile` refuses. (Destroying + an armed journal without calling either does NOT roll it back — see + `LandedFileJournal`'s own doc comment.) - **Both pickers ride `GetUserFileName`** — mode 1 for import, mode 0 for export. There is no platform split and no fallback: `main.cpp` defines `REAPERAPI_IMPLEMENT` without `REAPERAPI_MINIMAL` and aborts the extension load if any single name fails @@ -80,7 +84,11 @@ belong to the verbs. pair format but are `[verify — DAW]` on all three platforms — neither picker is exercised outside a live REAPER session. `GetUserFileName` also takes no owner window, so dialog parenting is REAPER's to do; the superseded Win32 path passed - `GetMainHwnd()` explicitly. + `GetMainHwnd()` explicitly. Also `[verify — DAW]`: whether mode 0's picker appends + an extension from `extension_list` when the user omits one — `pickPackageSavePath` + re-appends `.rsbank` itself so the returned path is correct regardless of how that + lands (the superseded Win32 path had `ofn.lpstrDefExt` for this; `GetUserFileName` + has no equivalent parameter). - `pickPackageSavePath`'s `suggestedPath` doubles as the dialog's starting directory when it is a full path. The verbs should seed it from the project directory — passing a bare name leaves the dialog on REAPER's process working directory, which diff --git a/src/shell/package/package_io.cpp b/src/shell/package/package_io.cpp index 901dc3f..26cd7e6 100644 --- a/src/shell/package/package_io.cpp +++ b/src/shell/package/package_io.cpp @@ -1,11 +1,13 @@ // package_io.cpp — see package_io.h for the seam's contract. Non-throwing at the -// boundary: every filesystem call uses the error_code form, so no filesystem_error -// crosses into a REAPER action body. +// boundary about filesystem_error: every filesystem call uses the error_code form. +// Allocation can still throw bad_alloc — readRange's sanity ceiling exists to keep +// that surface small, not to remove it. #include "shell/package/package_io.h" #include #include +#include #include #include "shell/package/package_path.h" @@ -16,6 +18,7 @@ #include #include #else +#include #include #include #endif @@ -147,7 +150,15 @@ PackageFileReader::PackageFileReader(const std::string& srcAbsPath) { PayloadBuffer PackageFileReader::readRange(std::uint64_t offset, std::uint64_t length) { // Overflow-safe range check: length is capped by the real file size before any // allocation happens, so a hostile offset/length pair cannot demand the moon. - if (!ok_ || length == 0 || length > size_ || offset > size_ - length) { + // Also rejected here rather than truncated: a length that would not fit in + // size_t (possible on a 32-bit build, where streamsize below stays 64-bit and + // so would read past a truncated allocation) and a length past the sanity + // ceiling, which exists so a merely large-but-real file size can't still hand + // std::vector a multi-gigabyte demand. + constexpr std::uint64_t kMaxReadRangeBytes = std::uint64_t{4} << 30; // 4 GiB + if (!ok_ || length == 0 || length > size_ || offset > size_ - length || + length > kMaxReadRangeBytes || + length > static_cast(std::numeric_limits::max())) { return PayloadBuffer{}; } in_.clear(); // a prior failed read must not poison this one @@ -208,6 +219,7 @@ bool writeFileExclusive(const std::string& absPath, const PayloadBuffer& payload static_cast(chunk)); #else const ssize_t n = ::write(fd, payload.data() + written, chunk); + if (n < 0 && errno == EINTR) continue; // a signal on the UI thread isn't a failure #endif if (n <= 0) { ok = false; @@ -241,7 +253,7 @@ std::vector listFolderFileNames(const std::string& dirAbsPath) { const auto& entry = *it; std::error_code reg_ec; if (!entry.is_regular_file(reg_ec)) continue; // skip subdirs / specials - names.push_back(entry.path().filename().u8string()); // never .string(): ANSI + names.push_back(pathToUtf8(entry.path().filename())); // never .string(): ANSI } std::sort(names.begin(), names.end()); return names; diff --git a/src/shell/package/package_io.h b/src/shell/package/package_io.h index 332a5f1..a33f66e 100644 --- a/src/shell/package/package_io.h +++ b/src/shell/package/package_io.h @@ -89,7 +89,9 @@ public: bool ok() const { return ok_; } std::uint64_t fileSize() const { return size_; } // Bytes [offset, offset+length). Range-checked against the real file size, so a - // hostile layout can never demand an allocation past the file's end. + // hostile layout can never demand an allocation past the file's end, and capped + // against a 4 GiB sanity ceiling so a merely large-but-real file can't still + // force a multi-gigabyte allocation out of one call. PayloadBuffer readRange(std::uint64_t offset, std::uint64_t length); private: diff --git a/src/shell/package/package_path.h b/src/shell/package/package_path.h index 9982496..e70db66 100644 --- a/src/shell/package/package_path.h +++ b/src/shell/package/package_path.h @@ -1,9 +1,9 @@ -// shell/package/package_path — the ONE narrow-string -> fs::path conversion for this -// seam. std::filesystem decodes a narrow path through the RUNTIME ANSI code page on -// Windows (measured: GetACP() == 1252 here), never UTF-8, so a bare +// shell/package/package_path — the ONE narrow-string <-> fs::path conversion pair for +// this seam. std::filesystem decodes a narrow path through the RUNTIME ANSI code page +// on Windows (measured: GetACP() == 1252 here), never UTF-8, so a bare // fs::path(std::string) turns every non-ASCII path this repo's UTF-8 convention // produces into mojibake. u8path is the C++17 spelling; it is deprecated in C++20, so -// a standard bump replaces the body here rather than at every call site. +// a standard bump replaces both bodies here rather than at every call site. #pragma once @@ -16,4 +16,11 @@ inline std::filesystem::path utf8Path(const std::string& utf8) { return std::filesystem::u8path(utf8); } +// u8string() returns std::u8string in C++20 — this is the one place that narrows it +// back to std::string, so a standard bump only widens this one body. +inline std::string pathToUtf8(const std::filesystem::path& path) { + const auto u8 = path.u8string(); + return std::string(u8.begin(), u8.end()); +} + } // namespace reasampler diff --git a/src/shell/package/package_pickers.cpp b/src/shell/package/package_pickers.cpp index 75af2a0..16395a6 100644 --- a/src/shell/package/package_pickers.cpp +++ b/src/shell/package/package_pickers.cpp @@ -5,6 +5,9 @@ #include "shell/package/package_pickers.h" +#include +#include + #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_GetUserFileName #include "reaper_plugin_functions.h" @@ -30,6 +33,14 @@ bool runPicker(int mode, const char* caption, const char* initial, return !outAbsPath.empty(); } +bool hasCaseInsensitiveSuffix(const std::string& path, const std::string& suffix) { + if (path.size() < suffix.size()) return false; + return std::equal(suffix.rbegin(), suffix.rend(), path.rbegin(), + [](unsigned char a, unsigned char b) { + return std::tolower(a) == std::tolower(b); + }); +} + } // namespace bool pickPackageForImport(std::string& outAbsPath) { @@ -39,7 +50,14 @@ bool pickPackageForImport(std::string& outAbsPath) { bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath) { // [verify — DAW] GetUserFileName takes no owner window, so the dialog's parenting // is REAPER's to do; the previous Win32 path passed GetMainHwnd() explicitly. - return runPicker(0, "Export bank package", suggestedPath.c_str(), outAbsPath); + if (!runPicker(0, "Export bank package", suggestedPath.c_str(), outAbsPath)) { + return false; + } + // GetUserFileName has no lpstrDefExt equivalent (the old Win32 picker's + // ofn.lpstrDefExt = L"rsbank"); whether mode 0 appends one itself from + // kExtList is [verify — DAW], so append it ourselves whenever it's missing. + if (!hasCaseInsensitiveSuffix(outAbsPath, ".rsbank")) outAbsPath += ".rsbank"; + return true; } } // namespace reasampler diff --git a/src/shell/package/package_pickers.h b/src/shell/package/package_pickers.h index 2cd8cf7..9f60512 100644 --- a/src/shell/package/package_pickers.h +++ b/src/shell/package/package_pickers.h @@ -1,7 +1,10 @@ // shell/package/package_pickers — the two package file pickers, both on REAPER's own // GetUserFileName (mode 1 = existing file, mode 0 = new file). No native/platform // split: the SDK's save mode is not optional on any build that can load this -// extension. Paths in and out are UTF-8, per the REAPER API contract. +// extension. Paths in and out are UTF-8, matching this tree's established practice +// for narrow strings crossing the REAPER API (see instrument_drop_win.cpp's +// path.u8string() to TrackFX_SetPreset, or prune_fs.cpp's CP_UTF8 conversion) — the +// SDK header itself never says "UTF-8". #pragma once diff --git a/src/shell/package/package_rollback.cpp b/src/shell/package/package_rollback.cpp index 1219292..c3162d0 100644 --- a/src/shell/package/package_rollback.cpp +++ b/src/shell/package/package_rollback.cpp @@ -24,7 +24,7 @@ bool LandedFileJournal::writeLandedFile(const std::string& destPath, std::error_code ec; const fs::path resolved = fs::absolute(utf8Path(destPath), ec); if (ec) return false; - const std::string absPath = resolved.u8string(); + const std::string absPath = pathToUtf8(resolved); if (!writeFileExclusive(absPath, payload)) return false; paths_.push_back(absPath); diff --git a/src/shell/package/package_rollback.h b/src/shell/package/package_rollback.h index cfc663d..438f168 100644 --- a/src/shell/package/package_rollback.h +++ b/src/shell/package/package_rollback.h @@ -19,6 +19,10 @@ struct RollbackResult { bool refused = false; // markIndexCommitted() ran: nothing was deleted }; +// Destroying an armed (uncommitted, un-rolled-back) journal is NOT an implicit +// rollback — the caller must call rollback() itself on the failure path it wants +// to undo. That's the fail-safe direction: a journal dropped by an unrelated early +// return leaves the landed files in place rather than silently deleting them. class LandedFileJournal { public: // Lands one payload at destPath through the exclusive create (which refuses an @@ -33,7 +37,9 @@ public: // Disarms the journal: the index mutation these files back is committed, so they // are now referenced bytes and the carve-out no longer covers them. This is the // half of prune's discriminator the journal cannot make structural on its own — - // the import verb MUST call it at the moment the index is committed. + // the import verb MUST call this only AFTER the index write has returned success. + // Calling it before, then having that write fail, strands the landed files with + // no index entry and a journal that now refuses to roll them back. void markIndexCommitted() { indexCommitted_ = true; } bool indexCommitted() const { return indexCommitted_; } diff --git a/tests/test_package_io.cpp b/tests/test_package_io.cpp index a533c72..d6237f6 100644 --- a/tests/test_package_io.cpp +++ b/tests/test_package_io.cpp @@ -293,11 +293,22 @@ static void testWriteFileExclusiveRefusesAnOccupiedPath() { CHECK(writeFileExclusive(path, PayloadBuffer(mine))); CHECK(readAll(path) == mine); // The create IS the check: a second call cannot replace the first file's bytes. + // (this pins the refusal, not O_EXCL's atomicity — a genuine race isn't portable) CHECK(!writeFileExclusive(path, PayloadBuffer(patternBytes(9, 8)))); CHECK(readAll(path) == mine); CHECK(!writeFileExclusive("pkg_io_excl_empty.bin", PayloadBuffer{})); CHECK(!exists("pkg_io_excl_empty.bin")); removeQuietly(path); + + // writeFileExclusive is the one call site using the wstring()/c_str() Windows + // open form rather than the fstream(fs::path) overload every other test here + // exercises — the only conversion whose reversion this file would otherwise miss. + const std::string cafePath = "pkg_io_excl_caf\xC3\xA9.bin"; + const std::vector cafeBytes = patternBytes(6, 4); + CHECK(writeFileExclusive(cafePath, PayloadBuffer(cafeBytes))); + CHECK(readAll(cafePath) == cafeBytes); + CHECK(!writeFileExclusive(cafePath, PayloadBuffer(patternBytes(3, 9)))); + removeQuietly(cafePath); } static void testListFolderFileNames() { diff --git a/tests/test_package_rollback.cpp b/tests/test_package_rollback.cpp index 8ddeb32..6f68bfc 100644 --- a/tests/test_package_rollback.cpp +++ b/tests/test_package_rollback.cpp @@ -22,7 +22,7 @@ static int g_fail = 0; // The journal records absolute paths, so every expectation is built the same way. static std::string scratch(const std::string& name) { - return (fs::current_path() / utf8Path(name)).u8string(); + return pathToUtf8(fs::current_path() / utf8Path(name)); } static std::vector patternBytes(std::size_t n, std::uint8_t seed) { @@ -64,6 +64,18 @@ static void testLandRecordsOnSuccessOnly() { CHECK(!exists(path)); // the recorded path denoted the file we asked for } +static void testLandNonAsciiPathRoundTripsAsUtf8() { + // The fs::absolute -> u8string round trip at writeLandedFile is otherwise + // untested with a non-ASCII path. + LandedFileJournal journal; + const std::string path = scratch("rb_caf\xC3\xA9.bin"); + const std::vector bytes = patternBytes(16, 2); + CHECK(journal.writeLandedFile(path, PayloadBuffer(bytes))); + CHECK(readAll(path) == bytes); + journal.rollback(); + CHECK(!exists(path)); +} + static void testRelativeInputIsRecordedAbsolute() { // The hazard: a bare name recorded verbatim, then a CWD change, and rollback // unlinks whatever now sits at that name in the new directory. @@ -169,6 +181,7 @@ static void testIndexCommitDisarmsRollback() { int main() { testLandRecordsOnSuccessOnly(); + testLandNonAsciiPathRoundTripsAsUtf8(); testRelativeInputIsRecordedAbsolute(); testExistingDestinationRefusedUntouched(); testEmptyPayloadRefused(); From f18884637017245d5b486508369d5892fd306e4a Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:52:25 -0400 Subject: [PATCH 4/4] docs(package): record known gaps and correct stale claims Notes the export verb's overwrite-consent obligation post-append, the append's extension-divergence behavior, and readFilePayload's 4GiB blind spot; fixes a stale u8string() reference and marks the 4GiB guard as accepted-unexercised. --- src/shell/package/CLAUDE.md | 19 ++++++++++++++++--- src/shell/package/package_io.cpp | 6 ++++++ src/shell/package/package_io.h | 4 +++- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index 7835e65..b4db0c4 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -19,7 +19,7 @@ belong to the verbs. `std::filesystem` decodes a narrow path through the runtime ANSI code page (measured `GetACP() == 1252`), so a bare `fs::path(std::string)` turns `café.rsbank` into `café.rsbank` or fails to open it. Every path a verb hands in or gets back — - including `listFolderFileNames`' results, which use `u8string()` and never + including `listFolderFileNames`' results, which go through `pathToUtf8()` and never `string()` — is UTF-8. `core/util/file_bytes` has the un-converted shape, which is why `readFilePayload` reads through this module's own `PackageFileReader` instead. - **Atomic package write, to the limit of a rename.** A package accumulates in a @@ -47,7 +47,14 @@ belong to the verbs. writer could win the race against. Collision handling (auto-rename) remains the import plan's job upstream. The package writer itself DOES replace an existing destination — the export save dialog's own overwrite confirm is the consent — and - that asymmetry is deliberate. + that asymmetry is deliberate. **Known gap, obligation on the export verb:** + `pickPackageSavePath`'s own `.rsbank` re-append (see its Gotcha below) can turn a + confirmed path `X` into a write target `X.rsbank` that the dialog never asked about. + The export verb MUST re-check `fileStatus()` on the path actually handed to + `PackageFileWriter` — after any extension append — and get its own consent if that + re-checked path is `Present`; the dialog's confirm only ever covered the pre-append + path. Not fixed at this seam: prompting is verb-level UX, and `pickPackageSavePath` + has no caller yet, so the gap is latent, not live. - **The rollback delete is prune's ONE carve-out, and only HALF of it is structural.** The citation and the full discriminator live at `package_rollback.cpp`'s header. "Did this call create it" is structural: only exclusively-created paths are @@ -88,7 +95,13 @@ belong to the verbs. an extension from `extension_list` when the user omits one — `pickPackageSavePath` re-appends `.rsbank` itself so the returned path is correct regardless of how that lands (the superseded Win32 path had `ofn.lpstrDefExt` for this; `GetUserFileName` - has no equivalent parameter). + has no equivalent parameter). The re-append is suffix-blind: it only skips when the + path already ends in `.rsbank`, so a path carrying a DIFFERENT extension gets + `.rsbank` appended after it (`mybank.bak` → `mybank.bak.rsbank`), unlike the + superseded `ofn.lpstrDefExt`, which appended only when the path had no extension at + all. Defensible for a format-locked export, but a real divergence from the old + picker's behavior — whoever tests the picker under `[verify — DAW]` should expect + the double-extension result on a path that already has one. - `pickPackageSavePath`'s `suggestedPath` doubles as the dialog's starting directory when it is a full path. The verbs should seed it from the project directory — passing a bare name leaves the dialog on REAPER's process working directory, which diff --git a/src/shell/package/package_io.cpp b/src/shell/package/package_io.cpp index 26cd7e6..c268742 100644 --- a/src/shell/package/package_io.cpp +++ b/src/shell/package/package_io.cpp @@ -155,6 +155,12 @@ PayloadBuffer PackageFileReader::readRange(std::uint64_t offset, std::uint64_t l // so would read past a truncated allocation) and a length past the sanity // ceiling, which exists so a merely large-but-real file size can't still hand // std::vector a multi-gigabyte demand. + // + // `length > size_` short-circuits before the two branches below ever see a real + // file, so neither is reachable without a genuine >4 GiB fixture — this guard + // ships unexercised by test_package_io.cpp, which covers past-the-end, + // starts-at-the-end, and zero-length only. The ordering (cheap size check first) + // is deliberate and correct; it is not reordered to make the branch testable. constexpr std::uint64_t kMaxReadRangeBytes = std::uint64_t{4} << 30; // 4 GiB if (!ok_ || length == 0 || length > size_ || offset > size_ - length || length > kMaxReadRangeBytes || diff --git a/src/shell/package/package_io.h b/src/shell/package/package_io.h index a33f66e..adf60ec 100644 --- a/src/shell/package/package_io.h +++ b/src/shell/package/package_io.h @@ -101,7 +101,9 @@ private: }; // One source file read whole as one entry's payload — a bank file IS the streaming -// unit. Empty on any failure, per PackageFileReader. +// unit. Empty on any failure, per PackageFileReader — including a source file over +// readRange's 4 GiB ceiling, which reads as empty exactly like an unreadable file; +// fileStatus() cannot tell the two apart either, since it only checks openability. PayloadBuffer readFilePayload(const std::string& absPath); // Export must tell a missing indexed file from an unreadable one in its refusal