diff --git a/src/shell/bank_ops/CLAUDE.md b/src/shell/bank_ops/CLAUDE.md index 219229f..6f67a05 100644 --- a/src/shell/bank_ops/CLAUDE.md +++ b/src/shell/bank_ops/CLAUDE.md @@ -33,7 +33,13 @@ calls into that model, it does not reimplement its rules. a Design View mode switch's FX reclaim span is open: without it the verb's own close landed at depth 2->1, minted nothing, and the rename or delete was absorbed into the mode switch, un-undoable on its own. Reason and mechanism: - `src/shell/undo/CLAUDE.md`. + `src/shell/undo/CLAUDE.md`. ACCEPTED, same cost `view_lanes`' `mintManagedLanes` + already carries (`src/shell/view/view_lanes.cpp`'s note at its own `UndoBlock`): + opening `persistBankOp`'s block ends any pending reclaim span unconditionally, + including on the unsaved-project path above, which then records no point of its + own — the switch's point still mints early, and its remaining parking mints again + under `kReclaimUndoLabel`. Bounded at one extra undo point; the alternative + (nesting over the span) destroys the switch's point outright. ## Modules diff --git a/src/shell/bank_ops/bank_ops.cpp b/src/shell/bank_ops/bank_ops.cpp index 5b4989b..1631621 100644 --- a/src/shell/bank_ops/bank_ops.cpp +++ b/src/shell/bank_ops/bank_ops.cpp @@ -44,15 +44,14 @@ std::string mintBankId() { // ext-state is the block's "after" image. // // UNSAVED-PROJECT GUARDRAIL: on an unsaved/no-active project saveToActiveProject() -// no-ops; we still CLOSE the block, but with an empty label + zero flag so REAPER -// discards the point instead of recording a no-effect undo entry. Quiet persist by -// design (mirrors capture, not Design-View) — deliberately no Save-As prompt. +// no-ops, so recordPoint below is skipped — the block closes on its own default +// discard form (empty label, zero flag) instead of recording a no-effect undo entry. +// Quiet persist by design (mirrors capture, not Design-View) — deliberately no +// Save-As prompt. void persistBankOp(ReaSamplerSession& session, const char* label, bool bumpGeneration) { UndoBlock undo(nullptr); if (bumpGeneration) session.bumpBankGeneration(); - // No ext-state write -> record no point; the block's default discard form is what - // keeps an unsaved project from collecting no-effect undo entries. if (session.saveToActiveProject()) undo.recordPoint(label, UNDO_STATE_MISCCFG); } diff --git a/src/shell/undo/CLAUDE.md b/src/shell/undo/CLAUDE.md index 427c8c5..a6e4811 100644 --- a/src/shell/undo/CLAUDE.md +++ b/src/shell/undo/CLAUDE.md @@ -11,13 +11,19 @@ nests over it. ## Invariants -- **`Undo_BeginBlock2` is not callable outside this directory, and that is enforced - by the compiler, not by review.** Only `undo_block.cpp` and `shell/view`'s - `fx_reclaim.cpp` (which owns the span's own begin/end) define - `REAPERAPI_WANT_Undo_BeginBlock2` / `..._Undo_EndBlock2`. Every other TU compiles - under `REAPERAPI_MINIMAL` with no header in its include graph pulling the SDK in - unguarded, so a raw `Undo_BeginBlock2` there is an undeclared identifier. Adding - the WANT define back to a third TU is the one way to defeat this — do not. +- **`Undo_BeginBlock2` is not callable outside this directory in every TU but one, and + that is enforced by the compiler, not by review.** Only `undo_block.cpp` and + `shell/view`'s `fx_reclaim.cpp` (which owns the span's own begin/end) define + `REAPERAPI_WANT_Undo_BeginBlock2` / `..._Undo_EndBlock2`; every other TU compiles + under `REAPERAPI_MINIMAL`, so a raw `Undo_BeginBlock2` there is an undeclared + identifier. **`app/main.cpp` is the one exception** — as the sole + `REAPERAPI_IMPLEMENT` TU it never sets `REAPERAPI_MINIMAL` (it must allocate storage + for whatever ANY TU wants), so the whole API, `Undo_*` included, compiles clean + there too. That TU stays out of the undo business by discipline + (`src/app/CLAUDE.md`: pointers + entry + dispatch only), not because the compiler + stops it — do not add business logic there that would make this matter. Adding the + WANT define back to a third TU is a second, easier way to defeat the enforcement + that DOES exist — do not. - **Ending the reclaim span is part of opening, and ONLY the outermost block does it.** At depth 0 the span is the only block that can be open, so ending it mints the pending mode switch's point whole before this block exists. At depth > 0 an @@ -26,6 +32,12 @@ nests over it. is the same criterion `applyMode`'s reapply is gated on; it lives in the constructor so no call site has to make the judgment. Full reasoning for the rule itself: `src/shell/view/CLAUDE.md`. +- **The constructor's body is one `try`.** `fxReclaimEndUndoSpan` copies a + `std::string` and can throw; a throwing constructor runs no destructor, so an + uncompensated depth increment would stick forever — `undoBlockOpen()` reads true + for the rest of the session, `fxReclaimTick` refuses every tick, and no later + `UndoBlock` is ever outermost. Any throw between the increment and the end of the + constructor rolls the depth back before propagating. - **Closing is a destructor.** No throw between the open and the label decision can leave a block open in the user's project, which is why several call sites can allocate freely between the two. @@ -46,6 +58,15 @@ nests over it. ## Gotchas +- **`Undo_EndBlock2` is guarded by `ValidatePtr2` only when `project_` is a real, + non-null pointer.** `nullptr` means "the active project" — the same convention + `Undo_BeginBlock2` was handed — and `ValidatePtr2` does not accept `nullptr` as a + valid object of any type, so guarding it unconditionally would silently stop every + `UndoBlock undo(nullptr)` site (most of them) from ever closing. A real pointer, by + contrast, can go stale between open and close (several call sites allocate freely + in between), so only that case is validated — the same asymmetry `fx_reclaim.cpp`'s + `closeSpan` already has, for the same reason: its span always holds a real pinned + pointer, never `nullptr` for "active". - The depth counter deliberately does NOT count the reclaim span. The span is the thing this stands off, not a peer. - The counter drops AFTER `Undo_EndBlock2`, not before: the close that reaches depth diff --git a/src/shell/undo/undo_block.cpp b/src/shell/undo/undo_block.cpp index bf3922d..9f10d76 100644 --- a/src/shell/undo/undo_block.cpp +++ b/src/shell/undo/undo_block.cpp @@ -10,6 +10,7 @@ #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_Undo_BeginBlock2 #define REAPERAPI_WANT_Undo_EndBlock2 +#define REAPERAPI_WANT_ValidatePtr2 #include "reaper_plugin_functions.h" namespace reasampler { @@ -31,15 +32,36 @@ UndoBlock::UndoBlock(ReaProject* proj) : project_(proj) { // enclosing UndoBlock already did it, and a close from here would land at depth // 2->1 — minting nothing and hijacking the enclosing block's label instead. That is // the same criterion applyMode's reapply is gated on. - if (g_depth++ == 0) fxReclaimEndUndoSpan(); - Undo_BeginBlock2(proj); + // + // The whole body is one try: fxReclaimEndUndoSpan's closeSpan copies a std::string + // and can throw. A throwing constructor runs no destructor, so an uncompensated + // ++g_depth would stick forever — undoBlockOpen() reads true for the rest of the + // session, fxReclaimTick refuses every tick, and no later UndoBlock is ever + // outermost. Roll the depth back on any throw so the type degrades to "this one + // block failed to open," not "every block after it is now poisoned." + try { + if (g_depth++ == 0) fxReclaimEndUndoSpan(); + Undo_BeginBlock2(proj); + } catch (...) { + --g_depth; + throw; + } } UndoBlock::~UndoBlock() { // Depth drops AFTER the SDK call, not before: the close that reaches depth 0 mints a // point, and anything re-entered through that must still read a block as open rather // than let the drain open a span underneath it. - Undo_EndBlock2(project_, label_.c_str(), mask_); + // + // Guarded the same way fx_reclaim.cpp's closeSpan guards its own Undo_EndBlock2, but + // only for a REAL pinned pointer: `project_ == nullptr` means "the active project", + // the same convention Undo_BeginBlock2 was handed, and ValidatePtr2 does not accept + // nullptr as a valid object of any type — guarding it unconditionally would silently + // stop every `UndoBlock undo(nullptr)` site (most of them) from ever closing. A real + // pointer, by contrast, can go stale: several call sites allocate freely between open + // and close, and the project it named may have closed underneath by now. + if (!project_ || ValidatePtr2(nullptr, project_, "ReaProject*")) + Undo_EndBlock2(project_, label_.c_str(), mask_); --g_depth; } diff --git a/src/shell/undo/undo_block.h b/src/shell/undo/undo_block.h index 7e89723..ef99eb5 100644 --- a/src/shell/undo/undo_block.h +++ b/src/shell/undo/undo_block.h @@ -3,9 +3,12 @@ // opening, so no block can ever nest over it, and the close is a destructor, so no throw // can leave one open in the user's project. // -// Enforced by the SDK's own opt-in: only this TU and fx_reclaim.cpp (which owns the span -// itself) define REAPERAPI_WANT_Undo_BeginBlock2, so `Undo_BeginBlock2` does not even -// name a declared symbol anywhere else in the tree. +// The SDK's WANT opt-in stops a raw Undo_BeginBlock2/_EndBlock2 call in every OTHER TU — +// only this file and fx_reclaim.cpp (the span's own owner) request them, so the symbol is +// undeclared elsewhere. app/main.cpp is the one exception: the sole REAPERAPI_IMPLEMENT TU +// cannot set REAPERAPI_MINIMAL (it must allocate storage for whatever ANY TU wants), so a +// raw call compiles clean there too — kept out by discipline (src/app/CLAUDE.md), not by +// the compiler. #include @@ -16,8 +19,10 @@ namespace reasampler { class UndoBlock { public: - // `proj` is nullptr for the active project, matching Undo_BeginBlock2's own contract - // (reaper_plugin_functions.h:7753). + // `proj` is nullptr for the active project — the general REAPER convention for a + // ReaProject* parameter. reaper_plugin_functions.h documents no proj semantics at all + // for Undo_BeginBlock2/_EndBlock2 (the line once cited here was the #if WANT guard, + // not a doc comment). `[verify — DAW]` explicit UndoBlock(ReaProject* proj); ~UndoBlock(); diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index 1840215..1f7f005 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -159,6 +159,15 @@ decide membership or mode rules. The mirror half is still `applyMode`'s: a REAPPLY never closes a span, precisely because it is reachable from inside one of those blocks — only a real switch supersedes, and a real switch is always fired at depth 0. + **Consequently, a span is never open at depth > 0.** The only two openers are a real + switch (always fired straight from an action handler, never from inside another + block) and the reclaimer's own lazy `writeOffline`, which the residual above makes + unreachable at depth > 0. This is what lets `fxReclaimAbandonForStateLoad` and + `fxReclaimFlushRelease` close the span unconditionally with no depth check of their + own, even when the hook that calls them fires from inside an extension `UndoBlock` — + a capture's own save can (`capture_orchestrator`'s `RunCaptureItemAssign` wraps + `RunCapture`, which reaches `Main_SaveProject`). `closeSpan()` there just finds + nothing open. ACCEPTED, and not fixed: `mintManagedLanes` opens its block once its plan is non-empty, but a non-empty plan whose writes are all already satisfied still mints nothing — so a background detect tick can end a switch's span having competed with it diff --git a/src/shell/view/fx_reclaim.cpp b/src/shell/view/fx_reclaim.cpp index 9b3d4cc..fc6979c 100644 --- a/src/shell/view/fx_reclaim.cpp +++ b/src/shell/view/fx_reclaim.cpp @@ -221,6 +221,11 @@ FxReclaimActivity fxReclaimActivity() { // The pin is only re-tested at beginTick; a poll landing between two ticks can find the // project already gone, and GetPlayStateEx is about to be handed this pointer. if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return FxReclaimActivity::Idle; + // A FOURTH stand-off, alongside no-project/invalid-project/transport: fxReclaimTick + // refuses the tick outright while undoBlockOpen(), so nothing may move for as long as + // one is open — the same "Idle whenever nothing may move" promise this reports for the + // debounce hold and the transport gate. + if (undoBlockOpen()) return FxReclaimActivity::Idle; return g_queue.activity(monotonicMs(), transportBlocksModeSwitch(proj)); } @@ -247,7 +252,10 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) { // Refused whole rather than written unspanned, which would instead absorb the FX // toggles into the user's act. Starves nothing: the wakeup, the derived queue and the // project pin are all untouched (this returns before beginTick), so the first tick - // after the block closes drains exactly as if this one had never been due. + // after the block closes drains exactly as if this one had never been due. That last + // claim is this function's own control-flow order — this TU is REAPER-linked, off the + // pure fx_reclaim_tests target, so unlike the queue/span state machine above it, the + // ordering is DAW-verified, not unit-tested. if (undoBlockOpen()) return; DrainLatch latch; @@ -293,7 +301,11 @@ void fxReclaimEndUndoSpan() { closeSpan(); } void fxReclaimAbandonForStateLoad() { // Close FIRST and unconditionally. The abandon below stops the drain and puts the loop // to sleep; the block is what a Ctrl-Z would otherwise close over the wrong state, and - // it is open whether or not a drain is on the stack at all. + // it is open whether or not a drain is on the stack at all. Safe even if this hook were + // reachable from inside an extension UndoBlock (see fxReclaimFlushRelease below for a + // confirmed case of that shape on the save side) — the span can never be open at depth + // > 0 in the first place: src/shell/view/CLAUDE.md's "NOTHING MAY OPEN AN UNDO BLOCK + // WHILE THE SPAN IS OPEN" invariant. closeSpan() just finds nothing to close there. closeSpan(); g_queue.abandon(); } @@ -307,7 +319,9 @@ void fxReclaimFlushRelease(ReaSamplerSession& session) { // different project saved and write into that wrong project's ext state. ReaProject* saving = GetCurrentProjectInLoadSave(); // BEFORE the arm test, and unconditionally: a save is a user act the span must not - // absorb, and the arm is not what decides whether one is open. + // absorb, and the arm is not what decides whether one is open. Same "depth > 0 never + // holds an open span" invariant as fxReclaimAbandonForStateLoad above makes this safe + // even when the save itself is issued from inside an extension UndoBlock. closeSpan(); if (!g_releaseFlush.pendingFor(saving)) return; // Promptless, unlike design_view_actions' persistViewState, which can open a modal diff --git a/tests/test_fx_reclaim.cpp b/tests/test_fx_reclaim.cpp index 9eb7273..b37c5a1 100644 --- a/tests/test_fx_reclaim.cpp +++ b/tests/test_fx_reclaim.cpp @@ -1478,7 +1478,19 @@ static void testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep() { // 2. The state-load terminator. `abandon` is the one move that clears the wakeup with // work still owed, which is what makes the close-FIRST ordering load-bearing rather - // than tidy — reverse the two and the loop is asleep under an open block. + // than tidy. Proved both ways, not asserted by construction: the WRONG order below + // (abandon, then close) is caught mid-sequence by the same predicate the RIGHT order + // is checked against — swapping the two lines here would make the first CHECK below + // fail, which is the teeth `fxReclaimAbandonForStateLoad` closing FIRST relies on. + { + FxReclaimQueue q; + UndoSpan span; + q.invalidate(); + span.begin(kProjA, "ReaSampler: activate Design view", 9); + q.abandon(); // WRONG order: wakeup cleared before the span closes + CHECK(asleepWithTheSpanOpen(q, span)); // the exact leak the real order avoids + span.end(); + } { FxReclaimQueue q; UndoSpan span; @@ -1492,8 +1504,13 @@ static void testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep() { // 3. The project moving out from under the span. No active project sleeps the loop // (beginTick's nullptr arm), a tab switch does not — but both close, because the - // span is keyed on its OWN project and neither one is it. - for (ReaProject* arrived : {static_cast(nullptr), kProjB}) { + // span is keyed on its OWN project and neither one is it. kProjA is the control: a + // tick landing back on the SAME project must NOT close the span, or a live switch's + // block would drop the flags-and-items mask it opened with mid-drain. Sweeping all + // three, with a branch-dependent CHECK, is what gives the guard real teeth — the + // original two-value sweep always took the close branch, so the no-close branch was + // never exercised. + for (ReaProject* arrived : {kProjA, static_cast(nullptr), kProjB}) { FxReclaimQueue q; UndoSpan span; q.invalidate(); @@ -1503,21 +1520,16 @@ static void testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep() { if (span.project() != arrived) span.end(); q.beginTick(arrived); - CHECK(!span.open()); + if (arrived == kProjA) CHECK(span.open()); // same project: nothing to close + else CHECK(!span.open()); CHECK(!asleepWithTheSpanOpen(q, span)); } // 4. The terminators with no queue move at all — a project save, an extension unload, - // the next real switch superseding. Each is the idempotent close alone, so the - // wakeup is whatever it already was; the implication has to hold either way. - for (bool woken : {false, true}) { - FxReclaimQueue q; - UndoSpan span; - if (woken) q.invalidate(); - span.begin(kProjA, "ReaSampler: activate Design view", 9); - span.end(); - CHECK(!asleepWithTheSpanOpen(q, span)); - } + // the next real switch superseding. Each is the idempotent close alone, with no + // guard and nothing else to sweep, so it reduces to end()'s own idempotency + // (testTheSpanOpensOnceAndClosesOnceHoweverManyTimesItIsAsked) rather than adding a + // separate claim here. } int main() {