Fix timeout races and other issues from the v2 review (#198)

* Fix races between timeouts and report/log writing

- validate: guard the log stream with a mutex so the timeout thread can't
  reset it while a test is writing to it, and stop the timeout thread once
  the tests finish so it can't write its report over the real one.
- test: the worker and the watchdog now claim the report with an atomic
  flag and only the first to claim it writes. The placeholder report is
  written before the watchdog starts.

* Fix issues from the v2 review

- Crash log: back in the per-user temp directory (path computed up front for
  the signal handler) instead of a shared /tmp file, opened with O_NOFOLLOW.
- Crash handler: don't pass null image/symbol names to %s; cast the offset
  for %ld; include <string.h> and <algorithm>.
- --output-filename without --output-dir is resolved against the working
  directory rather than building a juce::File from a relative path.
- pluginval test accepts --name=value options, as validate does.
- GitHub Actions: delete each report before running so a stale one from an
  earlier run is never read.
- TestConfig::getPluginPathOrID uses settings_parser::resolvePluginPath.
- vst3validator: delete the extracted binary if writing or chmod fails.
- Typos, spacing, closing-namespace comments, an em dash, and the JUCE
  version in CLAUDE.md.

* Fix remaining issues from the v2 review

- pluginval test: --record-missing=<value> is an error rather than the value
  being taken as the config path.
- Message-thread helpers share one callOnMessageThreadAndWait() that rethrows
  an exception on the calling thread, so a throw can't leave the caller
  waiting forever. Used by the VST3 helpers, the editor helpers, the plugin
  scan, message-thread tests and the acceptance plugin teardown, which now
  can't throw out of its destructor.
- Child process output is decoded with ChildProcessOutputDecoder, so a UTF-8
  character split across two reads is no longer garbled.
- operator delete no longer throws (it's noexcept, so that terminated the
  process); operator new throws std::bad_alloc rather than returning null.
  Removed the unused violationError().
- Replaced the em/en dashes in CLAUDE.md and the acceptance design doc.

Author: drowaudio

.github/actions/acceptance/acceptance.sh

@@ -39,6 +39,7 @@ for config in ${configs[@]+"${configs[@]}"}; do
     index=$((index + 1))
     name="${config#"$config_dir"/}"
     report="$report_dir/$index-$(sanitise "$(basename "$config" .json)").xml"
+    rm -f "$report"   # so an earlier run's report can't be read if this one writes none
 
     echo "::group::pluginval test: $name"
 

.github/actions/validate/validate.sh

@@ -28,6 +28,7 @@ for plugin in ${plugins[@]+"${plugins[@]}"}; do
 
     index=$((index + 1))
     report="$report_dir/$index-$(sanitise "$(basename "$plugin")").xml"
+    rm -f "$report"   # so an earlier run's report can't be read if this one writes none
 
     echo "::group::pluginval validate: $plugin"
     # INPUT_ARGS is deliberately unquoted so it splits into separate arguments.

CLAUDE.md

@@ -217,11 +217,11 @@ The command line is structured into subcommands, peeled off by a thin verb
 dispatcher in front of the settings pipeline (it does **not** use CLI11-native
 subcommands, so the env/config/CLI layering below is untouched):
 
-- `pluginval validate [options] <plugin>` — the default; `<plugin>` is a
+- `pluginval validate [options] <plugin>` - the default; `<plugin>` is a
   positional argument. `pluginval <plugin>` and `pluginval [options] <plugin>`
   (no verb) also resolve to validate.
-- `pluginval run-tests` — runs the internal unit tests.
-- `pluginval strictness-help [level]` — lists tests at a strictness level.
+- `pluginval run-tests` - runs the internal unit tests.
+- `pluginval strictness-help [level]` - lists tests at a strictness level.
 
 `settings_parser::dispatch()` (in `SettingsParser.cpp`) takes the `tokenise()`d
 command line and returns a `DispatchResult { command, validateTokens,
@@ -248,7 +248,7 @@ that CLI11 binds to directly. A single instance is filled by successive layers,
 **lowest to highest precedence: defaults → environment → `--config` → CLI**
 (in `SettingsParser::parseTokens`):
 
-1. **preprocess** the raw command line — rewrite the deprecated `strictnessLevel`,
+1. **preprocess** the raw command line - rewrite the deprecated `strictnessLevel`,
    strip the macOS `-NSDocumentRevisionsDebugMode YES` flag, and insert an
    implicit `--validate` when the last argument is a bare plugin path.
 2. **Environment layer.** Env-var names are *derived* from the registered
@@ -267,7 +267,7 @@ uses a `CheckedTransformer`, and the hex/int seed is a small callback.
 `PluginTests::Options` at the boundary.
 
 Adding a new option is three edits: a struct member, an entry in the nlohmann
-macro list, and one `add_option(...)` line — its environment variable then works
+macro list, and one `add_option(...)` line - its environment variable then works
 automatically. `SettingsSerializer` handles JSON load/save plus the two
 remaining conversions (hex seed, disabled-tests file).
 
@@ -279,7 +279,7 @@ are handled by CLI11 (auto usage + a footer with the env-var/commands notes).
 ### Acceptance Testing (`pluginval test`)
 
 A **parallel subsystem** to validate, in `source/acceptance/`. It answers "does
-this plugin produce the expected output for a known input + state?" — a
+this plugin produce the expected output for a known input + state?" - a
 deterministic *render + golden-file comparison*, not a unit-test pass/fail. Full
 spec: `tests/acceptance/Acceptance testing design.md`; end-user guide:
 `docs/Acceptance testing.md`.
@@ -289,13 +289,13 @@ spec: `tests/acceptance/Acceptance testing design.md`; end-user guide:
   `DispatchResult::testConfigPath`, plus `recordMissing` / `timeoutMs` from
   `--record-missing` / `--timeout-ms` or `RECORD_MISSING` / `TIMEOUT_MS`, and
   `testPluginOverride` from `--plugin`, which replaces the config's `plugin`
-  in `runTestFile()` - so `plugin` is only required when `--plugin` is absent),
+  in `loadAndRunTestFile()` - so `plugin` is only required when `--plugin` is absent),
   `isCommandLine()` and `getFooterText()`; `CommandLine.cpp`'s
   `performCommandLine()` has a `Command::test` branch that starts an
   `acceptance::TestRunner`, which renders on a **background thread** (message
   thread left free; plugin deleted on it), with a watchdog for the timeout, and
   quits via `callAsync` when done.
-- **Config**: `acceptance::TestConfig` (`TestConfig.cpp/h`) — std-typed struct,
+- **Config**: `acceptance::TestConfig` (`TestConfig.cpp/h`) - std-typed struct,
   **snake_case JSON keys** mapped via explicit `to_json`/`from_json` (members
   stay camelCase). It is **independent** of `PluginvalSettings` / the `--config`
   layering: the test config is a positional argument loaded standalone.
@@ -314,7 +314,7 @@ spec: `tests/acceptance/Acceptance testing design.md`; end-user guide:
   tolerance, default one 16-bit LSB = `1/32768`; `0` = bit-exact). Adding
   `spectrum`/`crosscorr`/etc. is one registry entry, no config/runner changes.
 - **Dogfood + self-tests**: three minimal deterministic `juce_add_plugin` targets
-  behind `PLUGINVAL_BUILD_TEST_PLUGINS` — `tests/test_plugins/tone_generator/`
+  behind `PLUGINVAL_BUILD_TEST_PLUGINS` - `tests/test_plugins/tone_generator/`
   (closed-form sine/square generator, phase resets on `prepareToPlay`),
   `tests/test_plugins/gain/` (a gain effect, dogfoods the `input.audio` path) and
   `tests/test_plugins/playhead_probe/` (writes the host transport to its output,
@@ -352,8 +352,10 @@ log are unaffected.
   completion, so a crash leaves an error report rather than a stale or missing
   one. On a timeout the thread that ends the process writes a "timed out" error
   report as its last act (`PluginsUnitTestRunner`'s `onTimeout` for validate, the
-  acceptance `Watchdog` for test), so the report always agrees with the exit code
-  even if the tests finish in the moment before the process is terminated.
+  acceptance `Watchdog` for test). The two writers never overlap: validate stops
+  the timeout thread before writing the real report, and for test the worker and
+  the `Watchdog` each claim the report (an atomic flag) and only the first to
+  claim it writes, so the report always agrees with the exit code.
 
 ### Test Framework
 
@@ -399,8 +401,8 @@ The VST3 validator (Steinberg's vstvalidator) is embedded into pluginval when bu
 5. When the `VST3validator` test runs, it spawns the extracted validator as a subprocess
 
 **Key files:**
-- `cmake/GenerateBinaryHeader.cmake` — binary-to-C-header conversion script
-- `source/vst3validator/VST3ValidatorRunner.h/cpp` — extracts embedded binary, returns `juce::File`
+- `cmake/GenerateBinaryHeader.cmake` - binary-to-C-header conversion script
+- `source/vst3validator/VST3ValidatorRunner.h/cpp` - extracts embedded binary, returns `juce::File`
 
 **Disabling embedded validator:**
 ```bash
@@ -449,6 +451,8 @@ Located in `TestUtilities.h`:
 - `countNaNs()`, `countInfs()`, `countSubnormals()` - Audio validation
 - `ScopedEditorShower` - RAII editor creation/destruction
 - `callPrepareToPlayOnMessageThreadIfVST3()` - VST3-safe lifecycle
+- `callOnMessageThreadAndWait()` - Run code on the message thread and wait; rethrows its exceptions on the caller
+- `ChildProcessOutputDecoder` - Decode child-process output read in chunks without splitting UTF-8 characters
 - `ScopedAllocationDisabler` - Detect allocations in audio thread
 
 ### Real-time Safety Checking
@@ -573,7 +577,7 @@ Without the actions:
 ## Dependencies
 
 ### External
-- **JUCE** (v8.0.x) - Audio application framework (git submodule)
+- **JUCE** (v9.0.x) - Audio application framework (git submodule)
 - **magic_enum** (v0.9.7) - Enum reflection (fetched via CPM)
 - **CLI11** (v2.6.2) - CLI argument parsing, header-only (fetched via CPM)
 - **nlohmann/json** (3.12.0) - JSON settings layering/serialisation (fetched via CPM)

source/CommandLineTests.cpp

@@ -586,6 +586,32 @@ struct CommandLineTests : public juce::UnitTest
             }
         }
 
+        beginTest ("Subcommand: test options in --name=value form");
+        {
+            {
+                const auto d = settings_parser::dispatch (settings_parser::tokenise ("test --timeout-ms=500 --junit-xml=out/r.xml --json-results=out/r.json --plugin=build/P.vst3 config.json"));
+                expect (d.error.isEmpty());
+                expectEquals (d.timeoutMs, (juce::int64) 500);
+                expectEquals (d.junitXmlPath, juce::String ("out/r.xml"));
+                expectEquals (d.jsonResultsPath, juce::String ("out/r.json"));
+                expectEquals (d.testPluginOverride, juce::String ("build/P.vst3"));
+                expectEquals (d.testConfigPath, juce::String ("config.json"));
+            }
+            {
+                const auto d = settings_parser::dispatch (settings_parser::tokenise ("test config.json --plugin="));
+                expect (d.error.contains ("--plugin"));
+            }
+            {
+                const auto d = settings_parser::dispatch (settings_parser::tokenise ("test --output-dir=out config.json"));
+                expect (d.error.contains ("--output-dir"));
+            }
+            {
+                // A flag doesn't take a value, so the value isn't taken as the config path
+                const auto d = settings_parser::dispatch (settings_parser::tokenise ("test --record-missing=1 config.json"));
+                expect (d.error.contains ("--record-missing"));
+            }
+        }
+
         beginTest ("Report writer: JUnit XML");
         {
             report_writer::Report report;

source/CrashHandler.cpp

@@ -17,7 +17,9 @@
 
 #if ! JUCE_WINDOWS
  #include "native/pluginval_native_posix.h"
- static const auto crashLogPath = "/tmp/pluginval_crash.txt";
+
+ // Set by initialiseCrashHandler(), as the crash handler can't allocate to build it
+ static char crashLogPath[1024] {};
 #endif
 
 namespace
@@ -29,11 +31,7 @@ namespace
 
     static juce::File getCrashTraceFile()
     {
-       #if JUCE_WINDOWS
         return juce::File::getSpecialLocation (juce::File::tempDirectory).getChildFile ("pluginval_crash.txt");
-       #else
-        return juce::File (crashLogPath);
-       #endif
     }
 
    #if JUCE_WINDOWS
@@ -66,7 +64,13 @@ namespace
 void initialiseCrashHandler()
 {
     // Delete the crash file, this will be created if possible
-    getCrashTraceFile().deleteFile();
+    const auto crashTraceFile = getCrashTraceFile();
+    crashTraceFile.deleteFile();
+
+   #if ! JUCE_WINDOWS
+    crashTraceFile.getFullPathName().copyToUTF8 (crashLogPath, sizeof (crashLogPath));
+   #endif
+
     juce::SystemStats::setApplicationCrashHandler (handleCrash);
 }
 

source/MainComponent.cpp

@@ -259,7 +259,7 @@ namespace
 
     void showOutputDirDialog()
     {
-        juce::String message = TRANS("Set a desintation directory to place log files");
+        juce::String message = TRANS("Set a destination directory to place log files");
         auto dir = getOutputDir();
 
         if (dir.getFullPathName().isNotEmpty())

source/PluginTests.cpp

@@ -117,13 +117,10 @@ void PluginTests::runTest()
     {
         beginTest ("Scan for plugins located in: " + fileOrID);
 
-        juce::WaitableEvent completionEvent;
-        juce::MessageManager::callAsync ([&, this]() mutable
-                                   {
-                                       knownPluginList.scanAndAddDragAndDroppedFiles (formatManager, juce::StringArray (fileOrID), typesFound);
-                                       completionEvent.signal();
-                                   });
-        completionEvent.wait();
+        callOnMessageThreadAndWait ([&]
+        {
+            knownPluginList.scanAndAddDragAndDroppedFiles (formatManager, juce::StringArray (fileOrID), typesFound);
+        });
 
         logMessage ("Num plugins found: " + juce::String (typesFound.size()));
         expect (! typesFound.isEmpty(),
@@ -212,13 +209,7 @@ void PluginTests::testType (const juce::PluginDescription& pd)
 
                     if (t->needsToRunOnMessageThread())
                     {
-                        juce::WaitableEvent completionEvent;
-                        juce::MessageManager::callAsync ([&, this]() mutable
-                                                          {
-                                                              t->runTest (*this, *instance);
-                                                              completionEvent.signal();
-                                                          });
-                        completionEvent.wait();
+                        callOnMessageThreadAndWait ([&] { t->runTest (*this, *instance); });
                     }
                     else
                     {

source/ReportWriter.h

@@ -75,4 +75,4 @@ namespace report_writer
         failed run).
     */
     bool write (const Report&, const juce::File& junitXml, const juce::File& jsonResults);
-}
+} // namespace report_writer

source/SettingsParser.cpp

@@ -393,17 +393,41 @@ Precedence (lowest to highest): defaults, environment variables, --config, comma
 
                 // The options, plus the positional acceptance-test config (the first
                 // non-option token after the verb).
+                // Accept "--name=value" as well as "--name value", as validate (CLI11) does
+                juce::StringArray tokens;
+
                 for (int i = 1; i < tokensIn.size(); ++i)
                 {
                     const auto& token = tokensIn.getReference (i);
 
+                    if (token.startsWith ("--record-missing="))
+                    {
+                        result.error = "--record-missing doesn't take a value (got \"" + token + "\")";
+                        return result;
+                    }
+
+                    if (token.startsWith ("--") && token.contains ("="))
+                    {
+                        tokens.add (token.upToFirstOccurrenceOf ("=", false, false));
+                        tokens.add (token.fromFirstOccurrenceOf ("=", false, false));
+                    }
+                    else
+                    {
+                        tokens.add (token);
+                    }
+                }
+
+                for (int i = 0; i < tokens.size(); ++i)
+                {
+                    const auto& token = tokens.getReference (i);
+
                     if (token == "--record-missing")
                     {
                         result.recordMissing = true;
                     }
                     else if (token == "--timeout-ms")
                     {
-                        const auto value = i + 1 < tokensIn.size() ? tokensIn.getReference (++i) : juce::String();
+                        const auto value = i + 1 < tokens.size() ? tokens.getReference (++i) : juce::String();
                         const auto digits = value.startsWith ("-") ? value.substring (1) : value;
 
                         if (digits.isEmpty() || ! digits.containsOnly ("0123456789"))
@@ -416,7 +440,7 @@ Precedence (lowest to highest): defaults, environment variables, --config, comma
                     }
                     else if (token == "--junit-xml" || token == "--json-results")
                     {
-                        const auto value = i + 1 < tokensIn.size() ? tokensIn.getReference (++i) : juce::String();
+                        const auto value = i + 1 < tokens.size() ? tokens.getReference (++i) : juce::String();
 
                         if (value.isEmpty() || value.startsWith ("--"))
                         {
@@ -428,7 +452,7 @@ Precedence (lowest to highest): defaults, environment variables, --config, comma
                     }
                     else if (token == "--plugin")
                     {
-                        const auto value = i + 1 < tokensIn.size() ? tokensIn.getReference (++i) : juce::String();
+                        const auto value = i + 1 < tokens.size() ? tokens.getReference (++i) : juce::String();
 
                         if (value.isEmpty() || value.startsWith ("--"))
                         {
@@ -599,4 +623,4 @@ Precedence (lowest to highest): defaults, environment variables, --config, comma
         args.addArray ({ "validate", "--config-base64", b64, fileOrID });
         return args;
     }
-}
+} // namespace settings_parser

source/SettingsParser.h

@@ -119,4 +119,4 @@ namespace settings_parser
 
     /** The "--version" output string. */
     juce::String getVersionString();
-}
+} // namespace settings_parser