diff --git a/src/audio/addon/AddonContext.h b/src/audio/addon/AddonContext.h index 2c2a1b9..5f0ca2d 100644 --- a/src/audio/addon/AddonContext.h +++ b/src/audio/addon/AddonContext.h @@ -54,6 +54,58 @@ 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. +// +// 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) +{ +#if JUCE_WINDOWS + // 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; +} + // 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..47457ca 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 { @@ -125,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()); @@ -215,8 +231,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 +246,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 +324,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 +346,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 +375,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(); }