Terminate the FX reclaimer's undo span on EVERY project-state load, not just undo

BeginLoadProjectState now routes unbranched through fxReclaimAbandonForStateLoad; only the session's deferred reload stays undo-only.
2026-08-07 13:57:06 -04:00
parent 9b6f725747
commit 39b995e824
3 changed files with 48 additions and 34 deletions
@@ -241,21 +241,19 @@ static void OnTimer()
 // TIMING: BeginLoadProjectState fires BEFORE any state restore, so reading
 // GetProjExtState here would return the PRE-undo value. Instead we raise a one-shot
 // reload request that OnTimer's poll() drains on the NEXT tick, once REAPER has
-// finished restoring the <EXTSTATE> block. A normal project open also fires this
-// (isUndo=false); ignored here so a normal open flows solely through the timer's
-// identity-transition Load path (no double load).
+// finished restoring the <EXTSTATE> block. Only the RELOAD REQUEST is undo-only: a
+// normal open's reload belongs to the timer's identity-transition Load path, whose
+// forked-sibling re-GUID branch requesting a reload here would bypass.
 static void OnBeginLoadProjectState(bool isUndo, project_config_extension_t* /*reg*/)
 {
-    if (isUndo)
-    {
-        // BEFORE the reload request, and the whole reason this is a hook rather than
-        // something the reload's own reapply does on arrival: the reclaimer's undo block
-        // is open for at least kParkHoldMs after every real mode switch, and a switch is
-        // exactly what this undo is rolling back. Closing it here is the last moment it
-        // still closes over the state it was opened on.
+    // UNBRANCHED, and the reason the terminator is a hook at all: this is REAPER's one
+    // entry for a state load — "project load/undo", plus a new project
+    // (reaper_plugin.h:1203-1205) — and the last moment before the restore. Why that
+    // matters, and why isUndo does not narrow it: fxReclaimAbandonForStateLoad.
    reasampler::fxReclaimAbandonForStateLoad();
+
+    if (isUndo)
        g_session.requestReload();
-    }
 }

 // Intentional no-op: ReaSampler stores state via project EXT STATE, not this
@@ -84,25 +84,35 @@ decide membership or mode rules.
  block swallows every edit the user makes afterwards, indefinitely, so every
  terminating path routes through ONE idempotent close (`fxReclaimEndUndoSpan`): the
  drain settling, aborting on a generation bump, or deferring on the transport; the
-  project retargeting or closing; the next apply superseding it; a USER-DRIVEN STATE
+  project retargeting or closing; the next apply superseding it; ANY PROJECT-STATE
  LOAD; a project save; extension unload. The span may stay open only across a wait it
  can BOUND — the 3s debounce, which is the dead time between the switch and its first
  FX write, and the only reason the span exists. A TRANSPORT deferral is not bounded
  (it lasts as long as the playback pass), so it ends the span and the parking that
  follows mints its own point (`spanEndsAfter`).
- **A Ctrl-Z terminates the span itself, at the hook, before REAPER restores
-  anything.** `fxReclaimAbandonForStateLoad` runs from `BeginLoadProjectState`
+- **EVERY project-state load terminates the span itself, at the hook, before REAPER
+  restores anything — `isUndo` does not narrow it.**
+  `fxReclaimAbandonForStateLoad` runs UNBRANCHED from `BeginLoadProjectState`
  (`main.cpp`) — the only point at which the block still closes over the state it was
-  OPENED on. The span is open for at least `kParkHoldMs` after every real switch and a
-  switch is exactly what a user undoes, so letting the close happen where it used to —
-  as a side effect of the reapply the reloaded model triggers, a full timer tick later
-  — closed the pre-undo block over POST-undo state, minting a spurious point and
-  destroying the redo branch. The same call stops any drain on the stack where it
+  OPENED on, and the only entry REAPER gives for a state load at all, so undo, redo, a
+  project open, a `File > Revert` and a new project (`reaper_plugin.h:1203-1205`) all
+  reach the one terminator. The span is open for at least `kParkHoldMs` after every
+  real switch and a switch is exactly what a user undoes, so letting the close happen
+  where it used to — as a side effect of the reapply the reloaded model triggers, a
+  full timer tick later — closed the pre-undo block over POST-undo state, minting a
+  spurious point and destroying the redo branch. A NON-undo load left to the next
+  tick's project-retarget close is the identical defect one branch away: the same block
+  closes over the same post-load state, a tick late. `isUndo` decides one thing only,
+  in `main.cpp`: whether the SESSION owes a deferred ext-state reload (a normal open's
+  belongs to the identity poll's Load branch, whose forked-sibling re-GUID a reload
+  request here would bypass). The same call stops any drain on the stack where it
  stands (`FxReclaimQueue::abandon`, refused at both host entry points), which is
  stronger than an invalidation: an invalidation still finishes the track it is on,
-  because a step is atomic. Daniel's ruling, and the accepted cost: undoing a switch
-  mid-drain leaves a PARTIAL park — some FX offlined, some not — covered by the one
-  point the terminator closed.
+  because a step is atomic. A load that keeps the SAME `ReaProject` — a revert — is
+  what makes that stop load-bearing rather than tidy, for the reason stated at
+  `abandon`. Daniel's ruling, and the accepted cost: undoing a switch mid-drain leaves
+  a PARTIAL park — some FX offlined, some not — covered by the one point the
+  terminator closed.
 - **A reapply that wrote nothing leaves no undo point** — and a reapply never holds
  the span open, so a project open, a membership edit and `render_in_place`'s inner
  reapply all keep the block shape they had. The load-tick reapply
@@ -161,7 +171,7 @@ applies the resulting lane state to live tracks.
 - `view_reconcile` — the repair pass for a project state REAPER reloaded under us: `noteAppliedState` records what an apply left driven; `reconcileAppliedFlags` diffs that fingerprint against the reloaded model and hands back any track the reload no longer records but still reads driven — the fix for `UNDO_STATE_TRACKCFG` not carrying `B_SHOWINTCP`/`B_SHOWINMIXER` (DAW-confirmed).
 - `view_tracks` — the scaffolding every write pass sets up: `TrackHandles`/`TrackByGuid`, `readFolderEntries`/`indexHandles`, and `UiRefreshHold` (the `PreventUIRefresh` RAII pair every write phase runs under).
 - `view_solo` — the `I_SOLO` read/write pair behind the per-mode solo surface, plus `clearTrackSolos`/`restoreTrackSolos`, the outgoing-clear and incoming-replay entry points `view` drives them through. Holds no policy: what to cache, clear, or replay is `core/view/solo_cache`.
- `fx_reclaim` — the FX convergence loop, off the switch's synchronous path (`TrackFX_SetOffline` was 1897ms of a measured 1942ms switch). `drainTick` moves one track's FX per step against `core/view/fx_state`'s derived work list (Online before Offline; an Offline item waits out `kParkHoldMs` so a fast A→B→A flip moves nothing), budgeted per tick (`kOnlineBudgetMs`/`kOfflineBudgetMs`) and gated off the transport for the Offline direction only. `FxReclaimQueue` owns the derived queue's lifetime across re-entrancy (a plugin unload is assumed to pump). `UndoSpan` owns the ONE undo block the whole drain writes under — opened by `applyMode` on a real switch, else lazily by the first `writeOffline`, and closed through the single idempotent `fxReclaimEndUndoSpan` that every terminating path routes through (`spanEndsAfter` decides which ticks terminate; `fxReclaimAbandonForStateLoad` is the undo/redo terminator, which also stops the drain). `ReleaseFlush`/`fxReclaimFlushRelease` make a completed record's spend durable on the project's next save, never on the drain's own initiative. `fxReclaimActivity` reports WHICH DIRECTION the drain is moving for an indicator to paint — Idle whenever nothing may move, so a debounced or transport-held offline item never names a direction nothing is travelling in; undirected `fxReclaimPending` stays the timer's one idle test and must not drive a UI (it is true throughout the debounce, when only the OPPOSITE direction is eligible).
+- `fx_reclaim` — the FX convergence loop, off the switch's synchronous path (`TrackFX_SetOffline` was 1897ms of a measured 1942ms switch). `drainTick` moves one track's FX per step against `core/view/fx_state`'s derived work list (Online before Offline; an Offline item waits out `kParkHoldMs` so a fast A→B→A flip moves nothing), budgeted per tick (`kOnlineBudgetMs`/`kOfflineBudgetMs`) and gated off the transport for the Offline direction only. `FxReclaimQueue` owns the derived queue's lifetime across re-entrancy (a plugin unload is assumed to pump). `UndoSpan` owns the ONE undo block the whole drain writes under — opened by `applyMode` on a real switch, else lazily by the first `writeOffline`, and closed through the single idempotent `fxReclaimEndUndoSpan` that every terminating path routes through (`spanEndsAfter` decides which ticks terminate; `fxReclaimAbandonForStateLoad` is the state-load terminator, which also stops the drain). `ReleaseFlush`/`fxReclaimFlushRelease` make a completed record's spend durable on the project's next save, never on the drain's own initiative. `fxReclaimActivity` reports WHICH DIRECTION the drain is moving for an indicator to paint — Idle whenever nothing may move, so a debounced or transport-held offline item never names a direction nothing is travelling in; undirected `fxReclaimPending` stays the timer's one idle test and must not drive a UI (it is true throughout the debounce, when only the OPPOSITE direction is eligible).

 ## Gotchas

@@ -360,12 +360,15 @@ public:
        pending_ = true;
    }

-    // A project-state load the USER drove (undo/redo). The whole project is about to be
-    // replaced, so this is STRONGER than an invalidation: the queue's work is discarded as
-    // usual, AND any drain on the stack — reachable only through a write's own message
-    // pump — stops where it stands instead of finishing the track it is on. Accepted, and
-    // the reason a step's atomicity yields here: a park half-landed this way stays
-    // half-landed, covered by the one point the terminator closed.
+    // A project-state load — undo, redo, an open, a revert, a new project. The whole
+    // project is about to be replaced, so this is STRONGER than an invalidation: the
+    // queue's work is discarded as usual, AND any drain on the stack — reachable only
+    // through a write's own message pump — stops where it stands instead of finishing the
+    // track it is on. Accepted, and the reason a step's atomicity yields here: a park
+    // half-landed this way stays half-landed, covered by the one point the terminator
+    // closed. A load that keeps the SAME ReaProject (a revert) is what makes the stop
+    // load-bearing rather than tidy: the drain's pin and cached track handle both still
+    // validate afterwards, so nothing else would keep it out of the reloaded project.
    void abandon() {
        invalidate();
        abandoned_ = true;
@@ -657,20 +660,23 @@ void fxReclaimLabelUndoSpan(const char* label, int mask);

 // The ONE close, idempotent: every terminating path routes through it — the drain
 // settling, aborting, or deferring on the transport; the project retargeting or closing;
-// the next apply superseding it; a user-driven state load; a project save; extension
+// the next apply superseding it; ANY project-state load; a project save; extension
 // unload. A leaked open block would swallow everything the user does afterwards, so
 // nothing may return past a terminating condition without calling this.
 void fxReclaimEndUndoSpan();

-// A project-state load the USER drove — Ctrl-Z or Ctrl-Shift-Z. Ends the span and stops
-// any drain on the stack (FxReclaimQueue::abandon).
+// EVERY project-state load, undo-driven or not — Ctrl-Z, Ctrl-Shift-Z, a project open, a
+// File > Revert, a new project. Ends the span and stops any drain on the stack
+// (FxReclaimQueue::abandon).
 //
 // Called from the projectconfig BeginLoadProjectState hook, which fires BEFORE REAPER
 // restores anything: that is the ONLY point at which the block can still close over the
 // state it was opened on. The span is open for at least kParkHoldMs after every real
 // switch and a switch is exactly what a user undoes, so closing it as a side effect of
-// the reapply the load triggers — several calls later, over the restored project — would
-// mint the point over post-undo state and destroy the redo branch.
+// the load — several calls later, over the restored project — would mint the point over
+// post-load state and, for an undo, destroy the redo branch. That defect does not turn on
+// isUndo: a non-undo load left to the next tick's project-retarget close lands the same
+// point over the same wrong state, one hook branch away, so this hook does not branch.
 void fxReclaimAbandonForStateLoad();

 // Writes an in-memory release into the SAVING project's view state (identified via