From 1359c7c24ded13539442e3a02098dadc125404a3 Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Fri, 17 Jul 2026 18:28:06 +0200 Subject: [PATCH 1/3] =?UTF-8?q?fix(audio):=20P0=20lifecycle=20executor=20?= =?UTF-8?q?=E2=80=94=20serialized=20ops,=20generation=20guard,=20module=20?= =?UTF-8?q?pinning?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three identical field crash dumps (2026-07-17, CFG fail-fast 0xc0000409 param 0xa = GUARD_ICALL_CHECK_FAILURE on the JUCE message thread, plugin WndProc frame on the stack) traced to the dispatch timeout desync: every lifecycle call site could give up after 15 s while its op was still queued, leaving two owners mutating engine/plugin lifecycle state with no ordering authority. - New LifecycleExecutor (guide §12 P0): named lifecycle ops, strictly FIFO on the message thread, engine-generation stamped at submit and no-oped when stale. The wait is unbounded with a 15 s watchdog that reports and keeps waiting; false now always means "verifiably did not run" — the "false but it may still run later" state is gone. - initialize/doShutdown bump the generation and run as executor ops; shutdown is the one bounded caller (60 s, then leak the pump — beats hanging process exit behind a wedged driver call). - runDeviceLifecycleOp and closeAllPluginEditorWindows route through the executor; the editor-teardown 15 s give-up (delayed #56 UAF) is removed. - pinPluginModuleForever: plugin modules are pinned on load so JUCE's refcount can never unload them mid-session — a queued window/timer message into an unloaded module is the CFG fail-fast in the dumps. Co-Authored-By: Claude Fable 5 --- src/audio/CMakeLists.txt | 1 + src/audio/VSTHost.cpp | 13 +- src/audio/addon/AddonContext.cpp | 31 ++++- src/audio/addon/AddonContext.h | 39 +++--- src/audio/addon/EditorWindows.cpp | 39 ++---- src/audio/addon/LifecycleExecutor.cpp | 167 ++++++++++++++++++++++++++ src/audio/addon/LifecycleExecutor.h | 83 +++++++++++++ src/vst-host/CMakeLists.txt | 1 + 8 files changed, 317 insertions(+), 57 deletions(-) create mode 100644 src/audio/addon/LifecycleExecutor.cpp create mode 100644 src/audio/addon/LifecycleExecutor.h 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..933cb57 100644 --- a/src/audio/VSTHost.cpp +++ b/src/audio/VSTHost.cpp @@ -1,5 +1,6 @@ #include "VSTHost.h" #include "VSTTrace.h" +#include "addon/LifecycleExecutor.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..8a1f6af --- /dev/null +++ b/src/audio/addon/LifecycleExecutor.cpp @@ -0,0 +1,167 @@ +// 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 + +#if JUCE_WINDOWS + #define WIN32_LEAN_AND_MEAN + #include +#endif + +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): + // dispatch-and-wait on ourselves would deadlock. Run inline — we are by + // definition serialized with every queued op. + if (mm->isThisTheMessageThread()) + { + if (stamped != currentEngineGeneration()) + { + fprintf(stderr, "[lifecycle] op '%s': stale generation (inline); skipped\n", name); + return LifecycleOpResult::stale; + } + func(); + 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 + { + func(); + } + 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; +} + +void pinPluginModuleForever(const char* fileOrIdentifierUtf8) +{ +#if JUCE_WINDOWS + // JUCE loads the inner /Contents/x86_64-win/.vst3 (or a + // flat .vst3/.dll); either way the loaded module's base name is the + // path's final component. Pin by that name so the loader never unloads + // it, even when JUCE's module refcount hits zero. + const juce::String path = juce::String::fromUTF8(fileOrIdentifierUtf8); + const juce::String base = path.replaceCharacter('\\', '/') + .fromLastOccurrenceOf("/", false, false); + if (base.isEmpty()) + return; + + HMODULE h = nullptr; + if (GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN, + base.toWideCharPointer(), &h)) + { + fprintf(stderr, "[lifecycle] pinned plugin module '%s' for process " + "lifetime\n", base.toRawUTF8()); + } + // Not currently loaded under that name (sandboxed plugin, or the inner + // file is named differently): nothing to pin — the sandbox child owns + // its own modules, and an in-process module we can't resolve here was + // loaded under some other base name and will be pinned on a later load + // if it ever resolves. Silent by design; this is a belt on JUCE's + // refcount braces, not a correctness gate. +#else + (void) fileOrIdentifierUtf8; +#endif +} + +} // namespace slopsmith::addon diff --git a/src/audio/addon/LifecycleExecutor.h b/src/audio/addon/LifecycleExecutor.h new file mode 100644 index 0000000..df9921b --- /dev/null +++ b/src/audio/addon/LifecycleExecutor.h @@ -0,0 +1,83 @@ +#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; +} + +// Pin a plugin module so the OS never unloads it for the life of the +// process (guide §12 P0 item 4). 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 dumps. The pin is by module base name (the inner +// x86_64-win/*.vst3 file JUCE actually loads shares the bundle's base +// name). No-op off Windows and for modules that are not currently loaded. +void pinPluginModuleForever(const char* fileOrIdentifierUtf8); + +} // namespace slopsmith::addon diff --git a/src/vst-host/CMakeLists.txt b/src/vst-host/CMakeLists.txt index a6462f0..a516a64 100644 --- a/src/vst-host/CMakeLists.txt +++ b/src/vst-host/CMakeLists.txt @@ -63,6 +63,7 @@ endfunction() set(VST_HOST_SOURCES main.cpp ../audio/VSTHost.cpp + ../audio/addon/LifecycleExecutor.cpp ../audio/Sandbox/Protocol.cpp ../audio/Sandbox/ControlChannel_shared.cpp ../audio/Sandbox/AudioChannel_shared.cpp From 503f6658650cd73b46f715efedc52b9892caecde Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Fri, 17 Jul 2026 19:45:40 +0200 Subject: [PATCH 2/3] build(windows): dereference plugin-tree symlinks when bundling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rig_builder now ships vst/src/{pedals,racks}/_shared -> ../_shared; Git Bash cp -R fails to recreate symlinks without SeCreateSymbolicLink. The bundle wants real files anyway — copy with -L. Co-Authored-By: Claude Fable 5 --- scripts/bundle-slopsmith.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 {} + } From 4ac1b60766b6ddaac38e0e738f550761ba229520 Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Sat, 18 Jul 2026 00:01:52 +0200 Subject: [PATCH 3/3] fix(audio): header-only module pin; exception-safe ops; CI link fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI: slopsmith-vst-host (sandbox-e2e's own CMake project) and macOS's slopsmith-vst-scan compile VSTHost.cpp without LifecycleExecutor.cpp and failed to link pinPluginModuleForever. Move the pin to a header-only PluginModulePin.h so every target that compiles VSTHost.cpp gets it with no extra source-list entry; drop the now-unneeded LifecycleExecutor.cpp from VST_HOST_SOURCES. CodeRabbit review: - Pin by full module path, not base name: enumerate loaded modules (PSAPI_VERSION=2 K32 exports, no psapi.lib) and pin every one whose path starts with the plugin identifier — two plugins sharing a DLL base name can no longer pin the wrong module, and the bundle-dir identifier now matches the inner Contents// file it actually loaded. - Ops are exception-safe: a throwing op logs and still signals its waiter instead of leaving an unbounded caller blocked forever. - Documented why the message-thread inline path is intentional (nested submissions are part of the outer op; dispatch-and-wait on the message thread would self-deadlock; FIFO is the contract between off-thread submitters). Co-Authored-By: Claude Fable 5 --- src/audio/VSTHost.cpp | 2 +- src/audio/addon/LifecycleExecutor.cpp | 70 ++++++++----------- src/audio/addon/LifecycleExecutor.h | 13 ++-- src/audio/addon/PluginModulePin.h | 99 +++++++++++++++++++++++++++ src/vst-host/CMakeLists.txt | 1 - 5 files changed, 133 insertions(+), 52 deletions(-) create mode 100644 src/audio/addon/PluginModulePin.h diff --git a/src/audio/VSTHost.cpp b/src/audio/VSTHost.cpp index 933cb57..5f32424 100644 --- a/src/audio/VSTHost.cpp +++ b/src/audio/VSTHost.cpp @@ -1,6 +1,6 @@ #include "VSTHost.h" #include "VSTTrace.h" -#include "addon/LifecycleExecutor.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 diff --git a/src/audio/addon/LifecycleExecutor.cpp b/src/audio/addon/LifecycleExecutor.cpp index 8a1f6af..cc652dd 100644 --- a/src/audio/addon/LifecycleExecutor.cpp +++ b/src/audio/addon/LifecycleExecutor.cpp @@ -7,14 +7,10 @@ #include #include +#include #include #include -#if JUCE_WINDOWS - #define WIN32_LEAN_AND_MEAN - #include -#endif - namespace slopsmith::addon { static std::atomic engineGeneration{1}; @@ -50,9 +46,17 @@ LifecycleOpResult runLifecycleOp(const char* name, return LifecycleOpResult::neverRan; } - // Already on the message thread (engine init path, macOS inline mode): - // dispatch-and-wait on ourselves would deadlock. Run inline — we are by - // definition serialized with every queued op. + // 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()) @@ -60,7 +64,13 @@ LifecycleOpResult runLifecycleOp(const char* name, fprintf(stderr, "[lifecycle] op '%s': stale generation (inline); skipped\n", name); return LifecycleOpResult::stale; } - func(); + 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; } @@ -86,7 +96,16 @@ LifecycleOpResult runLifecycleOp(const char* name, } else { - func(); + // 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(); }); @@ -133,35 +152,4 @@ LifecycleOpResult runLifecycleOp(const char* name, : LifecycleOpResult::completed; } -void pinPluginModuleForever(const char* fileOrIdentifierUtf8) -{ -#if JUCE_WINDOWS - // JUCE loads the inner /Contents/x86_64-win/.vst3 (or a - // flat .vst3/.dll); either way the loaded module's base name is the - // path's final component. Pin by that name so the loader never unloads - // it, even when JUCE's module refcount hits zero. - const juce::String path = juce::String::fromUTF8(fileOrIdentifierUtf8); - const juce::String base = path.replaceCharacter('\\', '/') - .fromLastOccurrenceOf("/", false, false); - if (base.isEmpty()) - return; - - HMODULE h = nullptr; - if (GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN, - base.toWideCharPointer(), &h)) - { - fprintf(stderr, "[lifecycle] pinned plugin module '%s' for process " - "lifetime\n", base.toRawUTF8()); - } - // Not currently loaded under that name (sandboxed plugin, or the inner - // file is named differently): nothing to pin — the sandbox child owns - // its own modules, and an in-process module we can't resolve here was - // loaded under some other base name and will be pinned on a later load - // if it ever resolves. Silent by design; this is a belt on JUCE's - // refcount braces, not a correctness gate. -#else - (void) fileOrIdentifierUtf8; -#endif -} - } // namespace slopsmith::addon diff --git a/src/audio/addon/LifecycleExecutor.h b/src/audio/addon/LifecycleExecutor.h index df9921b..98bfceb 100644 --- a/src/audio/addon/LifecycleExecutor.h +++ b/src/audio/addon/LifecycleExecutor.h @@ -70,14 +70,9 @@ inline bool runLifecycleOpOk(const char* name, std::function func, == LifecycleOpResult::completed; } -// Pin a plugin module so the OS never unloads it for the life of the -// process (guide §12 P0 item 4). 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 dumps. The pin is by module base name (the inner -// x86_64-win/*.vst3 file JUCE actually loads shares the bundle's base -// name). No-op off Windows and for modules that are not currently loaded. -void pinPluginModuleForever(const char* fileOrIdentifierUtf8); +// 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 diff --git a/src/vst-host/CMakeLists.txt b/src/vst-host/CMakeLists.txt index a516a64..a6462f0 100644 --- a/src/vst-host/CMakeLists.txt +++ b/src/vst-host/CMakeLists.txt @@ -63,7 +63,6 @@ endfunction() set(VST_HOST_SOURCES main.cpp ../audio/VSTHost.cpp - ../audio/addon/LifecycleExecutor.cpp ../audio/Sandbox/Protocol.cpp ../audio/Sandbox/ControlChannel_shared.cpp ../audio/Sandbox/AudioChannel_shared.cpp