fx_reclaim: use GetCurrentProjectInLoadSave for the save hook, stop disarming ReleaseFlush on one write, validate project before cached-track lookup

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.
+// split, so the two stay spelled out rather than folded behind a flag. `project` is
-bool ReaSamplerSession::flushViewState() {
+// trusted as given rather than resolved via activeSavedProject() — see session.h.
-    ReaProject* proj = activeSavedProject();
+bool ReaSamplerSession::flushViewState(ReaProject* project) {
-    if (!proj) return false;
+    if (!project) return false;
 
-    writeViewState(proj, view_);
+    writeViewState(project, view_);
     return true;
 }
 

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
+    // ReleaseFlush). `project` is the SAVING project the caller resolved (e.g. via
-    // mark silently loses its write when the user closes without saving.
+    // GetCurrentProjectInLoadSave()) — this call trusts it rather than re-deriving the
-    bool flushViewState();
+    // 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

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
+// is not one this code may trust after it. The caller (ReclaimHost::resolveForStep)
-// validated once at entry, before any write has run; every call here happens AFTER
+// re-validates `proj` itself before calling this, on every arm, so this scan can
-// at least one prior pumping write might have, so the project itself — not just the
+// assume a live project.
-// track — needs its own re-check, or CountTracks/GetTrack below run on a project that
-// may have closed mid-drain.
 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
+    // Re-checks the project on EVERY call, both arms: the cached-track fast path used
-    // is defined only where the pointer is itself a project.
+    // 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
+    // GetCurrentProjectInLoadSave() names the project THIS hook is being asked to save
-    // mismatch means the release belongs to a tab this save is not writing, and skipping
+    // (reaper_plugin_functions.h:1582-1587) -- not merely the active tab, which a
-    // leaves the arm standing for that tab's own save.
+    // Save-All or a background-project save can diverge from. EnumProjects(-1, ...) used
-    if (!g_releaseFlush.pendingFor(EnumProjects(-1, nullptr, 0))) return;
+    // 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
+    // Save-As. [verify — DAW] whether this write reaches the <EXTSTATE> block of the
-    // nothing, and dropping the arm there would strand the spend for the whole session.
+    // file being saved right now, or lands one save late (see ReleaseFlush) — either
-    if (session.flushViewState()) g_releaseFlush.clear();
+    // way the arm is deliberately left standing rather than cleared on this one issue.
+    session.flushViewState(saving);
 }
 
 }  // namespace reasampler

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
+// Keyed on the project so a release for one project's model can never land in a
-// whichever project is active: firing for a different one would land this model in
+// different project's ext state (checked against GetCurrentProjectInLoadSave() at the
-// another project's ext state. A stale arm simply never fires — the same recovery leg.
+// 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
+// <EXTSTATE> 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
+// Writes an in-memory release into the SAVING project's view state (identified via
-// project-save hook and nowhere else: this is the ONE path that makes a spent record
+// GetCurrentProjectInLoadSave(), not the active tab). Called from the project-save
-// durable, and it deliberately does not mark the project dirty (see ReleaseFlush).
+// 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

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<ViewModeModel> 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();