From 5a35b71dc0eba23e053e35212c3934a26d0c5753 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 19:07:17 -0400 Subject: [PATCH 1/9] Add the FX reclaimer: a debounced, time-boxed convergence loop that offlines parked chains and restores returning ones off the switch path --- src/app/CMakeLists.txt | 4 + src/app/main.cpp | 10 + src/shell/view/fx_reclaim.cpp | 163 ++++++++++++++ src/shell/view/fx_reclaim.h | 233 ++++++++++++++++++++ src/shell/view/view.cpp | 7 + src/shell/view/view_park.cpp | 41 ++-- src/shell/view/view_park.h | 6 + tests/test_fx_reclaim.cpp | 398 ++++++++++++++++++++++++++++++++++ 8 files changed, 840 insertions(+), 22 deletions(-) create mode 100644 src/shell/view/fx_reclaim.cpp create mode 100644 src/shell/view/fx_reclaim.h create mode 100644 tests/test_fx_reclaim.cpp diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index 4a8cb89..217f589 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -39,6 +39,7 @@ add_library(reaper_reasampler MODULE ${LICE_SRC} ${REASAMPLER_SRC_DIR}/shell/capture/insert.cpp ${REASAMPLER_SRC_DIR}/shell/view/view.cpp + ${REASAMPLER_SRC_DIR}/shell/view/fx_reclaim.cpp ${REASAMPLER_SRC_DIR}/shell/view/view_lanes.cpp ${REASAMPLER_SRC_DIR}/shell/view/view_park.cpp ${REASAMPLER_SRC_DIR}/shell/view/view_reconcile.cpp @@ -72,6 +73,9 @@ target_include_directories(reaper_reasampler PRIVATE ${SDK_INC} ${WDL_INC}) # has none — its TUs are compiled into this target directly. reasampler_test(view_park LINK view_mode_model) reasampler_test(view_reconcile LINK applied_state view_mode_model) +# The reclaimer's drain loop is templated on its host for exactly this reason: the +# ordering, budget, debounce and resume bookkeeping run against a fake chain. +reasampler_test(fx_reclaim LINK fx_state) # 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/app/main.cpp b/src/app/main.cpp index a2755f9..e70606f 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -38,6 +38,7 @@ #include "shell/panel/panel_input.h" // bankPanelRefresh / bankPanelNotifyProjectLoaded #include "shell/panel/panel_window.h" // panel lifecycle (init/toggle/open-query/shutdown) #include "shell/persist/session.h" // ReaSamplerSession +#include "shell/view/fx_reclaim.h" // the FX reclaimer's tick #include "shell/view/view.h" // applyMode #include "shell/view/view_lanes.h" // reconcileManagedLanes #include "shell/view/view_reconcile.h" // reconcileAppliedFlags @@ -215,6 +216,15 @@ static void OnTimer() reasampler::applyMode(g_session.view(), g_session.view().activeModeId(), nullptr); } + // Converge the FX chains toward what the view state says they should be — the + // 1897ms of TrackFX_SetOffline a mode switch no longer pays synchronously. Same + // idle discipline as the realtime fast path above: a SINGLE BOOL TEST when there is + // nothing to converge. Ordered AFTER the load block deliberately — a reload replaces + // 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::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 new file mode 100644 index 0000000..1563118 --- /dev/null +++ b/src/shell/view/fx_reclaim.cpp @@ -0,0 +1,163 @@ +// See fx_reclaim.h. Compiled into the reaper_reasampler module; includes +// reaper_plugin_functions.h without REAPERAPI_IMPLEMENT (main.cpp owns that). +// The drain's decisions are pure (header, test_fx_reclaim.cpp); this file owns the +// queue's lifetime, the per-step track resolve, and the writes. + +#include "shell/view/fx_reclaim.h" + +#include +#include +#include +#include + +#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_TrackFX_GetCount +#define REAPERAPI_WANT_TrackFX_GetOffline +#define REAPERAPI_WANT_TrackFX_SetOffline +#define REAPERAPI_WANT_Undo_BeginBlock2 +#define REAPERAPI_WANT_Undo_EndBlock2 +#define REAPERAPI_WANT_ValidatePtr2 +#include "reaper_plugin_functions.h" + +namespace reasampler { + +namespace { + +// The derived queue and the project it was derived against. The pointer is +// COMPARE-ONLY and never dereferenced beyond the ValidatePtr2 the tick already makes — +// a tab switch has to be detectable without following a pointer that may name a closed +// project. +std::vector g_items; +ReaProject* g_project = nullptr; +ParkClock g_clock; + +// TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter +// the timer and land back here mid-drain. Fail closed: a re-entered drain would walk the +// same items the outer one is mutating and double-write from a stale resume cursor. +bool g_draining = false; +struct DrainLatch { + DrainLatch() { g_draining = true; } + ~DrainLatch() { g_draining = false; } + DrainLatch(const DrainLatch&) = delete; + DrainLatch& operator=(const DrainLatch&) = delete; +}; + +// Resolved fresh from the GUID on every step 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. +MediaTrack* resolveFresh(ReaProject* proj, const std::string& guid) { + TrackHandles handles; + readFolderEntries(proj, handles); + for (const auto& kv : handles) + if (kv.first == guid) return kv.second; + return nullptr; // deleted, or moved to another project +} + +std::set liveTrackGuids(ReaProject* proj) { + TrackHandles handles; + readFolderEntries(proj, handles); + std::set guids; + for (const auto& kv : handles) guids.insert(kv.first); + return guids; +} + +// The live half of drainTick's Host contract (see fx_reclaim.h). +struct ReclaimHost { + ReaProject* project = nullptr; + ViewModeModel* model = nullptr; + + // Monotonic and sub-millisecond: the budget below discriminates at 30ms, which + // GetTickCount's ~16ms granularity cannot do. No wraparound to guard against, so + // util::elapsedAtLeast (whose whole reason for existing is that wrap) does not apply. + double nowMs() const { + return std::chrono::duration( + std::chrono::steady_clock::now().time_since_epoch()) + .count(); + } + + bool liveChain(const std::string& guid, std::vector& out) { + MediaTrack* tr = resolveFresh(project, guid); + if (!tr) return false; + const int count = TrackFX_GetCount(tr); + out.clear(); + out.reserve(static_cast(count < 0 ? 0 : count)); + // The plain 0..count-1 enumeration, matching the park snapshot's — never the + // 0x1000000 input-FX or 0x2000000 container forms. + for (int fx = 0; fx < count; ++fx) out.push_back(fxGuidString(tr, fx)); + return true; + } + + bool writeOffline(const std::string& guid, int fxIndex, bool offline) { + MediaTrack* tr = resolveFresh(project, guid); + if (!tr) return false; + if (fxIndex < 0 || fxIndex >= TrackFX_GetCount(tr)) return false; // chain shrank + if (TrackFX_GetOffline(tr, fxIndex) == offline) return false; // already there + + // DAW-VERIFIED: the same ten TrackFX_SetOffline calls mint ten undo points + // unblocked and ZERO inside this pair. One block per write, not per tick: the + // write itself is what pumps, so the block cannot avoid spanning a pump — it can + // only span as little of one as possible. + Undo_BeginBlock2(project); + TrackFX_SetOffline(tr, fxIndex, offline); + Undo_EndBlock2(project, "", 0); + return true; + } + + void releaseFx(const std::string& guid) { model->releaseSnapshotFx(guid); } +}; + +// 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() { + g_items.clear(); + g_project = nullptr; + g_fxReclaimPending = false; +} + +} // namespace + +bool g_fxReclaimPending = false; + +void invalidateFxReclaim() { + goIdle(); + g_fxReclaimPending = true; +} + +void fxReclaimTick(ViewModeModel& model, 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. + if (modeApplyInProgress()) return; + DrainLatch latch; + + // Both arms below are "the project this queue belongs to is no longer the one in + // front of us": no active project at all, or a tab switch / close. Drop the queue + // rather than write another project's chains; returning to it re-derives, because + // poll() reads that as a project load and the load glue reapplies, which invalidates. + // The clock goes too — a hold measured in a project we stopped watching is not + // evidence about the one we come back to. + ReaProject* const project = proj ? proj : EnumProjects(-1, nullptr, 0); + if (!ValidatePtr2(nullptr, project, "ReaProject*") || (g_project && g_project != project)) { + goIdle(); + g_clock.forget(); + return; + } + + // DERIVED, never persisted: recomputed from view state every time the queue was + // discarded, so a REAPER quit mid-drain simply re-derives and finishes on reopen. + if (!g_project) { + g_project = project; + g_items = makeQueue( + view::desiredFxState(model, model.activeModeId(), liveTrackGuids(project))); + } + + ReclaimHost host{project, &model}; + if (drainTick(g_items, g_clock, host).settled) goIdle(); +} + +} // namespace reasampler diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h new file mode 100644 index 0000000..92dc8e2 --- /dev/null +++ b/src/shell/view/fx_reclaim.h @@ -0,0 +1,233 @@ +#pragma once +// The FX reclaimer: the convergence loop that drives every parked chain offline and +// every returning chain back to what its park record captured, off the mode switch's +// synchronous path (TrackFX_SetOffline was 1897ms of a measured 1942ms switch). The +// drain's decisions are pure and live here; the REAPER writes live in the .cpp. + +#include +#include +#include +#include + +#include "core/view/fx_state.h" + +// Forward-declared to keep this header SDK-free; the .cpp includes the real SDK header. +class ReaProject; + +namespace reasampler { + +// --- tuning ------------------------------------------------------------------ + +// A track's OFFLINE work fires only after it has been continuously parked this long. +// The consequence is wanted, not tolerated: a fast A->B->A flip moves zero plugins, +// because the offline never fires and the online that follows finds every chain +// already where the record wants it. +inline constexpr double kParkHoldMs = 3000.0; + +// Per-tick budgets, set against the measured per-plugin costs (Phase Plant 36ms +// offline / 105ms online; MTurboReverb 42ms / 240ms). A step is one plugin and cannot +// be split, so a budget decides only how many plugins a tick moves. +// +// ONLINE 120ms: onlining restores state the user owns and is waiting on, so a tick +// buys at least one typical load and two cheap ones. OFFLINE 30ms: under one typical +// unload, so opportunistic reclaim moves exactly ONE plugin per tick and yields -- +// background work must not make the UI feel worse than the switch already did. The +// budget is checked BEFORE each step and never mid-step, so a plugin dearer than the +// whole budget still runs; that is what makes forward progress unconditional. +inline constexpr double kOnlineBudgetMs = 120.0; +inline constexpr double kOfflineBudgetMs = 30.0; + +inline double tickBudgetMs(view::FxWorkKind kind) { + return kind == view::FxWorkKind::Online ? kOnlineBudgetMs : kOfflineBudgetMs; +} + +// --- the queue: a cache of intent, never state -------------------------------- + +// One derived work item plus this loop's own completion mark. The work — including its +// resume cursor — is core/view/fx_state's; nothing here is persisted, and the whole +// queue is discarded and re-derived on any model change. +struct ReclaimItem { + view::FxTrackWork work; + bool done = false; +}; + +inline std::vector makeQueue(std::vector work) { + std::vector items; + items.reserve(work.size()); + for (view::FxTrackWork& w : work) items.push_back(ReclaimItem{std::move(w), false}); + return items; +} + +inline bool queueSettled(const std::vector& items) { + for (const ReclaimItem& it : items) + if (!it.done) return false; + return true; +} + +// When each track owed OFFLINE work was first observed parked. Keyed on the track, not +// on the queue, so it OUTLIVES a re-derivation: a membership edit somewhere else must +// not restart an unrelated track's hold. An entry is dropped the moment its track stops +// owing offline work, which is what makes a flip start a fresh hold rather than resume +// the one it interrupted. +class ParkClock { +public: + void observe(const std::vector& items, double nowMs) { + std::map next; + for (const ReclaimItem& it : items) { + if (it.work.kind != view::FxWorkKind::Offline) continue; + auto prior = since_.find(it.work.trackGuid); + next[it.work.trackGuid] = prior != since_.end() ? prior->second : nowMs; + } + since_.swap(next); + } + + // Absent answers false: observe() runs first every tick, so an unknown GUID is one + // that owes no offline work at all. + bool matured(const std::string& guid, double nowMs) const { + auto it = since_.find(guid); + return it != since_.end() && nowMs - it->second >= kParkHoldMs; + } + + void forget() { since_.clear(); } + +private: + std::map since_; +}; + +// Online before offline, and an offline item only once its hold has matured. +// desiredFxState already sorts Online first, so a plain scan carries the direction +// priority; this adds only the debounce. +inline int nextEligible(const std::vector& items, const ParkClock& clock, + double nowMs) { + for (std::size_t i = 0; i < items.size(); ++i) { + if (items[i].done) continue; + if (items[i].work.kind == view::FxWorkKind::Offline && + !clock.matured(items[i].work.trackGuid, nowMs)) + continue; + return static_cast(i); + } + return -1; +} + +// --- the resume cursor -------------------------------------------------------- + +// A Slot-keyed ONLINE record (lifted from a project saved before FX identity existed) +// carries no identity in its targets, so an identity cursor would match nothing there +// and restart the item on every yield. Every other shape matches on the live identity, +// which is what survives a chain reorder between two ticks. +inline bool resumeBySlotOnly(const view::FxTrackWork& work) { + return work.kind == view::FxWorkKind::Online && !work.targets.empty() && + work.targets.front().keying == FxKeying::Slot; +} + +inline std::string slotIdentity(const std::vector& liveFxGuids, int slot) { + const auto i = static_cast(slot); + return slot >= 0 && i < liveFxGuids.size() ? liveFxGuids[i] : std::string{}; +} + +// Records the step just completed. The SLOT is written unconditionally because an FX +// REAPER reports no GUID for has nothing else to carry the cursor — see FxTrackWork's +// resumeAfterSlot for what the resolve does with the pair. +inline void markResumePoint(view::FxTrackWork& work, const std::vector& liveFxGuids, + int slot) { + work.resumeAfterFxGuid = resumeBySlotOnly(work) ? std::string{} : slotIdentity(liveFxGuids, slot); + work.resumeAfterSlot = slot; +} + +// --- the drain ---------------------------------------------------------------- + +// 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. +struct ReclaimTickResult { + int writes = 0; + bool settled = false; +}; + +// One tick of the convergence loop. Templated on `Host` so the ordering, budget, resume +// and release bookkeeping are all exercised without REAPER (tests/test_fx_reclaim.cpp); +// fx_reclaim.cpp holds the one live implementation. Host must provide: +// +// double nowMs() monotonic milliseconds +// bool liveChain(guid, std::vector&) FX identity per live slot; +// false when the track is gone +// bool writeOffline(guid, fxIndex, offline) compare-before-write inside its +// own discarded undo block; +// true iff it actually wrote +// void releaseFx(guid) release the record's FX half +template +ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, Host& host) { + ReclaimTickResult result; + clock.observe(items, host.nowMs()); + + const int first = nextEligible(items, clock, host.nowMs()); + if (first < 0) { + result.settled = queueSettled(items); + return result; + } + + const view::FxWorkKind tickKind = items[static_cast(first)].work.kind; + const double budget = tickBudgetMs(tickKind); + const double start = host.nowMs(); + + for (;;) { + const int i = nextEligible(items, clock, host.nowMs()); + if (i < 0) break; + ReclaimItem& item = items[static_cast(i)]; + // Offline work never rides the online budget: the two directions are priced + // apart deliberately, and a tick that started on one must not spend the other's. + if (item.work.kind != tickKind) break; + + std::vector liveFxGuids; + if (!host.liveChain(item.work.trackGuid, liveFxGuids)) { + // Track gone. Nothing to write, and no record to release either — the next + // apply's reconcile drops the whole record for a GUID that left the project. + item.done = true; + continue; + } + + // Held in a named local: through C++20 a range-for does not extend the lifetime + // of a temporary reached through a member access, so iterating the resolution's + // `.writes` directly off the call would dangle. + const FxRestoreResolution resolved = view::resolveFxWork(item.work, liveFxGuids); + + bool yielded = false; + for (const FxOfflineWrite& w : resolved.writes) { + if (host.nowMs() - start >= budget) { yielded = true; break; } + if (host.writeOffline(item.work.trackGuid, w.fxIndex, w.offline)) ++result.writes; + markResumePoint(item.work, liveFxGuids, w.fxIndex); + } + if (yielded) return result; // the resume cursor picks this item up next tick + + item.done = true; + // The record's FX half is released HERE and only here: the chain has now been + // driven to every state the record captured, so the record is spent. Releasing + // it any earlier destroys the only account of which plugins the USER had + // offline before the park, and the next online blanket-enables them. + if (item.work.kind == view::FxWorkKind::Online) host.releaseFx(item.work.trackGuid); + } + + result.settled = queueSettled(items); + return result; +} + +// --- shell entry points -------------------------------------------------------- + +// The ONE test the timer makes per tick, mirroring the realtime capture's single +// pointer test. False means the queue is settled and there is nothing to converge. +extern bool g_fxReclaimPending; + +// Discards the derived queue. Called from every path that can change what the FX chains +// SHOULD be — which is applyMode's success path and nothing else, because every +// membership, snapshot and active-mode mutation reapplies through it. The debounce clock +// deliberately survives: it is keyed on the track, not on the queue. +void invalidateFxReclaim(); + +// 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 (TrackFX_SetOffline is assumed to +// pump the message loop), or when the active project is not the one the queue was +// derived against. +void fxReclaimTick(ViewModeModel& model, ReaProject* proj); + +} // namespace reasampler diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 132bc3e..42c9b66 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -13,6 +13,7 @@ #include "core/view/solo_cache.h" #include "core/view/view_tree.h" +#include "shell/view/fx_reclaim.h" #include "shell/view/view_lanes.h" #include "shell/view/view_park.h" #include "shell/view/view_reconcile.h" @@ -201,6 +202,12 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject // The tool's own account of what it just left driven — the only one that survives // an undo rolling the model back underneath it. noteAppliedState(project, model); + + // Every membership, snapshot and active-mode mutation in the system reaches the + // project through this call, so this is the whole invalidation surface: what the FX + // chains SHOULD be has just changed, and the reclaimer's queue is a cache of that + // intent, never state. + invalidateFxReclaim(); return true; } diff --git a/src/shell/view/view_park.cpp b/src/shell/view/view_park.cpp index ef74936..58f4785 100644 --- a/src/shell/view/view_park.cpp +++ b/src/shell/view/view_park.cpp @@ -22,28 +22,6 @@ namespace reasampler { namespace { -// The FX's own durable identity, braced exactly like the track GUID keys. Empty -// when REAPER reports none — an FX we cannot name is one we cannot restore, and -// fx_offline treats it that way rather than guessing at its slot. Lifetime is -// settled: the string copy is taken immediately and the GUID* is never held past -// this call (reaper_plugin_functions.h:7348 documents no null contract for -// TrackFX_GetFXGUID; treating null as "no identity" is the safe read). -// -// [verify — DAW] STABILITY across a chain mutation is not settled the same way: -// confirm the GUID for one FX instance survives a native drag-reorder, an SWS move -// (SNM_MoveOrRemoveTrackFX — SWS issue #802 reports the FXID lines do not follow -// the plugin after that call, i.e. wrong-plugin restores or mass drops through -// fx_offline on that path specifically), a save/reload round trip, and two live -// instances of one plugin type staying distinguishable. See src/shell/view/CLAUDE.md's -// Gotchas for the SWS-path risk this leaves open. -std::string fxGuidString(MediaTrack* tr, int fx) { - GUID* g = TrackFX_GetFXGUID(tr, fx); - if (!g) return {}; - char buf[64] = {0}; // guidToString needs a >=64-char destination (SDK contract) - guidToString(g, buf); - return std::string(buf); -} - // Captures prior driven-flag state, and the chain's per-FX offline state, before // parking. Never reads B_MUTE/I_SOLO; ints preserve whatever REAPER reported // (TrackSnapshot's defensive contract). The plain 0..TrackFX_GetCount-1 enumeration @@ -96,6 +74,25 @@ std::vector g_reportedExposed; const int kViewUndoMask = UNDO_STATE_TRACKCFG | UNDO_STATE_ITEMS | UNDO_STATE_MISCCFG; +// Lifetime is settled: the string copy is taken immediately and the GUID* is never held +// past this call (reaper_plugin_functions.h:7348 documents no null contract for +// TrackFX_GetFXGUID; treating null as "no identity" is the safe read). +// +// [verify — DAW] STABILITY across a chain mutation is not settled the same way: confirm +// the GUID for one FX instance survives a native drag-reorder, an SWS move +// (SNM_MoveOrRemoveTrackFX — SWS issue #802 reports the FXID lines do not follow the +// plugin after that call, i.e. wrong-plugin restores or mass drops through fx_offline on +// that path specifically), a save/reload round trip, and two live instances of one plugin +// type staying distinguishable. See src/shell/view/CLAUDE.md's Gotchas for the SWS-path +// risk this leaves open. +std::string fxGuidString(MediaTrack* tr, int fx) { + GUID* g = TrackFX_GetFXGUID(tr, fx); + if (!g) return {}; + char buf[64] = {0}; // guidToString needs a >=64-char destination (SDK contract) + guidToString(g, buf); + return std::string(buf); +} + bool writeIfChanged(MediaTrack* tr, const char* parm, double value) { if (GetMediaTrackInfo_Value(tr, parm) == value) return false; SetMediaTrackInfo_Value(tr, parm, value); diff --git a/src/shell/view/view_park.h b/src/shell/view/view_park.h index 4b99706..5150db2 100644 --- a/src/shell/view/view_park.h +++ b/src/shell/view/view_park.h @@ -29,6 +29,12 @@ inline const char* trackFlagParm(Flag f) { return "B_SHOWINTCP"; // unreachable; keeps the compiler quiet } +// The FX's own durable identity, braced exactly like the track GUID keys, empty when +// REAPER reports none. The ONE such read: the park's snapshot and the reclaimer's +// resolve must address the same chain by the same names, or a restore lands one +// plugin's state on another. See its definition for the stability caveat. +std::string fxGuidString(MediaTrack* tr, int fx); + // A re-park's FX half, unioned rather than overwritten: keeps every FX identity // `prior` already names (the user's real pre-park state, still offline while the // reclaimer hasn't run) and appends whatever identity `live` sees that `prior` diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp new file mode 100644 index 0000000..bb91e62 --- /dev/null +++ b/tests/test_fx_reclaim.cpp @@ -0,0 +1,398 @@ +// Standalone tests for the FX reclaimer's drain — no REAPER, no test framework. The +// loop is templated on its host precisely so the four properties that make it safe to +// run in the background are assertable here: the debounce, the per-tick budget's +// yield-and-resume, the queue being a cache of intent that a model change discards, and +// the user's own offline plugins surviving an online. + +#include "../src/shell/view/fx_reclaim.h" + +#include +#include +#include +#include +#include + +using namespace reasampler; +using namespace reasampler::view; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +namespace { + +// -- a fake project ------------------------------------------------------------ + +// One live FX: its identity (empty when REAPER reports none) and whether it is offline. +struct FakeFx { + std::string guid; + bool offline = false; +}; + +// Stands in for the REAPER half of drainTick's Host contract. The clock advances only +// on a REAL write, mirroring the live host: a compare-before-write that finds the FX +// already where it wants it costs a read, not a plugin load. +struct FakeHost { + std::map> chains; + std::set released; + std::vector writeLog; + double clock = 0.0; + double stepCostMs = 50.0; + + double nowMs() const { return clock; } + + bool liveChain(const std::string& guid, std::vector& out) { + auto it = chains.find(guid); + if (it == chains.end()) return false; + out.clear(); + for (const FakeFx& fx : it->second) out.push_back(fx.guid); + return true; + } + + bool writeOffline(const std::string& guid, int fxIndex, bool offline) { + auto it = chains.find(guid); + if (it == chains.end()) return false; + if (fxIndex < 0 || fxIndex >= static_cast(it->second.size())) return false; + if (it->second[static_cast(fxIndex)].offline == offline) return false; + + it->second[static_cast(fxIndex)].offline = offline; + clock += stepCostMs; + writeLog.push_back(guid + ":" + std::to_string(fxIndex) + (offline ? "=off" : "=on")); + return true; + } + + void releaseFx(const std::string& guid) { released.insert(guid); } + + bool offlineAt(const std::string& guid, std::size_t slot) const { + auto it = chains.find(guid); + return it != chains.end() && slot < it->second.size() && it->second[slot].offline; + } +}; + +// -- model helpers ------------------------------------------------------------- + +TrackSnapshot recordWithFx(std::vector fx) { + TrackSnapshot snap; + snap.showInTcp = 1; snap.showInMixer = 1; snap.mainSend = 1; snap.fxEnable = 1; + snap.fxOffline = std::move(fx); + return snap; +} + +// What the shell does on every invalidation: throw the queue away and recompute it. +std::vector derive(const ViewModeModel& vm, const std::set& live) { + return makeQueue(desiredFxState(vm, vm.activeModeId(), live)); +} + +std::vector onlineChain(const std::vector& guids) { + std::vector chain; + for (const std::string& g : guids) chain.push_back(FakeFx{g, false}); + return chain; +} + +std::vector offlineChain(const std::vector& guids) { + std::vector chain; + for (const std::string& g : guids) chain.push_back(FakeFx{g, true}); + return chain; +} + +// A Design leaf the tool has parked: record held, both halves, nothing user-offlined. +ViewModeModel parkedDesignLeaf(const std::vector& fxGuids) { + ViewModeModel vm; + vm.membership().tag("{T}", kDesignModeId); // active mode is Arrange ⇒ {T} is parked + std::vector fx; + for (const std::string& g : fxGuids) fx.push_back(FxOfflineState{g, 0}); + vm.storeSnapshot("{T}", recordWithFx(fx)); + return vm; +} + +// Puts the queue's offline items past their debounce the way the live loop does: the +// hold starts at the FIRST observation, so advancing the clock before that one just +// moves the origin and matures nothing. +void matureHold(std::vector& items, ParkClock& clock, FakeHost& host) { + clock.observe(items, host.nowMs()); + host.clock += kParkHoldMs; +} + +// Runs ticks until the queue settles, bounded so a non-terminating drain fails the test +// rather than hanging CTest. +int drainToSettled(std::vector& items, ParkClock& clock, FakeHost& host, + int maxTicks = 64) { + for (int t = 1; t <= maxTicks; ++t) + if (drainTick(items, clock, host).settled) return t; + CHECK(false); // did not settle — the drain is not making forward progress + return -1; +} + +} // namespace + +// -- 1. debounce --------------------------------------------------------------- + +static void testFastFlipMovesZeroPluginsButAHeldParkStillReclaims() { + // A→B: {T} parks. The reclaimer must not touch it yet. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}"}); + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + CHECK(items.size() == 1 && items[0].work.kind == FxWorkKind::Offline); + CHECK(drainTick(items, clock, host).writes == 0); + CHECK(!drainTick(items, clock, host).settled); // waiting out its hold, not finished + + // B→A half a second later: the flags come back and the FX half is still held, so the + // reclaimer is now owed an ONLINE — over a chain it never got round to offlining. + host.clock = 500.0; + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + items = derive(vm, {"{T}"}); + CHECK(items.size() == 1 && items[0].work.kind == FxWorkKind::Online); + drainToSettled(items, clock, host); + + // The whole point of the 3s hold: a fast A→B→A flip moved NO plugins. + CHECK(host.writeLog.empty()); + CHECK(!host.offlineAt("{T}", 0) && !host.offlineAt("{T}", 1) && !host.offlineAt("{T}", 2)); + CHECK(host.released.count("{T}") == 1); // the chain is verifiably online; record spent + + // The contrast, so the assertion above is about the hold and not about the drain + // being inert: the same park held past 3s DOES reclaim. + ViewModeModel held = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}"}); + FakeHost host2; + host2.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}"}); + ParkClock clock2; + std::vector items2 = derive(held, {"{T}"}); + CHECK(drainTick(items2, clock2, host2).writes == 0); // observed parked at t=0 + host2.clock = kParkHoldMs; + drainToSettled(items2, clock2, host2); + CHECK(host2.writeLog.size() == 3); + CHECK(host2.offlineAt("{T}", 0) && host2.offlineAt("{T}", 1) && host2.offlineAt("{T}", 2)); + CHECK(host2.released.empty()); // an OFFLINE never releases — the record is still owed +} + +// -- 2. budget: yield and resume ------------------------------------------------ + +static void testOfflineDrainYieldsOnBudgetAndResumesWhereItStopped() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); + host.stepCostMs = 50.0; // dearer than kOfflineBudgetMs ⇒ exactly one plugin per tick + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + matureHold(items, clock, host); + CHECK(drainTick(items, clock, host).writes == 1); + CHECK(items[0].work.resumeAfterSlot == 0); + CHECK(items[0].work.resumeAfterFxGuid == "{FX-A}"); + CHECK(!items[0].done); + + CHECK(drainTick(items, clock, host).writes == 1); + CHECK(items[0].work.resumeAfterSlot == 1); + + // Slots 2 and 3, one per tick; the tick that writes the last one also exhausts the + // item's write list, so it settles in the same pass rather than costing a spare tick. + CHECK(drainToSettled(items, clock, host) == 2); + + // Each slot moved exactly once and in order — no slot re-walked across a yield. + const std::vector expected{"{T}:0=off", "{T}:1=off", "{T}:2=off", "{T}:3=off"}; + CHECK(host.writeLog == expected); +} + +static void testOnlineDrainGetsTheLargerBudget() { + // Same per-plugin cost, same chain length: the only difference is the direction, and + // onlining is the one the user is waiting on. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + FakeHost host; + host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); + host.stepCostMs = 50.0; + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + CHECK(items.size() == 1 && items[0].work.kind == FxWorkKind::Online); + CHECK(drainTick(items, clock, host).writes == 3); // 120ms budget vs 30ms +} + +static void testATickStartedOnOnlineWorkDoesNotSpendIntoOfflineWork() { + ViewModeModel vm; + vm.membership().tag("{Z-back}", kArrangeModeId); // active; owed an online + vm.membership().tag("{A-park}", kDesignModeId); // parked; owed an offline + vm.storeSnapshot("{Z-back}", recordWithFx({{"{FX-1}", 0}})); + vm.storeSnapshot("{A-park}", recordWithFx({{"{FX-2}", 0}})); + + FakeHost host; + host.chains["{Z-back}"] = offlineChain({"{FX-1}"}); + host.chains["{A-park}"] = onlineChain({"{FX-2}"}); + host.stepCostMs = 1.0; // cheap enough that only the kind break can stop the tick + ParkClock clock; + + std::vector items = derive(vm, {"{Z-back}", "{A-park}"}); + CHECK(items.size() == 2); + CHECK(items[0].work.kind == FxWorkKind::Online); // fx_state's ordering + matureHold(items, clock, host); // the parked track's hold is no longer the blocker + + CHECK(drainTick(items, clock, host).writes == 1); + CHECK(host.writeLog == std::vector{"{Z-back}:0=on"}); + CHECK(!host.offlineAt("{A-park}", 0)); // opportunistic reclaim waits for its own tick + + CHECK(drainTick(items, clock, host).writes == 1); + CHECK(host.offlineAt("{A-park}", 0)); +} + +// -- 3. invalidation: the queue is a cache of intent ---------------------------- + +static void testModelChangeDiscardsDrainProgressButNotTheParkHold() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}"}); + vm.membership().tag("{U}", kDesignModeId); // a second parked leaf, left alone below + vm.storeSnapshot("{U}", recordWithFx({{"{FX-U}", 0}})); + + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}"}); + host.chains["{U}"] = onlineChain({"{FX-U}"}); + host.stepCostMs = 50.0; + ParkClock clock; + + std::vector items = derive(vm, {"{T}", "{U}"}); + drainTick(items, clock, host); // observes both parked at t=0 + host.clock = kParkHoldMs + 100.0; + drainTick(items, clock, host); // one plugin moves; {T} now carries a cursor + CHECK(items[0].work.resumeAfterSlot >= 0); + + // The model changes: {T} is retagged into the active mode and restored, while {U} + // stays exactly as parked as it was. The queue is discarded whole — direction, + // cursors, completion marks and all — and re-derived. + vm.membership().tag("{T}", kArrangeModeId); + vm.releaseSnapshotFlags("{T}"); + items = derive(vm, {"{T}", "{U}"}); + CHECK(items.size() == 2); + CHECK(items[0].work.kind == FxWorkKind::Online); + CHECK(items[0].work.trackGuid == "{T}"); + CHECK(items[0].work.resumeAfterSlot == -1); // no drain progress survives a re-derive + CHECK(items[0].work.resumeAfterFxGuid.empty()); + CHECK(!items[0].done); + + // ...but {U}'s hold does survive it. {U} never stopped being parked, and restarting + // its 3s on every unrelated model change would defer its reclaim indefinitely. + clock.observe(items, host.clock); + CHECK(clock.matured("{U}", host.clock)); + CHECK(nextEligible(items, clock, host.clock) == 0); // online first, then {U} available + + // The mirror: a track that LEAVES the offline set and comes back starts a fresh hold. + // {T} was dropped from the clock by the observation above, while it was online. + vm.membership().tag("{T}", kDesignModeId); + items = derive(vm, {"{T}", "{U}"}); + clock.observe(items, host.clock); + CHECK(!clock.matured("{T}", host.clock)); + CHECK(!clock.matured("{T}", host.clock + kParkHoldMs - 1.0)); + CHECK(clock.matured("{T}", host.clock + kParkHoldMs)); + CHECK(clock.matured("{U}", host.clock)); // and {U}'s own hold still is not restarted +} + +// -- 4. the user's own offline plugins ------------------------------------------ + +static void testAPluginTheUserOfflinedIsNeverOnlinedByTheDrain() { + // {FX-B} was offline before the park because the USER put it there; the record is + // the only account of that, and an online must honor it. + ViewModeModel vm; + vm.membership().tag("{T}", kDesignModeId); + vm.setActiveMode(kDesignModeId); + vm.storeSnapshot("{T}", recordWithFx({{"{FX-A}", 0}, {"{FX-B}", 1}, {"{FX-C}", 0}})); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}", "{FX-C}"}); // fully parked + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + drainToSettled(items, clock, host); + + CHECK(!host.offlineAt("{T}", 0)); + CHECK(host.offlineAt("{T}", 1)); // still offline — never blanket-onlined + CHECK(!host.offlineAt("{T}", 2)); + for (const std::string& entry : host.writeLog) CHECK(entry != "{T}:1=on"); + CHECK(host.writeLog.size() == 2); + CHECK(host.released.count("{T}") == 1); +} + +// -- forward progress: the cursor cases that decide termination ----------------- + +static void testResumeCursorAdvancesPastAnFxWithNoIdentity() { + // The case Wave 1's slot cursor exists for. Slot 1 is an FX REAPER reports no GUID + // for; with the identity alone the cursor would read as "nothing done yet" and the + // item would re-walk from slot 0 on every tick. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}", "", "{FX-C}"}); + host.stepCostMs = 50.0; // one plugin per tick on the offline budget + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + matureHold(items, clock, host); + drainTick(items, clock, host); + drainTick(items, clock, host); + CHECK(items[0].work.resumeAfterFxGuid.empty()); // slot 1 has no identity to record + CHECK(items[0].work.resumeAfterSlot == 1); // ...so the slot is what carries it + + CHECK(drainToSettled(items, clock, host) == 1); // slot 2, and the item is exhausted + const std::vector expected{"{T}:0=off", "{T}:1=off", "{T}:2=off"}; + CHECK(host.writeLog == expected); +} + +static void testSlotKeyedOnlineRecordCarriesItsCursorAsASlot() { + // A record lifted from a project saved before FX identity existed: its targets hold + // no identity, so a cursor taken from the LIVE identity would match none of them and + // the item would re-walk its whole chain after every yield. + ViewModeModel vm; + vm.membership().tag("{T}", kDesignModeId); + vm.setActiveMode(kDesignModeId); + TrackSnapshot snap = recordWithFx({{"", 0}, {"", 1}, {"", 0}}); + snap.fxKeying = FxKeying::Slot; + vm.storeSnapshot("{T}", snap); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.chains["{T}"] = offlineChain({"{FX-X}", "{FX-Y}", "{FX-Z}"}); + host.stepCostMs = kOnlineBudgetMs + 10.0; // one plugin per tick, then yield + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + CHECK(items.size() == 1 && resumeBySlotOnly(items[0].work)); + drainTick(items, clock, host); + CHECK(items[0].work.resumeAfterFxGuid.empty()); // NOT "{FX-X}", which matches no target + CHECK(items[0].work.resumeAfterSlot == 0); + + drainToSettled(items, clock, host); + const std::vector expected{"{T}:0=on", "{T}:2=on"}; + CHECK(host.writeLog == expected); // slot 1 stays offline, as the record captured + CHECK(host.offlineAt("{T}", 1)); +} + +static void testATrackThatLeftTheProjectCompletesRatherThanStalls() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; // no chain registered ⇒ liveChain reports the track gone + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + matureHold(items, clock, host); + CHECK(drainTick(items, clock, host).settled); + CHECK(host.writeLog.empty()); + CHECK(host.released.empty()); // the next apply's reconcile drops the whole record +} + +int main() { + testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); + + testOfflineDrainYieldsOnBudgetAndResumesWhereItStopped(); + testOnlineDrainGetsTheLargerBudget(); + testATickStartedOnOnlineWorkDoesNotSpendIntoOfflineWork(); + + testModelChangeDiscardsDrainProgressButNotTheParkHold(); + + testAPluginTheUserOfflinedIsNeverOnlinedByTheDrain(); + + testResumeCursorAdvancesPastAnFxWithNoIdentity(); + testSlotKeyedOnlineRecordCarriesItsCursorAsASlot(); + testATrackThatLeftTheProjectCompletesRatherThanStalls(); + + if (g_fail == 0) std::printf("All tests passed.\n"); + return g_fail ? 1 : 0; +} From f04c7043539a260be7c4497b09b6264ec8f1ae2c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 20:48:17 -0400 Subject: [PATCH 2/9] Make an FX reclaim step one whole track's chain: drop the resume cursor, yield only between tracks --- src/core/view/fx_state.cpp | 41 ++------- src/core/view/fx_state.h | 36 +++----- src/shell/view/fx_reclaim.cpp | 12 ++- src/shell/view/fx_reclaim.h | 88 +++++++------------ tests/test_fx_reclaim.cpp | 159 +++++++++++++++++----------------- tests/test_fx_state.cpp | 72 --------------- 6 files changed, 134 insertions(+), 274 deletions(-) diff --git a/src/core/view/fx_state.cpp b/src/core/view/fx_state.cpp index 00e4073..e28b202 100644 --- a/src/core/view/fx_state.cpp +++ b/src/core/view/fx_state.cpp @@ -7,32 +7,9 @@ namespace reasampler::view { namespace { -// Everything up to and including the resume point is already done. An empty -// `resumeAfterFxGuid` is ambiguous by itself — "nothing done yet" and "the last -// completed op had no identity" look identical — so an empty guid falls back to -// matching by slot instead; `resumeAfterSlot < 0` is what actually means -// "nothing done yet". Not found ⇒ nothing dropped: the resume point left the -// chain between ticks, and re-walking a compare-before-write list beats -// trusting a cursor that no longer points at anything. -std::vector remainingAfter(const std::vector& ops, - const std::string& resumeAfterFxGuid, - int resumeAfterSlot) { - if (resumeAfterFxGuid.empty() && resumeAfterSlot < 0) return ops; - for (std::size_t i = 0; i < ops.size(); ++i) { - const bool isResumePoint = resumeAfterFxGuid.empty() - ? ops[i].slot == resumeAfterSlot - : ops[i].fxGuid == resumeAfterFxGuid; - if (isResumePoint) - return std::vector(ops.begin() + static_cast(i) + 1, - ops.end()); - } - return ops; -} - // Slot keying, deliberately: the live chain IS the target set, so its positions // are current by construction. Identity keying would drop an FX REAPER reports no -// GUID for, and reclaim has to reach those too — the fxGuid rides along only so -// the resume point has something to match. +// GUID for, and reclaim has to reach those too. std::vector offlineTargets(const std::string& trackGuid, const std::vector& liveFxGuids) { std::vector ops; @@ -70,14 +47,13 @@ std::vector desiredFxState(const ViewModeModel& model, if (liveTrackGuids.count(guid) == 0) continue; // dead track — nothing to write if (!model.leafBelongsToMode(guid, targetMode)) { - offline.push_back(FxTrackWork{guid, FxWorkKind::Offline, {}, {}}); + offline.push_back(FxTrackWork{guid, FxWorkKind::Offline, {}}); continue; } // Active with the FX half already released: the chain is back online and // the record is only waiting on its flags. Nothing owed either way. if (!snap.fxHeld) continue; - online.push_back( - FxTrackWork{guid, FxWorkKind::Online, fxRestoreTargets(model, guid), {}}); + online.push_back(FxTrackWork{guid, FxWorkKind::Online, fxRestoreTargets(model, guid)}); } online.reserve(online.size() + offline.size()); @@ -87,14 +63,9 @@ std::vector desiredFxState(const ViewModeModel& model, FxRestoreResolution resolveFxWork(const FxTrackWork& work, const std::vector& liveFxGuids) { - if (work.kind == FxWorkKind::Offline) { - return resolveFxRestore( - remainingAfter(offlineTargets(work.trackGuid, liveFxGuids), work.resumeAfterFxGuid, - work.resumeAfterSlot), - liveFxGuids); - } - return resolveFxRestore( - remainingAfter(work.targets, work.resumeAfterFxGuid, work.resumeAfterSlot), liveFxGuids); + if (work.kind == FxWorkKind::Offline) + return resolveFxRestore(offlineTargets(work.trackGuid, liveFxGuids), liveFxGuids); + return resolveFxRestore(work.targets, liveFxGuids); } } // namespace reasampler::view diff --git a/src/core/view/fx_state.h b/src/core/view/fx_state.h index c74e242..b804ef1 100644 --- a/src/core/view/fx_state.h +++ b/src/core/view/fx_state.h @@ -19,9 +19,13 @@ enum class FxWorkKind { Offline, // reclaim: every live FX offline }; -// One track's whole FX move. Track-granular so a drain may stop between tracks, -// and resumable WITHIN a track by FX identity: a slot cursor would silently skip -// or repeat a plugin if the chain were edited between two ticks. +// One track's whole FX move, and the ATOMIC unit of work: a drain may stop +// between items, never inside one, so an item is either untouched or complete +// and no partial state survives a tick boundary. Do not reintroduce a +// within-track resume cursor — carrying one across ticks meant reconciling it +// against a chain the user may have edited in between, and the case an FX with +// no reported identity forced (a cursor that reads as "nothing done yet" and +// re-walks the item forever) was a live non-termination hazard. struct FxTrackWork { std::string trackGuid; FxWorkKind kind = FxWorkKind::Offline; @@ -32,28 +36,8 @@ struct FxTrackWork { // read it, so resolveFxWork synthesises the targets from what it is handed. std::vector targets; - // Identity of the last FX this item's drain completed; empty means EITHER - // nothing has completed yet OR the completed FX itself carried no identity - // (REAPER reports no GUID for it, or it came from a Slot-keyed record) — see - // resumeAfterSlot below, which disambiguates the two. This is load-bearing - // for forward progress, not an optimisation: without the disambiguation, an - // unidentified FX would look like "none yet" and the drain would restart at - // the top of the list every tick, forever. Compare-before-write makes every - // write idempotent, so an identity that has since left the chain safely - // restarts the item rather than stranding it. - std::string resumeAfterFxGuid; - - // Position of the last completed op, consulted only when resumeAfterFxGuid - // is empty, to tell "resumed past an unidentified FX at this slot" apart - // from "nothing done yet" (-1). Every FxOfflineOp carries its slot - // regardless of keying, so this is available for both Online and Offline - // work. - int resumeAfterSlot = -1; - bool operator==(const FxTrackWork& o) const { - return trackGuid == o.trackGuid && kind == o.kind && targets == o.targets && - resumeAfterFxGuid == o.resumeAfterFxGuid && - resumeAfterSlot == o.resumeAfterSlot; + return trackGuid == o.trackGuid && kind == o.kind && targets == o.targets; } }; @@ -82,8 +66,8 @@ std::vector fxRestoreTargets(const ViewModeModel& model, // Resolves one work item against the chain as it stands, where `liveFxGuids[i]` // is the identity of the FX at slot `i` right now (empty when REAPER reported -// none). Honors the resume point, then matches by identity: an identity that is -// not live is DROPPED, never re-pointed at a slot. +// none) — the item's WHOLE move, matched by identity: an identity that is not +// live is DROPPED, never re-pointed at a slot. FxRestoreResolution resolveFxWork(const FxTrackWork& work, const std::vector& liveFxGuids); diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 1563118..3360ae3 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -38,7 +38,8 @@ ParkClock g_clock; // TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter // the timer and land back here mid-drain. Fail closed: a re-entered drain would walk the -// same items the outer one is mutating and double-write from a stale resume cursor. +// same queue the outer one holds a reference into, re-running the very item that outer +// step is still writing. bool g_draining = false; struct DrainLatch { DrainLatch() { g_draining = true; } @@ -99,9 +100,12 @@ struct ReclaimHost { if (TrackFX_GetOffline(tr, fxIndex) == offline) return false; // already there // DAW-VERIFIED: the same ten TrackFX_SetOffline calls mint ten undo points - // unblocked and ZERO inside this pair. One block per write, not per tick: the - // write itself is what pumps, so the block cannot avoid spanning a pump — it can - // only span as little of one as possible. + // unblocked and ZERO inside this pair. One block per WRITE even though a step is + // now one whole track: the write itself is what pumps, so the block cannot avoid + // spanning a pump — it can only span as little of one as possible. Widening it to + // the track would multiply that exposure by the chain length while suppressing no + // extra undo point (the discard form mints none either way), and a user action + // landing inside a discarded block can lose its own Ctrl-Z. Undo_BeginBlock2(project); TrackFX_SetOffline(tr, fxIndex, offline); Undo_EndBlock2(project, "", 0); diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 92dc8e2..2e5cfa3 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -25,15 +25,17 @@ namespace reasampler { inline constexpr double kParkHoldMs = 3000.0; // Per-tick budgets, set against the measured per-plugin costs (Phase Plant 36ms -// offline / 105ms online; MTurboReverb 42ms / 240ms). A step is one plugin and cannot -// be split, so a budget decides only how many plugins a tick moves. +// offline / 105ms online; MTurboReverb 42ms / 240ms). A step is one whole track's +// chain and cannot be split, so a budget decides only how many TRACKS a tick moves -- +// and a single track with a long chain overruns it by as much as that chain costs. // // ONLINE 120ms: onlining restores state the user owns and is waiting on, so a tick // buys at least one typical load and two cheap ones. OFFLINE 30ms: under one typical -// unload, so opportunistic reclaim moves exactly ONE plugin per tick and yields -- +// unload, so opportunistic reclaim moves exactly ONE track per tick and yields -- // background work must not make the UI feel worse than the switch already did. The -// budget is checked BEFORE each step and never mid-step, so a plugin dearer than the -// whole budget still runs; that is what makes forward progress unconditional. +// budget is checked only AFTER a step, never before the first and never mid-step, so +// a track dearer than the whole budget still runs; that is what makes forward +// progress unconditional. inline constexpr double kOnlineBudgetMs = 120.0; inline constexpr double kOfflineBudgetMs = 30.0; @@ -43,9 +45,9 @@ inline double tickBudgetMs(view::FxWorkKind kind) { // --- the queue: a cache of intent, never state -------------------------------- -// One derived work item plus this loop's own completion mark. The work — including its -// resume cursor — is core/view/fx_state's; nothing here is persisted, and the whole -// queue is discarded and re-derived on any model change. +// One derived work item plus this loop's own completion mark. The work is +// core/view/fx_state's; nothing here is persisted, and the whole queue is discarded +// and re-derived on any model change. struct ReclaimItem { view::FxTrackWork work; bool done = false; @@ -109,31 +111,6 @@ inline int nextEligible(const std::vector& items, const ParkClock& return -1; } -// --- the resume cursor -------------------------------------------------------- - -// A Slot-keyed ONLINE record (lifted from a project saved before FX identity existed) -// carries no identity in its targets, so an identity cursor would match nothing there -// and restart the item on every yield. Every other shape matches on the live identity, -// which is what survives a chain reorder between two ticks. -inline bool resumeBySlotOnly(const view::FxTrackWork& work) { - return work.kind == view::FxWorkKind::Online && !work.targets.empty() && - work.targets.front().keying == FxKeying::Slot; -} - -inline std::string slotIdentity(const std::vector& liveFxGuids, int slot) { - const auto i = static_cast(slot); - return slot >= 0 && i < liveFxGuids.size() ? liveFxGuids[i] : std::string{}; -} - -// Records the step just completed. The SLOT is written unconditionally because an FX -// REAPER reports no GUID for has nothing else to carry the cursor — see FxTrackWork's -// resumeAfterSlot for what the resolve does with the pair. -inline void markResumePoint(view::FxTrackWork& work, const std::vector& liveFxGuids, - int slot) { - work.resumeAfterFxGuid = resumeBySlotOnly(work) ? std::string{} : slotIdentity(liveFxGuids, slot); - work.resumeAfterSlot = slot; -} - // --- the drain ---------------------------------------------------------------- // What one tick did. `settled` means every item is complete and the caller may go idle @@ -144,8 +121,8 @@ struct ReclaimTickResult { bool settled = false; }; -// One tick of the convergence loop. Templated on `Host` so the ordering, budget, resume -// and release bookkeeping are all exercised without REAPER (tests/test_fx_reclaim.cpp); +// One tick of the convergence loop. Templated on `Host` so the ordering, budget and +// release bookkeeping are all exercised without REAPER (tests/test_fx_reclaim.cpp); // fx_reclaim.cpp holds the one live implementation. Host must provide: // // double nowMs() monotonic milliseconds @@ -178,33 +155,26 @@ ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, H // apart deliberately, and a tick that started on one must not spend the other's. if (item.work.kind != tickKind) break; + // A track gone from the project writes nothing and releases nothing — the next + // apply's reconcile drops the whole record for a GUID that left it. std::vector liveFxGuids; - if (!host.liveChain(item.work.trackGuid, liveFxGuids)) { - // Track gone. Nothing to write, and no record to release either — the next - // apply's reconcile drops the whole record for a GUID that left the project. - item.done = true; - continue; + if (host.liveChain(item.work.trackGuid, liveFxGuids)) { + // Held in a named local: through C++20 a range-for does not extend the + // lifetime of a temporary reached through a member access, so iterating the + // resolution's `.writes` directly off the call would dangle. + 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; + + // The record's FX half is released HERE and only here: the chain has now + // been driven to every state the record captured, so the record is spent. + // Releasing it any earlier destroys the only account of which plugins the + // USER had offline before the park, and the next online blanket-enables them. + if (item.work.kind == view::FxWorkKind::Online) host.releaseFx(item.work.trackGuid); } + item.done = true; // both arms: a step is atomic, so no item is half-moved - // Held in a named local: through C++20 a range-for does not extend the lifetime - // of a temporary reached through a member access, so iterating the resolution's - // `.writes` directly off the call would dangle. - const FxRestoreResolution resolved = view::resolveFxWork(item.work, liveFxGuids); - - bool yielded = false; - for (const FxOfflineWrite& w : resolved.writes) { - if (host.nowMs() - start >= budget) { yielded = true; break; } - if (host.writeOffline(item.work.trackGuid, w.fxIndex, w.offline)) ++result.writes; - markResumePoint(item.work, liveFxGuids, w.fxIndex); - } - if (yielded) return result; // the resume cursor picks this item up next tick - - item.done = true; - // The record's FX half is released HERE and only here: the chain has now been - // driven to every state the record captured, so the record is spent. Releasing - // it any earlier destroys the only account of which plugins the USER had - // offline before the park, and the next online blanket-enables them. - if (item.work.kind == view::FxWorkKind::Online) host.releaseFx(item.work.trackGuid); + if (host.nowMs() - start >= budget) break; // between tracks only } result.settled = queueSettled(items); diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index bb91e62..6577d02 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -1,8 +1,8 @@ // Standalone tests for the FX reclaimer's drain — no REAPER, no test framework. The // loop is templated on its host precisely so the four properties that make it safe to // run in the background are assertable here: the debounce, the per-tick budget's -// yield-and-resume, the queue being a cache of intent that a model change discards, and -// the user's own offline plugins surviving an online. +// between-track yield, the queue being a cache of intent that a model change discards, +// and the user's own offline plugins surviving an online. #include "../src/shell/view/fx_reclaim.h" @@ -95,16 +95,23 @@ std::vector offlineChain(const std::vector& guids) { return chain; } -// A Design leaf the tool has parked: record held, both halves, nothing user-offlined. -ViewModeModel parkedDesignLeaf(const std::vector& fxGuids) { +// Design leaves the tool has parked: record held, both halves, nothing user-offlined. +// Active mode stays Arrange, so every one of them is parked. +ViewModeModel parkedDesignLeaves(const std::map>& tracks) { ViewModeModel vm; - vm.membership().tag("{T}", kDesignModeId); // active mode is Arrange ⇒ {T} is parked - std::vector fx; - for (const std::string& g : fxGuids) fx.push_back(FxOfflineState{g, 0}); - vm.storeSnapshot("{T}", recordWithFx(fx)); + for (const auto& [guid, fxGuids] : tracks) { + vm.membership().tag(guid, kDesignModeId); + std::vector fx; + for (const std::string& g : fxGuids) fx.push_back(FxOfflineState{g, 0}); + vm.storeSnapshot(guid, recordWithFx(fx)); + } return vm; } +ViewModeModel parkedDesignLeaf(const std::vector& fxGuids) { + return parkedDesignLeaves({{"{T}", fxGuids}}); +} + // Puts the queue's offline items past their debounce the way the live loop does: the // hold starts at the FIRST observation, so advancing the clock before that one just // moves the origin and matures nothing. @@ -168,48 +175,72 @@ static void testFastFlipMovesZeroPluginsButAHeldParkStillReclaims() { CHECK(host2.released.empty()); // an OFFLINE never releases — the record is still owed } -// -- 2. budget: yield and resume ------------------------------------------------ +// -- 2. budget: the step is a whole track ---------------------------------------- -static void testOfflineDrainYieldsOnBudgetAndResumesWhereItStopped() { - ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); +static void testAWholeChainMovesInOneStepAndTheYieldFallsBetweenTracks() { + // {T1}'s chain costs 4x the offline budget on its own. The budget cannot split it — + // a step is one track — so the tick overruns to finish it and yields only once the + // NEXT track would start. Slot 1 carries no identity, which the drain no longer has + // to reason about at all now that nothing resumes mid-chain. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}", "", "{FX-C}", "{FX-D}"}}, + {"{T2}", {"{FX-E}"}}, + {"{T3}", {"{FX-F}"}}}); FakeHost host; - host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); - host.stepCostMs = 50.0; // dearer than kOfflineBudgetMs ⇒ exactly one plugin per tick + host.chains["{T1}"] = onlineChain({"{FX-A}", "", "{FX-C}", "{FX-D}"}); + host.chains["{T2}"] = onlineChain({"{FX-E}"}); + host.chains["{T3}"] = onlineChain({"{FX-F}"}); + host.stepCostMs = 50.0; // dearer than kOfflineBudgetMs ⇒ one TRACK per tick ParkClock clock; - std::vector items = derive(vm, {"{T}"}); + std::vector items = derive(vm, {"{T1}", "{T2}", "{T3}"}); + CHECK(items.size() == 3); matureHold(items, clock, host); - CHECK(drainTick(items, clock, host).writes == 1); - CHECK(items[0].work.resumeAfterSlot == 0); - CHECK(items[0].work.resumeAfterFxGuid == "{FX-A}"); - CHECK(!items[0].done); - CHECK(drainTick(items, clock, host).writes == 1); - CHECK(items[0].work.resumeAfterSlot == 1); + CHECK(drainTick(items, clock, host).writes == 4); // all of {T1}, 200ms over a 30ms budget + CHECK(items[0].done && !items[1].done && !items[2].done); + const std::vector firstTick{"{T1}:0=off", "{T1}:1=off", "{T1}:2=off", + "{T1}:3=off"}; + CHECK(host.writeLog == firstTick); - // Slots 2 and 3, one per tick; the tick that writes the last one also exhausts the - // item's write list, so it settles in the same pass rather than costing a spare tick. + // {T2} then {T3}, one track per tick; the tick that finishes the last one exhausts + // the queue in the same pass rather than costing a spare tick. CHECK(drainToSettled(items, clock, host) == 2); - - // Each slot moved exactly once and in order — no slot re-walked across a yield. - const std::vector expected{"{T}:0=off", "{T}:1=off", "{T}:2=off", "{T}:3=off"}; + std::vector expected = firstTick; + expected.push_back("{T2}:0=off"); + expected.push_back("{T3}:0=off"); CHECK(host.writeLog == expected); } static void testOnlineDrainGetsTheLargerBudget() { - // Same per-plugin cost, same chain length: the only difference is the direction, and + // Same per-plugin cost, same track count, one plugin each so neither direction is + // distorted by an overrunning chain: the only difference is the direction, and // onlining is the one the user is waiting on. - ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); - vm.setActiveMode(kDesignModeId); - vm.releaseSnapshotFlags("{T}"); - FakeHost host; - host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); - host.stepCostMs = 50.0; - ParkClock clock; + const std::map> tracks{ + {"{T1}", {"{FX-1}"}}, {"{T2}", {"{FX-2}"}}, {"{T3}", {"{FX-3}"}}, {"{T4}", {"{FX-4}"}}}; - std::vector items = derive(vm, {"{T}"}); - CHECK(items.size() == 1 && items[0].work.kind == FxWorkKind::Online); - CHECK(drainTick(items, clock, host).writes == 3); // 120ms budget vs 30ms + ViewModeModel back = parkedDesignLeaves(tracks); + back.setActiveMode(kDesignModeId); + FakeHost onlineHost; + for (const auto& [guid, fx] : tracks) { + back.releaseSnapshotFlags(guid); + onlineHost.chains[guid] = offlineChain(fx); + } + onlineHost.stepCostMs = 50.0; + ParkClock onlineClock; + + std::vector onlineItems = derive(back, {"{T1}", "{T2}", "{T3}", "{T4}"}); + CHECK(onlineItems.size() == 4 && onlineItems[0].work.kind == FxWorkKind::Online); + CHECK(drainTick(onlineItems, onlineClock, onlineHost).writes == 3); // 120ms buys three + + ViewModeModel parked = parkedDesignLeaves(tracks); + FakeHost offlineHost; + for (const auto& [guid, fx] : tracks) offlineHost.chains[guid] = onlineChain(fx); + offlineHost.stepCostMs = 50.0; + ParkClock offlineClock; + + std::vector offlineItems = derive(parked, {"{T1}", "{T2}", "{T3}", "{T4}"}); + matureHold(offlineItems, offlineClock, offlineHost); + CHECK(drainTick(offlineItems, offlineClock, offlineHost).writes == 1); // 30ms buys one } static void testATickStartedOnOnlineWorkDoesNotSpendIntoOfflineWork() { @@ -254,21 +285,19 @@ static void testModelChangeDiscardsDrainProgressButNotTheParkHold() { std::vector items = derive(vm, {"{T}", "{U}"}); drainTick(items, clock, host); // observes both parked at t=0 host.clock = kParkHoldMs + 100.0; - drainTick(items, clock, host); // one plugin moves; {T} now carries a cursor - CHECK(items[0].work.resumeAfterSlot >= 0); + drainTick(items, clock, host); // {T}'s whole chain moves, then the tick yields + CHECK(items[0].done && !items[1].done); // The model changes: {T} is retagged into the active mode and restored, while {U} // stays exactly as parked as it was. The queue is discarded whole — direction, - // cursors, completion marks and all — and re-derived. + // completion marks and all — and re-derived. vm.membership().tag("{T}", kArrangeModeId); vm.releaseSnapshotFlags("{T}"); items = derive(vm, {"{T}", "{U}"}); CHECK(items.size() == 2); CHECK(items[0].work.kind == FxWorkKind::Online); CHECK(items[0].work.trackGuid == "{T}"); - CHECK(items[0].work.resumeAfterSlot == -1); // no drain progress survives a re-derive - CHECK(items[0].work.resumeAfterFxGuid.empty()); - CHECK(!items[0].done); + CHECK(!items[0].done && !items[1].done); // no drain progress survives a re-derive // ...but {U}'s hold does survive it. {U} never stopped being parked, and restarting // its 3s on every unrelated model change would defer its reclaim indefinitely. @@ -313,34 +342,11 @@ static void testAPluginTheUserOfflinedIsNeverOnlinedByTheDrain() { CHECK(host.released.count("{T}") == 1); } -// -- forward progress: the cursor cases that decide termination ----------------- +// -- forward progress ----------------------------------------------------------- -static void testResumeCursorAdvancesPastAnFxWithNoIdentity() { - // The case Wave 1's slot cursor exists for. Slot 1 is an FX REAPER reports no GUID - // for; with the identity alone the cursor would read as "nothing done yet" and the - // item would re-walk from slot 0 on every tick. - ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}"}); - FakeHost host; - host.chains["{T}"] = onlineChain({"{FX-A}", "", "{FX-C}"}); - host.stepCostMs = 50.0; // one plugin per tick on the offline budget - ParkClock clock; - - std::vector items = derive(vm, {"{T}"}); - matureHold(items, clock, host); - drainTick(items, clock, host); - drainTick(items, clock, host); - CHECK(items[0].work.resumeAfterFxGuid.empty()); // slot 1 has no identity to record - CHECK(items[0].work.resumeAfterSlot == 1); // ...so the slot is what carries it - - CHECK(drainToSettled(items, clock, host) == 1); // slot 2, and the item is exhausted - const std::vector expected{"{T}:0=off", "{T}:1=off", "{T}:2=off"}; - CHECK(host.writeLog == expected); -} - -static void testSlotKeyedOnlineRecordCarriesItsCursorAsASlot() { +static void testSlotKeyedOnlineRecordRestoresByPosition() { // A record lifted from a project saved before FX identity existed: its targets hold - // no identity, so a cursor taken from the LIVE identity would match none of them and - // the item would re-walk its whole chain after every yield. + // no identity at all, so position is the only thing that can address them. ViewModeModel vm; vm.membership().tag("{T}", kDesignModeId); vm.setActiveMode(kDesignModeId); @@ -351,16 +357,14 @@ static void testSlotKeyedOnlineRecordCarriesItsCursorAsASlot() { FakeHost host; host.chains["{T}"] = offlineChain({"{FX-X}", "{FX-Y}", "{FX-Z}"}); - host.stepCostMs = kOnlineBudgetMs + 10.0; // one plugin per tick, then yield + // Dearer per plugin than the whole online budget: the step still runs the chain to + // the end, because the budget cannot split one track's move. + host.stepCostMs = kOnlineBudgetMs + 10.0; ParkClock clock; std::vector items = derive(vm, {"{T}"}); - CHECK(items.size() == 1 && resumeBySlotOnly(items[0].work)); - drainTick(items, clock, host); - CHECK(items[0].work.resumeAfterFxGuid.empty()); // NOT "{FX-X}", which matches no target - CHECK(items[0].work.resumeAfterSlot == 0); - - drainToSettled(items, clock, host); + CHECK(items.size() == 1); + CHECK(drainToSettled(items, clock, host) == 1); const std::vector expected{"{T}:0=on", "{T}:2=on"}; CHECK(host.writeLog == expected); // slot 1 stays offline, as the record captured CHECK(host.offlineAt("{T}", 1)); @@ -381,7 +385,7 @@ static void testATrackThatLeftTheProjectCompletesRatherThanStalls() { int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); - testOfflineDrainYieldsOnBudgetAndResumesWhereItStopped(); + testAWholeChainMovesInOneStepAndTheYieldFallsBetweenTracks(); testOnlineDrainGetsTheLargerBudget(); testATickStartedOnOnlineWorkDoesNotSpendIntoOfflineWork(); @@ -389,8 +393,7 @@ int main() { testAPluginTheUserOfflinedIsNeverOnlinedByTheDrain(); - testResumeCursorAdvancesPastAnFxWithNoIdentity(); - testSlotKeyedOnlineRecordCarriesItsCursorAsASlot(); + testSlotKeyedOnlineRecordRestoresByPosition(); testATrackThatLeftTheProjectCompletesRatherThanStalls(); if (g_fail == 0) std::printf("All tests passed.\n"); diff --git a/tests/test_fx_state.cpp b/tests/test_fx_state.cpp index c20dadc..0ce5b1b 100644 --- a/tests/test_fx_state.cpp +++ b/tests/test_fx_state.cpp @@ -240,74 +240,6 @@ static void testSlotKeyedRecordStillRestoresByPosition() { CHECK(writeAt(res, 2, false)); } -// -- the resume point --------------------------------------------------------- - -static void testResumePointSkipsWhatTheDrainAlreadyDid() { - ViewModeModel vm = parkedWithThreeFx(); - CHECK(vm.setActiveMode(kDesignModeId)); - std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); - CHECK(work.size() == 1); - if (work.empty()) return; - - work[0].resumeAfterFxGuid = "{FX-A}"; - const FxRestoreResolution res = resolveFxWork(work[0], {"{FX-A}", "{FX-B}", "{FX-C}"}); - CHECK(res.writes.size() == 2); - CHECK(!anyWriteAt(res, 0)); // A is done - CHECK(writeAt(res, 1, true)); - CHECK(writeAt(res, 2, false)); - - // Resuming after the LAST identity leaves nothing owed — the item is finished. - work[0].resumeAfterFxGuid = "{FX-C}"; - CHECK(resolveFxWork(work[0], {"{FX-A}", "{FX-B}", "{FX-C}"}).writes.empty()); -} - -static void testResumePointThatLeftTheChainRestartsRatherThanStrands() { - ViewModeModel vm = parkedWithThreeFx(); - CHECK(vm.setActiveMode(kDesignModeId)); - std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); - CHECK(work.size() == 1); - if (work.empty()) return; - - // The drain's cursor named a plugin the user has since deleted. Every write is - // compare-before-write, so re-walking the item is cheap and correct; honoring a - // cursor that matches nothing would strand the rest of the chain. - work[0].resumeAfterFxGuid = "{FX-GONE}"; - const FxRestoreResolution res = resolveFxWork(work[0], {"{FX-A}", "{FX-B}", "{FX-C}"}); - CHECK(res.writes.size() == 3); - CHECK(writeAt(res, 1, true)); - - // Same rule on the offline side, where the cursor keys into the live chain. - ViewModeModel parked = parkedWithThreeFx(); // Arrange active ⇒ {T} parks - std::vector offlineWork = desiredFxState(parked, kArrangeModeId, {"{T}"}); - CHECK(offlineWork.size() == 1); - if (offlineWork.empty()) return; - offlineWork[0].resumeAfterFxGuid = "{FX-A}"; - CHECK(resolveFxWork(offlineWork[0], {"{FX-A}", "{FX-B}", "{FX-C}"}).writes.size() == 2); - offlineWork[0].resumeAfterFxGuid = "{FX-GONE}"; - CHECK(resolveFxWork(offlineWork[0], {"{FX-A}", "{FX-B}", "{FX-C}"}).writes.size() == 3); -} - -static void testResumeAfterUnidentifiedFxMakesForwardProgress() { - // The drain completed slot 1, an FX REAPER reports no GUID for. Before the - // slot-cursor fix, an empty resumeAfterFxGuid was indistinguishable from - // "nothing done yet" and this item would restart at slot 0 every tick, - // forever — the non-terminating drain this test pins against. - ViewModeModel parked = parkedWithThreeFx(); // Arrange active ⇒ {T} parks (Offline, slot-keyed) - std::vector work = desiredFxState(parked, kArrangeModeId, {"{T}"}); - CHECK(work.size() == 1); - if (work.empty()) return; - - work[0].resumeAfterFxGuid = ""; - work[0].resumeAfterSlot = 1; - const FxRestoreResolution res = - resolveFxWork(work[0], {"{FX-A}", "", "{FX-B}", "{FX-C}"}); - CHECK(res.writes.size() == 2); // slots 2 and 3 only — 0 and 1 already done - CHECK(!anyWriteAt(res, 0)); - CHECK(!anyWriteAt(res, 1)); - CHECK(anyWriteAt(res, 2)); - CHECK(anyWriteAt(res, 3)); -} - int main() { testActiveTrackIsOwedOnlineInactiveIsOwedOffline(); testOnlineWorkSortsBeforeOfflineWork(); @@ -321,10 +253,6 @@ int main() { testAddedFxIsLeftAlone(); testSlotKeyedRecordStillRestoresByPosition(); - testResumePointSkipsWhatTheDrainAlreadyDid(); - testResumePointThatLeftTheChainRestartsRatherThanStrands(); - testResumeAfterUnidentifiedFxMakesForwardProgress(); - if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } From 5dcfd3a53f9af8da0b00e22a87bfdd345d1501c0 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 21:08:34 -0400 Subject: [PATCH 3/9] Reclaim drain owns its work: invalidation only bumps a generation, and a generation that moved aborts the tick's commit and its release. --- src/app/main.cpp | 16 +- src/shell/actions/design_view_actions.cpp | 5 +- src/shell/view/fx_reclaim.cpp | 34 ++-- src/shell/view/fx_reclaim.h | 124 +++++++++++++-- src/shell/view/view.cpp | 28 +++- src/shell/view/view.h | 3 + tests/test_fx_reclaim.cpp | 179 ++++++++++++++++++---- 7 files changed, 321 insertions(+), 68 deletions(-) diff --git a/src/app/main.cpp b/src/app/main.cpp index e70606f..4817593 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -189,7 +189,10 @@ static void OnTimer() // this timer mid-apply), same retry-next-tick shape as the consume guard below. // Deferred WHOLE, not just the reload branches: poll()'s identity tracking is // cheap to skip for one tick and resumes correctly, unlatched, on the next. - if (!reasampler::modeApplyInProgress()) g_session.poll(); + // The reclaim drain is latched for the same reason and is the same window: it + // holds that view model for the length of a plugin unload, which pumps. + if (!reasampler::modeApplyInProgress() && !reasampler::fxReclaimDrainInProgress()) + g_session.poll(); // persist stays MODEL-ONLY (loads the saved view model but does not apply // visibility, to avoid coupling persist to the view shell); poll() raises a @@ -198,11 +201,12 @@ static void OnTimer() // signal re-arms the bank panel's new-content detector — notified BEFORE the // reapply so re-arm and model restore ride the one load event (otherwise // pre-existing tracks can be mis-detected as "new" and mass-tagged). - // modeApplyInProgress is tested BEFORE the consume, not after the applyMode call: - // the load signal is one-shot, a re-entered applyMode fails closed, and a reapply - // dropped here is the saved mode never applied at all. Holding the whole block - // back one tick also keeps the panel re-arm ahead of the reapply. - if (!reasampler::modeApplyInProgress() && g_session.consumeLoadSignal()) { + // Both latches are tested BEFORE the consume, not after the applyMode call: the load + // signal is one-shot, a re-entered applyMode fails closed, and a reapply dropped here + // is the saved mode never applied at all. Holding the whole block back one tick also + // keeps the panel re-arm ahead of the reapply. + if (!reasampler::modeApplyInProgress() && !reasampler::fxReclaimDrainInProgress() && + g_session.consumeLoadSignal()) { reasampler::bankPanelNotifyProjectLoaded(); // Reconcile lane ownership against the live project's lanes (P_LANENAME, // the cross-session source of truth) BEFORE reapplying visibility. Never diff --git a/src/shell/actions/design_view_actions.cpp b/src/shell/actions/design_view_actions.cpp index 8f7bdba..e83e5e1 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/src/shell/actions/design_view_actions.cpp @@ -104,7 +104,10 @@ std::vector selectedTrackGuids() { } // Reapplies the CURRENT active mode so a membership mutation takes visible effect -// immediately (park/unpark/re-derive parents). +// immediately (park/unpark/re-derive parents). The return is deliberately discarded: a +// refused reapply leaves the membership edit standing and unapplied, and the user's next +// edit or mode switch reapplies it. What must NOT be dropped with it is the reclaimer's +// queue, which is why applyMode invalidates on its refusal path too (view.cpp). void reapplyActiveMode() { applyMode(g_session->view(), g_session->view().activeModeId(), nullptr); } diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 3360ae3..27c1b55 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -32,14 +32,12 @@ namespace { // COMPARE-ONLY and never dereferenced beyond the ValidatePtr2 the tick already makes — // a tab switch has to be detectable without following a pointer that may name a closed // project. -std::vector g_items; -ReaProject* g_project = nullptr; -ParkClock g_clock; +FxReclaimQueue g_queue; +ReaProject* g_project = nullptr; // TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter -// the timer and land back here mid-drain. Fail closed: a re-entered drain would walk the -// same queue the outer one holds a reference into, re-running the very item that outer -// step is still writing. +// the timer and land back here mid-drain. Fail closed: a re-entered drain would derive a +// second queue for the same project and race the outer one's writes track for track. bool g_draining = false; struct DrainLatch { DrainLatch() { g_draining = true; } @@ -112,13 +110,15 @@ struct ReclaimHost { return true; } + // 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); } }; // 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() { - g_items.clear(); + g_queue.discard(); g_project = nullptr; g_fxReclaimPending = false; } @@ -128,10 +128,15 @@ void goIdle() { bool g_fxReclaimPending = false; void invalidateFxReclaim() { - goIdle(); + // A generation bump and a flag, and deliberately nothing more. Discarding the queue + // here is what let a re-entered apply free the element the outer drain was writing + // through, and clear the very wakeup this call had just raised. + g_queue.invalidate(); g_fxReclaimPending = true; } +bool fxReclaimDrainInProgress() { return g_draining; } + void fxReclaimTick(ViewModeModel& model, ReaProject* proj) { if (g_draining) return; // The model is mid-rewrite and its records are being consumed out from under any @@ -148,20 +153,21 @@ void fxReclaimTick(ViewModeModel& model, ReaProject* proj) { ReaProject* const project = proj ? proj : EnumProjects(-1, nullptr, 0); if (!ValidatePtr2(nullptr, project, "ReaProject*") || (g_project && g_project != project)) { goIdle(); - g_clock.forget(); + g_queue.forgetHolds(); return; } // DERIVED, never persisted: recomputed from view state every time the queue was - // discarded, so a REAPER quit mid-drain simply re-derives and finishes on reopen. - if (!g_project) { + // invalidated, so a REAPER quit mid-drain simply re-derives and finishes on reopen. + if (g_queue.stale()) { g_project = project; - g_items = makeQueue( - view::desiredFxState(model, model.activeModeId(), liveTrackGuids(project))); + g_queue.adopt(makeQueue( + view::desiredFxState(model, model.activeModeId(), liveTrackGuids(project)))); } ReclaimHost host{project, &model}; - if (drainTick(g_items, g_clock, host).settled) goIdle(); + const ReclaimRunResult run = g_queue.run(host); + if (!run.aborted && run.settled) goIdle(); } } // namespace reasampler diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 2e5cfa3..a1db774 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -92,6 +92,9 @@ public: void forget() { since_.clear(); } + // The owner swaps the clock out for the duration of a drain — see FxReclaimQueue. + void swap(ParkClock& other) { since_.swap(other.since_); } + private: std::map since_; }; @@ -119,6 +122,12 @@ inline int nextEligible(const std::vector& items, const ParkClock& struct ReclaimTickResult { int writes = 0; bool settled = false; + + // Tracks whose park record the tick drove fully online, so the record's FX half is + // spent. REPORTED, not applied: releasing is a model write and the model is exactly + // what a re-entered load can replace mid-drain, so only the owner may commit it — + // see FxReclaimQueue::run. + std::vector released; }; // One tick of the convergence loop. Templated on `Host` so the ordering, budget and @@ -131,7 +140,14 @@ struct ReclaimTickResult { // bool writeOffline(guid, fxIndex, offline) compare-before-write inside its // own discarded undo block; // true iff it actually wrote -// void releaseFx(guid) release the record's FX half +// void releaseFx(guid) release the record's FX half; +// called by FxReclaimQueue::run +// after the drain has unwound, +// never from inside the drain +// +// `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 +// that true for the live loop. template ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, Host& host) { ReclaimTickResult result; @@ -166,11 +182,12 @@ ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, H for (const FxOfflineWrite& w : resolved.writes) if (host.writeOffline(item.work.trackGuid, w.fxIndex, w.offline)) ++result.writes; - // The record's FX half is released HERE and only here: the chain has now - // been driven to every state the record captured, so the record is spent. - // Releasing it any earlier destroys the only account of which plugins the - // USER had offline before the park, and the next online blanket-enables them. - if (item.work.kind == view::FxWorkKind::Online) host.releaseFx(item.work.trackGuid); + // 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 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) + result.released.push_back(item.work.trackGuid); } item.done = true; // both arms: a step is atomic, so no item is half-moved @@ -181,23 +198,104 @@ ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, H return result; } +// --- the queue's owner ---------------------------------------------------------- + +struct ReclaimRunResult { + int writes = 0; + bool settled = false; + + // Something invalidated the queue while the drain was on the stack. The work and + // 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; +}; + +// Owns the derived queue and the debounce clock across a tick that can be re-entered: +// TrackFX_SetOffline is ASSUMED to pump the message loop, so an invalidation can arrive +// with a drain half-way through a track. +// +// The two halves of that safety are structural, not guarded. `invalidate()` bumps a +// counter and touches NOTHING else, so there is nothing for it to clear under a drain. +// `run()` moves the queue and the clock into locals for the drain's whole duration, so +// the object holds nothing the drain is reading — and re-checks the counter before it +// commits either the surviving queue or a single release. +class FxReclaimQueue { +public: + void invalidate() { ++generation_; } + + // No queue at all, or one derived before the last invalidation. + bool stale() const { return !derived_ || derivedAt_ != generation_; } + + void adopt(std::vector items) { + items_ = std::move(items); + derived_ = true; + derivedAt_ = generation_; + } + + void discard() { + items_.clear(); + derived_ = false; + } + + void forgetHolds() { clock_.forget(); } + + const std::vector& items() const { return items_; } + + template + ReclaimRunResult run(Host& host) { + const unsigned long long entered = generation_; + + std::vector work; + ParkClock clock; + work.swap(items_); + clock.swap(clock_); + + const ReclaimTickResult tick = drainTick(work, clock, host); + + // The hold is keyed on the track and outlives any re-derivation, so it comes + // back whether or not the drain's own work survives. + clock_.swap(clock); + + if (generation_ != entered) return ReclaimRunResult{tick.writes, false, true}; + + 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}; + } + +private: + std::vector items_; + ParkClock clock_; + unsigned long long generation_ = 0; + unsigned long long derivedAt_ = 0; + bool derived_ = false; +}; + // --- shell entry points -------------------------------------------------------- // The ONE test the timer makes per tick, mirroring the realtime capture's single // pointer test. False means the queue is settled and there is nothing to converge. extern bool g_fxReclaimPending; -// Discards the derived queue. Called from every path that can change what the FX chains -// SHOULD be — which is applyMode's success path and nothing else, because every -// membership, snapshot and active-mode mutation reapplies through it. The debounce clock -// deliberately survives: it is keyed on the track, not on the queue. +// Marks the derived queue for re-derivation by the NEXT tick; discards nothing here, +// because this is reachable from inside a drain. Called from applyMode and nothing else +// — every membership, snapshot and active-mode mutation reapplies through it — on the +// refusal path as well as the success one (see view.cpp for why the refusal is the +// load-bearing half). The debounce clock deliberately survives: it is keyed on the +// track, not on the queue. void invalidateFxReclaim(); +// True while a drain is on the stack. Exposed so the timer's other work can stand off +// the model the drain is holding, the same way it stands off an applyMode. +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 (TrackFX_SetOffline is assumed to -// pump the message loop), or when the active project is not the one the queue was -// derived against. +// 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); } // namespace reasampler diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 42c9b66..685f48a 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -62,7 +62,9 @@ bool transportBlocksModeSwitch(ReaProject* proj) { bool modeApplyInProgress() { return g_applying; } -bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj) { +namespace { + +bool applyModeImpl(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj) { if (g_applying) return false; // see ApplyLatch; same fail-closed shape as the guards below ApplyLatch latch; @@ -203,12 +205,26 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject // an undo rolling the model back underneath it. noteAppliedState(project, model); - // Every membership, snapshot and active-mode mutation in the system reaches the - // project through this call, so this is the whole invalidation surface: what the FX - // chains SHOULD be has just changed, and the reclaimer's queue is a cache of that - // intent, never state. - invalidateFxReclaim(); return true; } +} // namespace + +bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj) { + const bool applied = applyModeImpl(model, targetModeId, proj); + + // Every membership, snapshot and active-mode mutation in the system reaches the + // project through this call, so this is the whole invalidation surface: the + // reclaimer's queue is a cache of what the FX chains SHOULD be, never state. + // + // Dropped on a REFUSAL too, and that is the load-bearing half. Callers mutate the + // model FIRST and reapply to make it visible (design_view_actions' tag/untag/ + // show-both, render_in_place, the load tick), and every one of them discards this + // return — so a refusal is exactly the case where the mutation stands and nothing + // else would drop a queue that now converges toward the mode the user just left. + // The cost when the refusal followed no mutation is one re-derivation on the timer. + invalidateFxReclaim(); + return applied; +} + } // namespace reasampler diff --git a/src/shell/view/view.h b/src/shell/view/view.h index 0efb4b5..d91e5c4 100644 --- a/src/shell/view/view.h +++ b/src/shell/view/view.h @@ -59,6 +59,9 @@ bool modeApplyInProgress(); // (modeApplyInProgress), or if the project does not validate. A reapply (target == // active) is never gated and never touches solo. `proj` == nullptr means the current // project, resolved ONCE and used for every call this apply makes. +// +// Either way — applied or refused — this drops the FX reclaimer's derived queue, which +// is why callers may safely discard the return after mutating the model. bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject* proj); } // namespace reasampler diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index 6577d02..9e9d040 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -1,8 +1,9 @@ // Standalone tests for the FX reclaimer's drain — no REAPER, no test framework. The -// loop is templated on its host precisely so the four 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, -// and the user's own offline plugins surviving an online. +// 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. #include "../src/shell/view/fx_reclaim.h" @@ -39,6 +40,13 @@ struct FakeHost { double clock = 0.0; double stepCostMs = 50.0; + // Models a re-entered timer landing inside TrackFX_SetOffline's message pump: on the + // Nth successful write, the write itself does to the queue what a nested applyMode + // used to do — clear it and mark it stale. + FxReclaimQueue* reentrant = nullptr; + int invalidateAfterWrite = -1; + int itemsSeenMidDrain = -1; // queue size as the re-entrant caller finds it + double nowMs() const { return clock; } bool liveChain(const std::string& guid, std::vector& out) { @@ -58,6 +66,12 @@ struct FakeHost { it->second[static_cast(fxIndex)].offline = offline; clock += stepCostMs; writeLog.push_back(guid + ":" + std::to_string(fxIndex) + (offline ? "=off" : "=on")); + + if (reentrant && static_cast(writeLog.size()) == invalidateAfterWrite) { + itemsSeenMidDrain = static_cast(reentrant->items().size()); + reentrant->discard(); + reentrant->invalidate(); + } return true; } @@ -121,15 +135,29 @@ void matureHold(std::vector& items, ParkClock& clock, FakeHost& hos } // Runs ticks until the queue settles, bounded so a non-terminating drain fails the test -// rather than hanging CTest. +// rather than hanging CTest. The two overloads are the two entry points: the pure drain +// over the caller's own storage, and the owned queue the shell actually runs. int drainToSettled(std::vector& items, ParkClock& clock, FakeHost& host, int maxTicks = 64) { for (int t = 1; t <= maxTicks; ++t) if (drainTick(items, clock, host).settled) return t; + CHECK(false); + return -1; +} + +int drainToSettled(FxReclaimQueue& queue, FakeHost& host, int maxTicks = 64) { + for (int t = 1; t <= maxTicks; ++t) + if (queue.run(host).settled) return t; CHECK(false); // did not settle — the drain is not making forward progress return -1; } +FxReclaimQueue queueOf(const ViewModeModel& vm, const std::set& live) { + FxReclaimQueue q; + q.adopt(derive(vm, live)); + return q; +} + } // namespace // -- 1. debounce --------------------------------------------------------------- @@ -139,21 +167,20 @@ static void testFastFlipMovesZeroPluginsButAHeldParkStillReclaims() { ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}"}); FakeHost host; host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}"}); - ParkClock clock; - std::vector items = derive(vm, {"{T}"}); - CHECK(items.size() == 1 && items[0].work.kind == FxWorkKind::Offline); - CHECK(drainTick(items, clock, host).writes == 0); - CHECK(!drainTick(items, clock, host).settled); // waiting out its hold, not finished + FxReclaimQueue q = queueOf(vm, {"{T}"}); + CHECK(q.items().size() == 1 && q.items()[0].work.kind == FxWorkKind::Offline); + CHECK(q.run(host).writes == 0); + CHECK(!q.run(host).settled); // waiting out its hold, not finished // B→A half a second later: the flags come back and the FX half is still held, so the // reclaimer is now owed an ONLINE — over a chain it never got round to offlining. host.clock = 500.0; vm.setActiveMode(kDesignModeId); vm.releaseSnapshotFlags("{T}"); - items = derive(vm, {"{T}"}); - CHECK(items.size() == 1 && items[0].work.kind == FxWorkKind::Online); - drainToSettled(items, clock, host); + q.adopt(derive(vm, {"{T}"})); + CHECK(q.items().size() == 1 && q.items()[0].work.kind == FxWorkKind::Online); + drainToSettled(q, host); // The whole point of the 3s hold: a fast A→B→A flip moved NO plugins. CHECK(host.writeLog.empty()); @@ -165,11 +192,10 @@ static void testFastFlipMovesZeroPluginsButAHeldParkStillReclaims() { ViewModeModel held = parkedDesignLeaf({"{FX-A}", "{FX-B}", "{FX-C}"}); FakeHost host2; host2.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}"}); - ParkClock clock2; - std::vector items2 = derive(held, {"{T}"}); - CHECK(drainTick(items2, clock2, host2).writes == 0); // observed parked at t=0 + FxReclaimQueue q2 = queueOf(held, {"{T}"}); + CHECK(q2.run(host2).writes == 0); // observed parked at t=0 host2.clock = kParkHoldMs; - drainToSettled(items2, clock2, host2); + drainToSettled(q2, host2); CHECK(host2.writeLog.size() == 3); CHECK(host2.offlineAt("{T}", 0) && host2.offlineAt("{T}", 1) && host2.offlineAt("{T}", 2)); CHECK(host2.released.empty()); // an OFFLINE never releases — the record is still owed @@ -329,10 +355,9 @@ static void testAPluginTheUserOfflinedIsNeverOnlinedByTheDrain() { FakeHost host; host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}", "{FX-C}"}); // fully parked - ParkClock clock; - std::vector items = derive(vm, {"{T}"}); - drainToSettled(items, clock, host); + FxReclaimQueue q = queueOf(vm, {"{T}"}); + drainToSettled(q, host); CHECK(!host.offlineAt("{T}", 0)); CHECK(host.offlineAt("{T}", 1)); // still offline — never blanket-onlined @@ -360,11 +385,10 @@ static void testSlotKeyedOnlineRecordRestoresByPosition() { // Dearer per plugin than the whole online budget: the step still runs the chain to // the end, because the budget cannot split one track's move. host.stepCostMs = kOnlineBudgetMs + 10.0; - ParkClock clock; - std::vector items = derive(vm, {"{T}"}); - CHECK(items.size() == 1); - CHECK(drainToSettled(items, clock, host) == 1); + FxReclaimQueue q = queueOf(vm, {"{T}"}); + CHECK(q.items().size() == 1); + CHECK(drainToSettled(q, host) == 1); const std::vector expected{"{T}:0=on", "{T}:2=on"}; CHECK(host.writeLog == expected); // slot 1 stays offline, as the record captured CHECK(host.offlineAt("{T}", 1)); @@ -373,15 +397,110 @@ static void testSlotKeyedOnlineRecordRestoresByPosition() { static void testATrackThatLeftTheProjectCompletesRatherThanStalls() { ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); FakeHost host; // no chain registered ⇒ liveChain reports the track gone - ParkClock clock; - std::vector items = derive(vm, {"{T}"}); - matureHold(items, clock, host); - CHECK(drainTick(items, clock, host).settled); + FxReclaimQueue q = queueOf(vm, {"{T}"}); + CHECK(!q.run(host).settled); // observes the hold at t=0; not matured yet + host.clock += kParkHoldMs; + CHECK(q.run(host).settled); CHECK(host.writeLog.empty()); CHECK(host.released.empty()); // the next apply's reconcile drops the whole record } +// -- 5. re-entrancy: an invalidation arriving with a drain on the stack ------------ + +static void testAReentrantInvalidationReachesNothingTheDrainIsUsing() { + // The second of {T1}'s four writes re-enters and does what a nested applyMode used to + // do to the shared queue: clear it. The drain is mid-item at that moment. + ViewModeModel vm = parkedDesignLeaves({{"{T1}", {"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}}, + {"{T2}", {"{FX-E}"}}}); + FakeHost host; + host.chains["{T1}"] = onlineChain({"{FX-A}", "{FX-B}", "{FX-C}", "{FX-D}"}); + host.chains["{T2}"] = onlineChain({"{FX-E}"}); + host.stepCostMs = 50.0; + + FxReclaimQueue q = queueOf(vm, {"{T1}", "{T2}"}); + q.run(host); // observes both parked at t=0 + host.clock += kParkHoldMs; + + host.reentrant = &q; + host.invalidateAfterWrite = 2; + const ReclaimRunResult r = q.run(host); + + // The ownership claim: for the whole length of the drain the queue object holds + // nothing, so there is no element for a re-entrant caller to destroy. + CHECK(host.itemsSeenMidDrain == 0); + + // The drain finished the track it was on out of its own copy — a step is atomic. + const std::vector whole{"{T1}:0=off", "{T1}:1=off", "{T1}:2=off", + "{T1}:3=off"}; + CHECK(host.writeLog == whole); + CHECK(r.writes == 4); + + // ...and then refused to commit work derived from a model that has since moved. + CHECK(r.aborted); + CHECK(!r.settled); + CHECK(q.stale()); + CHECK(q.items().empty()); +} + +static void testAnInvalidationDuringTheLastItemLeavesTheTickUnsettled() { + // One item, so the drain would otherwise report settled and the shell would go idle — + // clearing the pending flag the re-entrant invalidation had just raised and stranding + // its work until the next mode switch. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + q.run(host); + host.clock += kParkHoldMs; + + host.reentrant = &q; + host.invalidateAfterWrite = 1; + const ReclaimRunResult r = q.run(host); + CHECK(r.writes == 1); + CHECK(!r.settled); // the one bit that decides whether the shell clears its wakeup + CHECK(r.aborted); + CHECK(q.stale()); // and the next tick re-derives rather than finding an empty queue + + // The contrast, so the assertion above is about the invalidation and not about this + // queue being unable to settle: the same drain settles once nothing re-enters it. + host.reentrant = nullptr; + q.adopt(derive(vm, {"{T}"})); + CHECK(q.run(host).settled); +} + +static void testAnInvalidationDuringAnOnlineDropsThatTicksRelease() { + // Releasing spends the record's FX half — the only account of which plugins the USER + // had offline before the park. An invalidation mid-drain is the signal that the model + // underneath may have been replaced (a project load replaces it in place), so the + // release must not land on it. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + CHECK(q.items().size() == 1 && q.items()[0].work.kind == FxWorkKind::Online); + host.reentrant = &q; + host.invalidateAfterWrite = 1; + + const ReclaimRunResult r = q.run(host); + 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 + + // Re-owed, not lost: the re-derived item finds the chain already where the record + // wants it, writes nothing more, and releases then. + host.reentrant = nullptr; + q.adopt(derive(vm, {"{T}"})); + CHECK(q.run(host).settled); + CHECK(host.writeLog.size() == 2); + CHECK(host.released.count("{T}") == 1); +} + int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); @@ -396,6 +515,10 @@ int main() { testSlotKeyedOnlineRecordRestoresByPosition(); testATrackThatLeftTheProjectCompletesRatherThanStalls(); + testAReentrantInvalidationReachesNothingTheDrainIsUsing(); + testAnInvalidationDuringTheLastItemLeavesTheTickUnsettled(); + testAnInvalidationDuringAnOnlineDropsThatTicksRelease(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } From 7687ea76fcf033e4c70353f4010bd201916d23b2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 21:26:55 -0400 Subject: [PATCH 4/9] 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; } From 35fdfca805fc31ade146db598dfc7f395fda986c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 21:36:08 -0400 Subject: [PATCH 5/9] Harden fx_reclaim against project closure mid-drain; cache resolved track per item; correct two stale budget comments --- src/shell/view/fx_reclaim.cpp | 45 ++++++++++++++++++++++++++++------- src/shell/view/fx_reclaim.h | 19 +++++++++------ 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index b575c11..6d116e8 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -48,10 +48,15 @@ struct DrainLatch { DrainLatch& operator=(const DrainLatch&) = delete; }; -// Resolved fresh from the GUID on every step 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. +// 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. 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) @@ -72,6 +77,24 @@ struct ReclaimHost { ReaProject* project = nullptr; ReaSamplerSession* session = nullptr; + // One item's liveChain call plus every writeOffline call it drives all name the + // SAME track, back to back, before drainTick can move to the next item — so the + // full O(T) resolveFresh scan is owed once per item, not once per call. Every + // call after the first re-validates the SAME pointer instead: cheap, and still + // catches a track (or its whole project) that a prior write's pump took down, + // which the scan-per-call version caught too, just at O(T) instead of O(1). + std::string cachedGuid_; + MediaTrack* cachedTrack_ = nullptr; + + MediaTrack* resolveForStep(const std::string& guid) { + if (guid == cachedGuid_ && cachedTrack_ && + ValidatePtr2(nullptr, cachedTrack_, "MediaTrack*")) + return cachedTrack_; + cachedTrack_ = resolveFresh(project, guid); + cachedGuid_ = guid; + return cachedTrack_; + } + // Monotonic and sub-millisecond: the budget below discriminates at 30ms, which // GetTickCount's ~16ms granularity cannot do. No wraparound to guard against, so // util::elapsedAtLeast (whose whole reason for existing is that wrap) does not apply. @@ -82,7 +105,7 @@ struct ReclaimHost { } bool liveChain(const std::string& guid, std::vector& out) { - MediaTrack* tr = resolveFresh(project, guid); + MediaTrack* tr = resolveForStep(guid); if (!tr) return false; const int count = TrackFX_GetCount(tr); out.clear(); @@ -94,7 +117,7 @@ struct ReclaimHost { } bool writeOffline(const std::string& guid, int fxIndex, bool offline) { - MediaTrack* tr = resolveFresh(project, guid); + MediaTrack* tr = resolveForStep(guid); if (!tr) return false; if (fxIndex < 0 || fxIndex >= TrackFX_GetCount(tr)) return false; // chain shrank if (TrackFX_GetOffline(tr, fxIndex) == offline) return false; // already there @@ -177,10 +200,14 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { if (modeApplyInProgress()) return; DrainLatch latch; - // Both arms below are "the project this queue belongs to is no longer the one in - // front of us": no active project at all, or a tab switch / close. Drop the queue - // rather than write another project's chains; returning to it re-derives, because - // poll() reads that as a project load and the load glue reapplies, which invalidates. + // Belt-and-braces, not the tab-switch mechanism: OnTimer runs session.poll() before + // this tick, and poll() already classifies a tab switch as a Load whose reapply + // invalidates the queue — so by the time this test runs, a switch has normally + // already been handled and g_project already matches. This still catches it when + // poll() was itself skipped this tick (modeApplyInProgress/drain-in-progress + // deferral) and the two go stale together. Both arms below mean the same thing — + // no active project at all, or a project this queue no longer belongs to — so drop + // the queue rather than write another project's chains; returning to it re-derives. // The clock goes too — a hold measured in a project we stopped watching is not // evidence about the one we come back to. ReaProject* const project = proj ? proj : EnumProjects(-1, nullptr, 0); diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 3e571f9..f898e5d 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -31,13 +31,18 @@ inline constexpr double kParkHoldMs = 3000.0; // chain and cannot be split, so a budget decides only how many TRACKS a tick moves -- // and a single track with a long chain overruns it by as much as that chain costs. // -// ONLINE 120ms: onlining restores state the user owns and is waiting on, so a tick -// buys at least one typical load and two cheap ones. OFFLINE 30ms: under one typical -// unload, so opportunistic reclaim moves exactly ONE track per tick and yields -- -// background work must not make the UI feel worse than the switch already did. The -// budget is checked only AFTER a step, never before the first and never mid-step, so -// a track dearer than the whole budget still runs; that is what makes forward -// progress unconditional. +// ONLINE 120ms: onlining restores state the user owns and is waiting on. The check +// runs only AFTER a step, so against the cited 105ms typical load it admits TWO +// typical tracks per tick (105 < 120, so a second runs; 210 >= 120 stops a third), +// not one -- worst case a tick runs over budget by up to one track's whole cost. +// OFFLINE 30ms: background work must not make the UI feel worse than the switch +// already did. At the cited 36-42ms costs one track alone already clears the +// budget, so opportunistic reclaim moves ONE track and yields -- but that count is a +// property of those costs, not the budget: a cheap chain (a stock JS at ~1ms) stays +// under 30ms for many tracks in a row and all of them move in one tick. Checked only +// AFTER a step either way, never before the first and never mid-step, so a track +// dearer than the whole budget still runs; that is what makes forward progress +// unconditional. inline constexpr double kOnlineBudgetMs = 120.0; inline constexpr double kOfflineBudgetMs = 30.0; From 224dcf84df4cbc4be0b565892667ee56adb4c1de Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 22:16:41 -0400 Subject: [PATCH 6/9] Fix the reclaimer stranding a tab-switched-into project: lift the pin and wakeup into FxReclaimQueue, retarget instead of going idle Also validate the cached track in its project, and stop a drop-free tick resetting the report memo. --- src/app/main.cpp | 4 +- src/shell/view/fx_reclaim.cpp | 48 ++++------- src/shell/view/fx_reclaim.h | 92 ++++++++++++++++----- tests/test_fx_reclaim.cpp | 146 +++++++++++++++++++++++++++++++++- 4 files changed, 234 insertions(+), 56 deletions(-) diff --git a/src/app/main.cpp b/src/app/main.cpp index f7cb606..4d9457d 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -222,11 +222,11 @@ static void OnTimer() // Converge the FX chains toward what the view state says they should be — the // 1897ms of TrackFX_SetOffline a mode switch no longer pays synchronously. Same - // idle discipline as the realtime fast path above: a SINGLE BOOL TEST when there is + // idle discipline as the realtime fast path above: ONE bool read when there is // nothing to converge. Ordered AFTER the load block deliberately — a reload replaces // 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) + if (reasampler::fxReclaimPending()) 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 6d116e8..091cffd 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -30,12 +30,9 @@ namespace reasampler { namespace { -// The derived queue and the project it was derived against. The pointer is -// COMPARE-ONLY and never dereferenced beyond the ValidatePtr2 the tick already makes — -// a tab switch has to be detectable without following a pointer that may name a closed -// project. +// The derived queue, the project it was derived against, and the timer's wakeup — see +// FxReclaimQueue, which owns all three. FxReclaimQueue g_queue; -ReaProject* g_project = nullptr; // TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter // the timer and land back here mid-drain. Fail closed: a re-entered drain would derive a @@ -86,9 +83,11 @@ 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. MediaTrack* resolveForStep(const std::string& guid) { if (guid == cachedGuid_ && cachedTrack_ && - ValidatePtr2(nullptr, cachedTrack_, "MediaTrack*")) + ValidatePtr2(project, cachedTrack_, "MediaTrack*")) return cachedTrack_; cachedTrack_ = resolveFresh(project, guid); cachedGuid_ = guid; @@ -171,24 +170,15 @@ void reportDrops(ReaProject* proj, const std::vector& drops) { 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() { - g_queue.discard(); - g_project = nullptr; - g_fxReclaimPending = false; -} - } // namespace -bool g_fxReclaimPending = false; +bool fxReclaimPending() { return g_queue.pending(); } void invalidateFxReclaim() { // A generation bump and a flag, and deliberately nothing more. Discarding the queue // here is what let a re-entered apply free the element the outer drain was writing // through, and clear the very wakeup this call had just raised. g_queue.invalidate(); - g_fxReclaimPending = true; } bool fxReclaimDrainInProgress() { return g_draining; } @@ -200,27 +190,17 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { if (modeApplyInProgress()) return; DrainLatch latch; - // Belt-and-braces, not the tab-switch mechanism: OnTimer runs session.poll() before - // this tick, and poll() already classifies a tab switch as a Load whose reapply - // invalidates the queue — so by the time this test runs, a switch has normally - // already been handled and g_project already matches. This still catches it when - // poll() was itself skipped this tick (modeApplyInProgress/drain-in-progress - // deferral) and the two go stale together. Both arms below mean the same thing — - // no active project at all, or a project this queue no longer belongs to — so drop - // the queue rather than write another project's chains; returning to it re-derives. - // The clock goes too — a hold measured in a project we stopped watching is not - // evidence about the one we come back to. - ReaProject* const project = proj ? proj : EnumProjects(-1, nullptr, 0); - if (!ValidatePtr2(nullptr, project, "ReaProject*") || (g_project && g_project != project)) { - goIdle(); - g_queue.forgetHolds(); - return; - } + // beginTick owns what a pin/project mismatch means. What is local to here: the model + // the re-derivation below reads is the ACTIVE project's, because reaching this line + // means OnTimer ran session.poll() this tick — the two guards above are the same pair + // that gate that poll. + ReaProject* project = proj ? proj : EnumProjects(-1, nullptr, 0); + if (!ValidatePtr2(nullptr, project, "ReaProject*")) project = nullptr; + if (!g_queue.beginTick(project)) return; // DERIVED, never persisted: recomputed from view state every time the queue was // 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)))); @@ -229,7 +209,7 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { ReclaimHost host{project, &session}; const ReclaimRunResult run = g_queue.run(host); reportDrops(project, run.drops); - if (!run.aborted && run.settled) goIdle(); + if (!run.aborted && run.settled) g_queue.goIdle(); } } // namespace reasampler diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index f898e5d..55bd25c 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -137,8 +137,14 @@ struct TrackFxDrops { // 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. +// +// A tick that lost nothing is never worth printing AND must leave the memo alone — it +// is not evidence about the last set that DID lose something, and letting it overwrite +// the memo re-arms the identical report for the next re-derivation to print again. +// Answering false is what holds the memo back, so this is the one gate for both. inline bool shouldReportDrops(bool sameOwnerAsLast, const std::vector& last, const std::vector& now) { + if (now.empty()) return false; return !(sameOwnerAsLast && last == now); } @@ -256,18 +262,64 @@ struct ReclaimRunResult { std::vector drops; }; -// Owns the derived queue and the debounce clock across a tick that can be re-entered: -// TrackFX_SetOffline is ASSUMED to pump the message loop, so an invalidation can arrive -// with a drain half-way through a track. +// Owns the derived queue, the debounce clock, the project both were derived against, and +// the timer's wakeup — together, because it is their interaction that decides whether a +// tick may happen at all. A `ReaProject*` here is COMPARE-ONLY and never dereferenced, so +// the whole state machine is exercisable without REAPER (tests/test_fx_reclaim.cpp). // -// The two halves of that safety are structural, not guarded. `invalidate()` bumps a -// counter and touches NOTHING else, so there is nothing for it to clear under a drain. -// `run()` moves the queue and the clock into locals for the drain's whole duration, so -// the object holds nothing the drain is reading — and re-checks the counter before it -// commits either the surviving queue or a single release. +// The queue must also survive being re-entered: TrackFX_SetOffline is ASSUMED to pump the +// message loop, so an invalidation can arrive with a drain half-way through a track. The +// two halves of that safety are structural, not guarded. `invalidate()` bumps a counter +// and raises the wakeup and touches NOTHING else, so there is nothing for it to clear +// under a drain. `run()` moves the queue and the clock into locals for the drain's whole +// duration, so the object holds nothing the drain is reading — and re-checks the counter +// before it commits either the surviving queue or a single release. class FxReclaimQueue { public: - void invalidate() { ++generation_; } + void invalidate() { + ++generation_; + pending_ = true; + } + + // The ONE test the timer makes per tick. False means settled: nothing to converge + // until the next invalidation. + bool pending() const { return pending_; } + + // The project the current queue and holds were derived against, or nullptr when idle. + ReaProject* project() const { return project_; } + + // The tick's entry transition, and the ONE place the pin moves. `project` is the + // validated active project, or nullptr when there is none; false means the caller + // must return without touching REAPER. + // + // A project that does not match the pin is a TAB SWITCH — the ordinary path, not a + // corner. Neither the derived work nor the holds are evidence about the project we + // arrived in, so both go, and the tick proceeds to re-derive against the new one. The + // WAKEUP STANDS: clearing it here (an earlier goIdle in this branch did) stranded the + // incoming project's whole reclaim, because the timer gates every tick on it and + // nothing re-raises it short of a mode switch or a membership edit there. Only the + // no-project arm may clear it — nothing to converge, and nothing to come back to. + bool beginTick(ReaProject* project) { + if (!project) { + goIdle(); + clock_.forget(); + return false; + } + if (project_ && project_ != project) { + discard(); + clock_.forget(); + } + project_ = project; + return true; + } + + // Settled: drop the queue and the pin and stop the timer's ticks. The debounce clock + // deliberately survives — it is keyed on the track, not on the queue. + void goIdle() { + discard(); + project_ = nullptr; + pending_ = false; + } // No queue at all, or one derived before the last invalidation. bool stale() const { return !derived_ || derivedAt_ != generation_; } @@ -283,8 +335,6 @@ public: derived_ = false; } - void forgetHolds() { clock_.forget(); } - const std::vector& items() const { return items_; } template @@ -327,19 +377,22 @@ public: private: std::vector items_; ParkClock clock_; + ReaProject* project_ = nullptr; unsigned long long generation_ = 0; unsigned long long derivedAt_ = 0; bool derived_ = false; + bool pending_ = false; }; // --- shell entry points -------------------------------------------------------- // The ONE test the timer makes per tick, mirroring the realtime capture's single // pointer test. False means the queue is settled and there is nothing to converge. -extern bool g_fxReclaimPending; +bool fxReclaimPending(); -// Marks the derived queue for re-derivation by the NEXT tick; discards nothing here, -// because this is reachable from inside a drain. Called from applyMode and nothing else +// Raises the wakeup and marks the derived queue for re-derivation by the NEXT tick; +// discards nothing here, because this is reachable from inside a drain. Called from +// applyMode and nothing else // — every membership, snapshot and active-mode mutation reapplies through it — on the // refusal path as well as the success one (see view.cpp for why the refusal is the // load-bearing half). The debounce clock deliberately survives: it is keyed on the @@ -351,11 +404,12 @@ void invalidateFxReclaim(); bool fxReclaimDrainInProgress(); // Drains one tick against `proj` (nullptr = current project), re-deriving the queue from -// 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. +// the session's view model first if it was invalidated or if the active project moved +// (FxReclaimQueue::beginTick). Returns without touching the project while an applyMode is +// on the stack, while already draining, or when there is no valid active project at all. +// 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 1dfa6cf..ecd70b2 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -4,10 +4,12 @@ // yield, the queue being a cache of intent that a model change discards, the user's own // 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. +// reaches nothing the drain is using. The queue owns the project pin and the timer's +// wakeup for the same reason: a compare-only pointer makes the tab switch assertable. #include "../src/shell/view/fx_reclaim.h" +#include #include #include #include @@ -176,6 +178,11 @@ FxReclaimQueue queueOf(const ViewModeModel& vm, const std::set& liv return q; } +// Two project handles. The queue treats a project pointer as COMPARE-ONLY and never +// dereferences it, so a fabricated address is a faithful stand-in for REAPER's. +ReaProject* const kProjA = reinterpret_cast(static_cast(0x1)); +ReaProject* const kProjB = reinterpret_cast(static_cast(0x2)); + } // namespace // -- 1. debounce --------------------------------------------------------------- @@ -633,6 +640,138 @@ static void testAnOfflineTickReportsNoDrops() { CHECK(r.drops.empty()); } +// -- 8. the pin and the wakeup ------------------------------------------------------ + +static void testATabSwitchRetargetsTheQueueAndLeavesTheWakeupStanding() { + // Project A, one parked leaf owed offline work, derived against A and pinned to it. + ViewModeModel a = parkedDesignLeaf({"{FX-A}"}); + FakeHost hostA; + hostA.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q; + q.invalidate(); // what applyMode does on its way out of the switch that parked {T} + CHECK(q.pending()); + CHECK(q.beginTick(kProjA)); + CHECK(q.project() == kProjA); + q.adopt(derive(a, {"{T}"})); + CHECK(!q.run(hostA).settled); // observes the hold at t=0; still owed + CHECK(!q.stale()); + + // The tab switch. poll()'s Load reapply invalidates on the way in, and NOTHING there + // moves the pin, so the tick arrives holding A's work with B active. + q.invalidate(); + CHECK(q.beginTick(kProjB)); // the tick proceeds — it does not bail on the mismatch + CHECK(q.pending()); // and the timer will still call it: the strand's one bit + CHECK(q.project() == kProjB); + CHECK(q.stale()); // A's work is gone, B's is owed + CHECK(q.items().empty()); + + // B's own parked leaf converges on this very tick and every one after it. + ViewModeModel b = parkedDesignLeaves({{"{T2}", {"{FX-B}"}}}); + FakeHost hostB; + hostB.chains["{T2}"] = onlineChain({"{FX-B}"}); + q.adopt(derive(b, {"{T2}"})); + CHECK(!q.run(hostB).settled); // B's own hold starts here + hostB.clock += kParkHoldMs; + const ReclaimRunResult done = q.run(hostB); + CHECK(done.settled); + CHECK(hostB.writeLog == std::vector{"{T2}:0=off"}); + CHECK(hostB.offlineAt("{T2}", 0)); + + // ...and A's chain was never touched from B: the queue was dropped, not run. + CHECK(hostA.writeLog.empty()); + + // Only now, settled, does the shell stop the ticks. + q.goIdle(); + CHECK(!q.pending()); + CHECK(q.project() == nullptr); +} + +static void testGoingIdleKeepsTheParkHoldWhileARetargetDropsIt() { + // A hold is evidence about a track in ONE project, so a tab switch drops it. An + // ordinary settle does not: restarting every 3s hold on the next unrelated membership + // edit would defer an unchanged track's reclaim indefinitely. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q; + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + q.run(host); // hold starts at t=0 + host.clock = kParkHoldMs; + CHECK(q.run(host).settled); + CHECK(host.writeLog.size() == 1); + + // Settled: the queue and the pin go, the hold stays. A membership edit elsewhere + // re-derives the SAME offline item, which is matured on arrival — one tick, no + // second 3s wait, and nothing to write because the chain is already there. + q.goIdle(); + q.invalidate(); + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + const ReclaimRunResult resumed = q.run(host); + CHECK(resumed.settled); + CHECK(resumed.writes == 0); + CHECK(host.writeLog.size() == 1); + + // The mirror: away to B and back to A, and the same item must wait a fresh hold. + q.invalidate(); + CHECK(q.beginTick(kProjB)); + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + const double back = host.clock; + CHECK(!q.run(host).settled); // the hold restarts here, at `back` + host.clock = back + kParkHoldMs - 1.0; + CHECK(!q.run(host).settled); + host.clock = back + kParkHoldMs; + CHECK(q.run(host).settled); +} + +static void testOnlyANoProjectTickClearsTheWakeup() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q; + q.invalidate(); + CHECK(q.beginTick(kProjA)); + q.adopt(derive(vm, {"{T}"})); + q.run(host); + host.clock += kParkHoldMs; + + // An abort leaves the wakeup up: the work is re-owed, and the shell's settle test + // never runs on this path. + host.reentrant = &q; + host.invalidateAfterWrite = 1; + const ReclaimRunResult r = q.run(host); + CHECK(r.aborted); + CHECK(q.pending()); + + // No active project at all — the one arm with nothing to converge and nothing to + // come back to. Everything goes, wakeup included. + CHECK(!q.beginTick(nullptr)); + CHECK(!q.pending()); + CHECK(q.project() == nullptr); + CHECK(q.stale()); + CHECK(q.items().empty()); +} + +static void testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo() { + const std::vector none; + const std::vector lost{TrackFxDrops{"{T}", FxRestoreDrops{1, 0, 0}}}; + + CHECK(shouldReportDrops(/*sameOwnerAsLast=*/false, none, lost)); // first sighting + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, lost, lost)); // the re-derivation + + // The regression: every ordinary tick carries no drops at all. Treating that as a + // change would overwrite the memo and re-arm the identical report for the next + // re-derivation — the shell holds the memo back on exactly this answer. + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, lost, none)); + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/false, lost, none)); + CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, lost, lost)); +} + int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); @@ -657,6 +796,11 @@ int main() { testADropIsReportedOnceAcrossRepeatedRederivations(); testAnOfflineTickReportsNoDrops(); + testATabSwitchRetargetsTheQueueAndLeavesTheWakeupStanding(); + testGoingIdleKeepsTheParkHoldWhileARetargetDropsIt(); + testOnlyANoProjectTickClearsTheWakeup(); + testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } From 02790f1270409897a625256de3849627108fde4c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 22:31:47 -0400 Subject: [PATCH 7/9] Gate the FX reclaimer on the transport, both directions; move the release's persist off the drain and onto the project save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drain no longer dirties a project on its own initiative — a spent park record rides the user's next save via OnSaveExtensionConfig. --- src/app/main.cpp | 15 +- src/shell/persist/ext_state_io.cpp | 10 ++ src/shell/persist/session.h | 7 + src/shell/view/fx_reclaim.cpp | 38 +++-- src/shell/view/fx_reclaim.h | 75 +++++++--- tests/test_fx_reclaim.cpp | 222 +++++++++++++++++++++++------ 6 files changed, 288 insertions(+), 79 deletions(-) diff --git a/src/app/main.cpp b/src/app/main.cpp index 4d9457d..554568b 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -250,18 +250,25 @@ static void OnBeginLoadProjectState(bool isUndo, project_config_extension_t* /*r g_session.requestReload(); } -// Intentional no-ops: ReaSampler stores state via project EXT STATE, not this -// extension's own project lines. The struct is registered ONLY for the -// BeginLoadProjectState undo/redo notification. +// Intentional no-op: ReaSampler stores state via project EXT STATE, not this +// extension's own project lines. static bool OnProcessExtensionLine(const char* /*line*/, ProjectStateContext* /*ctx*/, bool /*isUndo*/, project_config_extension_t* /*reg*/) { return false; // we own no project lines — ext state carries our data } -static void OnSaveExtensionConfig(ProjectStateContext* /*ctx*/, bool /*isUndo*/, +// We write no lines into `ctx` — this is the FX reclaimer's one durability point. Its +// spent park records ride the user's save rather than dirtying the project on the +// reclaimer's own initiative (fx_reclaim.h's ReleaseFlush). +// +// A save only: an isUndo pass is REAPER capturing an undo point, not writing the file, +// and clearing the pending flush on one would leave the record unspent on disk for good. +static void OnSaveExtensionConfig(ProjectStateContext* /*ctx*/, bool isUndo, project_config_extension_t* /*reg*/) { + if (!isUndo) + reasampler::fxReclaimFlushRelease(g_session); } // Storage must outlive registration — REAPER holds this pointer until we unregister it. diff --git a/src/shell/persist/ext_state_io.cpp b/src/shell/persist/ext_state_io.cpp index 02bdaf3..5852e11 100644 --- a/src/shell/persist/ext_state_io.cpp +++ b/src/shell/persist/ext_state_io.cpp @@ -211,6 +211,16 @@ bool ReaSamplerSession::saveViewStateOnly() { return true; } +// 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; + + writeViewState(proj, view_); + return true; +} + bool ReaSamplerSession::writeAssignmentRequest(const std::string& wire) { ReaProject* proj = activeSavedProject(); if (!proj) return false; diff --git a/src/shell/persist/session.h b/src/shell/persist/session.h index e8c4706..a7f9522 100644 --- a/src/shell/persist/session.h +++ b/src/shell/persist/session.h @@ -144,6 +144,13 @@ public: // Returns on the SAME rule as saveToActiveProject above, prohibition included. bool saveViewStateOnly(); + // 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(); + // Report-only prune dry-run: feeds the pure core with (present, referenced, // owned) — `present` from the folder enumeration, the other two from the // tracking authority. FAIL-SAFE: tracking state the authority cannot read diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 091cffd..5cba6bb 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -10,8 +10,8 @@ #include #include -#include "shell/persist/session.h" // saveViewStateOnly — the release's durable half -#include "shell/view/view.h" // modeApplyInProgress +#include "shell/persist/session.h" // flushViewState — the release's durable half +#include "shell/view/view.h" // modeApplyInProgress, transportBlocksModeSwitch #include "shell/view/view_park.h" // fxGuidString — the one FX-identity read #include "shell/view/view_tracks.h" @@ -34,6 +34,9 @@ namespace { // FxReclaimQueue, which owns all three. FxReclaimQueue g_queue; +// A release applied to the in-memory model and not yet written — see ReleaseFlush. +ReleaseFlush g_releaseFlush; + // TrackFX_SetOffline is ASSUMED to pump the message loop, so a plugin load can re-enter // the timer and land back here mid-drain. Fail closed: a re-entered drain would derive a // second queue for the same project and race the outer one's writes track for track. @@ -103,6 +106,11 @@ struct ReclaimHost { .count(); } + // The SAME predicate applyMode refuses a real mode switch on, deliberately shared + // rather than re-read here: what counts as a transport that must not have plugins + // moved under it is one fact with one home (view.h). + bool transportRolling() const { return transportBlocksModeSwitch(project); } + bool liveChain(const std::string& guid, std::vector& out) { MediaTrack* tr = resolveForStep(guid); if (!tr) return false; @@ -136,17 +144,10 @@ struct ReclaimHost { // Reached only after the drain has unwound and the queue has proved its generation // 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 spend lands in memory here and reaches disk on the project's next save. + void releaseFx(const std::string& guid) { + session->view().releaseSnapshotFx(guid); + g_releaseFlush.arm(project); } }; @@ -212,4 +213,15 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { if (!run.aborted && run.settled) g_queue.goIdle(); } +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; + // 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(); +} + } // namespace reasampler diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 55bd25c..6a4e6d7 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -173,18 +173,19 @@ struct ReclaimTickResult { // fx_reclaim.cpp holds the one live implementation. Host must provide: // // double nowMs() monotonic milliseconds +// bool transportRolling() playing or recording — the tick +// may move no plugins at all // bool liveChain(guid, std::vector&) FX identity per live slot; // false when the track is gone // bool writeOffline(guid, fxIndex, offline) compare-before-write inside its // own discarded undo block; // true iff it actually wrote -// void releaseFx(guid) release the record's FX half; -// 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 +// void releaseFx(guid) release the record's FX half and +// arm the save-time flush that +// makes the spend durable; called +// by FxReclaimQueue::run after the +// drain has unwound, never from +// inside the drain // // `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 @@ -192,6 +193,18 @@ struct ReclaimTickResult { template ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, Host& host) { ReclaimTickResult result; + + // Mirrors applyMode's refusal (view.h, transportBlocksModeSwitch), and now covers + // every FX write the tool makes, since this loop owns them all. BOTH directions, not + // just the opportunistic offline one: onlining re-instantiates the very plugins the + // user is listening through — the same destruction the switch already refuses — and a + // step is a whole chain with no yield inside it. Nothing is observed on this path, so + // every debounce hold keeps its origin and the first ungated tick fires with no wait. + if (host.transportRolling()) { + result.settled = queueSettled(items); + return result; + } + clock.observe(items, host.nowMs()); const int first = nextEligible(items, clock, host.nowMs()); @@ -358,19 +371,9 @@ public: 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. - 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(); - } + // release fires then. Applied in MEMORY only — the spend reaches disk on the + // project's next save (ReleaseFlush), never on the drain's own initiative. + for (const std::string& guid : tick.released) host.releaseFx(guid); return ReclaimRunResult{tick.writes, tick.settled, false, tick.drops}; } @@ -384,6 +387,30 @@ private: bool pending_ = false; }; +// --- the release's durable half -------------------------------------------------- + +// A park record spent in MEMORY only, waiting for the project's next save. +// +// The drain used to persist it itself, and that dirtied the project: open one saved +// mid-reclaim, let the drain finish, and the user is asked to save a project they only +// opened. The tool no longer marks a project dirty on its own initiative — it writes what +// it owes when the USER saves. The exposure that buys is the window between the two: die +// 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. +class ReleaseFlush { +public: + void arm(ReaProject* project) { project_ = project; } + bool pendingFor(ReaProject* active) const { return project_ && project_ == active; } + void clear() { project_ = nullptr; } + +private: + ReaProject* project_ = nullptr; // COMPARE-ONLY, never dereferenced +}; + // --- shell entry points -------------------------------------------------------- // The ONE test the timer makes per tick, mirroring the realtime capture's single @@ -408,8 +435,12 @@ bool fxReclaimDrainInProgress(); // (FxReclaimQueue::beginTick). Returns without touching the project while an applyMode is // on the stack, while already draining, or when there is no valid active project at all. // 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. +// 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). +void fxReclaimFlushRelease(ReaSamplerSession& session); + } // namespace reasampler diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index ecd70b2..9a0e2b3 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -2,10 +2,11 @@ // 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, 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. The queue owns the project pin and the timer's -// wakeup for the same reason: a compare-only pointer makes the tab switch assertable. +// offline plugins surviving an online, the transport gate, the release reaching disk on +// the user's save rather than on the drain, and — via a host that invalidates from inside +// a write — that a re-entered invalidation reaches nothing the drain is using. The queue +// owns the project pin and the timer's wakeup for the same reason: a compare-only pointer +// makes the tab switch assertable. #include "../src/shell/view/fx_reclaim.h" @@ -28,6 +29,11 @@ namespace { // -- a fake project ------------------------------------------------------------ +// Two project handles. The queue and the flush treat a project pointer as COMPARE-ONLY +// and never dereference it, so a fabricated address is a faithful stand-in for REAPER's. +ReaProject* const kProjA = reinterpret_cast(static_cast(0x1)); +ReaProject* const kProjB = reinterpret_cast(static_cast(0x2)); + // One live FX: its identity (empty when REAPER reports none) and whether it is offline. struct FakeFx { std::string guid; @@ -51,7 +57,10 @@ struct FakeHost { int invalidateAfterWrite = -1; int itemsSeenMidDrain = -1; // queue size as the re-entrant caller finds it + bool rolling = false; // the transport gate's input + double nowMs() const { return clock; } + bool transportRolling() const { return rolling; } bool liveChain(const std::string& guid, std::vector& out) { auto it = chains.find(guid); @@ -80,21 +89,18 @@ struct FakeHost { } // `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; + // on disk — the blob a reopen or an undo/redo loadFromProject rebuilds from. The + // drain writes the model and NEVER `persisted`: only saveProject() below does, which + // is what makes "the spend rides the user's save" assertable. + ViewModeModel* model = nullptr; + ReleaseFlush* flush = nullptr; + ReaProject* project = kProjA; 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(); + if (flush) flush->arm(project); } bool offlineAt(const std::string& guid, std::size_t slot) const { @@ -103,6 +109,14 @@ 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. +void saveProject(FakeHost& host, ReleaseFlush& flush, ReaProject* active) { + if (!flush.pendingFor(active)) return; + host.persisted = host.model->serialize(); + flush.clear(); +} + // -- model helpers ------------------------------------------------------------- TrackSnapshot recordWithFx(std::vector fx) { @@ -178,11 +192,6 @@ FxReclaimQueue queueOf(const ViewModeModel& vm, const std::set& liv return q; } -// Two project handles. The queue treats a project pointer as COMPARE-ONLY and never -// dereferences it, so a fabricated address is a faithful stand-in for REAPER's. -ReaProject* const kProjA = reinterpret_cast(static_cast(0x1)); -ReaProject* const kProjB = reinterpret_cast(static_cast(0x2)); - } // namespace // -- 1. debounce --------------------------------------------------------------- @@ -505,6 +514,8 @@ static void testAnInvalidationDuringAnOnlineDropsThatTicksRelease() { vm.releaseSnapshotFlags("{T}"); FakeHost host; + ReleaseFlush flush; + host.flush = &flush; host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}"}); FxReclaimQueue q = queueOf(vm, {"{T}"}); @@ -514,9 +525,9 @@ static void testAnInvalidationDuringAnOnlineDropsThatTicksRelease() { const ReclaimRunResult r = q.run(host); 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 + CHECK(r.writes == 2); // the item still completed — a step is atomic + CHECK(host.released.empty()); // but its record was NOT spent + CHECK(!flush.pendingFor(kProjA)); // and no save is owed one that never happened // Re-owed, not lost: the re-derived item finds the chain already where the record // wants it, writes nothing more, and releases then. @@ -525,45 +536,97 @@ static void testAnInvalidationDuringAnOnlineDropsThatTicksRelease() { CHECK(q.run(host).settled); CHECK(host.writeLog.size() == 2); CHECK(host.released.count("{T}") == 1); - CHECK(host.persists == 1); + CHECK(flush.pendingFor(kProjA)); } // -- 6. the release has to outlive the session ------------------------------------ -static void testACompletedOnlineIsPersistedSoAReloadPlansNoWork() { +static void testACompletedOnlineReachesDiskOnTheSaveAndNotOnTheDrain() { 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 + // The bug the flush 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.persisted = vm.serialize(); // the project as it sits on disk, record unspent host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}"}); + ReleaseFlush flush; + host.flush = &flush; 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()); + // The drain wrote NOTHING durable. That is the whole change: a background tick that + // persisted would also have dirtied the project, asking the user to save one they + // had only opened. + const std::optional beforeSave = ViewModeModel::deserialize(host.persisted); + CHECK(beforeSave.has_value()); + CHECK(beforeSave->snapshot("{T}") != nullptr); + CHECK(derive(*beforeSave, {"{T}"}).size() == 1); // still stale on disk + 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. + saveProject(host, flush, 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 + CHECK(derive(*afterSave, {"{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. +static void testAReleaseKilledBeforeTheFlushIsSimplyReEarned() { + // The exposure the save-time flush opens: between the in-memory release and the next + // save, the record on disk is still unspent. Quitting there must cost a redo of the + // reclaim, never a repair. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.model = &vm; + host.persisted = vm.serialize(); + host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}"}); + ReleaseFlush flush; + host.flush = &flush; + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + drainToSettled(q, host); + CHECK(host.writeLog.size() == 2); // the chain really was driven online + CHECK(flush.pendingFor(kProjA)); + + // Killed here: no save, so the model in memory goes and the disk record survives. + const std::optional reopened = ViewModeModel::deserialize(host.persisted); + CHECK(reopened.has_value()); + ViewModeModel reloaded = *reopened; + + FakeHost after; + after.chains = host.chains; // the DAW state outlives the reload: the chain is online + after.model = &reloaded; + ReleaseFlush flushAfter; + after.flush = &flushAfter; + + FxReclaimQueue q2 = queueOf(reloaded, {"{T}"}); + CHECK(q2.items().size() == 1); // the surviving record re-derives the same Online work + CHECK(drainToSettled(q2, after) == 1); + CHECK(after.writeLog.empty()); // compare-before-write: nothing left to move + CHECK(after.released.count("{T}") == 1); + CHECK(flushAfter.pendingFor(kProjA)); // and the spend is owed to the next save again +} + +static void testATickThatSpendsNoRecordOwesNoSaveAnything() { + // OFFLINE work spends no record, so there is nothing to make durable — and the + // reclaimer must never give the user's next save work it did not earn. ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); FakeHost host; host.model = &vm; host.chains["{T}"] = onlineChain({"{FX-A}"}); + ReleaseFlush flush; + host.flush = &flush; FxReclaimQueue q = queueOf(vm, {"{T}"}); q.run(host); // observes the hold at t=0 @@ -572,7 +635,8 @@ static void testATickThatSpendsNoRecordPersistsNothing() { CHECK(host.writeLog.size() == 1); // it did move the plugin CHECK(host.released.empty()); - CHECK(host.persists == 0); + CHECK(!flush.pendingFor(kProjA)); + saveProject(host, flush, kProjA); CHECK(host.persisted.empty()); } @@ -772,6 +836,79 @@ static void testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo() { CHECK(!shouldReportDrops(/*sameOwnerAsLast=*/true, lost, lost)); } +// -- 9. the transport gate ------------------------------------------------------------ + +static void testARollingTransportMovesNoPluginsAndKeepsTheHold() { + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + CHECK(!q.run(host).settled); // the hold starts at t=0 + + host.rolling = true; + host.clock += kParkHoldMs; // matured — the debounce is no longer what holds it back + const ReclaimRunResult gated = q.run(host); + CHECK(gated.writes == 0); + CHECK(host.writeLog.empty()); + CHECK(!gated.aborted); + CHECK(!gated.settled); // the work is still owed, so the shell must not go idle + CHECK(!q.stale()); // and the queue it will resume from survives untouched + + // The hold kept its origin across the gated tick: the drain fires on the FIRST tick + // after the transport stops, with no fresh 3s wait. + host.rolling = false; + CHECK(q.run(host).settled); + CHECK(host.writeLog.size() == 2); +} + +static void testTheGateStopsTheOnlineDirectionToo() { + // The ruling is symmetric because both directions load or unload plugins: onlining + // re-instantiates the very chain the user is listening through. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + + FakeHost host; + host.model = &vm; + host.chains["{T}"] = offlineChain({"{FX-A}", "{FX-B}"}); + ReleaseFlush flush; + host.flush = &flush; + host.rolling = true; + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + CHECK(q.items().size() == 1 && q.items()[0].work.kind == FxWorkKind::Online); + const ReclaimRunResult gated = q.run(host); + CHECK(gated.writes == 0); + CHECK(!gated.settled); + CHECK(host.released.empty()); // no record spent while the transport rolls + CHECK(!flush.pendingFor(kProjA)); + CHECK(host.offlineAt("{T}", 0)); // the chain is untouched, still parked + + // The contrast, so the assertions above are about the gate and not about an online + // queue that cannot drain: stop the transport and the same tick completes. + host.rolling = false; + CHECK(q.run(host).settled); + CHECK(host.writeLog.size() == 2); + CHECK(host.released.count("{T}") == 1); +} + +static void testAGatedTickStillGoesIdleWithNothingLeftToConverge() { + // A settled queue must not spin the timer for the length of a playback pass: the + // gate's answer is "no writes", not "never finished". + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + host.chains["{T}"] = onlineChain({"{FX-A}"}); + + FxReclaimQueue q = queueOf(vm, {"{T}"}); + q.run(host); + host.clock += kParkHoldMs; + CHECK(q.run(host).settled); + + host.rolling = true; + CHECK(q.run(host).settled); +} + int main() { testFastFlipMovesZeroPluginsButAHeldParkStillReclaims(); @@ -790,8 +927,9 @@ int main() { testAnInvalidationDuringTheLastItemLeavesTheTickUnsettled(); testAnInvalidationDuringAnOnlineDropsThatTicksRelease(); - testACompletedOnlineIsPersistedSoAReloadPlansNoWork(); - testATickThatSpendsNoRecordPersistsNothing(); + testACompletedOnlineReachesDiskOnTheSaveAndNotOnTheDrain(); + testAReleaseKilledBeforeTheFlushIsSimplyReEarned(); + testATickThatSpendsNoRecordOwesNoSaveAnything(); testADropIsReportedOnceAcrossRepeatedRederivations(); testAnOfflineTickReportsNoDrops(); @@ -801,6 +939,10 @@ int main() { testOnlyANoProjectTickClearsTheWakeup(); testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo(); + testARollingTransportMovesNoPluginsAndKeepsTheHold(); + testTheGateStopsTheOnlineDirectionToo(); + testAGatedTickStillGoesIdleWithNothingLeftToConverge(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; } From c95203718ac07a73ba3f16b5c6ee04587fde9241 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 22:52:57 -0400 Subject: [PATCH 8/9] 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 | 10 ++++---- src/shell/persist/session.h | 12 ++++++--- src/shell/view/fx_reclaim.cpp | 38 +++++++++++++++++----------- src/shell/view/fx_reclaim.h | 24 +++++++++++++----- tests/test_fx_reclaim.cpp | 40 +++++++++++++++++++++++++++--- 5 files changed, 91 insertions(+), 33 deletions(-) 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(); From 4f6cf607404eedf6587606e371a8e0712ee61568 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 23:04:02 -0400 Subject: [PATCH 9/9] Narrow the FX reclaimer's transport gate to defer offline work only; let online drain through, and always run the park clock's observe --- src/shell/view/fx_reclaim.cpp | 4 +- src/shell/view/fx_reclaim.h | 41 ++++++++----- tests/test_fx_reclaim.cpp | 111 ++++++++++++++++++++++++++++------ 3 files changed, 120 insertions(+), 36 deletions(-) diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 2a63b56..36ef36e 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -110,8 +110,8 @@ struct ReclaimHost { } // The SAME predicate applyMode refuses a real mode switch on, deliberately shared - // rather than re-read here: what counts as a transport that must not have plugins - // moved under it is one fact with one home (view.h). + // rather than re-read here: "the transport is rolling" is one fact with one home + // (view.h). drainTick narrows what it gates with this answer to offline work only. bool transportRolling() const { return transportBlocksModeSwitch(project); } bool liveChain(const std::string& guid, std::vector& out) { diff --git a/src/shell/view/fx_reclaim.h b/src/shell/view/fx_reclaim.h index 3050d32..b56fa28 100644 --- a/src/shell/view/fx_reclaim.h +++ b/src/shell/view/fx_reclaim.h @@ -173,8 +173,9 @@ struct ReclaimTickResult { // fx_reclaim.cpp holds the one live implementation. Host must provide: // // double nowMs() monotonic milliseconds -// bool transportRolling() playing or recording — the tick -// may move no plugins at all +// bool transportRolling() playing or recording — gates the +// OFFLINE direction only; a tick +// may still online while it rolls // bool liveChain(guid, std::vector&) FX identity per live slot; // false when the track is gone // bool writeOffline(guid, fxIndex, offline) compare-before-write inside its @@ -194,20 +195,30 @@ template ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, Host& host) { ReclaimTickResult result; - // Mirrors applyMode's refusal (view.h, transportBlocksModeSwitch), and now covers - // every FX write the tool makes, since this loop owns them all. BOTH directions, not - // just the opportunistic offline one: onlining re-instantiates the very plugins the - // user is listening through — the same destruction the switch already refuses — and a - // step is a whole chain with no yield inside it. Nothing is observed on this path, so - // every debounce hold keeps its origin and the first ungated tick fires with no wait. - if (host.transportRolling()) { - result.settled = queueSettled(items); - return result; - } - + // Run every tick, rolling or not: matured()'s own contract ("observe() runs first + // every tick") already assumed this, and a track can stop owing offline work on a + // rolling tick too now that this loop keeps writing online through one — a + // continuously-parked track's origin is untouched (observe carries the prior + // timestamp forward), so the only thing this changes is dropping a hold the moment + // its track genuinely stops owing offline work, matured or not. clock.observe(items, host.nowMs()); - const int first = nextEligible(items, clock, host.nowMs()); + // Mirrors applyMode's refusal (view.h, transportBlocksModeSwitch), narrowed to the + // OFFLINE direction only: onlining is the mode switch's own unfinished promise — the + // user is waiting on it — while offlining is opportunistic reclaim that can always + // wait for the transport to stop. Onlining while rolling still costs a plugin load + // mid-playback; that is an accepted trade for this gate's shape, not a hazard it + // introduces. + const bool offlineAllowed = !host.transportRolling(); + auto nextAllowed = [&](double nowMs) { + const int i = nextEligible(items, clock, nowMs); + if (i >= 0 && !offlineAllowed && + items[static_cast(i)].work.kind == view::FxWorkKind::Offline) + return -1; + return i; + }; + + const int first = nextAllowed(host.nowMs()); if (first < 0) { result.settled = queueSettled(items); return result; @@ -218,7 +229,7 @@ ReclaimTickResult drainTick(std::vector& items, ParkClock& clock, H const double start = host.nowMs(); for (;;) { - const int i = nextEligible(items, clock, host.nowMs()); + const int i = nextAllowed(host.nowMs()); if (i < 0) break; ReclaimItem& item = items[static_cast(i)]; // Offline work never rides the online budget: the two directions are priced diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index e2fd6b1..f28a070 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -869,7 +869,7 @@ static void testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo() { // -- 9. the transport gate ------------------------------------------------------------ -static void testARollingTransportMovesNoPluginsAndKeepsTheHold() { +static void testARollingTransportDefersOfflineWorkAndKeepsTheHold() { ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); FakeHost host; host.chains["{T}"] = onlineChain({"{FX-A}", "{FX-B}"}); @@ -883,7 +883,7 @@ static void testARollingTransportMovesNoPluginsAndKeepsTheHold() { CHECK(gated.writes == 0); CHECK(host.writeLog.empty()); CHECK(!gated.aborted); - CHECK(!gated.settled); // the work is still owed, so the shell must not go idle + CHECK(!gated.settled); // the work is still owed — a gated item is deferred, not dropped CHECK(!q.stale()); // and the queue it will resume from survives untouched // The hold kept its origin across the gated tick: the drain fires on the FIRST tick @@ -893,9 +893,12 @@ static void testARollingTransportMovesNoPluginsAndKeepsTheHold() { CHECK(host.writeLog.size() == 2); } -static void testTheGateStopsTheOnlineDirectionToo() { - // The ruling is symmetric because both directions load or unload plugins: onlining - // re-instantiates the very chain the user is listening through. +static void testOnlineWorkProceedsWhileTheTransportRolls() { + // Daniel's ruling: the gate narrows to the offline direction. Onlining is the mode + // switch's own unfinished promise — restoreTrack already handed back visibility and + // B_MAINSEND/I_FXEN synchronously — so a full gate would leave the user listening to a + // dry, FX-enabled chain for the whole rest of the playback pass. Onlining still costs + // a plugin load mid-roll; that is the accepted trade, not a regression this test flags. ViewModeModel vm = parkedDesignLeaf({"{FX-A}", "{FX-B}"}); vm.setActiveMode(kDesignModeId); vm.releaseSnapshotFlags("{T}"); @@ -909,19 +912,87 @@ static void testTheGateStopsTheOnlineDirectionToo() { FxReclaimQueue q = queueOf(vm, {"{T}"}); CHECK(q.items().size() == 1 && q.items()[0].work.kind == FxWorkKind::Online); - const ReclaimRunResult gated = q.run(host); - CHECK(gated.writes == 0); - CHECK(!gated.settled); - CHECK(host.released.empty()); // no record spent while the transport rolls - CHECK(!flush.pendingFor(kProjA)); - CHECK(host.offlineAt("{T}", 0)); // the chain is untouched, still parked - - // The contrast, so the assertions above are about the gate and not about an online - // queue that cannot drain: stop the transport and the same tick completes. - host.rolling = false; - CHECK(q.run(host).settled); + const ReclaimRunResult run = q.run(host); + CHECK(run.writes == 2); + CHECK(run.settled); CHECK(host.writeLog.size() == 2); - CHECK(host.released.count("{T}") == 1); + CHECK(host.released.count("{T}") == 1); // the record's FX half is spent + CHECK(flush.pendingFor(kProjA)); + CHECK(!host.offlineAt("{T}", 0) && !host.offlineAt("{T}", 1)); +} + +static void testOfflineWorkStaysGatedWhileOnlineDrainsInTheSameTick() { + // A mixed queue while rolling: {Z-back} is owed an online, {A-park}'s hold is already + // matured and owed an offline. Only the online direction may move this tick. + ViewModeModel vm; + vm.membership().tag("{Z-back}", kArrangeModeId); + vm.membership().tag("{A-park}", kDesignModeId); + vm.storeSnapshot("{Z-back}", recordWithFx({{"{FX-1}", 0}})); + vm.storeSnapshot("{A-park}", recordWithFx({{"{FX-2}", 0}})); + + FakeHost host; + host.chains["{Z-back}"] = offlineChain({"{FX-1}"}); + host.chains["{A-park}"] = onlineChain({"{FX-2}"}); + ParkClock clock; + + std::vector items = derive(vm, {"{Z-back}", "{A-park}"}); + CHECK(items[0].work.kind == FxWorkKind::Online); + matureHold(items, clock, host); // {A-park}'s hold matures with time to spare below + + host.rolling = true; + host.clock += kParkHoldMs; + const ReclaimTickResult tick = drainTick(items, clock, host); + CHECK(tick.writes == 1); + CHECK(host.writeLog == std::vector{"{Z-back}:0=on"}); + CHECK(!host.offlineAt("{A-park}", 0)); // matured but gated — untouched this tick + CHECK(!tick.settled); // the offline item is still owed + + // The transport stops: the very next tick reclaims it, with no fresh hold to wait out. + host.rolling = false; + CHECK(drainTick(items, clock, host).settled); + CHECK(host.offlineAt("{A-park}", 0)); +} + +static void testAHoldDropsWhenItsTrackStopsOwingOfflineWorkWhileRolling() { + // ParkClock's own contract (fx_reclaim.h) says an entry is dropped the moment its + // track stops owing offline work — observe() has to run every tick, rolling or not, + // for that to hold once a rolling tick can itself change what a track owes (by + // draining its online item). This is the case that contract would otherwise miss. + ViewModeModel vm = parkedDesignLeaf({"{FX-A}"}); + FakeHost host; + // Offline in the DAW, matching a track truly parked — so bringing it online below is a + // real write, not a compare-before-write no-op. + host.chains["{T}"] = offlineChain({"{FX-A}"}); + ParkClock clock; + + std::vector items = derive(vm, {"{T}"}); + CHECK(drainTick(items, clock, host).writes == 0); // the hold starts at t=0 + CHECK(!clock.matured("{T}", 0.0)); + + // {T} comes back online while the transport rolls; the online direction still drains. + host.rolling = true; + vm.setActiveMode(kDesignModeId); + vm.releaseSnapshotFlags("{T}"); + items = derive(vm, {"{T}"}); + CHECK(items[0].work.kind == FxWorkKind::Online); + host.clock = 100.0; + CHECK(drainTick(items, clock, host).writes == 1); + CHECK(!clock.matured("{T}", host.clock)); // dropped: {T} owes no offline work now + + // {T} parks again, still rolling: the hold must start FRESH from here, not resume the + // one interrupted above — proof the drop above was real, not a no-op. + vm.setActiveMode(kArrangeModeId); + vm.storeSnapshot("{T}", recordWithFx({{"{FX-A}", 0}})); + items = derive(vm, {"{T}"}); + CHECK(items[0].work.kind == FxWorkKind::Offline); + const double repark = host.clock; + CHECK(!drainTick(items, clock, host).settled); // gated, and not matured either + CHECK(!clock.matured("{T}", repark + kParkHoldMs - 1.0)); + CHECK(clock.matured("{T}", repark + kParkHoldMs)); + + host.rolling = false; + host.clock = repark + kParkHoldMs; + CHECK(drainTick(items, clock, host).writes == 1); // matures once the gate lifts } static void testAGatedTickStillGoesIdleWithNothingLeftToConverge() { @@ -971,8 +1042,10 @@ int main() { testOnlyANoProjectTickClearsTheWakeup(); testATickThatLostNothingNeitherPrintsNorDisturbsTheDropMemo(); - testARollingTransportMovesNoPluginsAndKeepsTheHold(); - testTheGateStopsTheOnlineDirectionToo(); + testARollingTransportDefersOfflineWorkAndKeepsTheHold(); + testOnlineWorkProceedsWhileTheTransportRolls(); + testOfflineWorkStaysGatedWhileOnlineDrainsInTheSameTick(); + testAHoldDropsWhenItsTrackStopsOwingOfflineWorkWhileRolling(); testAGatedTickStillGoesIdleWithNothingLeftToConverge(); if (g_fail == 0) std::printf("All tests passed.\n");