Drain deferred FX parks before the view model is serialized; re-validate track handles per intent; pin the restore-plan round trip

src/app/CMakeLists.txt

@@ -63,8 +63,10 @@ target_include_directories(reaper_reasampler PRIVATE ${SDK_INC} ${WDL_INC})
 # The deferred FX-park queue's re-entrancy rule is pure (header-inline, no REAPER
 # types), so it is CTest-covered like a core/ module. Declared here rather than in a
 # src/shell/view/CMakeLists.txt because that directory deliberately has none — its
-# TUs are compiled into this target directly.
+# TUs are compiled into this target directly. view_mode_model is linked for
-reasampler_test(view_fx_park LINK fx_offline)
+# makeRestorePlan alone: the cancel path's round trip is a contract between the two,
+# and pinning it against hand-built ops would not catch a change to either half.
+reasampler_test(view_fx_park LINK fx_offline view_mode_model)
 
 # Bank-package import: the promptless verb plus its action skin. Kept as its own
 # appended block rather than merged into the lists above, so the two package

src/shell/actions/design_view_actions.cpp

@@ -26,6 +26,7 @@
 #include "shell/capture/track_guid.h"    // shared MediaTrack* -> canonical GUID key
 #include "shell/panel/panel_window.h"    // bankPanelInvalidate — footer toggle repaint
 #include "shell/view/view.h"             // applyMode + mintManagedLanes (D2 shell)
+#include "shell/view/view_fx_park.h"     // drainDeferredFxParks — see persistViewState
 
 #define REAPERAPI_MINIMAL
 #define REAPERAPI_WANT_CountSelectedTracks
@@ -138,6 +139,12 @@ std::vector<RetagItem> selectedRetagItems() {
 // the project is unsaved, prompts Save-As first (mirrors the flow capture uses) —
 // DAW-ONLY: Main_SaveProject(proj, true) blocks until the dialog is dismissed.
 void persistViewState() {
+    // BEFORE the model is serialized, and before the Save-As below can write a
+    // .rpp: a deferred FX restore leaves the chain offline while the model has
+    // already dropped the snapshot that would replan it. Why that combination is
+    // unrecoverable on reopen is at drainDeferredFxParks.
+    drainDeferredFxParks();
+
     if (!g_session->view().membership().empty()) {
         ReaProject* proj = EnumProjects(-1, nullptr, 0);
         if (proj) {

src/shell/capture/render_in_place.cpp

@@ -22,6 +22,7 @@
 #include "shell/panel/panel_input.h"             // bankPanelTailSetting
 #include "shell/persist/session.h"
 #include "shell/view/view.h"                     // applyMode / mintManagedLanes
+#include "shell/view/view_fx_park.h"             // drainDeferredFxParks
 
 #define REAPERAPI_MINIMAL
 #define REAPERAPI_WANT_CountTrackMediaItems
@@ -210,6 +211,7 @@ void RunRenderTrackInPlace(ReaSamplerSession& session) {
     // Persist outside the block. The offline render's own save gate already forced a
     // saved project, so the Save-As-guarded persist the Design View actions need
     // cannot have anything to prompt for here.
+    drainDeferredFxParks();  // the reapply above may have deferred a restore — see the contract there
     session.saveToActiveProject();
 
     if (!placed) {

src/shell/view/CLAUDE.md

@@ -54,7 +54,14 @@ decide membership or mode rules.
   lost. What it does change is the undo record: the offline writes land outside
   the switch's undo block, so the tool no longer re-drives them on an undo or a
   redo — what a Ctrl-Z then leaves the chain at is REAPER's own FX-state record,
-  `[verify — DAW]`.
+  `[verify — DAW]`. **The idle tick is not the only drain point.** Any path that
+  serializes the view model drains synchronously first (`persistViewState`,
+  `render_in_place`), because a save landing between a restore's synchronous flag
+  writes and its drain would record offline FX beside a model that no longer
+  carries the snapshot to replan them — unrecoverable on reopen. So an
+  action-driven switch does pay the FX hitch before it returns; the repaint and
+  the undo block have both closed by then, which is what the deferral was for.
+  Any new caller that reapplies a mode and then persists inherits this obligation.
 - **Stated DEVIATION — the undo mask does not keep FX out of a real switch.** The
   apply mask (`kApplyUndoMask`) drops `UNDO_STATE_FX` and ORs it back in when a
   driven flag in that domain moved; the only such flag is `I_FXEN`, which every
@@ -118,6 +125,12 @@ applies the resulting lane state to live tracks.
   under whatever mode id is active at that point, not the one the user undid back
   to. Pre-existing: `snapshots_` already carries this same model-vs-undo split;
   the solo cache inherits it rather than introducing it. Not fixed here.
+- An undo/redo also DISCARDS every pending FX intent (`discardDeferredFxParks`),
+  which is not the pure loss it reads as: the same tick reapplies the active mode
+  over the reloaded model, re-planning a park for every inactive leaf, so parked
+  FX converge on the following drain. The one case that does not self-heal is a
+  track whose reloaded model carries no snapshot — nothing plans its restore, so
+  FX left offline stay offline. Full contract at `discardDeferredFxParks`.
 - `fx_offline`'s identity keying (`TrackFX_GetFXGUID`) assumes the GUID stays
   attached to its plugin across a chain mutation while parked. That is
   `[verify — DAW]` (see `fxGuidString` in `view_fx_park.cpp`) and SWS issue #802 is a

src/shell/view/view.cpp

@@ -36,8 +36,7 @@
 #define REAPERAPI_WANT_TrackList_AdjustWindows
 #define REAPERAPI_WANT_UpdateArrange
 #define REAPERAPI_WANT_UpdateTimeline
-// Lane minting (D2 Wave 3): item-side lane reads/writes to assign each item to
+// Lane minting: item-side lane reads/writes assigning each item to its mode's lane.
-// its mode's managed lane.
 #define REAPERAPI_WANT_CountTrackMediaItems
 #define REAPERAPI_WANT_GetTrackMediaItem
 #define REAPERAPI_WANT_GetMediaItemInfo_Value
@@ -474,7 +473,8 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject
             // Enqueued FIRST: a park landing on this track's own pending restore
             // cancels it, and those ops are then the only surviving record of the
             // pre-park FX state. Snapshot ONCE — a snapshot already present means
-            // the track is still parked, so its live flags read as parked.
+            // the track is still parked, so recapturing would overwrite the true
+            // pre-park state with zeros and a later restore would hide it for good.
             const std::vector<FxOfflineOp> cancelled = deferFxPark(proj, guid);
             if (model.snapshot(guid) == nullptr)
                 model.storeSnapshot(guid, snapshotTrack(tr, cancelled));

src/shell/view/view_fx_park.cpp

@@ -20,6 +20,7 @@
 #define REAPERAPI_WANT_TrackFX_GetFXGUID
 #define REAPERAPI_WANT_TrackFX_GetOffline
 #define REAPERAPI_WANT_TrackFX_SetOffline
+#define REAPERAPI_WANT_ValidatePtr2
 #define REAPERAPI_WANT_guidToString
 #include "reaper_plugin_functions.h"
 
@@ -29,6 +30,17 @@ namespace {
 
 FxParkQueue g_queue;
 ReaProject* g_owner = nullptr;  // the project the pending intents were enqueued against
+bool        g_draining = false;
+
+// Makes "one drain at a time" explicit rather than implied by the call sites.
+// RAII because an apply can throw and a stuck flag would silence the queue for
+// the rest of the session.
+struct DrainScope {
+    DrainScope() { g_draining = true; }
+    ~DrainScope() { g_draining = false; }
+    DrainScope(const DrainScope&) = delete;
+    DrainScope& operator=(const DrainScope&) = delete;
+};
 
 ReaProject* currentProject() { return EnumProjects(-1, nullptr, 0); }
 
@@ -127,6 +139,7 @@ void discardDeferredFxParks() {
 
 void drainDeferredFxParks() {
     if (g_queue.empty()) return;
+    if (g_draining) return;  // re-entered mid-apply: those intents are the outer drain's next pass
 
     if (g_owner != currentProject()) {
         // The project the intents were planned against was closed or switched
@@ -139,9 +152,12 @@ void drainDeferredFxParks() {
     }
 
     ReaProject* const proj = g_owner;
+    const DrainScope scope;
 
-    // Detached before the first write: applying pumps the message loop (plugins
+    // Detached before the first write: [verify — DAW] applying is ASSUMED to pump
-    // load and unload), so a re-entrant switch can enqueue while this runs.
+    // the message loop (plugins load and unload), so a re-entrant switch can
+    // enqueue while this runs. Everything defensive below rests on that one
+    // assumption; each piece is correct regardless of whether it holds.
     const std::vector<FxParkIntent> draining = g_queue.take();
 
     std::unordered_map<std::string, MediaTrack*> byGuid;
@@ -159,6 +175,16 @@ void drainDeferredFxParks() {
     for (const FxParkIntent& intent : draining) {
         auto it = byGuid.find(intent.guid);
         if (it == byGuid.end()) continue;  // track deleted since the switch — prune
+
+        // Re-validated PER INTENT, not once above: under the same pumping
+        // assumption, a project closed or a track deleted between two applies
+        // leaves the handle resolved above dangling — a use-after-free, not a
+        // pruned intent. Same gate capture_realtime_shell's teardown uses; a null
+        // first argument validates the ReaProject* itself (SDK: proj is ignored
+        // when the pointer is a project).
+        if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return;
+        if (!ValidatePtr2(proj, it->second, "MediaTrack*")) continue;
+
         if (intent.park) {
             applyPark(it->second);
             continue;

src/shell/view/view_fx_park.h

@@ -41,6 +41,11 @@ public:
     // The caller needs them: that restore never ran, so the live chain still
     // reads the PARKED offline states and is no longer a source for a fresh
     // pre-park snapshot (see preParkFxFromCancelledRestore).
+    //
+    // The empty return is a LOAD-BEARING sentinel that deliberately conflates
+    // "nothing was cancelled" with "cancelled a restore carrying no ops": a
+    // zero-op restore held no FX state to hand back, so falling through to a
+    // live chain read is the same answer, not a worse one.
     std::vector<FxOfflineOp> park(const std::string& guid) {
         FxParkIntent* held = find(guid);
         if (!held) {
@@ -73,10 +78,12 @@ public:
     void clear() { pending_.clear(); }
 
     // Detaches everything pending, leaving the queue able to accept intents
-    // enqueued WHILE the caller applies what it took. Applying loads/unloads
+    // enqueued WHILE the caller applies what it took. [verify — DAW] applying
-    // plugins, which pumps the message loop, so a re-entrant switch can enqueue
+    // loads/unloads plugins, which is ASSUMED to pump the message loop, so a
-    // mid-apply: iterating the live queue would dangle on the push_back, and
+    // re-entrant switch can enqueue mid-apply: iterating the live queue would
-    // clearing it afterwards would discard whatever arrived during the apply.
+    // dangle on the push_back, and clearing it afterwards would discard
+    // whatever arrived during the apply. The detach is correct either way; only
+    // the need for it is unconfirmed.
     std::vector<FxParkIntent> take() {
         std::vector<FxParkIntent> taken;
         taken.swap(pending_);
@@ -109,6 +116,17 @@ struct PreParkFx {
 // cancelled) means the caller reads the live chain instead. The restore's own
 // keying travels with it so a slot-keyed snapshot lifted from a legacy
 // view_state does not silently become an identity-keyed one with no identities.
+//
+// FAITHFUL TO THE SNAPSHOT, NOT THE CHAIN. The ops describe the chain as it was
+// at the ORIGINAL park; applyPark enumerates it again at drain time. So an FX
+// added while the track was parked (a floating FX-chain window, ReaScript) is
+// absent from this reconstruction yet IS offlined by the cancelling park's
+// drain — and so never comes back online. Rare, and recoverable by hand in the
+// FX chain, but specific to the deferral.
+//
+// `offline` is already boolean by the time it arrives: makeRestorePlan narrowed
+// FxOfflineState's defensive int to FxOfflineOp's bool, so this widening back to
+// int restores the type, not lost information.
 inline PreParkFx preParkFxFromCancelledRestore(const std::vector<FxOfflineOp>& cancelled) {
     PreParkFx out;
     if (cancelled.empty()) return out;
@@ -133,12 +151,29 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector<FxOff
 // dropped where the restore is planned (applyMode). Discards the queue unapplied
 // if the project it was enqueued against is no longer current (close / switch);
 // an intent whose track is gone is pruned. Idle cost is one empty-queue test.
+// Re-entrant calls early-out: one drain at a time, the outer one owns the queue.
+//
+// Called on the idle tick AND synchronously before the view model is serialized
+// (persistViewState, render_in_place). The second call is not an optimization:
+// between a restore's synchronous flag writes and its drain the FX are still
+// offline while the model has already dropped the snapshot that would replan
+// them, so a save inside that window — deterministic under a custom action chain
+// like "toggle mode; save project" — records offline FX beside a snapshot-free
+// model, and nothing on reopen brings them back online. The cost is that an
+// action-driven switch pays the FX hitch before it returns; its repaint and undo
+// block have both closed by then, which is what the deferral was for.
 void drainDeferredFxParks();
 
 // Drops every pending intent without applying it. Called when the model the
 // intents were planned against has been replaced (project load/switch, undo/redo
 // state restore) — applying them then would write the pre-reload plan over the
 // project that replaced it.
+//
+// Not pure loss: the caller reapplies the active mode over the reloaded model
+// immediately after, which re-plans a park for every inactive leaf, so parked FX
+// converge on the following drain. The case that does NOT self-heal is a track
+// whose reloaded model carries no snapshot — nothing plans a restore for it, so
+// FX left offline stay offline.
 void discardDeferredFxParks();
 
 } // namespace reasampler

tests/test_view_fx_park.cpp

@@ -8,6 +8,8 @@
 
 #include "../src/shell/view/view_fx_park.h"
 
+#include "core/view/view_mode_model.h"  // makeRestorePlan — the ops' only producer
+
 #include <cstdio>
 #include <string>
 #include <vector>
@@ -189,6 +191,48 @@ static void testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities() {
     CHECK(fx.states.size() == 2 && fx.states[0].offline == 1 && fx.states[1].offline == 0);
 }
 
+// -- the makeRestorePlan <-> preParkFxFromCancelledRestore round trip ---------
+//
+// The cancel path's whole premise is that a planned restore's ops are a LOSSLESS
+// carrier of the snapshot's FX half. makeRestorePlan is their only producer, so
+// the real claim is that the pair composes to the identity on (fxOffline,
+// fxKeying). Asserting it against hand-built ops would let a change to
+// makeRestorePlan's field mapping or op ordering pass with every test green.
+
+static void testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim() {
+    TrackSnapshot snap;
+    snap.fxKeying  = FxKeying::Identity;
+    snap.fxOffline = {FxOfflineState{"{ONE}", 1}, FxOfflineState{"{TWO}", 0},
+                      FxOfflineState{"{THREE}", 1}};
+
+    const TrackPlan plan = makeRestorePlan("{TRACK}", snap);
+    const PreParkFx rebuilt = preParkFxFromCancelledRestore(plan.fxOffline);
+
+    CHECK(rebuilt.keying == FxKeying::Identity);
+    // Three DISTINCT entries, compared as a sequence: a dropped fxGuid, a flipped
+    // offline, or a reordering each fail here.
+    CHECK(rebuilt.states == snap.fxOffline);
+}
+
+static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() {
+    TrackSnapshot snap;
+    snap.fxKeying  = FxKeying::Slot;
+    snap.fxOffline = {FxOfflineState{"", 0}, FxOfflineState{"", 1}, FxOfflineState{"", 1}};
+
+    const TrackPlan plan = makeRestorePlan("{TRACK}", snap);
+
+    // Slot keying addresses by POSITION, so the identity holds only while the op
+    // at index i carries slot i.
+    CHECK(plan.fxOffline.size() == 3);
+    CHECK(plan.fxOffline.size() == 3 && plan.fxOffline[0].slot == 0 &&
+          plan.fxOffline[1].slot == 1 && plan.fxOffline[2].slot == 2);
+
+    const PreParkFx rebuilt = preParkFxFromCancelledRestore(plan.fxOffline);
+
+    CHECK(rebuilt.keying == FxKeying::Slot);
+    CHECK(rebuilt.states == snap.fxOffline);
+}
+
 // -- re-entrancy -------------------------------------------------------------
 
 static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() {
@@ -205,9 +249,10 @@ static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() {
 }
 
 static void testIntentsArrivingDuringADrainSurviveIt() {
-    // Applying an intent loads/unloads plugins, which pumps the message loop, so
+    // [verify — DAW] applying an intent loads/unloads plugins, which is ASSUMED to
-    // a switch can re-enter and enqueue mid-drain. Those intents belong to the
+    // pump the message loop, so a switch can re-enter and enqueue mid-drain. Those
-    // NEXT drain — the one in progress must neither see them nor discard them.
+    // intents belong to the NEXT drain — the one in progress must neither see them
+    // nor discard them.
     FxParkQueue q;
     q.park("{A}");
 
@@ -250,6 +295,8 @@ int main() {
     testCancelledRestoreOpsBecomeTheFreshSnapshotsFxHalf();
     testNothingCancelledLeavesTheFxHalfToTheCaller();
     testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities();
+    testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim();
+    testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim();
     testTakeDetachesEverythingAndLeavesTheQueueEmpty();
     testIntentsArrivingDuringADrainSurviveIt();
     testAReEntrantParkCancelsOnlyWhatIsStillPending();