From edfd7ead4d4c90cde982e5ff29a696ecf7ab69ed Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:15:12 -0400 Subject: [PATCH] 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);