diff --git a/src/shell/persist/ext_state_io.cpp b/src/shell/persist/ext_state_io.cpp index 5852e11..37eb5c9 100644 --- a/src/shell/persist/ext_state_io.cpp +++ b/src/shell/persist/ext_state_io.cpp @@ -212,12 +212,12 @@ bool ReaSamplerSession::saveViewStateOnly() { } // Its twin bar the dirty mark — the one line that differs is the whole point of the -// split, so the two stay spelled out rather than folded behind a flag. -bool ReaSamplerSession::flushViewState() { - ReaProject* proj = activeSavedProject(); - if (!proj) return false; +// split, so the two stay spelled out rather than folded behind a flag. `project` is +// trusted as given rather than resolved via activeSavedProject() — see session.h. +bool ReaSamplerSession::flushViewState(ReaProject* project) { + if (!project) return false; - writeViewState(proj, view_); + writeViewState(project, view_); return true; } diff --git a/src/shell/persist/session.h b/src/shell/persist/session.h index a7f9522..f2a05f8 100644 --- a/src/shell/persist/session.h +++ b/src/shell/persist/session.h @@ -31,6 +31,9 @@ #include "core/version/app_version.h" #include "core/view/view_mode_model.h" +// Forward-declared to keep this header SDK-free; the .cpp includes the real SDK header. +class ReaProject; + namespace reasampler { // Owns the session's BankBook (pool + named banks) and drives persistence @@ -147,9 +150,12 @@ public: // The same write with the dirty mark OMITTED, for a caller already inside REAPER's // own project save: marking dirty there would leave the project modified the instant // it was saved, which is the whole reason that caller exists (fx_reclaim's - // ReleaseFlush). Not a general-purpose sibling — a mutation site that skips the dirty - // mark silently loses its write when the user closes without saving. - bool flushViewState(); + // ReleaseFlush). `project` is the SAVING project the caller resolved (e.g. via + // GetCurrentProjectInLoadSave()) — this call trusts it rather than re-deriving the + // active tab, which is exactly the mismatch fx_reclaim's caller exists to avoid. Not + // a general-purpose sibling — a mutation site that skips the dirty mark silently + // loses its write when the user closes without saving. + bool flushViewState(ReaProject* project); // Report-only prune dry-run: feeds the pure core with (present, referenced, // owned) — `present` from the folder enumeration, the other two from the diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 5cba6bb..2a63b56 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -17,6 +17,7 @@ #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_EnumProjects +#define REAPERAPI_WANT_GetCurrentProjectInLoadSave #define REAPERAPI_WANT_ShowConsoleMsg #define REAPERAPI_WANT_TrackFX_GetCount #define REAPERAPI_WANT_TrackFX_GetOffline @@ -50,13 +51,10 @@ struct DrainLatch { // Resolved fresh from the GUID rather than held across a write, for the same pump // premise: a plugin load can run arbitrary REAPER code, and a handle taken before it -// is not one this code may trust after it. The tick's own project pointer is -// validated once at entry, before any write has run; every call here happens AFTER -// at least one prior pumping write might have, so the project itself — not just the -// track — needs its own re-check, or CountTracks/GetTrack below run on a project that -// may have closed mid-drain. +// is not one this code may trust after it. The caller (ReclaimHost::resolveForStep) +// re-validates `proj` itself before calling this, on every arm, so this scan can +// assume a live project. MediaTrack* resolveFresh(ReaProject* proj, const std::string& guid) { - if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return nullptr; TrackHandles handles; readFolderEntries(proj, handles); for (const auto& kv : handles) @@ -86,9 +84,14 @@ struct ReclaimHost { std::string cachedGuid_; MediaTrack* cachedTrack_ = nullptr; - // A track is validated IN a project (reaper_plugin_functions.h:7898); the null form - // is defined only where the pointer is itself a project. + // Re-checks the project on EVERY call, both arms: the cached-track fast path used + // to hand `project` straight to ValidatePtr2 without re-validating it first, and a + // prior pumping write can have closed it since the tick's own entry check ran. + // (reaper_plugin_functions.h:7898 says `proj` is IGNORED when the pointer passed + // IS itself a project — not that passing one is only valid when it is; passing the + // project here is still right, just not for the reason once claimed.) MediaTrack* resolveForStep(const std::string& guid) { + if (!ValidatePtr2(nullptr, project, "ReaProject*")) return nullptr; if (guid == cachedGuid_ && cachedTrack_ && ValidatePtr2(project, cachedTrack_, "MediaTrack*")) return cachedTrack_; @@ -214,14 +217,19 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { } void fxReclaimFlushRelease(ReaSamplerSession& session) { - // The save hook is handed no project, so the active one is the only candidate; a - // mismatch means the release belongs to a tab this save is not writing, and skipping - // leaves the arm standing for that tab's own save. - if (!g_releaseFlush.pendingFor(EnumProjects(-1, nullptr, 0))) return; + // GetCurrentProjectInLoadSave() names the project THIS hook is being asked to save + // (reaper_plugin_functions.h:1582-1587) -- not merely the active tab, which a + // Save-All or a background-project save can diverge from. EnumProjects(-1, ...) used + // to stand in for it here and was wrong on both of those: it could skip the flush for + // the project actually being written, or match the arm against the active tab while a + // different project saved and write into that wrong project's ext state. + ReaProject* saving = GetCurrentProjectInLoadSave(); + if (!g_releaseFlush.pendingFor(saving)) return; // Promptless, unlike design_view_actions' persistViewState, which can open a modal - // Save-As. Disarm only on a write that was ISSUED — a never-saved project writes - // nothing, and dropping the arm there would strand the spend for the whole session. - if (session.flushViewState()) g_releaseFlush.clear(); + // Save-As. [verify — DAW] whether this write reaches the block of the + // file being saved right now, or lands one save late (see ReleaseFlush) — either + // way the arm is deliberately left standing rather than cleared on this one issue. + session.flushViewState(saving); } } // namespace reasampler diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 6a4e6d7..3050d32 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -398,9 +398,20 @@ private: // in there and the record survives on disk unspent, so the re-derived Online item drives // the chain to it again, reverting any per-FX offline set since the reclaim finished. // -// Keyed on the project because the save hook is handed none and the session's model is -// whichever project is active: firing for a different one would land this model in -// another project's ext state. A stale arm simply never fires — the same recovery leg. +// Keyed on the project so a release for one project's model can never land in a +// different project's ext state (checked against GetCurrentProjectInLoadSave() at the +// hook — see fxReclaimFlushRelease). A stale arm simply never fires — the same +// recovery leg. +// +// `clear()` is deliberately NOT called after a single issued write. [verify — DAW] +// whether a SetProjExtState made from inside SaveExtensionConfig reaches the +// block of the very file being written now, or lands one save later — +// project_config_extension_t (reaper_plugin.h:1196-1208) documents no ordering, and +// SetProjExtState (reaper_plugin_functions.h:6288) promises only "restored the next +// time this project is loaded". If the unfavourable answer holds, clearing here would +// strand the spend in memory for the rest of the session with nothing to retry it — +// so the arm stays armed and a further save simply re-issues the same idempotent +// write. class ReleaseFlush { public: void arm(ReaProject* project) { project_ = project; } @@ -438,9 +449,10 @@ bool fxReclaimDrainInProgress(); // park record's FX half — see FxReclaimQueue::run. void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj); -// Writes an in-memory release into the active project's view state. Called from the -// project-save hook and nowhere else: this is the ONE path that makes a spent record -// durable, and it deliberately does not mark the project dirty (see ReleaseFlush). +// Writes an in-memory release into the SAVING project's view state (identified via +// GetCurrentProjectInLoadSave(), not the active tab). Called from the project-save +// hook and nowhere else: this is the ONE path that makes a spent record durable, and +// it deliberately does not mark the project dirty (see ReleaseFlush). void fxReclaimFlushRelease(ReaSamplerSession& session); } // namespace reasampler diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index 9a0e2b3..e2fd6b1 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -110,11 +110,11 @@ struct FakeHost { }; // What the project-save hook does (fxReclaimFlushRelease): write the in-memory model into -// this project's view state, but only when it is the project holding the release. +// this project's view state, but only when it is the project holding the release. Does +// NOT clear the arm — see ReleaseFlush — so calling this twice re-issues the same write. void saveProject(FakeHost& host, ReleaseFlush& flush, ReaProject* active) { if (!flush.pendingFor(active)) return; host.persisted = host.model->serialize(); - flush.clear(); } // -- model helpers ------------------------------------------------------------- @@ -570,9 +570,10 @@ static void testACompletedOnlineReachesDiskOnTheSaveAndNotOnTheDrain() { CHECK(flush.pendingFor(kProjA)); // ...and the save knows it is owed // The user's next save is what spends it — and afterwards a reload plans no work, - // so nothing re-drives the chain. + // so nothing re-drives the chain. The arm itself survives the write (see + // testTheFlushArmSurvivesASaveAndCanBeReIssued) — this test is about the disk content. saveProject(host, flush, kProjA); - CHECK(!flush.pendingFor(kProjA)); + CHECK(flush.pendingFor(kProjA)); const std::optional afterSave = ViewModeModel::deserialize(host.persisted); CHECK(afterSave.has_value()); CHECK(afterSave->snapshot("{T}") == nullptr); // both halves spent ⇒ record erased @@ -640,6 +641,36 @@ static void testATickThatSpendsNoRecordOwesNoSaveAnything() { CHECK(host.persisted.empty()); } +static void testTheFlushArmSurvivesASaveAndCanBeReIssued() { + // Whether a write made from inside the save hook reaches the very file being written + // now, or lands one save late, is unsettled (ReleaseFlush's [verify — DAW]). The arm + // must not treat one issued write as proof of a durable spend: it stays armed, so an + // unfavourable answer still gets a retry on the project's NEXT save rather than + // stranding the record in memory for the rest of the session. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.model = &vm; + ReleaseFlush flush; + flush.arm(kProjA); + + saveProject(host, flush, kProjA); + CHECK(!host.persisted.empty()); + CHECK(flush.pendingFor(kProjA)); // survives the flush — not disarmed by one write + + // A second save re-issues the identical write rather than finding nothing to do. + host.persisted.clear(); + saveProject(host, flush, kProjA); + CHECK(!host.persisted.empty()); + CHECK(flush.pendingFor(kProjA)); + + // A different project's save must still leave this arm untouched. + saveProject(host, flush, kProjB); + CHECK(flush.pendingFor(kProjA)); +} + // -- 7. captured FX state the online direction cannot place ------------------------- static void testADropIsReportedOnceAcrossRepeatedRederivations() { @@ -930,6 +961,7 @@ int main() { testACompletedOnlineReachesDiskOnTheSaveAndNotOnTheDrain(); testAReleaseKilledBeforeTheFlushIsSimplyReEarned(); testATickThatSpendsNoRecordOwesNoSaveAnything(); + testTheFlushArmSurvivesASaveAndCanBeReIssued(); testADropIsReportedOnceAcrossRepeatedRederivations(); testAnOfflineTickReportsNoDrops();