diff --git a/src/audio/SignalChain.cpp b/src/audio/SignalChain.cpp index 8f85c8a..e718e2e 100644 --- a/src/audio/SignalChain.cpp +++ b/src/audio/SignalChain.cpp @@ -700,12 +700,30 @@ const ProcessorSlot* SignalChain::getSlot(int slotId) const return idx >= 0 ? slots[idx] : nullptr; } -juce::Array SignalChain::getAllSlots() const +std::vector SignalChain::getSlotSummaries() const { - juce::Array result; + std::vector 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; } diff --git a/src/audio/SignalChain.h b/src/audio/SignalChain.h index 77e04a9..e04ee64 100644 --- a/src/audio/SignalChain.h +++ b/src/audio/SignalChain.h @@ -2,6 +2,7 @@ #include #include #include +#include // 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 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 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; } diff --git a/src/audio/addon/ChainBindings.cpp b/src/audio/addon/ChainBindings.cpp index f98df93..5102490 100644 --- a/src/audio/addon/ChainBindings.cpp +++ b/src/audio/addon/ChainBindings.cpp @@ -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 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 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 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().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().Int32Value(); auto base64 = info[1].As().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().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::max()) continue; changes.add({ (int) raw, bypVal.As().Value() }); } diff --git a/src/audio/addon/ChainOps.cpp b/src/audio/addon/ChainOps.cpp index 98ae709..c8ebfc2 100644 --- a/src/audio/addon/ChainOps.cpp +++ b/src/audio/addon/ChainOps.cpp @@ -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 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 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 mutate_; + bool releasesBarrier_ = false; + bool ok_ = false; +}; + +} // namespace + +Napi::Value queueChainMutation(Napi::Env env, + std::function 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(); } diff --git a/src/audio/addon/ChainOps.h b/src/audio/addon/ChainOps.h index 4a7d240..cf1a28b 100644 --- a/src/audio/addon/ChainOps.h +++ b/src/audio/addon/ChainOps.h @@ -26,6 +26,7 @@ #include #include +#include #include #include @@ -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 mutate, + bool releasesRebuildBarrier = false); // ── Rebuild barrier (editor-open gate) ──────────────────────────────────── // A chain clear/rebuild is a two-step dance: editors are torn down on the diff --git a/src/audio/addon/NapiHelpers.h b/src/audio/addon/NapiHelpers.h index e52511d..7815b0c 100644 --- a/src/audio/addon/NapiHelpers.h +++ b/src/audio/addon/NapiHelpers.h @@ -13,12 +13,13 @@ #include #include +#include #include 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 argInt(const Napi::CallbackInfo& info, size_t i, int minV = 0, int maxV = 4096) { @@ -30,9 +31,19 @@ inline std::optional 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 argSlotId(const Napi::CallbackInfo& info, size_t i) { - return argInt(info, i); + return argInt(info, i, 0, std::numeric_limits::max()); } // Finite float (parameter values, gains, pans). Range clamping stays with diff --git a/src/audio/engine/RendererBus.h b/src/audio/engine/RendererBus.h index 36f247e..2141de2 100644 --- a/src/audio/engine/RendererBus.h +++ b/src/audio/engine/RendererBus.h @@ -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 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 flushRequested{false}; + std::atomic 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). diff --git a/src/main/audio-bridge.ts b/src/main/audio-bridge.ts index c2d1866..c37f230 100644 --- a/src/main/audio-bridge.ts +++ b/src/main/audio-bridge.ts @@ -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(); }); diff --git a/src/renderer/screen.js b/src/renderer/screen.js index c7e5dae..444fea9 100644 --- a/src/renderer/screen.js +++ b/src/renderer/screen.js @@ -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 */ } } diff --git a/tests/engine_units/renderer_bus_test.cpp b/tests/engine_units/renderer_bus_test.cpp index 8e5fb8c..b26f8f9 100644 --- a/tests/engine_units/renderer_bus_test.cpp +++ b/tests/engine_units/renderer_bus_test.cpp @@ -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 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; } diff --git a/tests/napi-arg-fuzz.test.js b/tests/napi-arg-fuzz.test.js index 1beb593..3ac9f40 100644 --- a/tests/napi-arg-fuzz.test.js +++ b/tests/napi-arg-fuzz.test.js @@ -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 }); + } +});