diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index e2473d6..bfae2bd 100644 --- a/src/app/CMakeLists.txt +++ b/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. -reasampler_test(view_fx_park LINK fx_offline) +# TUs are compiled into this target directly. view_mode_model is linked for +# 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 diff --git a/src/shell/actions/design_view_actions.cpp b/src/shell/actions/design_view_actions.cpp index a6f45ca..6b5d914 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/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 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) { diff --git a/src/shell/capture/render_in_place.cpp b/src/shell/capture/render_in_place.cpp index cbe0da0..e0434aa 100644 --- a/src/shell/capture/render_in_place.cpp +++ b/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) { diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index bd80068..caa8743 100644 --- a/src/shell/view/CLAUDE.md +++ b/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 diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 2bc9466..e06cd3f 100644 --- a/src/shell/view/view.cpp +++ b/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 -// its mode's managed lane. +// Lane minting: item-side lane reads/writes assigning each item to its mode's 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 cancelled = deferFxPark(proj, guid); if (model.snapshot(guid) == nullptr) model.storeSnapshot(guid, snapshotTrack(tr, cancelled)); diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 22f99cf..467a80e 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/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 - // load and unload), so a re-entrant switch can enqueue while this runs. + // Detached before the first write: [verify — DAW] applying is ASSUMED to pump + // 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 draining = g_queue.take(); std::unordered_map 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; diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index ad054f3..aebbf6d 100644 --- a/src/shell/view/view_fx_park.h +++ b/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 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 - // plugins, which pumps the message loop, so a re-entrant switch can enqueue - // mid-apply: iterating the live queue would dangle on the push_back, and - // clearing it afterwards would discard whatever arrived during the apply. + // enqueued WHILE the caller applies what it took. [verify — DAW] applying + // loads/unloads plugins, which is ASSUMED to pump the message loop, so a + // re-entrant switch can enqueue mid-apply: iterating the live queue would + // 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 take() { std::vector 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& cancelled) { PreParkFx out; if (cancelled.empty()) return out; @@ -133,12 +151,29 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector #include #include @@ -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 - // a switch can re-enter and enqueue mid-drain. Those intents belong to the - // NEXT drain — the one in progress must neither see them nor discard them. + // [verify — DAW] applying an intent loads/unloads plugins, which is ASSUMED to + // pump the message loop, so a switch can re-enter and enqueue mid-drain. Those + // 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();