Fix orphaned requirement and bound clang subprocess timeout by Malcolmnixon · Pull Request #49 · demaconsulting/ApiMark

Malcolmnixon · 2026-10-04T12:04:33Z

Summary

Fixes a reqstream traceability error: ApiMarkDotNet-DotNetEmitter-RenderConstantFieldValue was missing from the system-level requirement's children list, so
eqstream --enforce reported it as orphaned.
Fixes a CI reliability issue: ClangAstParser.RunProcess had no timeout on the clang/xcrun subprocess wait, so a hung invocation (e.g. Render compile-time constant values in C# and C++ emitters #48's macOS job, cancelled after 47 minutes) could block the entire CI job indefinitely.

Details

Requirement traceability

Added \ApiMarkDotNet-DotNetEmitter-RenderConstantFieldValue\ to the \children\ list of \ApiMarkDotNet-GenerateDocumentationFromAssembliesAndXml\ in \docs/reqstream/api-mark-dot-net.yaml. Verified with \dotnet reqstream --requirements requirements.yaml --enforce\ — no orphan/error output.

Clang subprocess timeout

\RunProcess\ now bounds \WaitForExit\ to 120 seconds. On timeout it kills the process tree (reusing the existing \TryKillProcess\ helper used by the xcrun discovery probe) and throws \InvalidOperationException\ carrying:

OS description/architecture
Process ID
Elapsed time
Full command line
Partial stdout/stderr already produced (bounded to 2000 chars, captured with its own grace period so a dead pipe can't re-introduce a hang)

Testing

\dotnet reqstream --requirements requirements.yaml --enforce\ — passes, no orphans.
\dotnet build src\ApiMark.Cpp\ApiMark.Cpp.csproj -c Release\ — 0 warnings/errors (TreatWarningsAsErrors is on).
\dotnet test test\ApiMark.Cpp.Tests\ApiMark.Cpp.Tests.csproj -c Release -f net8.0\ with a real LLVM 22.1.8 install on PATH: 177 passed, 0 failed, 14 skipped (unchanged from baseline without this change).
\ix.ps1\ auto-fix pass run — no additional changes needed.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

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>

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>

Copilot

Copilot review overview

🟡 Changes recommended

An unresolved compile error and missing automated coverage for timeout behavior remain.

Review effort: Lite
Findings: 2

Open (2)

Add timeout branch coverage and cleanup assertions · New
Invalid string.Contains overload for net8.0 · New

What changed in this PR

This PR fixes missing ReqStream traceability and prevents indefinitely hung Clang subprocesses in CI.

Changes:

Links the orphaned .NET requirement to its system requirement.
Adds a 120-second timeout, process-tree termination, and diagnostics for Clang invocations.

File	Summary	Findings
`src/ApiMark.Cpp/CppAst/ClangAstParser.cs`	Bounds subprocess execution and reports timeout diagnostics.	Critical (1 vote): incompatible `string.Contains` overload for `net8.0`. Critical (4 votes): timeout behavior lacks automated coverage. Nit (1 vote): incomplete comment wording.
`docs/reqstream/api-mark-dot-net.yaml`	Restores the missing requirement hierarchy link.	None.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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>

Copilot

Copilot review overview

🟡 Changes recommended

Critical timeout and Windows test issues remain unresolved, along with cleanup and coverage gaps.

Review effort: Lite
Findings: 2 · 1

Open (3)

Bound redirected output reads after process timeout · New
Launch Windows batch helper through cmd.exe · New
Verify process termination and partial output cleanup · New

Resolved since last review (2)

Invalid string.Contains overload for net8.0
Add timeout branch coverage and cleanup assertions

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>

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved findings affect descendant cleanup and environment-variable test isolation, restoration, and portability.

Review effort: Lite
Findings: 2 · 2

Open (4)

Skip invalid-timeout test when clang is unavailable · New
Launch Windows batch helper through cmd.exe
Restore APIMARK_CLANG_TIMEOUT_MS instead of clearing it · New
Serialize tests that mutate the process-wide environment · New

Resolved since last review (2)

Bound redirected output reads after process timeout
Verify process termination and partial output cleanup

…, 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>

Copilot

Copilot review overview

🟡 Changes recommended

Address unbounded stream buffering, descendant cleanup after pipe failures, and missing serialization for other real-clang tests.

Review effort: Lite
Findings: 1

Open (1)

Real-clang tests remain parallel outside the new collection · New

Resolved since last review (4)

Skip invalid-timeout test when clang is unavailable
Launch Windows batch helper through cmd.exe
Serialize tests that mutate the process-wide environment
Restore APIMARK_CLANG_TIMEOUT_MS instead of clearing it

Previously missed (2)

In code that hasn't changed since last review

ReadToEndAsync buffers unbounded stdout and stderr

src/ApiMark.Cpp/CppAst/ClangAstParser.cs:843

Truncate limits only the exception text, not the ReadToEndAsync buffers that have already accumulated the complete stream. A hung/chatty clang process can therefore keep allocating stdout/stderr data for the entire timeout (especially when the timeout is raised), potentially exhausting CI memory before this line runs. Drain each pipe continuously while retaining only the first 2000 characters (and a truncation flag) for diagnostics.

Grandchild processes survive when parent exits during pipe draining

src/ApiMark.Cpp/CppAst/ClangAstParser.cs:934

When the parent exits but a grandchild keeps a redirected pipe open, this helper throws after the grace period without terminating that grandchild; RunProcess only calls TryKillProcess in the parent-timeout branch, and the parent is already exited here. A wrapper around clang can therefore leave an indefinitely running child behind on this failure path. Track and terminate inherited pipe holders before throwing, or otherwise make the wrapper/process-tree cleanup cover this case.

…ze 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>

Copilot

Copilot review overview

🟡 Changes recommended

Process-tree termination, bounded output capture, and related test coverage remain unresolved.

Review effort: Lite
Findings: 1 · 2

Open (3)

Bound output buffering during timed-out process reads · New
Terminate leaked grandchild processes after output timeout · New
Real-clang tests remain parallel outside the new collection

Malcolm Nixon and others added 2 commits October 4, 2026 08:03

Copilot AI lite review requested due to automatic review settings October 4, 2026 12:04

Copilot started reviewing on behalf of Malcolmnixon October 4, 2026 12:05 View session