fix(audio): close the PR #107 review findings

Seven fixes on top of the audio-engine TLC branch, each with the gate that catches its regression. Blocking: - Monitor-mute suppression leaked its refcount. setMonitorMuteSuppressed() became a refcounted acquire/release, but screen.js's callers are deliberately unpaired: resolveChainRebuildGuard() leaves the suppression on when a rebuild yields an empty chain, and returns early without releasing while a provider route is still resolving. Harmless against the old latched bool, a permanent +1 each against a refcount — after a failed tone rebuild the count never returned to zero and monitor mute was silently dead for the rest of the session. The renderer now holds at most one suppression. - Slot ids are monotonic HANDLES (nextSlotId, never reset by clear()), not bounded indices, so argSlotId's 4096 ceiling meant that once a session created its 4096th processor EVERY guarded binding — setBypass, setParameter, remove/moveProcessor, open/closePluginEditor — silently no-opped for the rest of the run. Ceiling removed (same for SetMultiBypass's hardcoded 4096); unknown ids are still rejected by SignalChain::findSlotIndex. - clearChain / removeProcessor / moveProcessor took chainMutationMutex with a blocking lock_guard on the N-API thread — Electron's main thread, and on macOS also the JUCE message thread. LoadPreset/LoadVST hold that mutex across an unbounded plugin init (done->wait() has no timeout by design), so a slow plugin froze the whole main process, every IPC channel with it. They now queue on a libuv worker via queueChainMutation() and resolve a promise; the bridge awaits them so callers still observe the mutation applied. Also: - getChainState() dereferenced raw ProcessorSlot* returned by getAllSlots() after the lock was dropped — a concurrent clear() frees them under the reader. Replaced with SignalChain::getSlotSummaries(), which copies under the lock. getAllSlots() is gone (it had one caller). - The device-settings migration removed the localStorage copy even when the file-store save failed or was unavailable, losing the user's settings. - SetSlotState and GetParameters kept the raw Int32Value() path: IsNumber() is true for NaN, so setSlotState(NaN) wrote onto slot 0 — the same coercion class the rest of the branch fixed. - RendererBus flushed to the LIVE writeIndex, so a disable→re-enable with no pull in between discarded the freshly pushed audio along with the stale tail. It now snapshots the flush target at disable time. - LoadPreset's rebuild barrier is now released by a scope guard, so a throw between arming it and Queue() can't block editor opens forever. Gates: new renderer-bus case (fails on the old flush), new slot-id-handle case (fails on the old ceiling). ctest 9/9, npm test 79 pass / 0 fail, chain-mutation storm green, addon export contract unchanged.
2026-09-10 22:14:10 +00:00 · 2026-07-14 14:29:03 +02:00
parent 1ba9b59e8a
commit a332c35c9b
11 changed files with 362 additions and 80 deletions
@@ -700,12 +700,30 @@ const ProcessorSlot* SignalChain::getSlot(int slotId) const
    return idx >= 0 ? slots[idx] : nullptr;
 }

-juce::Array<const ProcessorSlot*> SignalChain::getAllSlots() const
+std::vector<SignalChain::SlotSummary> SignalChain::getSlotSummaries() const
 {
-    juce::Array<const ProcessorSlot*> result;
+    std::vector<SlotSummary> result;
    const juce::ScopedLock sl(lock);
+    result.reserve((size_t) slots.size());
    for (auto* slot : slots)
-        result.add(slot);
+    {
+        SlotSummary s;
+        s.id        = slot->id;
+        s.type      = (int) slot->type;
+        s.name      = slot->name;
+        s.path      = slot->path;
+        s.bypassed  = slot->bypassed;
+        s.pan       = slot->pan;
+        s.branch    = slot->branch;
+        s.branchSrc = slot->branchSrc;
+        s.postGain  = slot->postGain;
+        // Safe under `lock`: clear() detaches the slots under the same lock
+        // before destroying them, so a slot reachable here cannot be freed
+        // mid-call. The RT thread uses a ScopedTryLock, so holding it for this
+        // metadata copy never blocks the audio callback.
+        s.hasEditor = slot->processor != nullptr && slot->processor->hasEditor();
+        result.push_back(std::move(s));
+    }
    return result;
 }

@@ -2,6 +2,7 @@
 #include <juce_audio_processors/juce_audio_processors.h>
 #include <juce_dsp/juce_dsp.h>
 #include <array>
+#include <vector>

 // Represents a single processor slot in the signal chain.
 // Can hold a VST3/AU/LV2 plugin, NAM model, or IR loader.
@@ -95,7 +96,26 @@ public:
    // Info
    int getNumSlots() const;
    const ProcessorSlot* getSlot(int slotId) const;
-    juce::Array<const ProcessorSlot*> getAllSlots() const;
+    // Metadata for every slot, copied UNDER the lock. Replaces getAllSlots(),
+    // which handed raw ProcessorSlot* back to the caller after dropping the
+    // lock: getChainState() then dereferenced them (down to
+    // processor->hasEditor()) while a concurrent clear()/loadPreset could free
+    // the slots underneath — a read-side use-after-free on the very rebuild
+    // window the chain-mutation serializer exists to police.
+    struct SlotSummary
+    {
+        int id = 0;
+        int type = 0;
+        juce::String name;
+        juce::String path;
+        bool bypassed = false;
+        float pan = 0.0f;
+        int branch = 0;
+        int branchSrc = 0;
+        float postGain = 1.0f;
+        bool hasEditor = false;
+    };
+    std::vector<SlotSummary> getSlotSummaries() const;
    // Current prepared playback format — used to prepare a processor that is
    // swapped in mid-session (replaceProcessor) at the same rate as the chain.
    double getCurrentSampleRate() const { return currentSampleRate; }
@@ -20,38 +20,40 @@ namespace slopsmith::addon {

 // ── Signal Chain Management ──────────────────────────────────────────────────

-// Pending in-process loads: each LoadVSTWorker / LoadPresetWorker that's
-// currently blocked on `done->wait()` registers its event here. doShutdown
-// signals them all so the workers unblock and return a clean "cancelled"
-// error instead of hanging forever when the JUCE message thread is about
-// to be stopped (and any unfired callback would never arrive).
+// The chain mutators resolve a promise instead of returning synchronously: the
+// chain-mutation mutex can be held for the length of a plugin init, and waiting
+// for it on the JS thread would freeze the main process (see ChainOps.h). Every
+// caller already reaches these through ipcRenderer.invoke, so the await is free.
+static Napi::Value resolvedBool(Napi::Env env, bool value)
+{
+    auto deferred = Napi::Promise::Deferred::New(env);
+    deferred.Resolve(Napi::Boolean::New(env, value));
+    return deferred.Promise();
+}
+
 Napi::Value RemoveProcessor(const Napi::CallbackInfo& info)
 {
    // Typed extractors (addon/NapiHelpers.h): NaN/Inf slot ids used to coerce
    // to slot 0 and mutate the wrong slot (deep-read §2) — now a clean no-op.
-    auto liveEngine = snapshotEngine();
+    auto env = info.Env();
    const auto slotId = slopsmith::addon::argSlotId(info, 0);
-    if (liveEngine && slotId)
-    {
-        std::lock_guard<std::mutex> chainLock(slopsmith::addon::chainMutationMutex());
-        liveEngine->getSignalChain().removeProcessor(*slotId);
-        slopsmith::addon::bumpChainGeneration();
-    }
-    return info.Env().Undefined();
+    if (!slotId) return resolvedBool(env, false);
+    const int id = *slotId;
+    return slopsmith::addon::queueChainMutation(env, [id](AudioEngine& eng) {
+        eng.getSignalChain().removeProcessor(id);
+    });
 }

 Napi::Value MoveProcessor(const Napi::CallbackInfo& info)
 {
-    auto liveEngine = snapshotEngine();
+    auto env = info.Env();
    const auto from = slopsmith::addon::argSlotId(info, 0);
    const auto to = slopsmith::addon::argSlotId(info, 1);
-    if (liveEngine && from && to)
-    {
-        std::lock_guard<std::mutex> chainLock(slopsmith::addon::chainMutationMutex());
-        liveEngine->getSignalChain().moveProcessor(*from, *to);
-        slopsmith::addon::bumpChainGeneration();
-    }
-    return info.Env().Undefined();
+    if (!from || !to) return resolvedBool(env, false);
+    const int f = *from, t = *to;
+    return slopsmith::addon::queueChainMutation(env, [f, t](AudioEngine& eng) {
+        eng.getSignalChain().moveProcessor(f, t);
+    });
 }

 Napi::Value SetBypass(const Napi::CallbackInfo& info)
@@ -74,35 +76,34 @@ Napi::Value SetBypass(const Napi::CallbackInfo& info)

 Napi::Value ClearChain(const Napi::CallbackInfo& info)
 {
+    auto env = info.Env();
+
    // Gate editor opens for the whole teardown+clear window (see the rebuild
    // barrier in ChainOps.h): without it, an editor opened between the
    // teardown below and the clear acquiring the mutex would point at a
    // processor the clear is about to free.
    slopsmith::addon::beginChainRebuild();
-    struct BarrierRelease {
-        ~BarrierRelease() { slopsmith::addon::endChainRebuild(); }
-    } barrierRelease;

-    // Tear editors down before their processors are freed just below (#56).
+    // Tear editors down before their processors are freed (#56). Must happen on
+    // THIS thread (main / message thread), not on the mutation worker: JUCE GUI
+    // objects may only be destroyed on the message thread.
    if (!closeAllPluginEditorWindows())
    {
        // Teardown refused/timed out: an editor may still be bound to a chain
        // processor. Clearing now would free it under the live editor — the
        // documented UAF. Skip the clear; the caller can retry.
+        slopsmith::addon::endChainRebuild();
        fprintf(stderr, "[audio-native] clearChain: editor teardown did not complete; "
                        "chain left untouched\n");
-        return info.Env().Undefined();
+        return resolvedBool(env, false);
    }
-    if (auto liveEngine = snapshotEngine())
-    {
-        // Serialized with the async chain workers (deep-read 1). May block
-        // briefly behind an in-flight preset/VST load -- that wait IS the fix
-        // for the interleaved clear-vs-rebuild corruption.
-        std::lock_guard<std::mutex> chainLock(slopsmith::addon::chainMutationMutex());
-        liveEngine->getSignalChain().clear();
-        slopsmith::addon::bumpChainGeneration();
-    }
-    return info.Env().Undefined();
+
+    // The worker now owns the barrier and releases it on every exit path. It
+    // takes the chain mutex on a libuv thread, so an in-flight preset/VST load
+    // delays the clear without blocking the JS thread behind it.
+    return slopsmith::addon::queueChainMutation(env, [](AudioEngine& eng) {
+        eng.getSignalChain().clear();
+    }, /*releasesRebuildBarrier=*/true);
 }

 // Stereo routing (St-1). setPan(slotId, -1..+1); setBranch(slotId, 0=trunk/>=1).
@@ -173,20 +174,23 @@ Napi::Value GetChainState(const Napi::CallbackInfo& info)

    if (liveEngine)
    {
-        auto slots = liveEngine->getSignalChain().getAllSlots();
-        for (int i = 0; i < slots.size(); ++i)
+        // Summaries are copied under SignalChain's lock — the old getAllSlots()
+        // handed back raw slot pointers that a concurrent clear()/loadPreset
+        // could free before this loop dereferenced them.
+        const auto slots = liveEngine->getSignalChain().getSlotSummaries();
+        for (size_t i = 0; i < slots.size(); ++i)
        {
            auto obj = Napi::Object::New(env);
-            obj.Set("id", slots[i]->id);
-            obj.Set("type", (int)slots[i]->type);
-            obj.Set("name", slots[i]->name.toStdString());
-            obj.Set("path", slots[i]->path.toStdString());
-            obj.Set("bypassed", slots[i]->bypassed);
-            obj.Set("pan", slots[i]->pan);
-            obj.Set("branch", slots[i]->branch);
-            obj.Set("branchSrc", slots[i]->branchSrc);
-            obj.Set("postGain", slots[i]->postGain);
-            obj.Set("hasEditor", slots[i]->processor && slots[i]->processor->hasEditor());
+            obj.Set("id", slots[i].id);
+            obj.Set("type", slots[i].type);
+            obj.Set("name", slots[i].name.toStdString());
+            obj.Set("path", slots[i].path.toStdString());
+            obj.Set("bypassed", slots[i].bypassed);
+            obj.Set("pan", slots[i].pan);
+            obj.Set("branch", slots[i].branch);
+            obj.Set("branchSrc", slots[i].branchSrc);
+            obj.Set("postGain", slots[i].postGain);
+            obj.Set("hasEditor", slots[i].hasEditor);
            result.Set((uint32_t)i, obj);
        }
    }
@@ -200,10 +204,10 @@ Napi::Value GetParameters(const Napi::CallbackInfo& info)
 {
    auto env = info.Env();
    auto liveEngine = snapshotEngine();
-    if (!liveEngine || info.Length() < 1) return Napi::Array::New(env);
+    const auto slotId = slopsmith::addon::argSlotId(info, 0);
+    if (!liveEngine || !slotId) return Napi::Array::New(env);

-    int slotId = info[0].As<Napi::Number>().Int32Value();
-    auto params = liveEngine->getSignalChain().getParameters(slotId);
+    auto params = liveEngine->getSignalChain().getParameters(*slotId);
    auto result = Napi::Array::New(env, params.size());

    for (int i = 0; i < params.size(); ++i)
@@ -235,19 +239,22 @@ Napi::Value SetParameter(const Napi::CallbackInfo& info)
 Napi::Value SetSlotState(const Napi::CallbackInfo& info)
 {
    // Type-guard both args (NAPI_DISABLE_CPP_EXCEPTIONS): a malformed IPC
-    // payload is a clean no-op rather than a hard addon failure.
+    // payload is a clean no-op rather than a hard addon failure. The slot id
+    // goes through argSlotId like every other mutator — IsNumber() is true for
+    // NaN, and Int32Value() would have coerced it to slot 0 and written this
+    // state onto a real slot (deep-read §2).
    auto liveEngine = snapshotEngine();
-    if (liveEngine && info.Length() >= 2 && info[0].IsNumber() && info[1].IsString())
+    const auto slotId = slopsmith::addon::argSlotId(info, 0);
+    if (liveEngine && slotId && info.Length() >= 2 && info[1].IsString())
    {
-        int slotId = info[0].As<Napi::Number>().Int32Value();
        auto base64 = info[1].As<Napi::String>().Utf8Value();
-        const auto* slot = liveEngine->getSignalChain().getSlot(slotId);
+        const auto* slot = liveEngine->getSignalChain().getSlot(*slotId);
        const bool allowStandard = slot != nullptr
            && (slot->type == ProcessorSlot::Type::IR
                || slot->type == ProcessorSlot::Type::NAM);
        juce::MemoryBlock mb;
        if (decodeStateBlob(juce::String(base64), mb, allowStandard))
-            liveEngine->getSignalChain().setSlotState(slotId, mb);
+            liveEngine->getSignalChain().setSlotState(*slotId, mb);
    }
    return info.Env().Undefined();
 }
@@ -283,8 +290,12 @@ Napi::Value SetMultiBypass(const Napi::CallbackInfo& info)
        auto slotVal = item.Get("slotId");
        auto bypVal = item.Get("bypassed");
        if (!slotVal.IsNumber() || !bypVal.IsBoolean()) continue;
+        // Same rule as argSlotId: reject the NaN/Inf/fractional class, but do
+        // NOT impose an index ceiling — slot ids are monotonic handles, not
+        // indices (see NapiHelpers.h).
        const double raw = slotVal.As<Napi::Number>().DoubleValue();
-        if (!std::isfinite(raw) || raw != std::floor(raw) || raw < 0.0 || raw > 4096.0) continue;
+        if (!std::isfinite(raw) || raw != std::floor(raw)
+            || raw < 0.0 || raw > (double) std::numeric_limits<int>::max()) continue;
        changes.add({ (int) raw, bypVal.As<Napi::Boolean>().Value() });
    }

@@ -63,6 +63,64 @@ bool isChainRebuildPending()
    return chainRebuildsPending.load(std::memory_order_acquire) > 0;
 }

+// ── Async chain mutation (see ChainOps.h) ───────────────────────────────────
+// The synchronous mutators (clearChain / removeProcessor / moveProcessor) used
+// to take chainMutationMutex with a blocking lock_guard on the N-API thread.
+// That thread is Electron's main thread — and on macOS it is also the JUCE
+// message thread — so waiting there behind a LoadPreset worker that holds the
+// mutex across an unbounded plugin init froze the app (or deadlocked the pump
+// the load needs). Queue the mutation instead and let the worker do the waiting.
+
+namespace {
+
+class ChainMutationWorker : public Napi::AsyncWorker
+{
+public:
+    ChainMutationWorker(Napi::Env env, Napi::Promise::Deferred deferred,
+                        std::function<void(AudioEngine&)> mutate, bool releasesBarrier)
+        : Napi::AsyncWorker(env)
+        , deferred_(deferred)
+        , mutate_(std::move(mutate))
+        , releasesBarrier_(releasesBarrier) {}
+
+    void Execute() override
+    {
+        struct BarrierRelease {
+            bool armed;
+            ~BarrierRelease() { if (armed) slopsmith::addon::endChainRebuild(); }
+        } barrierRelease{ releasesBarrier_ };
+
+        std::lock_guard<std::mutex> chainLock(slopsmith::addon::chainMutationMutex());
+        auto liveEngine = snapshotEngine();
+        if (!liveEngine) return;            // ok_ stays false
+        mutate_(*liveEngine);
+        slopsmith::addon::bumpChainGeneration();   // still under chainLock
+        ok_ = true;
+    }
+
+    void OnOK() override { deferred_.Resolve(Napi::Boolean::New(Env(), ok_)); }
+    void OnError(const Napi::Error& e) override { deferred_.Reject(e.Value()); }
+
+private:
+    Napi::Promise::Deferred deferred_;
+    std::function<void(AudioEngine&)> mutate_;
+    bool releasesBarrier_ = false;
+    bool ok_ = false;
+};
+
+} // namespace
+
+Napi::Value queueChainMutation(Napi::Env env,
+                               std::function<void(AudioEngine&)> mutate,
+                               bool releasesRebuildBarrier)
+{
+    auto deferred = Napi::Promise::Deferred::New(env);
+    auto* worker = new ChainMutationWorker(env, deferred, std::move(mutate),
+                                           releasesRebuildBarrier);
+    worker->Queue();
+    return deferred.Promise();
+}
+
 // ── decodeStateBlob (moved verbatim) ────────────────────

 // Decode a state blob that may be in EITHER base64 flavour. JUCE's
@@ -904,11 +962,18 @@ Napi::Value LoadPreset(const Napi::CallbackInfo& info)
    // returning and the queued worker acquiring chainMutationMutex, nothing
    // else stops OpenPluginEditor from opening a fresh editor whose processor
    // the worker is about to free (#56). The barrier gates editor opens for
-    // the whole teardown+rebuild window; the worker releases it on every
-    // Execute() exit path. Nothing between here and Queue() can throw: the
-    // teardown-failure path below releases explicitly, and the worker's
-    // BarrierRelease guard covers every Execute() exit.
+    // the whole teardown+rebuild window.
+    //
+    // Ownership passes to the worker (whose BarrierRelease covers every
+    // Execute() exit) only once Queue() has actually taken it. Until then this
+    // guard holds it, so any early return — or a throwing allocation — releases
+    // instead of leaking a barrier that would block editor opens forever.
    slopsmith::addon::beginChainRebuild();
+    bool barrierHandedOff = false;
+    struct BarrierGuard {
+        const bool& handedOff;
+        ~BarrierGuard() { if (!handedOff) slopsmith::addon::endChainRebuild(); }
+    } barrierGuard{ barrierHandedOff };

    // Tear down any open in-process editor windows NOW, on the N-API/main
    // thread, before the AsyncWorker frees the chain's processors on a libuv
@@ -923,7 +988,6 @@ Napi::Value LoadPreset(const Napi::CallbackInfo& info)
        // bound to a chain processor. Clearing/rebuilding now would free that
        // processor under the live editor — the documented UAF. Abort the
        // load instead of proceeding.
-        slopsmith::addon::endChainRebuild();
        auto obj = Napi::Object::New(env);
        obj.Set("success", false);
        obj.Set("error", "editor teardown did not complete; preset load aborted");
@@ -933,6 +997,7 @@ Napi::Value LoadPreset(const Napi::CallbackInfo& info)

    auto worker = new LoadPresetWorker(env, deferred, std::move(json));
    worker->Queue();
+    barrierHandedOff = true;
    return deferred.Promise();
 }

@@ -26,6 +26,7 @@
 #include <juce_core/juce_core.h>

 #include <cstdint>
+#include <functional>
 #include <memory>
 #include <mutex>

@@ -48,6 +49,27 @@ uint64_t currentChainGeneration();
 //   ... clear/rebuild/add ...
 //   const uint64_t gen = bumpChainGeneration();   // still under the lock
 //   (return gen in the result object)
+//
+// ...but ONLY from a libuv worker thread. NOTHING may take this mutex with a
+// blocking lock on the N-API/JS thread: LoadPresetWorker holds it across the
+// full clear+rebuild, which includes an unbounded in-process plugin init
+// (loadVstSandboxAware's done->wait() has no timeout — a slow first-run plugin
+// is allowed to take as long as it needs). A blocking lock on the JS thread
+// would therefore freeze the whole Electron main process — every IPC channel
+// with it — for the duration of a plugin load, and on macOS (where the N-API
+// thread IS the JUCE message thread — see AddonContext's startJuceMessageThread)
+// it would deadlock the very pump that load is waiting on. The message-thread
+// call sites in EditorWindows use try_to_lock for exactly this reason; the
+// synchronous chain mutators go through queueChainMutation instead.
+//
+// Run `mutate` on a libuv worker under chainMutationMutex(), bump the chain
+// generation, and resolve the returned promise with true (false if the engine
+// went away). When `releasesRebuildBarrier`, the worker calls endChainRebuild()
+// on every exit path — the caller must have armed it with beginChainRebuild()
+// before tearing editors down.
+Napi::Value queueChainMutation(Napi::Env env,
+                               std::function<void(AudioEngine&)> mutate,
+                               bool releasesRebuildBarrier = false);

 // ── Rebuild barrier (editor-open gate) ────────────────────────────────────
 // A chain clear/rebuild is a two-step dance: editors are torn down on the
@@ -13,12 +13,13 @@
 #include <napi.h>

 #include <cmath>
+#include <limits>
 #include <optional>

 namespace slopsmith::addon {

 // Finite integer in [minV, maxV]. The 4096 default ceiling keeps the cast
-// well-defined for id-shaped args (slot ids, source ids, indices).
+// well-defined for index-shaped args (channel/branch indices and the like).
 inline std::optional<int> argInt(const Napi::CallbackInfo& info, size_t i,
                                 int minV = 0, int maxV = 4096)
 {
@@ -30,9 +31,19 @@ inline std::optional<int> argInt(const Napi::CallbackInfo& info, size_t i,
 }

 // Slot / source / param-index ids: finite non-negative integers.
+//
+// NOT bounded by argInt's 4096 index ceiling. A slot id is a monotonic HANDLE
+// from SignalChain::nextSlotId, which increments on every addProcessor and is
+// never reset by clear() — a long session (each song load and mid-song tone
+// switch rebuilds a chainful of slots) walks past 4096, and a ceiling here
+// would then make every guarded binding — setBypass, setParameter, remove/
+// moveProcessor, open/closePluginEditor — silently no-op for the rest of the
+// run. Ids that don't name a live slot are rejected by SignalChain's own
+// findSlotIndex; the job here is only to keep the NaN/Inf/fractional class out
+// (Int32Value() coerces NaN to 0, i.e. a real slot).
 inline std::optional<int> argSlotId(const Napi::CallbackInfo& info, size_t i)
 {
-    return argInt(info, i);
+    return argInt(info, i, 0, std::numeric_limits<int>::max());
 }

 // Finite float (parameter values, gains, pans). Range clamping stays with
@@ -48,6 +48,17 @@ public:
            // pull mid-drain could overwrite it with r + pull, replaying a
            // stale tail after re-enable, exactly what the drop was meant to
            // prevent. Only the consumer ever moves readIndex now.
+            //
+            // Snapshot WHERE to flush to rather than letting the consumer flush
+            // to whatever writeIndex it happens to see. If no output callback
+            // runs between this disable and a re-enable (a stopped device, a
+            // device swap), the next pull would otherwise discard the FRESH
+            // frames pushed since the re-enable along with the stale tail —
+            // silence until the bus re-primes. Pushes are gated on busEnabled,
+            // so nothing lands in (flushTo, re-enable) and this index is exactly
+            // the end of the stale tail.
+            flushTo.store(ring.writeIndex.load(std::memory_order_acquire),
+                          std::memory_order_relaxed);
            flushRequested.store(true, std::memory_order_release);
            primed.store(false, std::memory_order_relaxed);
        }
@@ -113,9 +124,18 @@ public:
    {
        // Consume a pending flush FIRST — even while disabled — so the tail
        // buffered before a disable is dropped by the ring's one legitimate
-        // readIndex writer (this consumer), never by the control thread.
+        // readIndex writer (this consumer), never by the control thread. Flush
+        // to the index captured at DISABLE time, not to the live writeIndex:
+        // anything pushed after a re-enable is fresh audio, not stale tail.
        if (flushRequested.exchange(false, std::memory_order_acq_rel))
-            ring.commitRead(ring.writeIndex.load(std::memory_order_acquire));
+        {
+            const uint64_t target = flushTo.load(std::memory_order_relaxed);
+            // Guard the already-drained case: the consumer may have run past
+            // the snapshot before it saw the flag, and readIndex must never
+            // move backwards.
+            if (target > ring.readIndex.load(std::memory_order_relaxed))
+                ring.commitRead(target);
+        }
        if (!busEnabled.load(std::memory_order_acquire)) return 0;
        const uint64_t w = ring.writeIndex.load(std::memory_order_acquire);
        uint64_t r = ring.readIndex.load(std::memory_order_relaxed);
@@ -209,7 +229,10 @@ private:
    std::atomic<bool> primed{false};
    // Set by setEnabled(false) on the control thread, consumed (exchange) by
    // pull() — the drop-on-disable request, honored by the single consumer.
+    // flushTo is the writeIndex as of that disable: the exact end of the stale
+    // tail, so a re-enable's fresh frames survive the pending flush.
    std::atomic<bool> flushRequested{false};
+    std::atomic<uint64_t> flushTo{0};
    // Producer-thread-only linear-resampler state (fractional read position
    // into the incoming chunk + the previous chunk's last frame for
    // interpolation continuity across pushes).
@@ -1194,13 +1194,17 @@ export function initAudioBridge(): void {
        return await audio?.replaceIR(slotId, irPath, typeof gain === 'number' ? gain : -1) ?? false;
    });

-    ipcMain.handle('audio:removeProcessor', (_event, slotId: number) => {
-        audio?.removeProcessor(slotId);
+    // The native chain mutators are async now (they take the chain-mutation
+    // mutex on a worker thread rather than freezing the main process behind an
+    // in-flight plugin load — see ChainOps.h). Await them so a renderer that
+    // awaits this IPC and then re-reads the chain sees the mutation applied.
+    ipcMain.handle('audio:removeProcessor', async (_event, slotId: number) => {
+        await audio?.removeProcessor(slotId);
        vstSlotPaths.delete(slotId);
    });

-    ipcMain.handle('audio:moveProcessor', (_event, from: number, to: number) => {
-        audio?.moveProcessor(from, to);
+    ipcMain.handle('audio:moveProcessor', async (_event, from: number, to: number) => {
+        await audio?.moveProcessor(from, to);
    });

    ipcMain.handle('audio:setBypass', (_event, slotId: number, bypassed: boolean) => {
@@ -1221,8 +1225,8 @@ export function initAudioBridge(): void {
        audio?.setBranchSrc?.(slotId, src);
    });

-    ipcMain.handle('audio:clearChain', () => {
-        audio?.clearChain();
+    ipcMain.handle('audio:clearChain', async () => {
+        await audio?.clearChain();
        vstSlotPaths.clear();
    });

@@ -270,14 +270,23 @@ window.__feedBackDesktopAudioHooks = window.__feedBackDesktopAudioHooks || {};
        if (browserSettings !== null) {
            const browserNewer = !fileSettings
                || getDeviceSettingsSavedAt(browserSettings) > getDeviceSettingsSavedAt(fileSettings);
+            // Drop the browser copy ONLY once it is safely in the file store —
+            // deleting it after a failed (or unavailable) save would throw the
+            // user's device settings away for good.
+            let migrated = !browserNewer;
            if (browserNewer) {
                try {
-                    if (typeof api.saveDeviceSettings === 'function') await api.saveDeviceSettings(browserSettings);
+                    if (typeof api.saveDeviceSettings === 'function') {
+                        await api.saveDeviceSettings(browserSettings);
+                        migrated = true;
+                    }
                } catch (e) {
                    console.warn('[audio-engine] device-settings migration save failed:', e);
                }
            }
-            try { localStorage.removeItem('slopsmith-audio-device'); } catch (_) {}
+            if (migrated) {
+                try { localStorage.removeItem('slopsmith-audio-device'); } catch (_) {}
+            }
            if (browserNewer) return browserSettings;
        }
        return fileSettings;
@@ -4367,14 +4376,28 @@ window.__feedBackDesktopAudioHooks = window.__feedBackDesktopAudioHooks || {};
    // the preload below. While the chain is empty the native engine's monitor
    // mute would silence the dry guitar. Suppress the mute for the rebuild
    // window so the guitar keeps sounding; resolve it once the chain settles.
+    // The native side refcounts suppressions (SourceChain's monitor-mute
+    // arbiter): true = acquire, false = release. This latch keeps the renderer
+    // to AT MOST ONE outstanding suppression, because the guard below is
+    // deliberately unpaired — resolveChainRebuildGuard() leaves the suppression
+    // on when the rebuild produced an empty chain, and returns early without
+    // releasing while a provider route is still resolving. Under the old latched
+    // bool those were self-correcting (repeated trues were idempotent, any false
+    // reset it). Against a refcount each one would leak a permanent +1, and
+    // after a couple of song loads the count could never return to zero — monitor
+    // mute would be silently dead for the rest of the session.
+    let aeMonitorMuteSuppressionHeld = false;
    function aeSetMonitorMuteSuppressed(suppressed) {
+        const want = !!suppressed;
+        if (want === aeMonitorMuteSuppressionHeld) return;   // idempotent, like the old bool
+        aeMonitorMuteSuppressionHeld = want;
        const api = window.feedBackDesktop?.audio;
        // Optional-chained: a downlevel native addon simply ignores this.
        // setMonitorMuteSuppressed is async (ipcRenderer.invoke) — the sync
        // try/catch only covers a missing method, so also swallow the
        // returned promise's rejection to avoid an unhandled rejection.
        try {
-            const r = api?.setMonitorMuteSuppressed?.(suppressed);
+            const r = api?.setMonitorMuteSuppressed?.(want);
            if (r && typeof r.catch === 'function') r.catch(() => {});
        } catch (_) { /* downlevel */ }
    }
@@ -166,6 +166,35 @@ static void testFlushOnDisable()
    assert(dl[1] == 7.0f && "post-re-enable audio must be the fresh push");
 }

+// A pending flush must drop the STALE tail only. If no output callback runs
+// between the disable and a re-enable (stopped device, device swap), the
+// flush is still pending when fresh audio arrives — flushing to the live
+// writeIndex at that point would discard the re-enabled bus's first frames
+// too, silencing it until it re-primed. The flush target is snapshotted at
+// disable time instead.
+static void testFlushSparesPostReEnableAudio()
+{
+    RendererBus bus;
+    bus.setEnabled(true, 1.0f);
+    const auto stale = rampChunk(RendererBus::kPrimeFrames * 2, 5.0f, 0.0f);
+    bus.push(stale.data(), RendererBus::kPrimeFrames * 2, 48000.0, 48000.0);
+
+    // Disable + re-enable with NO pull in between: the flush is still pending.
+    bus.setEnabled(false, 1.0f);
+    bus.setEnabled(true, 1.0f);
+
+    // Fresh audio pushed while the flush is still pending must survive it.
+    const auto fresh = rampChunk(RendererBus::kPrimeFrames + 65, 7.0f, 0.0f);
+    bus.push(fresh.data(), RendererBus::kPrimeFrames + 65, 48000.0, 48000.0);
+
+    std::vector<float> dl(64), dr(64);
+    assert(bus.pull(dl.data(), dr.data(), 64) == 64
+           && "fresh post-re-enable audio must not be flushed away with the stale tail");
+    // Frame 0 is the resampler's one-frame interpolation carry (by design);
+    // everything after must be the fresh push, never the flushed 5.0 tail.
+    assert(dl[1] == 7.0f && "flush must drop only the pre-disable tail");
+}
+
 // Rate validation (PR #107 review): non-finite rates cross the JS/IPC
 // boundary; NaN passes a plain `<= 0` check, and a subnormal source rate can
 // underflow step to 0 — both must be rejected before the resample loop.
@@ -202,6 +231,7 @@ int main()
    testDisabledIsInert();
    testGainApplied();
    testFlushOnDisable();
+    testFlushSparesPostReEnableAudio();
    std::puts("renderer_bus: all cases passed");
    return 0;
 }
@@ -26,7 +26,12 @@ function writeImpulseWav(file) {
    fs.writeFileSync(file, buf);
 }

-const GARBAGE = [NaN, Infinity, -Infinity, -1, 1.5, 4097, 'x', null, undefined, {}, []];
+// NB 2**31 (not 4097): a slot id is a monotonic HANDLE from nextSlotId, which
+// clear() never resets, so a long session legitimately hands out ids past any
+// small ceiling — see the slot-id-handle test below. What must be rejected is
+// the NaN/Inf/fractional/negative/non-number class, plus ids that don't fit an
+// int32 at all.
+const GARBAGE = [NaN, Infinity, -Infinity, -1, 1.5, 2 ** 31, 'x', null, undefined, {}, []];

 test('chain-mutating bindings no-op on garbage args and never touch slot 0', { skip: !HAVE_ADDON && 'addon not built' }, async () => {
    const audio = require(ADDON);
@@ -73,3 +78,53 @@ test('chain-mutating bindings no-op on garbage args and never touch slot 0', { s
        fs.rmSync(tmp, { recursive: true, force: true });
    }
 });
+
+// PR #107 review: slot ids are monotonic HANDLES (SignalChain::nextSlotId,
+// never reset by clear()), not bounded indices. A ceiling in the N-API arg
+// guard meant that once a session had created its 4096th processor — a few
+// hundred song loads / tone switches, each rebuilding a chainful — EVERY
+// guarded binding (setBypass, setParameter, remove/moveProcessor, open/close
+// PluginEditor) silently no-opped for the rest of the run, with no error.
+//
+// Deliberately slow (~40s): the only way to observe the bug through the public
+// surface is to actually push nextSlotId past the old ceiling and then drive a
+// real slot. Batched as 21 x 210-slot presets so only 210 IRLoaders are ever
+// live at once.
+test('slot ids are handles, not indices — bindings still work past the old 4096 ceiling',
+    { skip: !HAVE_ADDON && 'addon not built' }, async () => {
+    const audio = require(ADDON);
+    audio.init();
+    const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'slot-handle-'));
+    const ir = path.join(tmp, 'i.wav');
+    writeImpulseWav(ir);
+    const SLOTS = 210, LOADS = 21;   // 4410 ids > the old 4096 ceiling
+    try {
+        let slots = [];
+        for (let i = 0; i < LOADS; i++) {
+            const res = await audio.loadPreset(JSON.stringify({
+                chain: Array.from({ length: SLOTS }, (_, k) => ({
+                    type: 2, name: `handle-${k}`, path: ir, bypassed: false,
+                })),
+            }));
+            assert.ok(res?.success, `preset ${i} must load`);
+            slots = audio.getChainState();
+        }
+
+        const maxId = Math.max(...slots.map((s) => s.id));
+        assert.ok(maxId > 4096, `expected a slot id past the old ceiling, got ${maxId}`);
+
+        // The regression: with an index ceiling on the arg guard this was a
+        // silent no-op and bypassed stayed false.
+        audio.setBypass(maxId, true);
+        assert.equal(audio.getChainState().find((s) => s.id === maxId)?.bypassed, true,
+            'setBypass on a >4096 slot id must apply, not silently no-op');
+
+        await audio.removeProcessor(maxId);
+        assert.equal(audio.getChainState().find((s) => s.id === maxId), undefined,
+            'removeProcessor on a >4096 slot id must apply');
+    } finally {
+        await audio.clearChain?.();
+        audio.shutdown?.();
+        fs.rmSync(tmp, { recursive: true, force: true });
+    }
+});