Harden fx_reclaim against project closure mid-drain; cache resolved track per item; correct two stale budget comments

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
+// Resolved fresh from the GUID rather than held across a write, for the same pump
-// same pump premise: a plugin load can run arbitrary REAPER code, and a handle taken
+// premise: a plugin load can run arbitrary REAPER code, and a handle taken before it
-// before it is not one this code may trust after 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<std::string>& 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
+    // Belt-and-braces, not the tab-switch mechanism: OnTimer runs session.poll() before
-    // front of us": no active project at all, or a tab switch / close. Drop the queue
+    // this tick, and poll() already classifies a tab switch as a Load whose reapply
-    // rather than write another project's chains; returning to it re-derives, because
+    // invalidates the queue — so by the time this test runs, a switch has normally
-    // poll() reads that as a project load and the load glue reapplies, which invalidates.
+    // 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);

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
+// ONLINE 120ms: onlining restores state the user owns and is waiting on. The check
-// buys at least one typical load and two cheap ones. OFFLINE 30ms: under one typical
+// runs only AFTER a step, so against the cited 105ms typical load it admits TWO
-// unload, so opportunistic reclaim moves exactly ONE track per tick and yields --
+// typical tracks per tick (105 < 120, so a second runs; 210 >= 120 stops a third),
-// background work must not make the UI feel worse than the switch already did. The
+// not one -- worst case a tick runs over budget by up to one track's whole cost.
-// budget is checked only AFTER a step, never before the first and never mid-step, so
+// OFFLINE 30ms: background work must not make the UI feel worse than the switch
-// a track dearer than the whole budget still runs; that is what makes forward
+// already did. At the cited 36-42ms costs one track alone already clears the
-// progress unconditional.
+// 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;