From 354052bc7cec64aee7e5a3235185b5c9d33a7eb6 Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Thu, 16 Jul 2026 22:34:36 +0200 Subject: [PATCH 1/3] fix(audio): run device lifecycle ops on the JUCE message thread (Windows) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tester crash (dump 2026-07-15, Focusrite USB ASIO): the driver's deferred kAsioResetRequest fires a juce::Timer on the addon's JUCE message thread (ASIOAudioIODevice::timerCallback -> reloadChannelNames) while the Node thread concurrently destroys the device inside setAudioDevices/stopAudio — use-after-free, ~5 minutes after every launch. New runDeviceLifecycleOp() marshals every binding that can create or destroy a juce::AudioIODevice (setDevice, device-type switches, start/stop, stream output open/close, extra-input bind/unbind, add/removeSource) onto the message thread on Windows, serialising them with those timers. Inline on macOS (dispatch already inline) and Linux (ALSA main-thread contract unchanged), and inline when already on the message thread to avoid self-deadlock. Closures capture by value and return through shared_ptr so a timed-out dispatch that runs late can't touch the caller's dead stack. Co-Authored-By: Claude Fable 5 --- src/audio/addon/AddonContext.h | 38 +++++++++++++++++++++++ src/audio/addon/DetectionBindings.cpp | 38 ++++++++++++++++++++--- src/audio/addon/DeviceBindings.cpp | 44 +++++++++++++++++++++------ 3 files changed, 107 insertions(+), 13 deletions(-) diff --git a/src/audio/addon/AddonContext.h b/src/audio/addon/AddonContext.h index 2c2a1b9..882c05d 100644 --- a/src/audio/addon/AddonContext.h +++ b/src/audio/addon/AddonContext.h @@ -54,6 +54,44 @@ inline bool dispatchOnMessageThread(Func&& func) return dispatchOnMessageThreadImpl(std::function(std::forward(func))); } +// Run a device-lifecycle mutation (anything that can create or destroy a +// juce::AudioIODevice: setAudioDevices, start/stopAudio, device-type +// switches, stream-output open/close, extra-input add/remove) on the JUCE +// message thread. +// +// Why: on Windows, ASIO devices arm juce::Timers (the driver's deferred +// kAsioResetRequest, device-change detection) that fire on the message +// thread. Destroying the device from the Node thread while such a timer is +// queued or mid-callback is a use-after-free — tester crash 2026-07-15: +// ASIOAudioIODevice::timerCallback → reloadChannelNames on a freed device, +// ~5 min after start, every run (Focusrite USB ASIO reset request). +// Hopping the mutation onto the message thread serialises it with those +// timer callbacks, so a timer can never observe a half-destroyed device. +// +// Windows-only hop, by design: macOS's dispatchOnMessageThread already runs +// inline (no separate pump), and Linux/ALSA keeps its long-standing +// "called from the Node main thread" contract untouched. Inline when the +// caller already IS the message thread (engine init runs there), because +// dispatch-and-wait from the message thread would deadlock. +// +// Returns false when the dispatched work did not verifiably complete (post +// refused or 15 s timeout) — same contract as dispatchOnMessageThread. +// CAPTURE RULE: on timeout the queued closure may still run later, so the +// closure must own everything it touches — capture by value (engine +// snapshot, args) and write results through a shared_ptr, never through +// references to the caller's stack. +template +inline bool runDeviceLifecycleOp(Func&& func) +{ +#if JUCE_WINDOWS + if (auto* mm = juce::MessageManager::getInstanceWithoutCreating()) + if (!mm->isThisTheMessageThread()) + return dispatchOnMessageThread(std::forward(func)); +#endif + func(); + return true; +} + // Pending-async-load registry: LoadVSTWorker / LoadPresetWorker block on a // WaitableEvent until the message-thread continuation fires; doShutdown // signals every registered event so no worker waits forever once the pump diff --git a/src/audio/addon/DetectionBindings.cpp b/src/audio/addon/DetectionBindings.cpp index e82aeb8..ad95f0a 100644 --- a/src/audio/addon/DetectionBindings.cpp +++ b/src/audio/addon/DetectionBindings.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include namespace slopsmith::addon { @@ -506,7 +507,14 @@ Napi::Value AddSource(const Napi::CallbackInfo& info) const int k = info[1].As().Int32Value(); if (k >= 0) deviceKey = k; // negatives ignored → primary } - return Napi::Number::New(env, liveEngine->addSource(channel, deviceKey)); + // deviceKey != 0 opens an extra input AudioIODevice — device lifecycle, + // so it must run on the JUCE message thread (see runDeviceLifecycleOp). + auto sourceId = std::make_shared(-1); + if (!runDeviceLifecycleOp([liveEngine, channel, deviceKey, sourceId] { + *sourceId = liveEngine->addSource(channel, deviceKey); + })) + return Napi::Number::New(env, -1); + return Napi::Number::New(env, *sourceId); } // removeSource(sourceId) -> boolean. sources[0] cannot be removed. @@ -516,7 +524,15 @@ Napi::Value RemoveSource(const Napi::CallbackInfo& info) auto liveEngine = snapshotEngine(); if (!liveEngine || info.Length() < 1 || !info[0].IsNumber()) return Napi::Boolean::New(env, false); - return Napi::Boolean::New(env, liveEngine->removeSource(info[0].As().Int32Value())); + // May close the source's extra input AudioIODevice — message thread hop + // for the same reason as AddSource. + const int sourceId = info[0].As().Int32Value(); + auto removed = std::make_shared(false); + if (!runDeviceLifecycleOp([liveEngine, sourceId, removed] { + *removed = liveEngine->removeSource(sourceId); + })) + return Napi::Boolean::New(env, false); + return Napi::Boolean::New(env, *removed); } // listSources() -> [{ id, inputChannel, active }]. Null on a missing engine. @@ -572,7 +588,14 @@ Napi::Value BindInputDevice(const Napi::CallbackInfo& info) return Napi::String::New(env, "bindInputDevice(deviceKey:number, deviceName:string)"); const int deviceKey = info[0].As().Int32Value(); const std::string name = info[1].As().Utf8Value(); - return Napi::String::New(env, liveEngine->bindInputDevice(deviceKey, name).toStdString()); + // Opens a physical AudioIODevice — message thread hop (runDeviceLifecycleOp). + auto err = std::make_shared(); + if (!runDeviceLifecycleOp([liveEngine, deviceKey, name, err] { + *err = liveEngine->bindInputDevice(deviceKey, name); + })) + return Napi::String::New(env, + "bindInputDevice did not complete (message thread unavailable or timed out)"); + return Napi::String::New(env, err->toStdString()); } // unbindInputDevice(deviceKey) -> boolean. Stops + releases the extra device. @@ -582,7 +605,14 @@ Napi::Value UnbindInputDevice(const Napi::CallbackInfo& info) auto liveEngine = snapshotEngine(); if (!liveEngine || info.Length() < 1 || !info[0].IsNumber()) return Napi::Boolean::New(env, false); - return Napi::Boolean::New(env, liveEngine->unbindInputDevice(info[0].As().Int32Value())); + // Closes a physical AudioIODevice — message thread hop (runDeviceLifecycleOp). + const int deviceKey = info[0].As().Int32Value(); + auto unbound = std::make_shared(false); + if (!runDeviceLifecycleOp([liveEngine, deviceKey, unbound] { + *unbound = liveEngine->unbindInputDevice(deviceKey); + })) + return Napi::Boolean::New(env, false); + return Napi::Boolean::New(env, *unbound); } diff --git a/src/audio/addon/DeviceBindings.cpp b/src/audio/addon/DeviceBindings.cpp index f68d1d2..420e209 100644 --- a/src/audio/addon/DeviceBindings.cpp +++ b/src/audio/addon/DeviceBindings.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include namespace slopsmith::addon { @@ -215,8 +216,12 @@ Napi::Value SetDeviceType(const Napi::CallbackInfo& info) return Napi::Boolean::New(env, false); auto typeName = info[0].As().Utf8Value(); - bool result = liveEngine->setDeviceType(juce::String(typeName)); - return Napi::Boolean::New(env, result); + auto result = std::make_shared(false); + if (!runDeviceLifecycleOp([liveEngine, typeName, result] { + *result = liveEngine->setDeviceType(juce::String(typeName)); + })) + return Napi::Boolean::New(env, false); + return Napi::Boolean::New(env, *result); } Napi::Value SetOutputDeviceType(const Napi::CallbackInfo& info) @@ -226,7 +231,12 @@ Napi::Value SetOutputDeviceType(const Napi::CallbackInfo& info) if (!liveEngine || info.Length() < 1 || !info[0].IsString()) return Napi::Boolean::New(env, false); auto typeName = info[0].As().Utf8Value(); - return Napi::Boolean::New(env, liveEngine->setOutputDeviceType(juce::String(typeName))); + auto result = std::make_shared(false); + if (!runDeviceLifecycleOp([liveEngine, typeName, result] { + *result = liveEngine->setOutputDeviceType(juce::String(typeName)); + })) + return Napi::Boolean::New(env, false); + return Napi::Boolean::New(env, *result); } Napi::Value SetDevice(const Napi::CallbackInfo& info) @@ -299,7 +309,15 @@ Napi::Value SetDevice(const Napi::CallbackInfo& info) } // Main thread only — JUCE's ALSA backend deadlocks if called from a worker. - const auto r = liveEngine->setAudioDevices(cfg); + // On Windows this hops to the JUCE message thread (runDeviceLifecycleOp) + // so device destruction can't race the ASIO reset/device-change timers. + auto res = std::make_shared(); + if (!runDeviceLifecycleOp([liveEngine, cfg, res] { *res = liveEngine->setAudioDevices(cfg); })) + { + result.Set("error", "device reconfigure did not complete (message thread unavailable or timed out)"); + return result; + } + const auto& r = *res; result.Set("ok", r.ok); result.Set("duplex", r.duplex); result.Set("sampleRate", r.sampleRate); @@ -313,13 +331,15 @@ Napi::Value SetDevice(const Napi::CallbackInfo& info) Napi::Value StartAudio(const Napi::CallbackInfo& info) { - if (auto liveEngine = snapshotEngine()) liveEngine->startAudio(); + if (auto liveEngine = snapshotEngine()) + runDeviceLifecycleOp([liveEngine] { liveEngine->startAudio(); }); return info.Env().Undefined(); } Napi::Value StopAudio(const Napi::CallbackInfo& info) { - if (auto liveEngine = snapshotEngine()) liveEngine->stopAudio(); + if (auto liveEngine = snapshotEngine()) + runDeviceLifecycleOp([liveEngine] { liveEngine->stopAudio(); }); return info.Env().Undefined(); } @@ -340,15 +360,21 @@ Napi::Value SetStreamOutputDevice(const Napi::CallbackInfo& info) return Napi::String::New(env, "setStreamOutputDevice(typeName:string, deviceName:string)"); const std::string typeName = info[0].As().Utf8Value(); const std::string devName = info[1].As().Utf8Value(); - return Napi::String::New(env, - liveEngine->setStreamOutputDevice(juce::String(typeName), juce::String(devName)).toStdString()); + auto err = std::make_shared(); + if (!runDeviceLifecycleOp([liveEngine, typeName, devName, err] { + *err = liveEngine->setStreamOutputDevice(juce::String(typeName), juce::String(devName)); + })) + return Napi::String::New(env, + "stream output open did not complete (message thread unavailable or timed out)"); + return Napi::String::New(env, err->toStdString()); } // clearStreamOutput() -> undefined Napi::Value ClearStreamOutput(const Napi::CallbackInfo& info) { auto liveEngine = snapshotEngine(); - if (liveEngine) liveEngine->clearStreamOutput(); + if (liveEngine) + runDeviceLifecycleOp([liveEngine] { liveEngine->clearStreamOutput(); }); return info.Env().Undefined(); } From aa0bb9d1731c13c180bd721ef0b3c6eb9363aa3f Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Thu, 16 Jul 2026 22:54:22 +0200 Subject: [PATCH 2/3] fix(audio): fail runDeviceLifecycleOp when the MessageManager is gone A null MessageManager (pre-init or mid-shutdown) previously fell through to inline execution on the caller's thread while reporting success - exactly the unserialised device teardown this helper prevents. Return false instead, per the documented unavailable/timeout contract. Addresses CodeRabbit review on #113. Co-Authored-By: Claude Fable 5 --- src/audio/addon/AddonContext.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/audio/addon/AddonContext.h b/src/audio/addon/AddonContext.h index 882c05d..fec85f7 100644 --- a/src/audio/addon/AddonContext.h +++ b/src/audio/addon/AddonContext.h @@ -84,9 +84,15 @@ template inline bool runDeviceLifecycleOp(Func&& func) { #if JUCE_WINDOWS - if (auto* mm = juce::MessageManager::getInstanceWithoutCreating()) - if (!mm->isThisTheMessageThread()) - return dispatchOnMessageThread(std::forward(func)); + // No MessageManager means the pump is gone (pre-init or mid-shutdown): + // report failure instead of running the mutation unserialised on the + // caller's thread — that would reintroduce the race this helper exists + // to prevent (CodeRabbit #113 review). + auto* mm = juce::MessageManager::getInstanceWithoutCreating(); + if (mm == nullptr) + return false; + if (!mm->isThisTheMessageThread()) + return dispatchOnMessageThread(std::forward(func)); #endif func(); return true; From 895d0af969cb3b4991f120b8c6f646450642a86c Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Thu, 16 Jul 2026 23:45:29 +0200 Subject: [PATCH 3/3] fix(audio): route device probing through the message thread; document timeout desync Review follow-up (PR #113, finding 1): ProbeDeviceOptions constructs and destroys short-lived ASIO device objects (DeviceSetup::probeDual / rateSupportedBy createDevice) on the Node thread - the same create/destroy-off-the-message-thread pattern as the lifecycle ops, and a probe ASIOAudioIODevice owns the same reset-timer machinery. Wrap it in runDeviceLifecycleOp for consistency. Finding 2 (timeout desync - op reported failed may still complete late) is documented as a known limitation on the helper. Co-Authored-By: Claude Fable 5 --- src/audio/addon/AddonContext.h | 8 ++++++++ src/audio/addon/DeviceBindings.cpp | 21 ++++++++++++++++++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/audio/addon/AddonContext.h b/src/audio/addon/AddonContext.h index fec85f7..5f0ca2d 100644 --- a/src/audio/addon/AddonContext.h +++ b/src/audio/addon/AddonContext.h @@ -80,6 +80,14 @@ inline bool dispatchOnMessageThread(Func&& func) // closure must own everything it touches — capture by value (engine // snapshot, args) and write results through a shared_ptr, never through // references to the caller's stack. +// +// KNOWN LIMITATION (timeout desync): when the 15 s wait expires the binding +// reports failure to JS, but the closure may still complete afterwards — +// e.g. addSource returns -1 yet the source gets created, holding its input +// device until the next reconfigure. Memory-safe by the capture rule above, +// just not logically reconciled; only reachable with the message thread +// jammed > 15 s. Callers that care can re-query engine state (listSources, +// getCurrentDevice) after a reported failure. template inline bool runDeviceLifecycleOp(Func&& func) { diff --git a/src/audio/addon/DeviceBindings.cpp b/src/audio/addon/DeviceBindings.cpp index 420e209..47457ca 100644 --- a/src/audio/addon/DeviceBindings.cpp +++ b/src/audio/addon/DeviceBindings.cpp @@ -126,9 +126,24 @@ Napi::Value ProbeDeviceOptions(const Napi::CallbackInfo& info) return obj; } - auto options = liveEngine->probeDeviceOptionsDual( - juce::String(inputType), juce::String(inputName), - juce::String(outputType), juce::String(outputName)); + // Probing constructs + destroys short-lived ASIO device objects + // (DeviceSetup::probeDual / rateSupportedBy createDevice) — same + // create/destroy-off-the-message-thread pattern as the lifecycle ops, so + // it takes the same hop (PR #113 review finding 1). Query-only (never + // open()), but a probe ASIOAudioIODevice owns the same reset-timer + // machinery as the live one. + auto optionsPtr = std::make_shared(); + if (!runDeviceLifecycleOp([liveEngine, inputType, inputName, outputType, outputName, optionsPtr] { + *optionsPtr = liveEngine->probeDeviceOptionsDual( + juce::String(inputType), juce::String(inputName), + juce::String(outputType), juce::String(outputName)); + })) + { + obj.Set("error", "device probe did not complete (message thread unavailable or timed out)"); + obj.Set("compatible", false); + return obj; + } + const auto& options = *optionsPtr; obj.Set("type", options.inputType.toStdString()); // legacy alias obj.Set("inputType", options.inputType.toStdString()); obj.Set("outputType", options.outputType.toStdString());