From 4f6cf607404eedf6587606e371a8e0712ee61568 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 23:04:02 -0400 Subject: [PATCH] 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");