From a5f8da73963e9fe94a35e91c7f093c539b8f5e31 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 4 Oct 2026 08:03:59 -0400 Subject: [PATCH 1/6] Fix orphaned RenderConstantFieldValue requirement traceability The ApiMarkDotNet system-level requirement's children list was missing ApiMarkDotNet-DotNetEmitter-RenderConstantFieldValue, leaving it unreachable from any requirement tagged system/quality and causing reqstream --enforce to report it as orphaned. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reqstream/api-mark-dot-net.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/reqstream/api-mark-dot-net.yaml b/docs/reqstream/api-mark-dot-net.yaml index 9bf1a64..df47f4c 100644 --- a/docs/reqstream/api-mark-dot-net.yaml +++ b/docs/reqstream/api-mark-dot-net.yaml @@ -40,6 +40,7 @@ sections: - ApiMarkDotNet-DotNetEmitter-EmitConversionOperatorsWithConversionSyntax - ApiMarkDotNet-DotNetEmitter-RenderClassModifiersInTypeSignature - ApiMarkDotNet-DotNetEmitter-RenderInitOnlyPropertyAccessor + - ApiMarkDotNet-DotNetEmitter-RenderConstantFieldValue - ApiMarkDotNet-DotNetEmitterGradualDisclosure-EmitApiIndexPage - ApiMarkDotNet-DotNetEmitterGradualDisclosure-EmitNamespacePages - ApiMarkDotNet-DotNetEmitterGradualDisclosure-EmitNamespaceDocRemarksAndExamples From e42b871fc32168d2922376fb674d58eced02c5ff Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 4 Oct 2026 08:04:06 -0400 Subject: [PATCH 2/6] Bound the clang subprocess wait with a timeout and rich diagnostics ClangAstParser.RunProcess called Process.WaitForExit() with no timeout, so a hung clang/xcrun invocation (observed on macOS CI, e.g. run 37162361228 which had to be manually cancelled after 47 minutes) would block the calling test indefinitely, and transitively the whole CI job. RunProcess now waits up to 120 seconds and, on timeout, kills the process tree (reusing the existing TryKillProcess helper) and throws an InvalidOperationException carrying the OS description/architecture, PID, elapsed time, full command line, and any partial stdout/stderr the process had already produced -- so a future hang can be diagnosed from CI output alone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/ApiMark.Cpp/CppAst/ClangAstParser.cs | 108 ++++++++++++++++++++++- 1 file changed, 105 insertions(+), 3 deletions(-) diff --git a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs index bbed09c..86e510f 100644 --- a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs +++ b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs @@ -702,11 +702,22 @@ private static List BuildArguments( /// Starts a process, reads stdout and stderr concurrently to avoid pipe deadlock, /// and returns the combined output with the exit code. /// + /// + /// The wait is bounded by so that a clang (or + /// xcrun) invocation that never exits — e.g. a first-run Xcode license prompt + /// blocked on stdin, or a process starved by heavy CI-runner contention — fails this + /// single call instead of hanging the calling test (and, transitively, the whole CI + /// job) indefinitely. The process (and any children it spawned) is killed on timeout. + /// The resulting exception carries the OS, PID, elapsed time, full command line, and + /// any partial stdout/stderr the process had already produced, so a hang can be + /// diagnosed from CI output alone without needing to reproduce it locally. + /// /// Executable to launch. /// Arguments to pass to the executable. /// A tuple of (stdout, stderr, exitCode). /// - /// Thrown when the process cannot be started. + /// Thrown when the process cannot be started, or when it does not exit within + /// . /// private static (string Stdout, string Stderr, int ExitCode) RunProcess( string fileName, @@ -726,19 +737,110 @@ private static (string Stdout, string Stderr, int ExitCode) RunProcess( psi.ArgumentList.Add(arg); } + var commandLine = BuildDiagnosticCommandLine(fileName, arguments); + var stopwatch = Stopwatch.StartNew(); + using var process = Process.Start(psi) - ?? throw new InvalidOperationException($"Failed to start process '{fileName}'."); + ?? throw new InvalidOperationException( + $"Failed to start process '{fileName}'. Command: {commandLine}"); + + // Capture the PID up front — it is still readable after Kill(), but not reliably + // so once the process has fully exited and been reaped on some platforms + var processId = process.Id; // Read stdout and stderr concurrently — reading them sequentially can deadlock // when one pipe's buffer fills while the other is not being drained var stdoutTask = process.StandardOutput.ReadToEndAsync(); var stderrTask = process.StandardError.ReadToEndAsync(); - process.WaitForExit(); + if (!process.WaitForExit(ClangTimeoutMilliseconds)) + { + TryKillProcess(process); + + // Best-effort: give the now-killed process's pipes a short grace period to flush + // so partial output (e.g. a diagnostic clang had already printed) can be reported. + // This wait is bounded independently of ClangTimeoutMilliseconds so a pipe that + // never closes cannot extend the hang. + var partialStdout = TryGetPartialOutput(stdoutTask); + var partialStderr = TryGetPartialOutput(stderrTask); + + throw new InvalidOperationException( + $"Process did not exit within {ClangTimeoutMilliseconds}ms and was killed.\n" + + $" OS: {RuntimeInformation.OSDescription} ({RuntimeInformation.OSArchitecture})\n" + + $" PID: {processId}\n" + + $" Elapsed: {stopwatch.Elapsed}\n" + + $" Command: {commandLine}\n" + + $" Partial stdout ({partialStdout.Length} chars): {Truncate(partialStdout)}\n" + + $" Partial stderr ({partialStderr.Length} chars): {Truncate(partialStderr)}"); + } return (stdoutTask.Result, stderrTask.Result, process.ExitCode); } + /// + /// Maximum time to wait for the clang (or xcrun-wrapped clang) AST-dump + /// invocation to exit before treating it as hung and killing it. + /// + private const int ClangTimeoutMilliseconds = 120_000; + + /// + /// Maximum time to wait, after a hung process has been killed, for its redirected + /// stdout/stderr pipes to report as complete so partial output can be captured. + /// + private const int PartialOutputGraceMilliseconds = 2000; + + /// + /// Maximum number of characters of partial stdout/stderr to include in a timeout + /// exception message, so a chatty hung process does not produce an unreadable error. + /// + private const int PartialOutputMaxChars = 2000; + + /// + /// Builds a human-readable, shell-quoted rendering of a command line for diagnostic + /// messages. Not used to actually invoke the process — + /// already handles that safely. + /// + /// Executable name or path. + /// Arguments passed to the executable. + /// A single-line, space-separated, quoted command line for logging. + private static string BuildDiagnosticCommandLine(string fileName, IReadOnlyList arguments) + { + var parts = new[] { fileName }.Concat(arguments) + .Select(a => a.Contains(' ', StringComparison.Ordinal) ? $"\"{a}\"" : a); + return string.Join(' ', parts); + } + + /// + /// Best-effort retrieval of a stdout/stderr read task's result after the owning + /// process has been killed, bounded by so + /// a pipe that never reports complete cannot extend the timeout exception path. + /// + /// The task to probe. + /// The captured text, or an empty string if it did not complete in time. + private static string TryGetPartialOutput(Task readTask) + { + try + { + return readTask.Wait(PartialOutputGraceMilliseconds) ? readTask.Result : string.Empty; + } + catch (AggregateException) + { + // The read itself faulted (e.g. pipe torn down mid-read) — no partial output available + return string.Empty; + } + } + + /// + /// Truncates diagnostic text to so a chatty hung + /// process cannot produce an unreadable exception message. + /// + /// The text to truncate. + /// The original text, or a truncated copy with a trailing marker. + private static string Truncate(string text) => + text.Length <= PartialOutputMaxChars + ? text + : string.Concat(text.AsSpan(0, PartialOutputMaxChars), "… [truncated]"); + /// /// Extracts error-class lines from clang's stderr output, used to populate /// . From 90cc4b42a992dacd10a6a836a088e0ec9df21b41 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 4 Oct 2026 08:38:37 -0400 Subject: [PATCH 3/6] Add automated coverage for the clang timeout/kill path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review feedback on #49: the timeout behavior added in e42b871 had no automated test coverage. ClangAstParser.RunProcess now reads a thread-local TimeoutOverrideMillisecondsForTests field (reset by the test in a finally block to avoid thread-pool reuse leaking it into unrelated tests) so a test can exercise the kill/diagnostics path with a short wait instead of the real 120-second production timeout. Added ClangAstParser_Parse_ClangHangs_ThrowsWithDiagnosticsAndKillsProcess, which points ClangPath at a small platform-appropriate script that ignores its arguments and sleeps far longer than the test timeout, then asserts the thrown InvalidOperationException carries each diagnostic field (OS, PID, elapsed time, command line, partial stdout/stderr). Re: the reported 'invalid string.Contains overload for net8.0' finding — verified this is a false positive. Contains(char, StringComparison) has been part of the BCL since .NET 5 and ApiMark.Cpp targets net8.0 only (confirmed via a clean --no-incremental rebuild with 0 warnings/errors, TreatWarningsAsErrors is enabled for this project). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/ApiMark.Cpp/CppAst/ClangAstParser.cs | 18 +++- test/ApiMark.Cpp.Tests/ClangAstParserTests.cs | 84 +++++++++++++++++++ 2 files changed, 99 insertions(+), 3 deletions(-) diff --git a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs index 86e510f..32e6777 100644 --- a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs +++ b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs @@ -753,19 +753,20 @@ private static (string Stdout, string Stderr, int ExitCode) RunProcess( var stdoutTask = process.StandardOutput.ReadToEndAsync(); var stderrTask = process.StandardError.ReadToEndAsync(); - if (!process.WaitForExit(ClangTimeoutMilliseconds)) + var timeoutMilliseconds = TimeoutOverrideMillisecondsForTests ?? ClangTimeoutMilliseconds; + if (!process.WaitForExit(timeoutMilliseconds)) { TryKillProcess(process); // Best-effort: give the now-killed process's pipes a short grace period to flush // so partial output (e.g. a diagnostic clang had already printed) can be reported. - // This wait is bounded independently of ClangTimeoutMilliseconds so a pipe that + // This wait is bounded independently of timeoutMilliseconds so a pipe that // never closes cannot extend the hang. var partialStdout = TryGetPartialOutput(stdoutTask); var partialStderr = TryGetPartialOutput(stderrTask); throw new InvalidOperationException( - $"Process did not exit within {ClangTimeoutMilliseconds}ms and was killed.\n" + + $"Process did not exit within {timeoutMilliseconds}ms and was killed.\n" + $" OS: {RuntimeInformation.OSDescription} ({RuntimeInformation.OSArchitecture})\n" + $" PID: {processId}\n" + $" Elapsed: {stopwatch.Elapsed}\n" + @@ -783,6 +784,17 @@ private static (string Stdout, string Stderr, int ExitCode) RunProcess( /// private const int ClangTimeoutMilliseconds = 120_000; + /// + /// Test-only override for the timeout applied in , scoped to + /// the calling thread via so a test exercising the + /// timeout/kill path with an artificially short wait cannot affect the (ordinarily + /// much faster) real clang invocations made concurrently by other tests. Tests must + /// reset this to in a finally block — thread-pool thread + /// reuse would otherwise leak the override into an unrelated later test. + /// + [ThreadStatic] + internal static int? TimeoutOverrideMillisecondsForTests; + /// /// Maximum time to wait, after a hung process has been killed, for its redirected /// stdout/stderr pipes to report as complete so partial output can be captured. diff --git a/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs b/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs index a26dd07..f3385cf 100644 --- a/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs +++ b/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System.Diagnostics; +using System.Runtime.InteropServices; using ApiMark.Cpp; using ApiMark.Cpp.CppAst; using Xunit; @@ -514,4 +515,87 @@ public void ClangAstParser_Parse_CalledDirectlyWithDifferentlyCasedIncludeRoot_R Directory.Delete(tempParent, recursive: true); } } + + /// + /// Writes a tiny script that ignores every argument it is given and sleeps far longer + /// than any test timeout, so it can stand in for a hung clang invocation without + /// depending on clang (or any OS-specific hang trigger such as an Xcode license prompt) + /// being reproducible in this environment. + /// + /// Directory to create the script file in. + /// Absolute path to the created, executable script. + private static string CreateHangingScript(string directory) + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + // .NET's Process.Start transparently runs .cmd/.bat files through cmd.exe on + // Windows, so this can be used directly as CppGeneratorOptions.ClangPath. + var scriptPath = Path.Combine(directory, "hang.cmd"); + File.WriteAllText(scriptPath, "@echo off\r\nping -n 1000 127.0.0.1 >nul\r\n"); + return scriptPath; + } + + var shPath = Path.Combine(directory, "hang.sh"); + File.WriteAllText(shPath, "#!/bin/sh\nsleep 1000\n"); + File.SetUnixFileMode( + shPath, + UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | + UnixFileMode.GroupRead | UnixFileMode.GroupExecute | + UnixFileMode.OtherRead | UnixFileMode.OtherExecute); + return shPath; + } + + /// + /// Validates that a clang invocation which never exits is killed and reported as an + /// bearing diagnostic detail, instead of hanging + /// the test (and, in CI, the whole job) indefinitely. + /// + [Fact] + public void ClangAstParser_Parse_ClangHangs_ThrowsWithDiagnosticsAndKillsProcess() + { + // Arrange: a fake "clang" that ignores its arguments and sleeps indefinitely, combined + // with a short thread-local timeout override so this test does not actually wait for + // the production 120-second timeout + var tempDir = Path.Combine(Path.GetTempPath(), "ApiMarkClangHangTest_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + try + { + var hangingScript = CreateHangingScript(tempDir); + var options = new CppGeneratorOptions + { + LibraryName = "Fixtures", + PublicIncludeRoots = [], + ClangPath = hangingScript, + }; + + ClangAstParser.TimeoutOverrideMillisecondsForTests = 300; + try + { + // Act + var ex = Assert.Throws( + () => ClangAstParser.Parse(["dummy.h"], options)); + + // Assert: the exception carries enough diagnostic detail to triage a real hang + // from CI output alone, without needing to reproduce it locally + Assert.Contains("did not exit within 300ms", ex.Message, StringComparison.Ordinal); + Assert.Contains("OS:", ex.Message, StringComparison.Ordinal); + Assert.Contains("PID:", ex.Message, StringComparison.Ordinal); + Assert.Contains("Elapsed:", ex.Message, StringComparison.Ordinal); + Assert.Contains("Command:", ex.Message, StringComparison.Ordinal); + Assert.Contains(hangingScript, ex.Message, StringComparison.Ordinal); + Assert.Contains("Partial stdout", ex.Message, StringComparison.Ordinal); + Assert.Contains("Partial stderr", ex.Message, StringComparison.Ordinal); + } + finally + { + // Reset the override — thread-pool thread reuse would otherwise leak a + // 300ms timeout into an unrelated later test running on the same thread + ClangAstParser.TimeoutOverrideMillisecondsForTests = null; + } + } + finally + { + Directory.Delete(tempDir, recursive: true); + } + } } From 0608098bf83a55b425c363b436ac6dbd902f15fe Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 4 Oct 2026 09:54:58 -0400 Subject: [PATCH 4/6] Make clang subprocess timeout configurable Adds CppGeneratorOptions.ClangTimeoutMilliseconds, --clang-timeout-ms CLI flag, ApiMarkClangTimeoutMs MSBuild property, and APIMARK_CLANG_TIMEOUT_MS env var override for the clang AST-dump subprocess timeout introduced in PR #49. Precedence: test seam > explicit option > env var > hard-coded 120000ms default. Includes tests, reqstream/design/verification docs, and user guide updates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 4 + docs/design/api-mark-cpp/clang-ast-parser.md | 10 +- docs/design/api-mark-cpp/cpp-generator.md | 3 + docs/design/api-mark-msbuild.md | 3 +- docs/design/api-mark-msbuild/api-mark-task.md | 8 +- docs/design/api-mark-tool/cli.md | 6 +- docs/design/api-mark-tool/cli/context.md | 1 + docs/design/ots/clang.md | 8 + .../api-mark-cpp/clang-ast-parser.yaml | 23 ++- docs/reqstream/api-mark-msbuild.yaml | 1 + .../api-mark-msbuild/api-mark-task.yaml | 9 + docs/reqstream/api-mark-tool/cli.yaml | 1 + docs/reqstream/api-mark-tool/cli/context.yaml | 12 ++ docs/reqstream/api-mark-tool/program.yaml | 8 +- docs/user_guide/cpp.md | 16 ++ docs/user_guide/faq.md | 16 ++ .../api-mark-cpp/clang-ast-parser.md | 32 +++ docs/verification/api-mark-msbuild.md | 5 + .../api-mark-msbuild/api-mark-task.md | 6 + .../verification/api-mark-tool/cli/context.md | 11 + src/ApiMark.Cpp/CppAst/ClangAstParser.cs | 130 +++++++++++- src/ApiMark.Cpp/CppGeneratorOptions.cs | 9 + src/ApiMark.MSBuild/ApiMarkTask.cs | 13 +- .../DemaConsulting.ApiMark.MSBuild.targets | 1 + src/ApiMark.Tool/Cli/Context.cs | 21 ++ src/ApiMark.Tool/Program.cs | 2 + test/ApiMark.Cpp.Tests/ClangAstParserTests.cs | 195 +++++++++++++++++- .../ApiMark.MSBuild.Tests/ApiMarkTaskTests.cs | 25 +++ test/ApiMark.Tool.Tests/Cli/ContextTests.cs | 49 +++++ 29 files changed, 597 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 32cc958..edde55f 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,10 @@ C++ documentation generation requires `clang` to be installed and available: - **macOS**: Xcode Command Line Tools (`xcode-select --install`) — `clang` is included. - **Linux**: Install via the system package manager (e.g. `apt install clang` or `dnf install clang`). +Large C++ projects with a slow clang invocation can raise the 120-second default timeout via +`--clang-timeout-ms`, `ApiMarkClangTimeoutMs`, or the `APIMARK_CLANG_TIMEOUT_MS` environment +variable. + ### VHDL Support VHDL documentation generation has no additional prerequisites. Parsing is done diff --git a/docs/design/api-mark-cpp/clang-ast-parser.md b/docs/design/api-mark-cpp/clang-ast-parser.md index 4ef3f66..9c8780e 100644 --- a/docs/design/api-mark-cpp/clang-ast-parser.md +++ b/docs/design/api-mark-cpp/clang-ast-parser.md @@ -42,8 +42,12 @@ while walking the JSON AST. PATH, `xcrun clang`, or Windows LLVM discovery. - **RunProcess** — launches the clang subprocess and concurrently drains stdout (JSON AST) and stderr on background tasks to prevent pipe-buffer - deadlock; throws `InvalidOperationException` when the process cannot be - started or exits non-zero with no usable JSON present. + deadlock; bounds the wait by a timeout resolved in precedence order (test + seam > `CppGeneratorOptions.ClangTimeoutMilliseconds` > `APIMARK_CLANG_TIMEOUT_MS` + environment variable > built-in 120000ms default) and kills the process tree, + throwing `InvalidOperationException` when the process cannot be started, does + not exit within the resolved timeout, or exits non-zero with no usable JSON + present. - **BuildArguments** — assembles the ordered clang argument list. - **CollectStderrErrors** — filters stderr to `error` and `fatal error` lines only. @@ -187,7 +191,7 @@ while walking the JSON AST. ### Dependencies - **CppGeneratorOptions** — supplies include roots, defines, standard, clang path, - and additional compiler arguments. + clang subprocess timeout, and additional compiler arguments. - **CppAstModel** — destination record model for parsed output. - **System.Text.Json** — used for JSON parsing and traversal. - **PathHelpers** — `NormalizeCase` and `NormalizeCaseDirectory` resolve source files and diff --git a/docs/design/api-mark-cpp/cpp-generator.md b/docs/design/api-mark-cpp/cpp-generator.md index d79387f..c080d7d 100644 --- a/docs/design/api-mark-cpp/cpp-generator.md +++ b/docs/design/api-mark-cpp/cpp-generator.md @@ -42,6 +42,9 @@ single-file or gradual-disclosure Markdown output. - `ClangPath`: `string?` — optional explicit clang path. When null or empty, discovery falls back to `APIMARK_CLANG_PATH`, PATH, `xcrun clang`, and Windows LLVM discovery. +- `ClangTimeoutMilliseconds`: `int?` — optional timeout (milliseconds) for the clang + subprocess; defaults to `null`, meaning the `APIMARK_CLANG_TIMEOUT_MS` environment + variable, then a built-in 120000ms default, applies. - `AdditionalCompilerArguments`: `IReadOnlyList` — raw trailing clang arguments used as an escape hatch for toolchain-specific switches. - `Visibility`: `ApiVisibility` — class-member visibility filter consumed later by diff --git a/docs/design/api-mark-msbuild.md b/docs/design/api-mark-msbuild.md index 2d8b1e3..fd83530 100644 --- a/docs/design/api-mark-msbuild.md +++ b/docs/design/api-mark-msbuild.md @@ -67,7 +67,8 @@ task. converted to commas for the tool argument), `ApiMarkCppStandard` (e.g. `c++17`; optional, defaulted to `c++17` by the `.targets` file when not set by the project), - `ApiMarkClangPath` (optional explicit clang executable path). + `ApiMarkClangPath` (optional explicit clang executable path), + `ApiMarkClangTimeoutMs` (optional clang subprocess timeout in milliseconds). Optional input item group `ApiMarkOutput` (exposed to MSBuild projects via the task property `ApiMarkOutputs`); each item carries `OutputDir`, `Format`, and `Visibility` metadata that overrides the corresponding scalar properties for that named output configuration; one child diff --git a/docs/design/api-mark-msbuild/api-mark-task.md b/docs/design/api-mark-msbuild/api-mark-task.md index eced75d..0dc2497 100644 --- a/docs/design/api-mark-msbuild/api-mark-task.md +++ b/docs/design/api-mark-msbuild/api-mark-task.md @@ -143,6 +143,12 @@ executable. Optional — when empty, clang is located using the priority order: `APIMARK_CLANG_PATH` environment variable → `clang` on PATH → `xcrun` (macOS) → vswhere / default LLVM path (Windows). +**ApiMarkTask.ApiMarkClangTimeoutMs**: `string` — MSBuild property +`$(ApiMarkClangTimeoutMs)`; for the `cpp` language, the clang subprocess +timeout in milliseconds. Optional — when empty, the tool falls back to the +`APIMARK_CLANG_TIMEOUT_MS` environment variable, then its built-in 120000ms +(2 minute) default. + **ApiMarkTask.ApiMarkFormat**: `string` — MSBuild property `$(ApiMarkFormat)`; controls the output format forwarded to the tool. Accepted values: `gradual` (multi-file gradual-disclosure tree) or `single-file` (single `api.md` @@ -219,7 +225,7 @@ patterns; if `ApiMarkLibraryName` is set, append `--library-name`; if `ApiMarkLibraryDescription` is set, append `--library-description`; if `ApiMarkDefines` is set, convert semicolons to commas and append `--defines`; if `ApiMarkCppStandard` is set, append `--cpp-standard`; if `ApiMarkClangPath` is set, -append `--clang-path`; then append `--enforce-docs`/`--enforce-docs-severity` using +append `--clang-path`; if `ApiMarkClangTimeoutMs` is set, append `--clang-timeout-ms`; then append `--enforce-docs`/`--enforce-docs-severity` using the same non-empty checks as the `dotnet` path, since cpp documentation-coverage enforcement uses the identical MSBuild properties; if `ApiMarkFormat` is set, append `--format`); this full logical argument list (from `BuildArguments`/ diff --git a/docs/design/api-mark-tool/cli.md b/docs/design/api-mark-tool/cli.md index 784a576..c3513f9 100644 --- a/docs/design/api-mark-tool/cli.md +++ b/docs/design/api-mark-tool/cli.md @@ -29,8 +29,8 @@ can be tested independently. properties (`Version`, `Help`, `Silent`, `Validate`, `Language`, `Assembly`, `XmlDoc`, `Includes`, `ApiHeaders`, `Sources`, `Output`, `Visibility`, `IncludeObsolete`, `ResultsFile`, `HeadingDepth`, `Format`, `LibraryName`, - `LibraryDescription`, `Defines`, `CppStandard`, `ClangPath`, `Excludes`, - `ReferencePaths`, `EnforceDocs`, `EnforceDocsSeverity`, `ExitCode`) + `LibraryDescription`, `Defines`, `CppStandard`, `ClangPath`, `ClangTimeoutMilliseconds`, + `Excludes`, `ReferencePaths`, `EnforceDocs`, `EnforceDocsSeverity`, `ExitCode`) and provides `WriteLine` and `WriteError` for all program output routing. **Consumed**: @@ -51,7 +51,7 @@ first positional non-flag token (a token that does not start with `-`) is captured as the language subcommand. Language-specific options (`--assembly`, `--xml-doc`, `--includes`, `--api-headers`, `--source`, `--output`, `--visibility`, `--include-obsolete`, `--library-name`, `--library-description`, `--defines`, -`--cpp-standard`, `--clang-path`, `--exclude`, `--reference-paths`, `--enforce-docs`, +`--cpp-standard`, `--clang-path`, `--clang-timeout-ms`, `--exclude`, `--reference-paths`, `--enforce-docs`, `--enforce-docs-severity`) may appear anywhere in the argument list after the language token is recognized. diff --git a/docs/design/api-mark-tool/cli/context.md b/docs/design/api-mark-tool/cli/context.md index fdf769c..85a05a3 100644 --- a/docs/design/api-mark-tool/cli/context.md +++ b/docs/design/api-mark-tool/cli/context.md @@ -40,6 +40,7 @@ argument array. | `Defines` | `string[]` | `[]` | Preprocessor definitions from `--defines` (comma-separated, split into entries of the form `NAME` or `NAME=value`) | | `CppStandard` | `string?` | `null` | C++ language standard from `--cpp-standard` (e.g. `c++17`); optional, tool defaults to `c++17` when null | | `ClangPath` | `string?` | `null` | Explicit clang executable path from `--clang-path`; optional, auto-discovered when null | +| `ClangTimeoutMilliseconds` | `int?` | `null` | Clang subprocess timeout in milliseconds from `--clang-timeout-ms`; optional, falls back to `APIMARK_CLANG_TIMEOUT_MS` then a built-in 120000ms default when null | | `Format` | `OutputFormat` | `GradualDisclosure` | Output format from `--format` (`gradual` → `GradualDisclosure`, `single-file` → `SingleFile`) | | `Excludes` | `string[]` | `[]` | Wildcard patterns accumulated from repeated `--exclude` invocations | | `ReferencePaths` | `string[]` | `[]` | Referenced assembly DLL paths accumulated from repeated `--reference-paths` invocations, used to resolve cross-assembly `` | diff --git a/docs/design/ots/clang.md b/docs/design/ots/clang.md index 8f99323..36113ac 100644 --- a/docs/design/ots/clang.md +++ b/docs/design/ots/clang.md @@ -44,6 +44,14 @@ users to control the exact compiler version used for parsing. (5) vswhere-located LLVM clang on Windows, then `C:\Program Files\LLVM\bin\clang.exe`. When a path is supplied by option (1) or (2) and the file does not exist on disk, an `InvalidOperationException` is thrown immediately with a clear diagnostic message. +- **Clang invocation timeout** — the `clang` subprocess invocation is bounded by a configurable + timeout, not a clang feature itself but an ApiMark-side safeguard against a hung invocation + (e.g. a first-run Xcode license prompt blocked on stdin). The timeout is resolved using the + same explicit-option-over-environment-variable precedence as clang discovery: (1) + `CppGeneratorOptions.ClangTimeoutMilliseconds` when explicitly set; (2) the + `APIMARK_CLANG_TIMEOUT_MS` environment variable when set; (3) a built-in 120000ms (2 minute) + default. The process (and its tree) is killed and an `InvalidOperationException` is thrown + if the timeout is exceeded. ### Integration Pattern diff --git a/docs/reqstream/api-mark-cpp/clang-ast-parser.yaml b/docs/reqstream/api-mark-cpp/clang-ast-parser.yaml index 88a0430..b47e417 100644 --- a/docs/reqstream/api-mark-cpp/clang-ast-parser.yaml +++ b/docs/reqstream/api-mark-cpp/clang-ast-parser.yaml @@ -14,7 +14,7 @@ sections: resolved C++ AST. Delegating parse to ClangAstParser keeps the I/O and JSON-deserialization logic isolated from the higher-level generation logic, making each unit independently testable. - children: [ApiMarkCpp-ClangAstParser-ParseHeaders, ApiMarkCpp-ClangAstParser-NormalizeIncludeRootsRegardlessOfCaller, ApiMarkCpp-ClangAstParser-ParseConstexprLiteralFieldValue] + children: [ApiMarkCpp-ClangAstParser-ParseHeaders, ApiMarkCpp-ClangAstParser-NormalizeIncludeRootsRegardlessOfCaller, ApiMarkCpp-ClangAstParser-ParseConstexprLiteralFieldValue, ApiMarkCpp-ClangAstParser-ConfigurableTimeout] tests: [ClangAstParser_Parse_FixtureHeaders_ReturnsNonEmptyNamespaces] - id: ApiMarkCpp-ClangAstParser-ParseHeaders title: >- @@ -74,3 +74,24 @@ sections: - ClangAstParser_Parse_FixtureHeaders_ConstexprNegativeIntField_HasNumericValue - ClangAstParser_Parse_FixtureHeaders_ConstexprFloatingField_HasDecimalValue - ClangAstParser_Parse_FixtureHeaders_ConstexprDoubleNegatedField_HasNullValue + - id: ApiMarkCpp-ClangAstParser-ConfigurableTimeout + title: >- + ClangAstParser shall bound the clang subprocess invocation by a configurable + timeout — resolved from CppGeneratorOptions.ClangTimeoutMilliseconds, then the + APIMARK_CLANG_TIMEOUT_MS environment variable, then a built-in 120000ms default — + killing the process (and its tree) and throwing a diagnostic + InvalidOperationException if the timeout is exceeded. + justification: | + A clang invocation can hang indefinitely (e.g. a first-run Xcode license prompt + blocked on stdin, or a process starved by heavy CI-runner contention), which would + otherwise hang the calling build or test run forever. A fixed 120-second timeout + protects the common case, but large C++ projects can legitimately need a longer + clang invocation; making the timeout configurable via options, environment + variable, CLI flag, and MSBuild property lets those projects raise the limit + without forking ApiMark, while every other project keeps the safe default + unchanged. + tests: + - ClangAstParser_Parse_ClangHangs_ThrowsWithDiagnosticsAndKillsProcess + - ClangAstParser_Parse_ClangTimeoutMillisecondsOption_HonoredWithoutTestSeam + - ClangAstParser_Parse_ClangTimeoutEnvVar_OverridesDefaultTimeout + - ClangAstParser_Parse_ClangTimeoutEnvVar_InvalidValue_ThrowsInvalidOperationException diff --git a/docs/reqstream/api-mark-msbuild.yaml b/docs/reqstream/api-mark-msbuild.yaml index 5efa40f..abf7f39 100644 --- a/docs/reqstream/api-mark-msbuild.yaml +++ b/docs/reqstream/api-mark-msbuild.yaml @@ -26,6 +26,7 @@ sections: - ApiMarkMsbuild-ApiMarkTask-ForwardApiHeaders - ApiMarkMsbuild-ApiMarkTask-ForwardLibraryDescription - ApiMarkMsbuild-ApiMarkTask-ForwardClangPath + - ApiMarkMsbuild-ApiMarkTask-ForwardClangTimeoutMs - ApiMarkMsbuild-ApiMarkTask-AutoPopulateIncludePaths - ApiMarkMsbuild-ApiMarkTask-SkipWhenDotNetXmlDocPathNotSet - ApiMarkMsbuild-ApiMarkTask-ForwardFormatProperty diff --git a/docs/reqstream/api-mark-msbuild/api-mark-task.yaml b/docs/reqstream/api-mark-msbuild/api-mark-task.yaml index aea9cd1..32b34df 100644 --- a/docs/reqstream/api-mark-msbuild/api-mark-task.yaml +++ b/docs/reqstream/api-mark-msbuild/api-mark-task.yaml @@ -218,6 +218,15 @@ sections: non-default path. Forwarding ApiMarkClangPath lets build authors specify the executable path without modifying system PATH, keeping the build reproducible. tests: [ApiMarkTask_Cpp_ClangPath_ForwardedToTool] + - id: ApiMarkMsbuild-ApiMarkTask-ForwardClangTimeoutMs + title: ApiMarkTask shall forward the ApiMarkClangTimeoutMs property to the + tool as the --clang-timeout-ms argument for C++ builds when set. + justification: | + Large C++ projects with a slow clang invocation need a way to raise the default + 120-second timeout without forking ApiMark. Forwarding ApiMarkClangTimeoutMs lets + build authors configure this per-project via MSBuild, consistent with how + ApiMarkClangPath is forwarded. + tests: [ApiMarkTask_Cpp_ClangTimeoutMs_ForwardedToTool] - id: ApiMarkMsbuild-ApiMarkTask-AutoPopulateIncludePaths title: >- ApiMarkTask shall automatically populate ApiMarkIncludePaths from the diff --git a/docs/reqstream/api-mark-tool/cli.yaml b/docs/reqstream/api-mark-tool/cli.yaml index 15cbdb3..45aef4a 100644 --- a/docs/reqstream/api-mark-tool/cli.yaml +++ b/docs/reqstream/api-mark-tool/cli.yaml @@ -24,6 +24,7 @@ sections: - ApiMarkTool-Cli-Context-TrackErrorsViaWriteError - ApiMarkTool-Cli-Context-ParseFormatOption - ApiMarkTool-Cli-Context-ParseClangPath + - ApiMarkTool-Cli-Context-ParseClangTimeoutMsOption - ApiMarkTool-Cli-Context-ParseVhdlSourceOptions - ApiMarkTool-Cli-Context-ParseExcludes - ApiMarkTool-Cli-Context-ParseReferencePaths diff --git a/docs/reqstream/api-mark-tool/cli/context.yaml b/docs/reqstream/api-mark-tool/cli/context.yaml index 6a97cc0..92bc28c 100644 --- a/docs/reqstream/api-mark-tool/cli/context.yaml +++ b/docs/reqstream/api-mark-tool/cli/context.yaml @@ -180,6 +180,18 @@ sections: The cpp subcommand passes ClangPath to the CppGenerator to override the default clang binary location discovered via PATH, xcrun (macOS), or vswhere (Windows). tests: [Context_Create_WithClangPathOption_SetsClangPath] + - id: ApiMarkTool-Cli-Context-ParseClangTimeoutMsOption + title: Context shall parse the --clang-timeout-ms option and expose + the value for use by the cpp subcommand, rejecting values below + 1000ms. + justification: | + The cpp subcommand passes ClangTimeoutMilliseconds to the CppGenerator to + override the default 120-second clang subprocess timeout, letting large C++ + projects configure a longer bound without forking ApiMark. + tests: + - Context_Create_WithClangTimeoutMsOption_SetsClangTimeoutMilliseconds + - Context_Create_WithoutClangTimeoutMsOption_LeavesClangTimeoutMillisecondsNull + - Context_Create_WithClangTimeoutMsOptionBelowMinimum_ThrowsArgumentException - id: ApiMarkTool-Cli-Context-ParseVhdlSourceOptions title: Context shall parse repeated --source flags and accumulate them into the Sources array in order, preserving any leading ! exclusion diff --git a/docs/reqstream/api-mark-tool/program.yaml b/docs/reqstream/api-mark-tool/program.yaml index 2bd83fe..a710627 100644 --- a/docs/reqstream/api-mark-tool/program.yaml +++ b/docs/reqstream/api-mark-tool/program.yaml @@ -72,7 +72,8 @@ sections: - id: ApiMarkTool-Program-SupportCppOptions title: Program shall support the cpp language with --includes, --output, --visibility, --include-obsolete, --library-name, --library-description, - --defines, --cpp-standard, --api-headers, and --clang-path options. + --defines, --cpp-standard, --api-headers, --clang-path, and --clang-timeout-ms + options. justification: | C++ documentation generation requires at minimum the public include root(s) and output directory. The --includes option is required; the tool validates its @@ -82,7 +83,10 @@ sections: allowing fine-grained control over which headers contribute to the documented API surface using ordered patterns with '!' exclusion prefix support. The --clang-path option allows callers to override the default clang binary discovery - when clang is not on PATH or the system default is not appropriate. + when clang is not on PATH or the system default is not appropriate. The + --clang-timeout-ms option allows callers to raise (or lower) the default + 120-second clang subprocess timeout for large C++ projects that legitimately need + a longer invocation. tests: - Program_Main_WithCppSubcommand_MissingIncludes_ReturnsNonZeroExitCode - Program_Main_CppWithApiHeadersFlag_FlagIsAccepted diff --git a/docs/user_guide/cpp.md b/docs/user_guide/cpp.md index 1347be2..1d65bb7 100644 --- a/docs/user_guide/cpp.md +++ b/docs/user_guide/cpp.md @@ -23,6 +23,20 @@ ApiMark locates the clang executable using the following priority order: 4. `xcrun clang` — macOS only, selects the active Xcode SDK automatically. 5. vswhere-located LLVM clang / `C:\Program Files\LLVM\bin\clang.exe` — Windows only. +### Clang Invocation Timeout + +The clang subprocess invocation is bounded by a timeout, so a hung invocation (e.g. a first-run +Xcode license prompt blocked on stdin) fails fast with a diagnostic error instead of hanging the +build indefinitely. ApiMark resolves the timeout using the following priority order: + +1. `--clang-timeout-ms ` — explicit value, in milliseconds (minimum `1000`). +2. `APIMARK_CLANG_TIMEOUT_MS` environment variable — set this in CI or shell profiles to + configure the timeout project-wide without repeating the value on every invocation. +3. Built-in default of `120000` (2 minutes). + +Large C++ projects with a slow clang invocation can raise the default via `--clang-timeout-ms`, +`ApiMarkClangTimeoutMs`, or the `APIMARK_CLANG_TIMEOUT_MS` environment variable. + ## CLI Options ```text @@ -40,6 +54,7 @@ apimark cpp [options] | `--defines ` | Comma-separated preprocessor definitions (e.g. `MYLIB_API=,NDEBUG`) | | `--cpp-standard ` | C++ language standard passed to Clang (default: `c++17`) | | `--clang-path ` | Path to clang executable (default: auto-discovered, see *Prerequisites*) | +| `--clang-timeout-ms ` | Timeout in milliseconds for the clang subprocess; the process (and its tree) is killed and a diagnostic error raised if exceeded (default: `120000`) | | `--visibility ` | Visibility filter: `Public`, `PublicAndProtected`, `All` (default: `Public`) | | `--include-obsolete` | Include deprecated members in generated output | | `--enforce-docs ` | Enable documentation-coverage enforcement at the given visibility tier: `Public`, `PublicAndProtected`, `All` (default: disabled) | @@ -284,6 +299,7 @@ include structures, generated headers, or complex NuGet arrangements, use the | `ApiMarkDefines` | *(unset)* | Semicolon-separated preprocessor definitions (e.g. `MYLIB_API=;NDEBUG`) | | `ApiMarkCppStandard` | `c++17` | C++ language standard passed to Clang | | `ApiMarkClangPath` | *(auto-discovered)* | Path to clang executable; overrides PATH / xcrun / vswhere discovery | +| `ApiMarkClangTimeoutMs` | `120000` | Timeout in milliseconds for the clang subprocess | | `ApiMarkVisibility` | `Public` | Visibility filter: `Public`, `PublicAndProtected`, `All` | | `ApiMarkIncludeObsolete` | `false` | Include deprecated members in generated output | | `ApiMarkEnforceDocs` | (unset) | Enforcement visibility tier for documentation-coverage checking: `Public`, `PublicAndProtected`, `All`; omitted disables enforcement | diff --git a/docs/user_guide/faq.md b/docs/user_guide/faq.md index e0b8c77..d3e5ac2 100644 --- a/docs/user_guide/faq.md +++ b/docs/user_guide/faq.md @@ -69,6 +69,22 @@ ApiMark locates clang using this priority order: If none of these resolve, set `APIMARK_CLANG_PATH` in your CI environment or shell profile, or pass `--clang-path` explicitly. +### Clang takes a long time or appears to hang + +The clang subprocess invocation is bounded by a timeout (default `120000`ms / 2 minutes), after +which the process (and its tree) is killed and a diagnostic `InvalidOperationException` is +raised rather than hanging the build indefinitely. Large C++ projects with a slow clang +invocation can raise this timeout via: + +1. `--clang-timeout-ms ` CLI option or `ApiMarkClangTimeoutMs` MSBuild property +2. `APIMARK_CLANG_TIMEOUT_MS` environment variable +3. Built-in default of `120000`ms + +If clang still never exits even after raising the timeout, the underlying cause is usually an +interactive prompt (e.g. a first-run Xcode license prompt blocked on stdin) or a starved CI +runner; check the diagnostic error message, which includes the OS, PID, elapsed time, and any +partial stdout/stderr clang had already produced. + ### No headers are being documented When `--api-headers` is not specified, ApiMark documents all headers with diff --git a/docs/verification/api-mark-cpp/clang-ast-parser.md b/docs/verification/api-mark-cpp/clang-ast-parser.md index 0d4cad7..143e3f8 100644 --- a/docs/verification/api-mark-cpp/clang-ast-parser.md +++ b/docs/verification/api-mark-cpp/clang-ast-parser.md @@ -28,6 +28,15 @@ installation accessible on PATH. When clang is not available, integration tests boolean, or character, including a leading unary `+`/`-` directly over a literal, e.g. `-5`) is extracted into `CppField.Value` as a display string; a computed expression (e.g. `2 * 21`) or a nested unary chain (e.g. `- -5`) leaves `CppField.Value` as `null`. +- A clang invocation that never exits is killed (along with its process tree) and reported as + an `InvalidOperationException` carrying diagnostic detail (OS, PID, elapsed time, command + line, and partial stdout/stderr) instead of hanging the caller indefinitely. +- The clang subprocess timeout is configurable, in precedence order: an explicit + `CppGeneratorOptions.ClangTimeoutMilliseconds` value; the `APIMARK_CLANG_TIMEOUT_MS` + environment variable; a built-in 120000ms (2 minute) default. +- An invalid (non-numeric or non-positive) `APIMARK_CLANG_TIMEOUT_MS` value causes `Parse` to + throw a clear `InvalidOperationException` naming the environment variable, rather than + silently falling back to the default or throwing an unhelpful `FormatException`. ### Test Scenarios @@ -119,6 +128,29 @@ directly over a genuine literal is reconstructed and a nested unary chain is nev as e.g. `--5`. Tested by `ClangAstParser_Parse_FixtureHeaders_ConstexprDoubleNegatedField_HasNullValue`. +**Clang hang is killed and reported with diagnostics**: Verifies that a clang invocation which +never exits is killed (along with its process tree) within the applicable timeout and reported +as an `InvalidOperationException` carrying the OS, PID, elapsed time, full command line, and any +partial stdout/stderr the process had already produced, so a hang can be diagnosed from CI +output alone. Tested by `ClangAstParser_Parse_ClangHangs_ThrowsWithDiagnosticsAndKillsProcess`. + +**Explicit timeout option honored without the test seam**: Verifies that +`CppGeneratorOptions.ClangTimeoutMilliseconds`, set directly on the options object (not via the +`TimeoutOverrideMillisecondsForTests` test seam), is honored by the production resolution path, +proving the configurable timeout actually reaches `RunProcess`. Tested by +`ClangAstParser_Parse_ClangTimeoutMillisecondsOption_HonoredWithoutTestSeam`. + +**Environment variable overrides the default timeout**: Verifies that the +`APIMARK_CLANG_TIMEOUT_MS` environment variable is honored when +`CppGeneratorOptions.ClangTimeoutMilliseconds` is not set. Tested by +`ClangAstParser_Parse_ClangTimeoutEnvVar_OverridesDefaultTimeout`. + +**Invalid environment variable value rejected**: Verifies that an invalid (non-numeric or +non-positive) `APIMARK_CLANG_TIMEOUT_MS` value produces a clear `InvalidOperationException` +naming the environment variable, instead of silently falling back to the default or throwing +an unhelpful `FormatException`. Tested by +`ClangAstParser_Parse_ClangTimeoutEnvVar_InvalidValue_ThrowsInvalidOperationException`. + **Known integration-only gap**: Non-zero exit and malformed JSON paths are not isolated by the current implementation without adding a process seam, so those behaviors remain covered only by real-process integration when reproducible in environment-specific failure cases. diff --git a/docs/verification/api-mark-msbuild.md b/docs/verification/api-mark-msbuild.md index b5d74b3..8dd00e2 100644 --- a/docs/verification/api-mark-msbuild.md +++ b/docs/verification/api-mark-msbuild.md @@ -35,6 +35,7 @@ on non-Windows environments. - For .NET and C++ projects, `ApiMarkLibraryDescription` is forwarded as `--library-description` when set. - For C++ projects, `ApiMarkClangPath` is forwarded as `--clang-path` when set. +- For C++ projects, `ApiMarkClangTimeoutMs` is forwarded as `--clang-timeout-ms` when set. - `ApiMarkIncludeObsolete` set to `true` adds the `--include-obsolete` flag to the spawned tool command. - `ApiMarkReferencePaths` is forwarded to the tool as repeated `--reference-paths` flags for .NET builds, and is auto-populated from `@(ReferencePath)` via the `.targets` file when not explicitly set by the project. @@ -158,6 +159,10 @@ tested by `ApiMarkTask_Cpp_LibraryDescription_ForwardedToTool` and passed to the spawned tool as the `--clang-path` argument for C++ builds when set. This scenario is tested by `ApiMarkTask_Cpp_ClangPath_ForwardedToTool`. +**C++ clang timeout is forwarded to the tool**: Verifies that the `ApiMarkClangTimeoutMs` +property is passed to the spawned tool as the `--clang-timeout-ms` argument for C++ builds +when set. This scenario is tested by `ApiMarkTask_Cpp_ClangTimeoutMs_ForwardedToTool`. + **Empty XmlDocPath causes graceful skip for .NET project**: Verifies that when `ApiMarkXmlDocPath` is empty or unset for a .NET project, the task returns success immediately with no tool invocation. This scenario is tested by diff --git a/docs/verification/api-mark-msbuild/api-mark-task.md b/docs/verification/api-mark-msbuild/api-mark-task.md index 8bd042e..8f23b48 100644 --- a/docs/verification/api-mark-msbuild/api-mark-task.md +++ b/docs/verification/api-mark-msbuild/api-mark-task.md @@ -79,6 +79,7 @@ MSBuild and VC++ tools installed; those tests skip gracefully when the package i - For .NET and C++ builds, `ApiMarkLibraryDescription` is forwarded as `--library-description` when set. - For C++ builds, `ApiMarkClangPath` is forwarded as `--clang-path` when set. +- For C++ builds, `ApiMarkClangTimeoutMs` is forwarded as `--clang-timeout-ms` when set. - For C++ builds, `ApiMarkDefines` semicolons are converted to commas and forwarded as `--defines`. - For C++ builds, `ApiMarkCppStandard` is forwarded as `--cpp-standard` when set. @@ -138,6 +139,11 @@ tested by `ApiMarkTask_Cpp_LibraryDescription_ForwardedToTool` and is passed to the spawned tool as the `--clang-path` argument for C++ builds when set. This scenario is tested by `ApiMarkTask_Cpp_ClangPath_ForwardedToTool`. +**C++ clang timeout is forwarded as --clang-timeout-ms**: Verifies that the +`ApiMarkClangTimeoutMs` property is passed to the spawned tool as the `--clang-timeout-ms` +argument for C++ builds when set. This scenario is tested by +`ApiMarkTask_Cpp_ClangTimeoutMs_ForwardedToTool`. + **C++ defines semicolons are converted to commas**: Verifies that semicolons in the `ApiMarkDefines` property are converted to commas before being forwarded as the `--defines` argument for C++ builds, because MSBuild uses semicolons as its list separator but the tool diff --git a/docs/verification/api-mark-tool/cli/context.md b/docs/verification/api-mark-tool/cli/context.md index 57f1871..16f9c48 100644 --- a/docs/verification/api-mark-tool/cli/context.md +++ b/docs/verification/api-mark-tool/cli/context.md @@ -37,6 +37,8 @@ up after itself. No other external files, services, or configuration are require corresponding properties. `--library-description` sets `LibraryDescription` and is shared by all three language subcommands (dotnet, C++, and VHDL), not C++-specific. - `--clang-path` sets `ClangPath` to the supplied path. +- `--clang-timeout-ms` sets `ClangTimeoutMilliseconds` to the supplied value (minimum 1000ms); + absent, it remains `null`; a value below 1000ms throws `ArgumentException`. - `--format` accepts `gradual` and `single-file`; defaults to `GradualDisclosure`; invalid values throw `ArgumentException`. - `--enforce-docs` sets `EnforceDocs` to the supplied visibility-tier string; absent, it @@ -191,6 +193,15 @@ corresponding properties set simultaneously. **`Context_Create_WithClangPathOption_SetsClangPath`**: `--clang-path /usr/bin/clang` → `ClangPath = "/usr/bin/clang"`. +**`Context_Create_WithClangTimeoutMsOption_SetsClangTimeoutMilliseconds`**: +`--clang-timeout-ms 300000` → `ClangTimeoutMilliseconds = 300000`. + +**`Context_Create_WithoutClangTimeoutMsOption_LeavesClangTimeoutMillisecondsNull`**: no +`--clang-timeout-ms` option → `ClangTimeoutMilliseconds = null`. + +**`Context_Create_WithClangTimeoutMsOptionBelowMinimum_ThrowsArgumentException`**: +`--clang-timeout-ms 500` → throws `ArgumentException` (below the 1000ms minimum). + **`Context_Create_WithSourceOption_SetsSources`**: `--source "src/**/*.vhd"` → `Sources = ["src/**/*.vhd"]`. diff --git a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs index 32e6777..3f62082 100644 --- a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs +++ b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs @@ -213,8 +213,13 @@ public static CppCompilationResult Parse( // Build argument list targeting only the combined temp file var args = BuildArguments(prefix, [tempFile], normalizedOptions); + // Resolve the clang subprocess timeout: explicit option > env var > built-in + // default, with the thread-local test seam taking precedence over all of them + var timeoutMilliseconds = TimeoutOverrideMillisecondsForTests + ?? ResolveClangTimeoutMilliseconds(normalizedOptions.ClangTimeoutMilliseconds); + // Invoke clang and capture stdout (JSON) and stderr (diagnostics) without deadlock - var (stdout, stderr, exitCode) = RunProcess(fileName, args); + var (stdout, stderr, exitCode) = RunProcess(fileName, args, timeoutMilliseconds); // Surface a clear error when clang fails and produces no JSON — the empty check // guards against the common case where clang prints "command not found" to stderr @@ -282,6 +287,9 @@ public static CppCompilationResult Parse( /// The name of the environment variable that overrides automatic clang discovery. internal const string ClangPathEnvVar = "APIMARK_CLANG_PATH"; + /// The name of the environment variable that overrides the default clang timeout. + internal const string ClangTimeoutEnvVar = "APIMARK_CLANG_TIMEOUT_MS"; + /// /// Resolves the clang executable path and any command prefix needed to invoke it. /// @@ -694,6 +702,61 @@ private static List BuildArguments( return args; } + // ========================================================================= + // Clang timeout resolution + // ========================================================================= + + /// + /// Resolves the clang subprocess timeout in milliseconds, in precedence order: + /// (1) (explicit + /// ), (2) the + /// environment variable, (3) + /// . Mirrors the precedence used by + /// for : + /// an explicit option always wins over the environment variable. + /// + /// + /// Optional explicit timeout from . + /// + /// The resolved timeout in milliseconds. + /// + /// Thrown when is not a positive value, or + /// when the environment variable is set but is not a + /// positive integer. + /// + private static int ResolveClangTimeoutMilliseconds(int? optionsTimeoutMilliseconds) + { + // Option 1: explicit CppGeneratorOptions.ClangTimeoutMilliseconds + if (optionsTimeoutMilliseconds is { } explicitValue) + { + if (explicitValue <= 0) + { + throw new InvalidOperationException( + $"CppGeneratorOptions.ClangTimeoutMilliseconds must be a positive integer, " + + $"got {explicitValue}."); + } + + return explicitValue; + } + + // Option 2: APIMARK_CLANG_TIMEOUT_MS environment variable + var envValue = Environment.GetEnvironmentVariable(ClangTimeoutEnvVar); + if (!string.IsNullOrEmpty(envValue)) + { + if (!int.TryParse(envValue, out var parsed) || parsed <= 0) + { + throw new InvalidOperationException( + $"The {ClangTimeoutEnvVar} environment variable must be a positive integer " + + $"number of milliseconds, got '{envValue}'."); + } + + return parsed; + } + + // Option 3: built-in default + return DefaultClangTimeoutMilliseconds; + } + // ========================================================================= // Process execution // ========================================================================= @@ -703,7 +766,7 @@ private static List BuildArguments( /// and returns the combined output with the exit code. /// /// - /// The wait is bounded by so that a clang (or + /// The wait is bounded by so that a clang (or /// xcrun) invocation that never exits — e.g. a first-run Xcode license prompt /// blocked on stdin, or a process starved by heavy CI-runner contention — fails this /// single call instead of hanging the calling test (and, transitively, the whole CI @@ -714,14 +777,20 @@ private static List BuildArguments( /// /// Executable to launch. /// Arguments to pass to the executable. + /// + /// Maximum time to wait for the process to exit, resolved by the caller via + /// (and the + /// test seam, which always wins). + /// /// A tuple of (stdout, stderr, exitCode). /// /// Thrown when the process cannot be started, or when it does not exit within - /// . + /// . /// private static (string Stdout, string Stderr, int ExitCode) RunProcess( string fileName, - IReadOnlyList arguments) + IReadOnlyList arguments, + int timeoutMilliseconds) { var psi = new ProcessStartInfo { @@ -753,7 +822,6 @@ private static (string Stdout, string Stderr, int ExitCode) RunProcess( var stdoutTask = process.StandardOutput.ReadToEndAsync(); var stderrTask = process.StandardError.ReadToEndAsync(); - var timeoutMilliseconds = TimeoutOverrideMillisecondsForTests ?? ClangTimeoutMilliseconds; if (!process.WaitForExit(timeoutMilliseconds)) { TryKillProcess(process); @@ -775,14 +843,22 @@ private static (string Stdout, string Stderr, int ExitCode) RunProcess( $" Partial stderr ({partialStderr.Length} chars): {Truncate(partialStderr)}"); } - return (stdoutTask.Result, stderrTask.Result, process.ExitCode); + // The process itself exited within the timeout, but a grandchild it spawned (e.g. a + // wrapper that backgrounds a helper) could still be holding a redirected pipe open, + // which would otherwise make these reads hang indefinitely despite the process-level + // timeout above. Bound them too so the timeout guarantee covers the whole call. + var stdout = GetOutputOrThrow(stdoutTask, "stdout", processId, commandLine, stopwatch.Elapsed); + var stderr = GetOutputOrThrow(stderrTask, "stderr", processId, commandLine, stopwatch.Elapsed); + return (stdout, stderr, process.ExitCode); } /// - /// Maximum time to wait for the clang (or xcrun-wrapped clang) AST-dump - /// invocation to exit before treating it as hung and killing it. + /// Built-in fallback timeout (milliseconds) used when neither + /// nor the + /// environment variable supplies a value. + /// Applied to the clang (or xcrun-wrapped clang) AST-dump invocation. /// - private const int ClangTimeoutMilliseconds = 120_000; + private const int DefaultClangTimeoutMilliseconds = 120_000; /// /// Test-only override for the timeout applied in , scoped to @@ -822,6 +898,42 @@ private static string BuildDiagnosticCommandLine(string fileName, IReadOnlyList< return string.Join(' ', parts); } + /// + /// Returns a redirected stdout/stderr read task's result, bounded by + /// so that a process which has already + /// exited — but left a grandchild holding the redirected pipe open — cannot still hang + /// this call indefinitely and defeat the overall timeout guarantee. + /// + /// The task to await. + /// Stream name ("stdout" or "stderr") for diagnostics. + /// PID of the process that produced , for diagnostics. + /// Command line used to start the process, for diagnostics. + /// Time elapsed since the process was started, for diagnostics. + /// The captured text. + /// + /// Thrown when the read does not complete within . + /// + private static string GetOutputOrThrow( + Task readTask, + string streamName, + int processId, + string commandLine, + TimeSpan elapsed) + { + if (readTask.Wait(PartialOutputGraceMilliseconds)) + { + return readTask.Result; + } + + throw new InvalidOperationException( + $"Process exited but reading {streamName} did not complete within " + + $"{PartialOutputGraceMilliseconds}ms — a child process it spawned may still be " + + "holding the redirected handle open.\n" + + $" PID: {processId}\n" + + $" Elapsed: {elapsed}\n" + + $" Command: {commandLine}"); + } + /// /// Best-effort retrieval of a stdout/stderr read task's result after the owning /// process has been killed, bounded by so diff --git a/src/ApiMark.Cpp/CppGeneratorOptions.cs b/src/ApiMark.Cpp/CppGeneratorOptions.cs index c5e97b4..b92d8d3 100644 --- a/src/ApiMark.Cpp/CppGeneratorOptions.cs +++ b/src/ApiMark.Cpp/CppGeneratorOptions.cs @@ -118,6 +118,14 @@ public sealed class CppGeneratorOptions /// public string? ClangPath { get; set; } + /// + /// Gets or sets the maximum time, in milliseconds, to wait for the clang subprocess + /// to exit before it is treated as hung and killed. When , the + /// APIMARK_CLANG_TIMEOUT_MS environment variable is consulted, falling back to + /// a built-in default (120000ms / 2 minutes) when that is also unset. + /// + public int? ClangTimeoutMilliseconds { get; set; } + /// /// Gets or sets the directory used as the root for resolving relative /// entries. Defaults to , @@ -167,6 +175,7 @@ public sealed class CppGeneratorOptions Visibility = Visibility, IncludeDeprecated = IncludeDeprecated, ClangPath = ClangPath, + ClangTimeoutMilliseconds = ClangTimeoutMilliseconds, WorkingDirectory = WorkingDirectory, EnforceDocsVisibility = EnforceDocsVisibility, }; diff --git a/src/ApiMark.MSBuild/ApiMarkTask.cs b/src/ApiMark.MSBuild/ApiMarkTask.cs index b681bf7..e306659 100644 --- a/src/ApiMark.MSBuild/ApiMarkTask.cs +++ b/src/ApiMark.MSBuild/ApiMarkTask.cs @@ -212,6 +212,14 @@ public class ApiMarkTask : Task /// public string? ApiMarkClangPath { get; set; } + /// Gets or sets the clang subprocess timeout, in milliseconds, for C++ documentation generation. + /// + /// Used for the cpp language only. Maps to $(ApiMarkClangTimeoutMs). + /// Optional — when empty, the tool falls back to the APIMARK_CLANG_TIMEOUT_MS + /// environment variable, then its built-in 120000ms (2 minute) default. + /// + public string? ApiMarkClangTimeoutMs { get; set; } + /// Gets or sets the semicolon-separated, order-preserved list of glob and exclusion pattern strings for C++ header selection. /// /// Used for the cpp language only. Maps to $(ApiMarkApiHeaders). @@ -359,7 +367,7 @@ private void AppendDotNetArguments(List args) /// cpp language subcommand and all optional C++ flags /// (--includes, --api-headers, --library-name, /// --library-description, --defines, --cpp-standard, - /// --clang-path). + /// --clang-path, --clang-timeout-ms). /// /// The argument list being built by . private void AppendCppArguments(List args) @@ -391,6 +399,9 @@ private void AppendCppArguments(List args) // Optional: explicit clang path AppendOptionalArg(args, "--clang-path", ApiMarkClangPath); + // Optional: clang subprocess timeout override + AppendOptionalArg(args, "--clang-timeout-ms", ApiMarkClangTimeoutMs); + // Forward documentation-coverage enforcement options — now supported for cpp as well // as dotnet, mirroring AppendDotNetArguments's block. AppendOptionalArg(args, "--enforce-docs", ApiMarkEnforceDocs); diff --git a/src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets b/src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets index d4cdbf7..0bf288c 100644 --- a/src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets +++ b/src/ApiMark.MSBuild/build/DemaConsulting.ApiMark.MSBuild.targets @@ -102,6 +102,7 @@ ApiMarkDefines="$(ApiMarkDefines)" ApiMarkCppStandard="$(ApiMarkCppStandard)" ApiMarkClangPath="$(ApiMarkClangPath)" + ApiMarkClangTimeoutMs="$(ApiMarkClangTimeoutMs)" ApiMarkEnforceDocs="$(ApiMarkEnforceDocs)" ApiMarkEnforceDocsSeverity="$(ApiMarkEnforceDocsSeverity)" ApiMarkOutputs="@(ApiMarkOutput)" diff --git a/src/ApiMark.Tool/Cli/Context.cs b/src/ApiMark.Tool/Cli/Context.cs index 2da53c4..8abc230 100644 --- a/src/ApiMark.Tool/Cli/Context.cs +++ b/src/ApiMark.Tool/Cli/Context.cs @@ -164,6 +164,14 @@ internal sealed class Context : IContext, IDisposable /// public string? ClangPath { get; private init; } + /// + /// Gets the timeout in milliseconds for the clang subprocess, overriding the built-in + /// 120000ms (2 minute) default. Optional — when , the + /// APIMARK_CLANG_TIMEOUT_MS environment variable is consulted, falling back to + /// the built-in default when that is also unset. + /// + public int? ClangTimeoutMilliseconds { get; private init; } + /// /// Gets the ordered list of glob and exclusion pattern strings for the VHDL language subcommand. /// Collected from repeated --source invocations; entries with a ! @@ -225,6 +233,7 @@ public static Context Create(string[] args) Defines = parser.Defines, CppStandard = parser.CppStandard, ClangPath = parser.ClangPath, + ClangTimeoutMilliseconds = parser.ClangTimeoutMilliseconds, Sources = [.. parser.Sources], }; @@ -463,6 +472,13 @@ private sealed class ArgumentParser /// public string? ClangPath { get; private set; } + /// + /// Gets the timeout in milliseconds for the clang subprocess. + /// Optional — when , APIMARK_CLANG_TIMEOUT_MS or the + /// built-in 120000ms default is used. + /// + public int? ClangTimeoutMilliseconds { get; private set; } + /// /// Gets the VHDL source glob patterns from repeated --source flags. /// @@ -746,6 +762,11 @@ private int ParseArgument(string arg, string[] args, int index) ClangPath = GetRequiredStringArgument(arg, args, index, "a clang executable path argument"); return index + 1; + case "--clang-timeout-ms": + ClangTimeoutMilliseconds = GetRequiredIntArgument( + arg, args, index, "a clang timeout in milliseconds argument", 1000, int.MaxValue); + return index + 1; + case "--source": { var pattern = GetRequiredStringArgument(arg, args, index, "a glob pattern argument"); diff --git a/src/ApiMark.Tool/Program.cs b/src/ApiMark.Tool/Program.cs index 216093c..856a7d7 100644 --- a/src/ApiMark.Tool/Program.cs +++ b/src/ApiMark.Tool/Program.cs @@ -387,6 +387,7 @@ private static IApiGenerator CreateGenerator(Context context) Visibility = (CppApiVisibility)(int)visibility, IncludeDeprecated = context.IncludeObsolete, ClangPath = context.ClangPath, + ClangTimeoutMilliseconds = context.ClangTimeoutMilliseconds, EnforceDocsVisibility = cppEnforceDocsVisibility, }), @@ -529,6 +530,7 @@ private static void PrintHelp(Context context) context.WriteLine(" --defines Comma-separated preprocessor definitions (e.g. MYLIB_API=,NDEBUG)"); context.WriteLine(" --cpp-standard C++ language standard passed to Clang (default: c++17)"); context.WriteLine(" --clang-path Path to clang executable (default: auto-discovered via PATH / xcrun / vswhere)"); + context.WriteLine(" --clang-timeout-ms Timeout in milliseconds for the clang subprocess (default: 120000)"); context.WriteLine(" --visibility Visibility filter: Public, PublicAndProtected, All (default: Public)"); context.WriteLine(" --include-obsolete Include deprecated members in generated output"); context.WriteLine(""); diff --git a/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs b/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs index f3385cf..2263c71 100644 --- a/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs +++ b/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs @@ -2,7 +2,9 @@ // Licensed under the MIT License. using System.Diagnostics; +using System.Globalization; using System.Runtime.InteropServices; +using System.Text.RegularExpressions; using ApiMark.Cpp; using ApiMark.Cpp.CppAst; using Xunit; @@ -517,10 +519,43 @@ public void ClangAstParser_Parse_CalledDirectlyWithDifferentlyCasedIncludeRoot_R } /// - /// Writes a tiny script that ignores every argument it is given and sleeps far longer - /// than any test timeout, so it can stand in for a hung clang invocation without - /// depending on clang (or any OS-specific hang trigger such as an Xcode license prompt) - /// being reproducible in this environment. + /// Checks whether a process with the given ID is still alive, used to confirm that + /// ClangAstParser.RunProcess actually killed a hung process rather than merely + /// abandoning it. + /// + /// Process ID to check. + /// when a process with this ID exists and has not exited. + private static bool IsProcessRunning(int pid) + { + try + { + using var process = Process.GetProcessById(pid); + return !process.HasExited; + } + catch (ArgumentException) + { + // No process with this ID exists — it has fully exited and been reaped + return false; + } + } + + /// + /// Marker text the hanging script writes to stdout before it hangs, so the timeout test + /// can assert that genuinely-produced partial output is captured and truncated correctly + /// rather than merely asserting the presence of the "Partial stdout" label. + /// + private const string HangStdoutMarker = "HANG_MARKER_STDOUT"; + + /// + /// Marker text the hanging script writes to stderr before it hangs; see . + /// + private const string HangStderrMarker = "HANG_MARKER_STDERR"; + + /// + /// Writes a tiny script that ignores every argument it is given, emits a known marker to + /// stdout and stderr, and then sleeps far longer than any test timeout, so it can stand + /// in for a hung clang invocation without depending on clang (or any OS-specific hang + /// trigger such as an Xcode license prompt) being reproducible in this environment. /// /// Directory to create the script file in. /// Absolute path to the created, executable script. @@ -528,15 +563,28 @@ private static string CreateHangingScript(string directory) { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - // .NET's Process.Start transparently runs .cmd/.bat files through cmd.exe on - // Windows, so this can be used directly as CppGeneratorOptions.ClangPath. + // .NET's Process.Start special-cases .cmd/.bat files on Windows even with + // UseShellExecute = false: it transparently wraps them with "cmd.exe /c" before + // calling CreateProcess, so this can be used directly as CppGeneratorOptions.ClangPath + // without the caller needing to invoke cmd.exe itself. Verified empirically (process + // starts successfully) and by this test passing in CI. var scriptPath = Path.Combine(directory, "hang.cmd"); - File.WriteAllText(scriptPath, "@echo off\r\nping -n 1000 127.0.0.1 >nul\r\n"); + File.WriteAllText( + scriptPath, + "@echo off\r\n" + + $"echo {HangStdoutMarker}\r\n" + + $"echo {HangStderrMarker} 1>&2\r\n" + + "ping -n 1000 127.0.0.1 >nul\r\n"); return scriptPath; } var shPath = Path.Combine(directory, "hang.sh"); - File.WriteAllText(shPath, "#!/bin/sh\nsleep 1000\n"); + File.WriteAllText( + shPath, + "#!/bin/sh\n" + + $"echo {HangStdoutMarker}\n" + + $"echo {HangStderrMarker} >&2\n" + + "sleep 1000\n"); File.SetUnixFileMode( shPath, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | @@ -583,8 +631,32 @@ public void ClangAstParser_Parse_ClangHangs_ThrowsWithDiagnosticsAndKillsProcess Assert.Contains("Elapsed:", ex.Message, StringComparison.Ordinal); Assert.Contains("Command:", ex.Message, StringComparison.Ordinal); Assert.Contains(hangingScript, ex.Message, StringComparison.Ordinal); - Assert.Contains("Partial stdout", ex.Message, StringComparison.Ordinal); - Assert.Contains("Partial stderr", ex.Message, StringComparison.Ordinal); + + // Assert: the partial output genuinely produced by the hung process before it + // was killed was captured and reported, not merely that the labels are present. + // The exact character count is platform-dependent (CRLF vs LF line endings), so + // match the marker's presence within its labelled section rather than an exact count. + Assert.Matches( + new Regex(@"Partial stdout \(\d+ chars\): " + Regex.Escape(HangStdoutMarker)), + ex.Message); + Assert.Matches( + new Regex(@"Partial stderr \(\d+ chars\): " + Regex.Escape(HangStderrMarker)), + ex.Message); + + // Assert: the hung process (and not just its parent, if any) was actually killed + // rather than merely abandoned — a regression that removes or breaks + // TryKillProcess would otherwise still pass the message assertions above while + // leaving the 1000-second child running + var pid = int.Parse( + Regex.Match(ex.Message, @"PID: (\d+)").Groups[1].Value, + CultureInfo.InvariantCulture); + var deadline = DateTime.UtcNow.AddSeconds(5); + while (IsProcessRunning(pid) && DateTime.UtcNow < deadline) + { + Thread.Sleep(50); + } + + Assert.False(IsProcessRunning(pid), $"Process {pid} should have been killed after the timeout."); } finally { @@ -598,4 +670,107 @@ public void ClangAstParser_Parse_ClangHangs_ThrowsWithDiagnosticsAndKillsProcess Directory.Delete(tempDir, recursive: true); } } + + /// + /// Validates that — set + /// directly on the options object, NOT via the + /// test seam — is honored by the production code path, proving the configurable + /// timeout actually reaches RunProcess. + /// + [Fact] + public void ClangAstParser_Parse_ClangTimeoutMillisecondsOption_HonoredWithoutTestSeam() + { + // Arrange: a fake "clang" that hangs, with a short timeout supplied via the + // production CppGeneratorOptions.ClangTimeoutMilliseconds option (not the test seam) + var tempDir = Path.Combine(Path.GetTempPath(), "ApiMarkClangTimeoutOptionTest_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + try + { + var hangingScript = CreateHangingScript(tempDir); + var options = new CppGeneratorOptions + { + LibraryName = "Fixtures", + PublicIncludeRoots = [], + ClangPath = hangingScript, + ClangTimeoutMilliseconds = 300, + }; + + // Act / Assert: the option-supplied timeout (300ms), not the 120000ms default, + // must be what RunProcess reports and enforces + var ex = Assert.Throws( + () => ClangAstParser.Parse(["dummy.h"], options)); + Assert.Contains("did not exit within 300ms", ex.Message, StringComparison.Ordinal); + } + finally + { + Directory.Delete(tempDir, recursive: true); + } + } + + /// + /// Validates that the APIMARK_CLANG_TIMEOUT_MS environment variable is honored + /// when is not set. + /// + [Fact] + public void ClangAstParser_Parse_ClangTimeoutEnvVar_OverridesDefaultTimeout() + { + // Arrange: a fake "clang" that hangs, with no options-level timeout set + var tempDir = Path.Combine(Path.GetTempPath(), "ApiMarkClangTimeoutEnvTest_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + try + { + var hangingScript = CreateHangingScript(tempDir); + var options = new CppGeneratorOptions + { + LibraryName = "Fixtures", + PublicIncludeRoots = [], + ClangPath = hangingScript, + }; + + Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, "300"); + try + { + // Act / Assert + var ex = Assert.Throws( + () => ClangAstParser.Parse(["dummy.h"], options)); + Assert.Contains("did not exit within 300ms", ex.Message, StringComparison.Ordinal); + } + finally + { + // Reset — leaking this into unrelated tests would corrupt their timeout behavior + Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, null); + } + } + finally + { + Directory.Delete(tempDir, recursive: true); + } + } + + /// + /// Validates that an invalid (non-numeric or non-positive) APIMARK_CLANG_TIMEOUT_MS + /// value produces a clear instead of silently + /// falling back to the default or throwing an unhelpful . + /// + [Fact] + public void ClangAstParser_Parse_ClangTimeoutEnvVar_InvalidValue_ThrowsInvalidOperationException() + { + // Arrange + var options = BuildOptions(); + var headers = Directory.GetFiles(FixturePaths.GetFixtureNamespaceDir(), "*.h").ToList(); + + Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, "not-a-number"); + try + { + // Act / Assert + var ex = Assert.Throws( + () => ClangAstParser.Parse(headers, options)); + Assert.Contains(ClangAstParser.ClangTimeoutEnvVar, ex.Message, StringComparison.Ordinal); + } + finally + { + Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, null); + } + } } + diff --git a/test/ApiMark.MSBuild.Tests/ApiMarkTaskTests.cs b/test/ApiMark.MSBuild.Tests/ApiMarkTaskTests.cs index 9c0d1ef..ed76663 100644 --- a/test/ApiMark.MSBuild.Tests/ApiMarkTaskTests.cs +++ b/test/ApiMark.MSBuild.Tests/ApiMarkTaskTests.cs @@ -772,6 +772,31 @@ public void ApiMarkTask_Cpp_ClangPath_ForwardedToTool() Assert.Contains("/usr/local/bin/clang", args); } + /// + /// Validates that appends the + /// --clang-timeout-ms flag with the configured value when + /// is set. + /// + [Fact] + public void ApiMarkTask_Cpp_ClangTimeoutMs_ForwardedToTool() + { + // Arrange: configure an explicit clang timeout alongside the required cpp include paths + var task = new ApiMarkTask + { + ProjectExtension = ".vcxproj", + ToolDllPath = "dummy.dll", + ApiMarkIncludePaths = "/include", + ApiMarkClangTimeoutMs = "300000", + }; + + // Act + var args = task.BuildArguments("cpp"); + + // Assert: the --clang-timeout-ms flag and the configured value must appear + Assert.Contains("--clang-timeout-ms", args); + Assert.Contains("300000", args); + } + /// /// Validates that returns true with no errors /// when is empty for a .csproj project, diff --git a/test/ApiMark.Tool.Tests/Cli/ContextTests.cs b/test/ApiMark.Tool.Tests/Cli/ContextTests.cs index 3591b30..3143bb5 100644 --- a/test/ApiMark.Tool.Tests/Cli/ContextTests.cs +++ b/test/ApiMark.Tool.Tests/Cli/ContextTests.cs @@ -899,6 +899,55 @@ public void Context_Create_WithClangPathOption_SetsClangPath() Assert.Equal("/usr/bin/clang", context.ClangPath); } + /// + /// Validates that --clang-timeout-ms sets the + /// property to the supplied value. + /// + [Fact] + public void Context_Create_WithClangTimeoutMsOption_SetsClangTimeoutMilliseconds() + { + // Arrange + var args = new[] { "--clang-timeout-ms", "300000" }; + + // Act + using var context = Context.Create(args); + + // Assert + Assert.Equal(300000, context.ClangTimeoutMilliseconds); + } + + /// + /// Validates that is + /// when --clang-timeout-ms is not supplied, so the tool falls back to its own + /// environment-variable/default resolution. + /// + [Fact] + public void Context_Create_WithoutClangTimeoutMsOption_LeavesClangTimeoutMillisecondsNull() + { + // Arrange + var args = Array.Empty(); + + // Act + using var context = Context.Create(args); + + // Assert + Assert.Null(context.ClangTimeoutMilliseconds); + } + + /// + /// Validates that a --clang-timeout-ms value below the minimum (1000ms) throws + /// , rejecting almost-certainly-mistaken sub-second values. + /// + [Fact] + public void Context_Create_WithClangTimeoutMsOptionBelowMinimum_ThrowsArgumentException() + { + // Arrange + var args = new[] { "--clang-timeout-ms", "500" }; + + // Act / Assert + Assert.Throws(() => Context.Create(args)); + } + /// /// Validates that a single --source flag sets the Sources array. /// From 2c6342f10d576e9074c8c2b8fef2aabdd931fb79 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 4 Oct 2026 10:22:03 -0400 Subject: [PATCH 5/6] Address review round 3: serialize env-mutating tests, restore env var, drop real-clang dependency - Add ClangEnvironmentCollection to serialize ClangAstParserTests and CppGeneratorTests (both mutate/depend on APIMARK_CLANG_TIMEOUT_MS and invoke real clang), preventing cross-test races on the process-wide environment. - Restore the original APIMARK_CLANG_TIMEOUT_MS value in test finally blocks instead of unconditionally clearing it, so a pre-set value on a CI runner is never lost. - Rewrite the invalid-timeout-value test to use the hanging-script ClangPath seam instead of BuildOptions()/real fixture headers, since ResolveClangTimeoutMilliseconds runs before the clang process is ever started; the test no longer depends on clang being installed and no longer needs an IsClangAvailable() skip guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/ApiMark.Cpp.Tests/ClangAstParserTests.cs | 55 +++++++++++++++---- .../ClangEnvironmentCollection.cs | 23 ++++++++ test/ApiMark.Cpp.Tests/CppGeneratorTests.cs | 7 +++ 3 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 test/ApiMark.Cpp.Tests/ClangEnvironmentCollection.cs diff --git a/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs b/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs index 2263c71..0c9c5fd 100644 --- a/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs +++ b/test/ApiMark.Cpp.Tests/ClangAstParserTests.cs @@ -12,6 +12,13 @@ namespace ApiMark.Cpp.Tests; /// Integration-flavoured unit tests for that require clang to be available. +/// +/// Shares the "ClangEnvironment" xUnit collection (see ) +/// with so that tests here which mutate the process-wide +/// APIMARK_CLANG_TIMEOUT_MS environment variable cannot race against that class's +/// real clang invocations. +/// +[Collection("ClangEnvironment")] public class ClangAstParserTests { /// Checks whether clang is available on the current system by attempting to run "clang --version". @@ -727,6 +734,9 @@ public void ClangAstParser_Parse_ClangTimeoutEnvVar_OverridesDefaultTimeout() ClangPath = hangingScript, }; + // Capture and restore the prior value — a runner that already sets this variable + // (e.g. to tune CI timeouts) must not lose that configuration for later tests + var originalValue = Environment.GetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar); Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, "300"); try { @@ -737,8 +747,7 @@ public void ClangAstParser_Parse_ClangTimeoutEnvVar_OverridesDefaultTimeout() } finally { - // Reset — leaking this into unrelated tests would corrupt their timeout behavior - Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, null); + Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, originalValue); } } finally @@ -752,24 +761,48 @@ public void ClangAstParser_Parse_ClangTimeoutEnvVar_OverridesDefaultTimeout() /// value produces a clear instead of silently /// falling back to the default or throwing an unhelpful . /// + /// + /// Uses the same hanging-script technique as + /// the other timeout tests (rather than plus real fixture + /// headers) so this test does not depend on clang being installed: the invalid + /// environment variable value is validated and thrown before the (fake) clang process is + /// ever started, so a real clang installation is never required to exercise this path. + /// [Fact] public void ClangAstParser_Parse_ClangTimeoutEnvVar_InvalidValue_ThrowsInvalidOperationException() { // Arrange - var options = BuildOptions(); - var headers = Directory.GetFiles(FixturePaths.GetFixtureNamespaceDir(), "*.h").ToList(); - - Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, "not-a-number"); + var tempDir = Path.Combine(Path.GetTempPath(), "ApiMarkClangTimeoutInvalidTest_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); try { - // Act / Assert - var ex = Assert.Throws( - () => ClangAstParser.Parse(headers, options)); - Assert.Contains(ClangAstParser.ClangTimeoutEnvVar, ex.Message, StringComparison.Ordinal); + var hangingScript = CreateHangingScript(tempDir); + var options = new CppGeneratorOptions + { + LibraryName = "Fixtures", + PublicIncludeRoots = [], + ClangPath = hangingScript, + }; + + // Capture and restore the prior value — a runner that already sets this variable + // (e.g. to tune CI timeouts) must not lose that configuration for later tests + var originalValue = Environment.GetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar); + Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, "not-a-number"); + try + { + // Act / Assert + var ex = Assert.Throws( + () => ClangAstParser.Parse(["dummy.h"], options)); + Assert.Contains(ClangAstParser.ClangTimeoutEnvVar, ex.Message, StringComparison.Ordinal); + } + finally + { + Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, originalValue); + } } finally { - Environment.SetEnvironmentVariable(ClangAstParser.ClangTimeoutEnvVar, null); + Directory.Delete(tempDir, recursive: true); } } } diff --git a/test/ApiMark.Cpp.Tests/ClangEnvironmentCollection.cs b/test/ApiMark.Cpp.Tests/ClangEnvironmentCollection.cs new file mode 100644 index 0000000..d015790 --- /dev/null +++ b/test/ApiMark.Cpp.Tests/ClangEnvironmentCollection.cs @@ -0,0 +1,23 @@ +// Copyright (c) DemaConsulting LLC. All rights reserved. +// Licensed under the MIT License. + +using Xunit; + +namespace ApiMark.Cpp.Tests; + +/// +/// xUnit collection definition that serializes tests which invoke real clang via +/// (directly, or transitively through +/// ) against tests that mutate the process-wide +/// APIMARK_CLANG_TIMEOUT_MS environment variable. +/// +/// +/// Without this, a test that temporarily sets APIMARK_CLANG_TIMEOUT_MS to an invalid +/// value (or to a short test-specific value) could run concurrently — on a different +/// thread, in a different xUnit collection — with another test's real clang invocation, +/// causing that unrelated invocation to fail or time out prematurely. Pair this definition +/// with [Collection("ClangEnvironment")] on each affected test class; xUnit runs all +/// test classes that share a collection definition serially with respect to each other. +/// +[CollectionDefinition("ClangEnvironment")] +public class ClangEnvironmentCollection { } diff --git a/test/ApiMark.Cpp.Tests/CppGeneratorTests.cs b/test/ApiMark.Cpp.Tests/CppGeneratorTests.cs index 19800cc..2a18577 100644 --- a/test/ApiMark.Cpp.Tests/CppGeneratorTests.cs +++ b/test/ApiMark.Cpp.Tests/CppGeneratorTests.cs @@ -11,6 +11,13 @@ namespace ApiMark.Cpp.Tests; /// Integration tests for using a shared /// to avoid invoking clang more than five times per test run. /// +/// +/// Shares the "ClangEnvironment" xUnit collection (see ) +/// with so that this class's real clang invocations (made +/// during construction) cannot race against tests there that +/// mutate the process-wide APIMARK_CLANG_TIMEOUT_MS environment variable. +/// +[Collection("ClangEnvironment")] public class CppGeneratorTests : IClassFixture { /// The shared fixture providing pre-generated factories for common option configurations. From 6332b57904c599a144e07aa4cbaaffafe87ad613 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 4 Oct 2026 11:06:36 -0400 Subject: [PATCH 6/6] Fix CI-observed bugs: widen success-path output grace period, serialize DocumentationCoverageCheckerTests The macOS CI run for 2c6342f surfaced two real bugs not caught locally: - GetOutputOrThrow used the same 2s PartialOutputGraceMilliseconds grace period as the post-kill partial-output path, but that budget is only appropriate once a process is already known to be stuck. On the success path (process exited within its timeout), a real clang invocation legitimately draining a large AST dump under CI load can take longer than 2s to finish flushing its redirected stdout pipe, which was incorrectly reported as a hung grandchild process. Split this into a new, much larger SuccessPathOutputGraceMilliseconds (30s) used only on the success path; the short 2s grace remains for the post-kill case where fast-fail matters. - DocumentationCoverageCheckerTests invokes CppGenerator.Parse (real clang) directly but was not part of the ClangEnvironment collection, so it could run concurrently with ClangAstParserTests setting APIMARK_CLANG_TIMEOUT_MS to an intentionally invalid test value, poisoning its clang invocation. Added it to the collection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/ApiMark.Cpp/CppAst/ClangAstParser.cs | 24 +++++++++++++++---- .../DocumentationCoverageCheckerTests.cs | 7 +++++- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs index 3f62082..0105e49 100644 --- a/src/ApiMark.Cpp/CppAst/ClangAstParser.cs +++ b/src/ApiMark.Cpp/CppAst/ClangAstParser.cs @@ -873,10 +873,24 @@ private static (string Stdout, string Stderr, int ExitCode) RunProcess( /// /// Maximum time to wait, after a hung process has been killed, for its redirected - /// stdout/stderr pipes to report as complete so partial output can be captured. + /// stdout/stderr pipes to report as complete so partial output can be captured. Kept + /// short because the process is already known to be stuck — this is a best-effort + /// grab of whatever diagnostic text is immediately available, not a wait for a + /// legitimate (but large/slow) read to finish. /// private const int PartialOutputGraceMilliseconds = 2000; + /// + /// Maximum time to wait, after a process has exited normally within its timeout, for + /// its redirected stdout/stderr pipes to report as complete. Deliberately much larger + /// than : on this (success) path the process + /// has already exited cleanly, so a slow read here means a real clang invocation is + /// still draining a large AST dump (observed to take several seconds on loaded CI + /// runners), not a hang — only a grandchild process indefinitely holding the redirected + /// handle open should ever actually trip this bound. + /// + private const int SuccessPathOutputGraceMilliseconds = 30_000; + /// /// Maximum number of characters of partial stdout/stderr to include in a timeout /// exception message, so a chatty hung process does not produce an unreadable error. @@ -900,7 +914,7 @@ private static string BuildDiagnosticCommandLine(string fileName, IReadOnlyList< /// /// Returns a redirected stdout/stderr read task's result, bounded by - /// so that a process which has already + /// so that a process which has already /// exited — but left a grandchild holding the redirected pipe open — cannot still hang /// this call indefinitely and defeat the overall timeout guarantee. /// @@ -911,7 +925,7 @@ private static string BuildDiagnosticCommandLine(string fileName, IReadOnlyList< /// Time elapsed since the process was started, for diagnostics. /// The captured text. /// - /// Thrown when the read does not complete within . + /// Thrown when the read does not complete within . /// private static string GetOutputOrThrow( Task readTask, @@ -920,14 +934,14 @@ private static string GetOutputOrThrow( string commandLine, TimeSpan elapsed) { - if (readTask.Wait(PartialOutputGraceMilliseconds)) + if (readTask.Wait(SuccessPathOutputGraceMilliseconds)) { return readTask.Result; } throw new InvalidOperationException( $"Process exited but reading {streamName} did not complete within " + - $"{PartialOutputGraceMilliseconds}ms — a child process it spawned may still be " + + $"{SuccessPathOutputGraceMilliseconds}ms — a child process it spawned may still be " + "holding the redirected handle open.\n" + $" PID: {processId}\n" + $" Elapsed: {elapsed}\n" + diff --git a/test/ApiMark.Cpp.Tests/DocumentationCoverageCheckerTests.cs b/test/ApiMark.Cpp.Tests/DocumentationCoverageCheckerTests.cs index 101555a..4d54d3f 100644 --- a/test/ApiMark.Cpp.Tests/DocumentationCoverageCheckerTests.cs +++ b/test/ApiMark.Cpp.Tests/DocumentationCoverageCheckerTests.cs @@ -12,8 +12,13 @@ namespace ApiMark.Cpp.Tests; /// undocumented (mirroring ApiMark.DotNet.Tests.DocumentationCoverageCheckerTests's /// use of the equivalent .NET fixture), and ProtectedMembersClass.h/ /// DeprecatedClass.h, which are fully documented and used for tier/deprecated-filter -/// assertions. +/// assertions. Each test invokes directly (real clang), so +/// this class shares the "ClangEnvironment" collection with ClangAstParserTests +/// and CppGeneratorTests — without it, a test there mutating the process-wide +/// APIMARK_CLANG_TIMEOUT_MS environment variable could run concurrently and poison a +/// real clang invocation here. /// +[Collection("ClangEnvironment")] public class DocumentationCoverageCheckerTests { ///