diff --git a/scripts/bundle-slopsmith.sh b/scripts/bundle-slopsmith.sh index 38255ea..d609226 100755 --- a/scripts/bundle-slopsmith.sh +++ b/scripts/bundle-slopsmith.sh @@ -121,7 +121,11 @@ copy_plugin() { local dst="$2" mkdir -p "$dst" # Use cp -R rather than -r for portable symlink-following semantics. - cp -R "$src/." "$dst/" + # -L dereferences symlinks INSIDE the tree (rig_builder's + # vst/src/*/_shared -> ../_shared): Git Bash cannot create symlinks + # without Developer Mode / SeCreateSymbolicLink, and the bundle wants + # real files anyway. + cp -RL "$src/." "$dst/" find "$dst" -name '.git' -type d -prune -exec rm -rf {} + } diff --git a/src/audio/CMakeLists.txt b/src/audio/CMakeLists.txt index 41489d9..bcc3917 100644 --- a/src/audio/CMakeLists.txt +++ b/src/audio/CMakeLists.txt @@ -12,6 +12,7 @@ set(AUDIO_SOURCES engine/SourcePool.cpp engine/ExtraInputs.cpp addon/AddonContext.cpp + addon/LifecycleExecutor.cpp addon/ChainOps.cpp addon/EditorWindows.cpp addon/DeviceBindings.cpp diff --git a/src/audio/VSTHost.cpp b/src/audio/VSTHost.cpp index c82d7ae..5f32424 100644 --- a/src/audio/VSTHost.cpp +++ b/src/audio/VSTHost.cpp @@ -1,5 +1,6 @@ #include "VSTHost.h" #include "VSTTrace.h" +#include "addon/PluginModulePin.h" // The out-of-process scan path is compiled only into the audio addon // (SLOPSMITH_AUDIO_ADDON, set in src/audio/CMakeLists.txt). slopsmith-vst-host @@ -513,6 +514,13 @@ std::unique_ptr VSTHost::loadPlugin( return nullptr; } + // P0 (guide §12): never unload a plugin module mid-session. JUCE frees + // the module when its last instance dies; a message already queued to a + // window/timer in that module then indirect-calls into unmapped code — + // the CFG fail-fast in the 2026-07-17 field dumps. + slopsmith::addon::pinPluginModuleForever( + matchedDesc.fileOrIdentifier.toRawUTF8()); + return instance; } @@ -568,7 +576,8 @@ void VSTHost::loadPluginAsync( // copy threads through both lambda hops. formatManager.createPluginInstanceAsync( matchedDesc, sampleRate, blockSize, - [cb = std::move(callback), name = matchedDesc.name] + [cb = std::move(callback), name = matchedDesc.name, + fileOrId = matchedDesc.fileOrIdentifier] (std::unique_ptr instance, const juce::String& error) { VST_TRACE("VSTHost.loadPluginAsync: createPluginInstanceAsync END " @@ -584,6 +593,8 @@ void VSTHost::loadPluginAsync( : juce::String("Failed to create plugin instance")); return; } + // P0: pin — see the sync loadPlugin path for rationale. + slopsmith::addon::pinPluginModuleForever(fileOrId.toRawUTF8()); cb(std::move(instance), {}); }); } diff --git a/src/audio/addon/AddonContext.cpp b/src/audio/addon/AddonContext.cpp index 79ee2a9..14a89bc 100644 --- a/src/audio/addon/AddonContext.cpp +++ b/src/audio/addon/AddonContext.cpp @@ -164,6 +164,10 @@ void initialize(std::function uiTeardownHook) // instead of treating it as already-done. alreadyShutDown.store(false, std::memory_order_release); + // New engine lifetime: ops still queued against the previous engine + // (or a previous failed init) are stale from here on. + bumpEngineGeneration(); + // Start JUCE message thread first (no-op on macOS) startJuceMessageThread(); @@ -172,8 +176,13 @@ void initialize(std::function uiTeardownHook) std::this_thread::sleep_for(std::chrono::milliseconds(200)); #endif - // Create engine on the JUCE message thread (or inline on macOS) - const bool initialized = dispatchOnMessageThread([]() { + // Create engine on the JUCE message thread (or inline on macOS). + // Unbounded wait (P0): on machines where the first device enumeration + // blocks the message thread for minutes (Focusrite ASIO, 2026-07-17 + // dumps), init completes late instead of "failing" while the engine + // appears anyway — the desync that left JS driving a half-initialized + // addon. + const bool initialized = runLifecycleOpOk("engine-init", []() { std::shared_ptr liveEngine; { std::lock_guard lock(engineMutex); @@ -194,8 +203,9 @@ void initialize(std::function uiTeardownHook) types[i].outputDevices.size()); }); if (!initialized) - fprintf(stderr, "[audio-native] initialize: engine creation did not complete " - "on the message thread; audio bindings will no-op until re-init\n"); + fprintf(stderr, "[audio-native] initialize: engine creation did not run " + "(pump gone or generation moved); audio bindings will " + "no-op until re-init\n"); } void doShutdown() @@ -217,6 +227,12 @@ void doShutdown() bool expected = false; if (!alreadyShutDown.compare_exchange_strong(expected, true)) return; + // Invalidate every lifecycle op still queued against the live engine: + // they run before our teardown op (FIFO) but must not touch a dying + // engine's devices. The teardown op below is stamped AFTER this bump, + // so it alone survives the generation check. + bumpEngineGeneration(); + // Release any LoadVSTWorker / LoadPresetWorker currently blocked on a // pending async load. Without this they'd wait forever on the // WaitableEvent — the createPluginInstanceAsync callback can't fire @@ -225,7 +241,10 @@ void doShutdown() if (juceRunning.load() || snapshotEngine() || snapshotVstHost()) { - const bool toreDown = dispatchOnMessageThread([]() { + // Bounded wait — the ONE bounded caller (LifecycleExecutor.h): at + // process exit, leaking the pump beats hanging exit forever behind + // a wedged driver call. + const bool toreDown = runLifecycleOpOk("engine-shutdown", []() { // Editors reference their slot's processor; engine.reset() below // frees the whole chain, so destroy the editor windows first (#56). if (shutdownUiTeardown) shutdownUiTeardown(); @@ -239,7 +258,7 @@ void doShutdown() std::lock_guard lock(vstHostMutex); vstHost.reset(); } - }); + }, /*maxWaitMs=*/60000); if (!toreDown) { // Editor teardown / stopAudio / engine destruction have NOT diff --git a/src/audio/addon/AddonContext.h b/src/audio/addon/AddonContext.h index 5f0ca2d..ff254ce 100644 --- a/src/audio/addon/AddonContext.h +++ b/src/audio/addon/AddonContext.h @@ -20,6 +20,7 @@ #include "../AudioEngine.h" #include "../VSTHost.h" +#include "LifecycleExecutor.h" #include @@ -74,36 +75,26 @@ inline bool dispatchOnMessageThread(Func&& func) // 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. +// P0 (guide §12): routed through the LifecycleExecutor. False now means the +// op verifiably did NOT run (pump gone, or the engine generation moved while +// it was queued) — the old "false but it may still run later" desync state +// is gone. The wait is unbounded; a watchdog logs every 15 s while a driver +// call blocks the message thread. +// CAPTURE RULE unchanged: 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 - // 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 + return runLifecycleOpOk("device-lifecycle", + std::function(std::forward(func))); +#else + // macOS: dispatch runs inline (no separate pump). Linux/ALSA keeps its + // long-standing "called from the Node main thread" contract untouched. func(); return true; +#endif } // Pending-async-load registry: LoadVSTWorker / LoadPresetWorker block on a diff --git a/src/audio/addon/EditorWindows.cpp b/src/audio/addon/EditorWindows.cpp index 1beb886..c55e6fc 100644 --- a/src/audio/addon/EditorWindows.cpp +++ b/src/audio/addon/EditorWindows.cpp @@ -86,35 +86,22 @@ void destroyAllPluginEditorWindowsOnMessageThread() // timeout so a lingering-editor UAF stays diagnosable. bool closeAllPluginEditorWindows() { - auto* mm = juce::MessageManager::getInstanceWithoutCreating(); - if (mm != nullptr && mm->isThisTheMessageThread()) + // P0 (guide §12): serialized behind every other lifecycle op with an + // unbounded wait — the old 15 s give-up let a caller proceed to free + // slot processors while this teardown was still queued (#56, delayed). + // A false return now means the teardown verifiably did NOT run (pump + // gone / generation moved) — callers still must not free processors on + // false, but there is no longer a "maybe it runs later" state. + // Note: inline execution when already on the message thread is handled + // inside runLifecycleOp. + const bool toreDown = runLifecycleOpOk("editor-teardown", []() { destroyAllPluginEditorWindowsOnMessageThread(); - return true; - } - - auto done = std::make_shared(); - const bool posted = juce::MessageManager::callAsync([done]() - { - destroyAllPluginEditorWindowsOnMessageThread(); - done->signal(); }); - if (!posted) - { - fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: message queue refused the post; " - "editors may still be alive\n"); - return false; - } - if (!done->wait(15000)) - { - // The queued teardown hasn't run: editors may still hold pointers into - // the chain. Callers must NOT free slot processors on a false return — - // proceeding here is exactly the #56 use-after-free, just delayed. - fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: editor teardown did not complete " - "within 15s; caller must not free chain processors\n"); - return false; - } - return true; + if (!toreDown) + fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: teardown did " + "not run; caller must not free chain processors\n"); + return toreDown; } Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info) diff --git a/src/audio/addon/LifecycleExecutor.cpp b/src/audio/addon/LifecycleExecutor.cpp new file mode 100644 index 0000000..cc652dd --- /dev/null +++ b/src/audio/addon/LifecycleExecutor.cpp @@ -0,0 +1,155 @@ +// LifecycleExecutor implementation — see LifecycleExecutor.h for the +// contract and the field evidence that motivated it (guide §12 P0). + +#include "LifecycleExecutor.h" + +#include + +#include +#include +#include +#include +#include + +namespace slopsmith::addon { + +static std::atomic engineGeneration{1}; + +std::uint64_t currentEngineGeneration() +{ + return engineGeneration.load(std::memory_order_acquire); +} + +std::uint64_t bumpEngineGeneration() +{ + return engineGeneration.fetch_add(1, std::memory_order_acq_rel) + 1; +} + +// Submission lock: MessageManager::callAsync is itself FIFO, but two +// threads racing between "decide to submit" and "post" could otherwise +// observe an ordering different from the one their callers reasoned about. +// All lifecycle submissions serialize here so queue order == submit order. +static std::mutex submitMutex; + +static constexpr int kWatchdogIntervalMs = 15000; + +LifecycleOpResult runLifecycleOp(const char* name, + std::function func, + int maxWaitMs) +{ + const auto stamped = currentEngineGeneration(); + + auto* mm = juce::MessageManager::getInstanceWithoutCreating(); + if (mm == nullptr) + { + fprintf(stderr, "[lifecycle] op '%s': no message pump; refusing\n", name); + return LifecycleOpResult::neverRan; + } + + // Already on the message thread (engine init path, macOS inline mode, + // or a lifecycle op submitting a nested op): dispatch-and-wait on + // ourselves would deadlock, so run inline. + // + // Ordering note (CodeRabbit #120): an inline op runs as part of the + // CURRENT message-thread turn, i.e. ahead of ops still queued behind + // it. That is intentional, not a FIFO leak: a nested submission is + // semantically part of the outer op and must complete inside it, and a + // message-thread caller can never interleave with a *running* op — the + // queue drains on this very thread. The FIFO contract is between + // off-thread submitters, which all take the queued path below. + if (mm->isThisTheMessageThread()) + { + if (stamped != currentEngineGeneration()) + { + fprintf(stderr, "[lifecycle] op '%s': stale generation (inline); skipped\n", name); + return LifecycleOpResult::stale; + } + try { func(); } + catch (const std::exception& e) { + fprintf(stderr, "[lifecycle] op '%s' (inline) threw: %s\n", name, e.what()); + } + catch (...) { + fprintf(stderr, "[lifecycle] op '%s' (inline) threw (unknown)\n", name); + } + return LifecycleOpResult::completed; + } + + struct Shared { + juce::WaitableEvent done; + std::atomic wasStale{false}; + }; + auto shared = std::make_shared(); + + bool posted = false; + { + std::lock_guard lock(submitMutex); + posted = juce::MessageManager::callAsync( + [func = std::move(func), shared, stamped, name]() mutable { + if (stamped != currentEngineGeneration()) + { + fprintf(stderr, "[lifecycle] op '%s': engine generation moved " + "%llu -> %llu while queued; skipped\n", + name, + (unsigned long long) stamped, + (unsigned long long) currentEngineGeneration()); + shared->wasStale.store(true, std::memory_order_release); + } + else + { + // Always reach the signal below: a throwing op would + // otherwise leave its unbounded caller waiting forever + // (CodeRabbit #120). + try { func(); } + catch (const std::exception& e) { + fprintf(stderr, "[lifecycle] op '%s' threw: %s\n", name, e.what()); + } + catch (...) { + fprintf(stderr, "[lifecycle] op '%s' threw (unknown)\n", name); + } + } + shared->done.signal(); + }); + } + if (!posted) + { + fprintf(stderr, "[lifecycle] op '%s': message queue refused the post; " + "op will not run\n", name); + return LifecycleOpResult::neverRan; + } + + int waitedMs = 0; + while (!shared->done.wait(kWatchdogIntervalMs)) + { + waitedMs += kWatchdogIntervalMs; + + // Pump death while we wait means the op can never run. Distinguish + // from a merely blocked pump: the MessageManager singleton is + // destroyed by stopJuceMessageThread. + if (juce::MessageManager::getInstanceWithoutCreating() == nullptr) + { + fprintf(stderr, "[lifecycle] op '%s': message pump died after %d ms " + "wait; op will not run\n", name, waitedMs); + return LifecycleOpResult::neverRan; + } + + if (maxWaitMs > 0 && waitedMs >= maxWaitMs) + { + fprintf(stderr, "[lifecycle] op '%s': abandoning wait after %d ms " + "(bounded caller); op may still run\n", name, waitedMs); + return LifecycleOpResult::abandonedWait; + } + + // Watchdog: report, never decide (guide §12 P0 item 5). The usual + // culprit for a multi-second stall is an audio driver call (ASIO + // open/reset) blocking the message thread. + fprintf(stderr, "[lifecycle] watchdog: op '%s' still waiting after %d ms — " + "message thread blocked (audio driver call?); continuing " + "to wait\n", name, waitedMs); + } + + return shared->wasStale.load(std::memory_order_acquire) + ? LifecycleOpResult::stale + : LifecycleOpResult::completed; +} + +} // namespace slopsmith::addon diff --git a/src/audio/addon/LifecycleExecutor.h b/src/audio/addon/LifecycleExecutor.h new file mode 100644 index 0000000..98bfceb --- /dev/null +++ b/src/audio/addon/LifecycleExecutor.h @@ -0,0 +1,78 @@ +#pragma once + +// LifecycleExecutor — P0 of the audio architecture guide (§12 P0). +// +// One serialized queue for native lifecycle mutations (engine init/shutdown, +// device create/destroy, editor teardown). Ops are named, stamped with the +// engine generation at submit time, and executed strictly in submission +// order on the JUCE message thread. +// +// What this replaces: the "dispatch and give up after 15 s" contract +// (AddonContext.h, KNOWN LIMITATION). Field evidence 2026-07-17 (three +// identical CFG fail-fast dumps): a caller that abandons its wait proceeds +// to mutate lifecycle state while the abandoned op is still queued — two +// owners, no ordering authority. The executor removes the abandon path: +// +// - The wait is not bounded by a give-up. A watchdog logs every +// kWatchdogIntervalMs while an op is stalled (message thread blocked by +// a driver call) and keeps waiting. The op result the caller sees is +// always the truth of what ran. +// - The only false returns are "the op will never run" (message pump gone +// or post refused) and "the op was stale" (engine generation changed +// between submit and execution — see below). Callers can treat false as +// final; there is no "maybe it still runs later" state. +// - `maxWaitMs` exists for the one caller that must not block forever: +// process shutdown, where leaking the pump beats hanging exit. Everyone +// else passes 0 (unbounded). +// +// Generation guard: initialize/doShutdown bump the engine generation. An op +// stamped under an older generation no-ops when it finally executes, so +// work that was queued against a torn-down engine can never touch its +// replacement. This is the lifecycle-level generalization of +// chainGeneration. +// +// The executor serializes; it never authorizes. Policy (who may mutate +// what) belongs to the JS-side lease registry (guide §7.1). +// +// macOS: no background pump exists (AppKit owns the real main thread — +// AddonContext.cpp fork note), so ops execute inline on the caller thread, +// still generation-checked. Same contract, degenerate queue. + +#include +#include + +namespace slopsmith::addon { + +// Monotonic engine generation. Bumped by initialize()/doShutdown(). +std::uint64_t currentEngineGeneration(); +std::uint64_t bumpEngineGeneration(); + +enum class LifecycleOpResult { + completed, // op ran to completion under its submit-time generation + stale, // generation changed before the op ran; op no-oped + neverRan, // pump gone / post refused — op will never run + abandonedWait, // maxWaitMs expired; op may still run (shutdown-only path) +}; + +// Run `func` on the JUCE message thread, serialized behind every previously +// submitted lifecycle op. Blocks until the op completes (watchdog logs +// while stalled). `name` appears in every log line and in the watchdog. +// `maxWaitMs` = 0 means wait indefinitely (bail only if the pump dies). +LifecycleOpResult runLifecycleOp(const char* name, + std::function func, + int maxWaitMs = 0); + +// Convenience: true only for LifecycleOpResult::completed. +inline bool runLifecycleOpOk(const char* name, std::function func, + int maxWaitMs = 0) +{ + return runLifecycleOp(name, std::move(func), maxWaitMs) + == LifecycleOpResult::completed; +} + +// Plugin-module pinning (guide §12 P0 item 4) lives in PluginModulePin.h — +// header-only, because VSTHost.cpp is compiled into targets across three +// CMake projects and an out-of-line definition broke the ones that don't +// build this executor (PR #120 CI). + +} // namespace slopsmith::addon diff --git a/src/audio/addon/PluginModulePin.h b/src/audio/addon/PluginModulePin.h new file mode 100644 index 0000000..f2e84df --- /dev/null +++ b/src/audio/addon/PluginModulePin.h @@ -0,0 +1,99 @@ +#pragma once + +// Plugin-module pinning — P0 item 4 of the audio architecture guide (§12). +// +// JUCE refcounts VST3 modules and unloads them when the last instance dies; +// a window/timer message already queued to that module's code then fires +// into unmapped or reused pages — the CFG fail-fast +// (0xc0000409 / FAST_FAIL_GUARD_ICALL_CHECK_FAILURE) in the 2026-07-17 +// field dumps. Pinning tells the loader to never unload the module for the +// life of the process. +// +// Header-only ON PURPOSE: VSTHost.cpp is compiled into four different +// targets across three CMake projects (the addon, slopsmith-vst-host in +// two projects, slopsmith-vst-scan on macOS); an out-of-line definition +// broke every link that didn't add the extra .cpp (PR #120 CI). +// +// The pin matches loaded modules by PATH, not base name: the identifier we +// get is usually the bundle directory (…\Foo.vst3\), while the loader knows +// the inner …\Contents\x86_64-win\Foo.vst3 file — and two plugins may share +// a base name. Enumerate loaded modules and pin every one whose full path +// starts with the normalized identifier (K32* exports so no psapi.lib). +// No-op off Windows and for modules that are not currently loaded (e.g. +// sandboxed plugins — the child process owns those). + +#include + +#if defined(_WIN32) + #ifndef WIN32_LEAN_AND_MEAN + #define WIN32_LEAN_AND_MEAN + #endif + #ifndef NOMINMAX + #define NOMINMAX + #endif + #include + // PSAPI_VERSION 2 maps EnumProcessModules/GetModuleFileNameExW onto the + // kernel32-exported K32* variants — no psapi.lib link needed (matters for + // the extra targets this header serves; see the header-only note above). + #ifndef PSAPI_VERSION + #define PSAPI_VERSION 2 + #endif + #include + #include + #include +#endif + +#include + +namespace slopsmith::addon { + +inline void pinPluginModuleForever(const char* fileOrIdentifierUtf8) +{ +#if defined(_WIN32) + const juce::String wanted = + juce::String::fromUTF8(fileOrIdentifierUtf8).replaceCharacter('/', '\\'); + if (wanted.isEmpty()) + return; + + auto startsWithIgnoreCase = [](const wchar_t* full, const wchar_t* prefix) { + while (*prefix != 0) + { + if (*full == 0 + || std::towlower(static_cast(*full)) + != std::towlower(static_cast(*prefix))) + return false; + ++full; + ++prefix; + } + return true; + }; + + std::vector modules(1024); + DWORD needed = 0; + if (!EnumProcessModules(GetCurrentProcess(), modules.data(), + static_cast(modules.size() * sizeof(HMODULE)), + &needed)) + return; + modules.resize(needed / sizeof(HMODULE)); + + for (HMODULE mod : modules) + { + wchar_t modPath[MAX_PATH * 2] = {}; + if (GetModuleFileNameExW(GetCurrentProcess(), mod, modPath, + static_cast(sizeof(modPath) / sizeof(modPath[0]))) == 0) + continue; + if (!startsWithIgnoreCase(modPath, wanted.toWideCharPointer())) + continue; + + HMODULE pinned = nullptr; + if (GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN, modPath, &pinned)) + fprintf(stderr, "[lifecycle] pinned plugin module '%s' for process " + "lifetime\n", + juce::String(modPath).toRawUTF8()); + } +#else + (void) fileOrIdentifierUtf8; +#endif +} + +} // namespace slopsmith::addon