From 7687ea76fcf033e4c70353f4010bd201916d23b2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 21:26:55 -0400 Subject: [PATCH] Persist the reclaimer's FX release so a reload cannot re-drive a spent record, and report the FX-restore drops it was swallowing --- src/app/main.cpp | 2 +- src/shell/view/fx_reclaim.cpp | 47 +++++++++-- src/shell/view/fx_reclaim.h | 73 ++++++++++++++--- tests/test_fx_reclaim.cpp | 144 +++++++++++++++++++++++++++++++++- 4 files changed, 247 insertions(+), 19 deletions(-) diff --git a/src/app/main.cpp b/src/app/main.cpp index 4817593..f7cb606 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -227,7 +227,7 @@ static void OnTimer() // the view model wholesale, and the applyMode above is what re-derives the queue // from it, so draining first would spend a tick against the outgoing model. if (reasampler::g_fxReclaimPending) - reasampler::fxReclaimTick(g_session.view(), nullptr); + reasampler::fxReclaimTick(g_session, nullptr); reasampler::bankPanelRefresh(); // cheap fingerprint compare; no-op when unchanged/closed } diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 27c1b55..b575c11 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -10,12 +10,14 @@ #include #include +#include "shell/persist/session.h" // saveViewStateOnly — the release's durable half #include "shell/view/view.h" // modeApplyInProgress #include "shell/view/view_park.h" // fxGuidString — the one FX-identity read #include "shell/view/view_tracks.h" #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_EnumProjects +#define REAPERAPI_WANT_ShowConsoleMsg #define REAPERAPI_WANT_TrackFX_GetCount #define REAPERAPI_WANT_TrackFX_GetOffline #define REAPERAPI_WANT_TrackFX_SetOffline @@ -67,8 +69,8 @@ std::set liveTrackGuids(ReaProject* proj) { // The live half of drainTick's Host contract (see fx_reclaim.h). struct ReclaimHost { - ReaProject* project = nullptr; - ViewModeModel* model = nullptr; + ReaProject* project = nullptr; + ReaSamplerSession* session = nullptr; // Monotonic and sub-millisecond: the budget below discriminates at 30ms, which // GetTickCount's ~16ms granularity cannot do. No wraparound to guard against, so @@ -111,10 +113,41 @@ struct ReclaimHost { } // Reached only after the drain has unwound and the queue has proved its generation - // did not move, so `model` is still the object the queue was derived against. - void releaseFx(const std::string& guid) { model->releaseSnapshotFx(guid); } + // did not move, so the session still holds the model the queue was derived against. + void releaseFx(const std::string& guid) { session->view().releaseSnapshotFx(guid); } + + // saveViewStateOnly writes the ACTIVE project and takes none, so re-read that rather + // than trust the project this tick pinned: a write pumps, and a tab switch inside one + // would otherwise land this model in the wrong project's ext state. Skipping is + // self-healing — coming back to the tab reloads the model from the record still on + // disk, and the re-derived online writes nothing and releases again. Promptless, + // unlike design_view_actions' persistViewState, which can open a modal Save-As. + void persistModel() { + if (EnumProjects(-1, nullptr, 0) != project) return; + session->saveViewStateOnly(); + } }; +// The drop set the last console report named, and the project it named it for. Same +// one-per-changed-set memo as view_park's reportExposedTracks, which owns the reasoning; +// here the repeat it swallows is an aborted tick's re-derivation resolving identically. +// The pointer is COMPARE-ONLY and never dereferenced. +ReaProject* g_reportedDropOwner = nullptr; +std::vector g_reportedDrops; + +void reportDrops(ReaProject* proj, const std::vector& drops) { + if (!shouldReportDrops(proj == g_reportedDropOwner, g_reportedDrops, drops)) return; + g_reportedDropOwner = proj; + g_reportedDrops = drops; + + FxRestoreDrops total; + for (const TrackFxDrops& d : drops) total.add(d.drops); + const std::string msg = describeFxRestoreDrops(total, static_cast(drops.size())); + // "!SHOW:" so a background tick cannot force the console open + // (reaper_plugin_functions.h:6536). + if (!msg.empty()) ShowConsoleMsg(("!SHOW:" + msg).c_str()); +} + // Every path that stops scheduling ticks leaves the same state behind: no queue, no pin, // nothing pending. The debounce clock is NOT part of it — see fx_reclaim.h. void goIdle() { @@ -137,7 +170,7 @@ void invalidateFxReclaim() { bool fxReclaimDrainInProgress() { return g_draining; } -void fxReclaimTick(ViewModeModel& model, ReaProject* proj) { +void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { if (g_draining) return; // The model is mid-rewrite and its records are being consumed out from under any // plan derived from it. The apply invalidates on its way out, so nothing is lost. @@ -161,12 +194,14 @@ void fxReclaimTick(ViewModeModel& model, ReaProject* proj) { // invalidated, so a REAPER quit mid-drain simply re-derives and finishes on reopen. if (g_queue.stale()) { g_project = project; + const ViewModeModel& model = session.view(); g_queue.adopt(makeQueue( view::desiredFxState(model, model.activeModeId(), liveTrackGuids(project)))); } - ReclaimHost host{project, &model}; + ReclaimHost host{project, &session}; const ReclaimRunResult run = g_queue.run(host); + reportDrops(project, run.drops); if (!run.aborted && run.settled) goIdle(); } diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index a1db774..3e571f9 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -16,6 +16,8 @@ class ReaProject; namespace reasampler { +class ReaSamplerSession; + // --- tuning ------------------------------------------------------------------ // A track's OFFLINE work fires only after it has been continuously parked this long. @@ -116,6 +118,25 @@ inline int nextEligible(const std::vector& items, const ParkClock& // --- the drain ---------------------------------------------------------------- +// One ONLINE item's captured state the chain had no home for. An OFFLINE item can +// never produce one: its targets are the live chain's own slots, so every op resolves. +struct TrackFxDrops { + std::string trackGuid; + FxRestoreDrops drops; + + bool operator==(const TrackFxDrops& o) const { + return trackGuid == o.trackGuid && drops == o.drops; + } +}; + +// True when a drop set is worth printing: any change of owner or of the set itself. +// The one-per-changed-set memo this gates is view_park's `shouldReport`/ +// `reportExposedTracks`, which states the reason; the shell holds the memo. +inline bool shouldReportDrops(bool sameOwnerAsLast, const std::vector& last, + const std::vector& now) { + return !(sameOwnerAsLast && last == now); +} + // What one tick did. `settled` means every item is complete and the caller may go idle // until the next invalidation. An item merely waiting out its debounce is NOT settled — // going idle there would strand its reclaim for the rest of the session. @@ -128,6 +149,12 @@ struct ReclaimTickResult { // what a re-entered load can replace mid-drain, so only the owner may commit it — // see FxReclaimQueue::run. std::vector released; + + // Per track that lost any, what the tick could not place. Carried out for the shell + // to print through core/view/fx_offline's describeFxRestoreDrops — a plugin deleted + // while its track was parked is a degrade the user has to repair by hand, and + // swallowing it leaves no trace at all. + std::vector drops; }; // One tick of the convergence loop. Templated on `Host` so the ordering, budget and @@ -144,6 +171,9 @@ struct ReclaimTickResult { // called by FxReclaimQueue::run // after the drain has unwound, // never from inside the drain +// void persistModel() make the releases just applied +// durable; run calls it ONCE per +// tick and only after a release // // `items` and `clock` must be the caller's OWN storage, not shared state a re-entered // call could reach: a write pumps the message loop. FxReclaimQueue::run is what makes @@ -181,9 +211,14 @@ ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, H const FxRestoreResolution resolved = view::resolveFxWork(item.work, liveFxGuids); for (const FxOfflineWrite& w : resolved.writes) if (host.writeOffline(item.work.trackGuid, w.fxIndex, w.offline)) ++result.writes; + if (resolved.drops.total() > 0) + result.drops.push_back(TrackFxDrops{item.work.trackGuid, resolved.drops}); - // The record is spent only once the chain has been driven to every state it - // captured — releasing any earlier destroys the only account of which plugins + // The record is spent once the chain has been driven to every captured state + // that is still PLACEABLE — which is why the release fires with drops + // outstanding: a dropped entry names an FX the chain no longer has, so + // holding the record for it would replan work that can never resolve. + // Releasing any EARLIER is what destroys the only account of which plugins // the USER had offline before the park, and the next online blanket-enables // them. Reported rather than applied; see ReclaimTickResult::released. if (item.work.kind == view::FxWorkKind::Online) @@ -208,6 +243,12 @@ struct ReclaimRunResult { // the releases are DISCARDED, `settled` is never true, and the caller must not go // idle — the next tick re-derives against whatever changed. bool aborted = false; + + // Carried out on the abort path TOO, unlike the releases: a drop is a fact about the + // chain the tick just wrote, not a claim about the model a re-entered load may have + // replaced. The re-derivation that follows an abort resolves the very same set, + // which is what shouldReportDrops is there to swallow. + std::vector drops; }; // Owns the derived queue and the debounce clock across a tick that can be re-entered: @@ -256,14 +297,26 @@ public: // back whether or not the drain's own work survives. clock_.swap(clock); - if (generation_ != entered) return ReclaimRunResult{tick.writes, false, true}; + if (generation_ != entered) + return ReclaimRunResult{tick.writes, false, true, tick.drops}; items_.swap(work); // A release dropped by an abort is re-owed, not lost: the next derivation plans // the same Online item, its writes all find the chain already there, and the // release fires then. - for (const std::string& guid : tick.released) host.releaseFx(guid); - return ReclaimRunResult{tick.writes, tick.settled, false}; + if (!tick.released.empty()) { + for (const std::string& guid : tick.released) host.releaseFx(guid); + // The spend has to be as durable as the record it spends. A reload — a + // reopen, or the undo/redo loadFromProject — rebuilds the model from the + // PERSISTED view state, and a record still held there re-derives this same + // Online work and re-drives the chain, silently reverting whatever per-FX + // offline the user set once the reclaim had finished. One persist per tick, + // after every release the tick earned, and still killable in between: die + // there and the record simply survives on disk, whereupon the re-derived + // item finds the chain already online, writes nothing, and releases again. + host.persistModel(); + } + return ReclaimRunResult{tick.writes, tick.settled, false, tick.drops}; } private: @@ -293,9 +346,11 @@ void invalidateFxReclaim(); bool fxReclaimDrainInProgress(); // Drains one tick against `proj` (nullptr = current project), re-deriving the queue from -// `model` first if it was invalidated. Returns without touching the project while an -// applyMode is on the stack, while already draining, or when the active project is not -// the one the queue was derived against. -void fxReclaimTick(ViewModeModel& model, ReaProject* proj); +// the session's view model first if it was invalidated. Returns without touching the +// project while an applyMode is on the stack, while already draining, or when the active +// project is not the one the queue was derived against. Takes the SESSION rather than the +// model alone because completing an online spends the park record's FX half, and that +// spend has to reach the persisted view state — see FxReclaimQueue::run. +void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj); } // namespace reasampler diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index 9e9d040..1dfa6cf 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -2,13 +2,15 @@ // loop is templated on its host precisely so the properties that make it safe to run in // the background are assertable here: the debounce, the per-tick budget's between-track // yield, the queue being a cache of intent that a model change discards, the user's own -// offline plugins surviving an online, and — via a host that invalidates from inside a -// write — that a re-entered invalidation reaches nothing the drain is using. +// offline plugins surviving an online, the release reaching the persisted model, and — +// via a host that invalidates from inside a write — that a re-entered invalidation +// reaches nothing the drain is using. #include "../src/shell/view/fx_reclaim.h" #include #include +#include #include #include #include @@ -75,7 +77,23 @@ struct FakeHost { return true; } - void releaseFx(const std::string& guid) { released.insert(guid); } + // `model` is the live host's session model and `persisted` stands in for view_state + // on disk — the blob a reopen or an undo/redo loadFromProject rebuilds from. Set them + // and the release actually lands, so a persist ordered BEFORE it would serialize a + // record that is still held and fail the reload assertions. + ViewModeModel* model = nullptr; + std::string persisted; + int persists = 0; + + void releaseFx(const std::string& guid) { + released.insert(guid); + if (model) model->releaseSnapshotFx(guid); + } + + void persistModel() { + ++persists; + if (model) persisted = model->serialize(); + } bool offlineAt(const std::string& guid, std::size_t slot) const { auto it = chains.find(guid); @@ -491,6 +509,7 @@ static void testAnInvalidationDuringAnOnlineDropsThatTicksRelease() { CHECK(r.aborted); CHECK(r.writes == 2); // the item still completed — a step is atomic CHECK(host.released.empty()); // but its record was NOT spent + CHECK(host.persists == 0); // and nothing durable claims it was // Re-owed, not lost: the re-derived item finds the chain already where the record // wants it, writes nothing more, and releases then. @@ -499,6 +518,119 @@ static void testAnInvalidationDuringAnOnlineDropsThatTicksRelease() { CHECK(q.run(host).settled); CHECK(host.writeLog.size() == 2); CHECK(host.released.count("{T}") == 1); + CHECK(host.persists == 1); +} + +// -- 6. the release has to outlive the session ------------------------------------ + +static void testACompletedOnlineIsPersistedSoAReloadPlansNoWork() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + + // The bug this closes, stated as the baseline: the model as it stands BEFORE the + // release re-derives the same Online work when reloaded, and re-driving it would + // revert whatever per-FX offline the user set after the reclaim had finished. + const std::optional stale = ViewModeModel::deserialize(vm.serialize()); + CHECK(stale.has_value()); + CHECK(derive(*stale, {"{T}"}).size() == 1); + + FakeHost host; + host.model = &vm; + host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + drainToSettled(q, host); + CHECK(host.released.count("{T}") == 1); + CHECK(host.persists == 1); // once per tick that released, not once per release + + const std::optional reloaded = ViewModeModel::deserialize(host.persisted); + CHECK(reloaded.has_value()); + CHECK(reloaded->snapshot("{T}") == nullptr); // both halves spent ⇒ record erased + CHECK(derive(*reloaded, {"{T}"}).empty()); +} + +static void testATickThatSpendsNoRecordPersistsNothing() { + // OFFLINE work spends no record, so there is nothing to make durable — and a + // background reclaim must not dirty the project for work it can simply redo. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.model = &vm; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + q.run(host); // observes the hold at t=0 + host.clock += kParkHoldMs; + drainToSettled(q, host); + + CHECK(host.writeLog.size() == 1); // it did move the plugin + CHECK(host.released.empty()); + CHECK(host.persists == 0); + CHECK(host.persisted.empty()); +} + +// -- 7. captured FX state the online direction cannot place ------------------------- + +static void testADropIsReportedOnceAcrossRepeatedRederivations() { + // {FX-B} was deleted while the track was parked, so its captured state has nowhere + // to land. Swallowing that leaves the user no trace of it at all. + ViewModeModel vm; + vm.membership().tag("{T}", kDesignModeId); + vm.setActiveMode(kDesignModeId); + vm.storeSnapshot("{T}", recordWithFx({{"{FX-A}", 0}, {"{FX-B}", 1}})); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.model = &vm; + host.chains["{T}"] = offlineChain({"{FX-A}"}); // {FX-B} gone from the chain + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + host.reentrant = &q; + host.invalidateAfterWrite = 1; // aborts, so the identical item re-derives next tick + + const ReclaimRunResult first = q.run(host); + CHECK(first.aborted); + CHECK(first.drops.size() == 1); + CHECK(first.drops[0].trackGuid == "{T}"); + CHECK(first.drops[0].drops.missingIdentity == 1); + CHECK(first.drops[0].drops.total() == 1); + CHECK(host.released.empty()); // the abort still refuses to spend the record + + host.reentrant = nullptr; + q.adopt(derive(vm, {"{T}"})); + const ReclaimRunResult second = q.run(host); + CHECK(second.settled); + CHECK(second.drops == first.drops); // the re-derivation resolves the very same set + + // The gate the shell's console report sits behind: first sighting prints, that + // identical re-derivation does not, a different set does. + const std::vector none; + CHECK(shouldReportDrops(/*sameOwnerAsLast=*/false, none, first.drops)); + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, first.drops, second.drops)); + const std::vector elsewhere{TrackFxDrops{"{U}", FxRestoreDrops{1, 0, 0}}}; + CHECK(shouldReportDrops(/*sameOwnerAsLast=*/true, first.drops, elsewhere)); + + // The record IS spent with a drop outstanding: holding it for an FX the chain no + // longer has would replan work that can never resolve. + CHECK(host.released.count("{T}") == 1); + CHECK(!host.offlineAt("{T}", 0)); // and the placeable half did land +} + +static void testAnOfflineTickReportsNoDrops() { + // Offline targets are the live chain's own slots, so nothing can fail to resolve. + // Keeps the drop assertions above honest: they must catch a real degrade, not an + // accumulator that fills on every tick. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + q.run(host); + host.clock += kParkHoldMs; + const ReclaimRunResult r = q.run(host); + CHECK(r.settled); + CHECK(r.writes == 2); + CHECK(r.drops.empty()); } int main() { @@ -519,6 +651,12 @@ int main() { testAnInvalidationDuringTheLastItemLeavesTheTickUnsettled(); testAnInvalidationDuringAnOnlineDropsThatTicksRelease(); + testACompletedOnlineIsPersistedSoAReloadPlansNoWork(); + testATickThatSpendsNoRecordPersistsNothing(); + + testADropIsReportedOnceAcrossRepeatedRederivations(); + testAnOfflineTickReportsNoDrops(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; }