From ed51acaf75eac21f5c976c11c864f9af0665be9e Mon Sep 17 00:00:00 2001 From: Rendle Date: Fri, 11 Sep 2026 15:57:48 +0100 Subject: [PATCH 1/2] fix(test): bound process-output drains in test helpers Six test helpers ran pack-core.sh or dotnet build/pack/restore with redirected output and then awaited ReadToEndAsync with no timeout. A reusable MSBuild worker node inherits the pipe and outlives the script, so EOF never arrived until the node idled out (~15 min). This hung the three InitCommandTests, and Core's ComposedCoreFeedFixture has the same shape. All six now call one shared test/Shared/ProcessRunner.cs, linked into Dmon.Core.Tests and Dmon.Terminal.Tests. It sets MSBUILDDISABLENODEREUSE=1 on every child and bounds the post-exit drain with a grace, returning truncated output rather than blocking when a descendant still holds the pipe. Each measure is sufficient alone; both were forced from zero build nodes with node reuse on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015CJu5CxqY5B6o3GfMVAfsu --- .../Composition/ComposedCoreFeedFixture.cs | 39 +---- .../Composition/CompositionRootTests.cs | 38 +---- .../Composition/ProcessRunnerTests.cs | 61 +++++++ .../Composition/VersionRangeRestoreTests.cs | 62 ++----- test/Dmon.Core.Tests/Dmon.Core.Tests.csproj | 4 + .../Packaging/ToolPackTests.cs | 42 ++--- .../Dmon.Terminal.Tests.csproj | 4 + test/Dmon.Terminal.Tests/InitCommandTests.cs | 39 +---- test/Dmon.Terminal.Tests/InitFeedFixture.cs | 39 +---- test/Shared/ProcessRunner.cs | 153 ++++++++++++++++++ 10 files changed, 268 insertions(+), 213 deletions(-) create mode 100644 test/Dmon.Core.Tests/Composition/ProcessRunnerTests.cs create mode 100644 test/Shared/ProcessRunner.cs diff --git a/test/Dmon.Core.Tests/Composition/ComposedCoreFeedFixture.cs b/test/Dmon.Core.Tests/Composition/ComposedCoreFeedFixture.cs index d06116ea..509ac865 100644 --- a/test/Dmon.Core.Tests/Composition/ComposedCoreFeedFixture.cs +++ b/test/Dmon.Core.Tests/Composition/ComposedCoreFeedFixture.cs @@ -1,5 +1,5 @@ -using System.Diagnostics; using System.Reflection; +using Dmon.Tests.Shared; namespace Dmon.Core.Tests.Composition; @@ -53,41 +53,14 @@ private static string LocateRepoRoot() private static async Task RunAsync(string fileName, string arguments, string workingDirectory) { - ProcessStartInfo psi = new() - { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }; - - using Process proc = new() { StartInfo = psi }; - proc.Start(); - - Task stdoutTask = proc.StandardOutput.ReadToEndAsync(); - Task stderrTask = proc.StandardError.ReadToEndAsync(); - - using CancellationTokenSource cts = new(TimeSpan.FromMinutes(5)); - try - { - await proc.WaitForExitAsync(cts.Token); - } - catch (OperationCanceledException) - { - try { proc.Kill(entireProcessTree: true); } catch { /* best effort */ } - throw new TimeoutException($"pack-core.sh timed out after 5 minutes."); - } - - string output = await stdoutTask; - string errors = await stderrTask; + ProcessResult result = await ProcessRunner.RunAsync( + fileName, arguments, workingDirectory, TimeSpan.FromMinutes(5)); - if (proc.ExitCode != 0) + if (result.ExitCode != 0) { throw new InvalidOperationException( - $"pack-core.sh failed (exit {proc.ExitCode}).\nstdout: {output}\nstderr: {errors}"); + $"pack-core.sh failed (exit {result.ExitCode}){result.TruncatedNote}.\n" + + $"stdout: {result.StandardOutput}\nstderr: {result.StandardError}"); } } } diff --git a/test/Dmon.Core.Tests/Composition/CompositionRootTests.cs b/test/Dmon.Core.Tests/Composition/CompositionRootTests.cs index dc7ade9d..faa80366 100644 --- a/test/Dmon.Core.Tests/Composition/CompositionRootTests.cs +++ b/test/Dmon.Core.Tests/Composition/CompositionRootTests.cs @@ -4,6 +4,7 @@ using Dmon.Abstractions.Extensions; using Dmon.Hosting; using Dmon.Protocol; +using Dmon.Tests.Shared; using Microsoft.Extensions.AI; using Microsoft.Extensions.DependencyInjection; @@ -144,41 +145,14 @@ private static async Task RunDotnetAsync( string workingDirectory, int timeoutSeconds) { - ProcessStartInfo psi = new() - { - FileName = "dotnet", - Arguments = $"{verb} \"{targetPath}\"", - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }; - - using Process proc = new() { StartInfo = psi }; - proc.Start(); - - Task stdoutTask = proc.StandardOutput.ReadToEndAsync(); - Task stderrTask = proc.StandardError.ReadToEndAsync(); - - using CancellationTokenSource cts = new(TimeSpan.FromSeconds(timeoutSeconds)); - try - { - await proc.WaitForExitAsync(cts.Token); - } - catch (OperationCanceledException) - { - try { proc.Kill(entireProcessTree: true); } catch { /* best effort */ } - throw new TimeoutException($"dotnet {verb} timed out after {timeoutSeconds}s."); - } - - string output = await stdoutTask; - string errors = await stderrTask; + ProcessResult result = await ProcessRunner.RunAsync( + "dotnet", $"{verb} \"{targetPath}\"", workingDirectory, TimeSpan.FromSeconds(timeoutSeconds)); - if (proc.ExitCode != 0) + if (result.ExitCode != 0) { throw new InvalidOperationException( - $"dotnet {verb} failed (exit {proc.ExitCode}).\nstdout: {output}\nstderr: {errors}"); + $"dotnet {verb} failed (exit {result.ExitCode}){result.TruncatedNote}.\n" + + $"stdout: {result.StandardOutput}\nstderr: {result.StandardError}"); } } } diff --git a/test/Dmon.Core.Tests/Composition/ProcessRunnerTests.cs b/test/Dmon.Core.Tests/Composition/ProcessRunnerTests.cs new file mode 100644 index 00000000..a7ff7afb --- /dev/null +++ b/test/Dmon.Core.Tests/Composition/ProcessRunnerTests.cs @@ -0,0 +1,61 @@ +using System.Diagnostics; +using Dmon.Tests.Shared; + +namespace Dmon.Core.Tests.Composition; + +/// +/// Proves the shared never blocks past its drain grace when a +/// descendant process still holds the output pipes open — the shape that hung +/// InitFeedFixture/ComposedCoreFeedFixture when a leftover MSBuild worker +/// node inherited a redirected pipe from a dotnet pack child. +/// +public sealed class ProcessRunnerTests +{ + [Fact] + public async Task RunAsync_DescendantHoldsPipeOpen_ReturnsPromptlyWithTruncatedPartialOutput() + { + Stopwatch stopwatch = Stopwatch.StartNew(); + ProcessResult result = await ProcessRunner.RunAsync( + "bash", "-c \"sleep 30 & echo $!\"", Path.GetTempPath(), + timeout: TimeSpan.FromSeconds(10), drainGrace: TimeSpan.FromSeconds(1)); + stopwatch.Stop(); + + string pidLine = result.StandardOutput.Trim(); + try + { + // The bound only discriminates against the bug (waiting for EOF, which the + // backgrounded sleep would delay by ~30s) if it is comfortably under that. + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(10), + $"RunAsync took {stopwatch.Elapsed} — expected it to return promptly, not wait for the backgrounded sleep."); + Assert.Equal(0, result.ExitCode); + Assert.True(result.OutputTruncated); + Assert.False(string.IsNullOrWhiteSpace(pidLine)); + } + finally + { + if (int.TryParse(pidLine, out int pid)) TryKill(pid); + } + } + + [Fact] + public async Task RunAsync_NormalProcess_ReturnsFullUntruncatedOutput() + { + ProcessResult result = await ProcessRunner.RunAsync( + "bash", "-c \"echo hi\"", Path.GetTempPath(), + timeout: TimeSpan.FromSeconds(10), drainGrace: TimeSpan.FromSeconds(1)); + + Assert.Equal(0, result.ExitCode); + Assert.False(result.OutputTruncated); + Assert.Equal("hi", result.StandardOutput.Trim()); + } + + private static void TryKill(int pid) + { + try + { + using Process process = Process.GetProcessById(pid); + process.Kill(); + } + catch { /* best effort — process may already be gone */ } + } +} diff --git a/test/Dmon.Core.Tests/Composition/VersionRangeRestoreTests.cs b/test/Dmon.Core.Tests/Composition/VersionRangeRestoreTests.cs index 26b5f805..f1c83b2d 100644 --- a/test/Dmon.Core.Tests/Composition/VersionRangeRestoreTests.cs +++ b/test/Dmon.Core.Tests/Composition/VersionRangeRestoreTests.cs @@ -1,5 +1,5 @@ -using System.Diagnostics; using System.Text.Json; +using Dmon.Tests.Shared; namespace Dmon.Core.Tests.Composition; @@ -86,13 +86,14 @@ public async Task ProjectPinnedTo01Star_Restores019_NotVersion020() """; File.WriteAllText(Path.Combine(workDir, "nuget.config"), nugetConfigContent); - (int exitCode, string stdout, string stderr) = await RunDotnetAsync( - "restore", $"\"{Path.Combine(workDir, "pin-test.csproj")}\"", - workDir, timeoutSeconds: 120); + ProcessResult restoreResult = await ProcessRunner.RunAsync( + "dotnet", $"restore \"{Path.Combine(workDir, "pin-test.csproj")}\"", + workDir, TimeSpan.FromSeconds(120)); Assert.True( - exitCode == 0, - $"dotnet restore failed (exit {exitCode}).\nstdout: {stdout}\nstderr: {stderr}"); + restoreResult.ExitCode == 0, + $"dotnet restore failed (exit {restoreResult.ExitCode}){restoreResult.TruncatedNote}.\n" + + $"stdout: {restoreResult.StandardOutput}\nstderr: {restoreResult.StandardError}"); string assetsPath = Path.Combine(workDir, "obj", "project.assets.json"); Assert.True( @@ -131,14 +132,15 @@ private async Task PackStubAsync(string version) """; File.WriteAllText(Path.Combine(stubDir, "dmoncore-stub.csproj"), csprojContent); - (int exitCode, string stdout, string stderr) = await RunDotnetAsync( - "pack", $"dmoncore-stub.csproj -c Release -o \"{_tempFeed}\" --nologo", - stubDir, timeoutSeconds: 60); + ProcessResult result = await ProcessRunner.RunAsync( + "dotnet", $"pack dmoncore-stub.csproj -c Release -o \"{_tempFeed}\" --nologo", + stubDir, TimeSpan.FromSeconds(60)); - if (exitCode != 0) + if (result.ExitCode != 0) { throw new InvalidOperationException( - $"Stub pack at {version} failed (exit {exitCode}).\nstdout: {stdout}\nstderr: {stderr}"); + $"Stub pack at {version} failed (exit {result.ExitCode}){result.TruncatedNote}.\n" + + $"stdout: {result.StandardOutput}\nstderr: {result.StandardError}"); } } @@ -175,44 +177,6 @@ private static IEnumerable GetLibraryKeys(JsonElement libraries) yield return lib.Name; } - private static async Task<(int ExitCode, string Stdout, string Stderr)> RunDotnetAsync( - string verb, string arguments, string workingDirectory, int timeoutSeconds) - { - ProcessStartInfo psi = new() - { - FileName = "dotnet", - Arguments = $"{verb} {arguments}", - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }; - - using Process proc = new() { StartInfo = psi }; - proc.Start(); - - Task stdoutTask = proc.StandardOutput.ReadToEndAsync(); - Task stderrTask = proc.StandardError.ReadToEndAsync(); - - using CancellationTokenSource cts = new(TimeSpan.FromSeconds(timeoutSeconds)); - try - { - await proc.WaitForExitAsync(cts.Token); - } - catch (OperationCanceledException) - { - try { proc.Kill(entireProcessTree: true); } catch { /* best effort */ } - throw new TimeoutException( - $"dotnet {verb} timed out after {timeoutSeconds} seconds."); - } - - string stdout = await stdoutTask; - string stderr = await stderrTask; - - return (proc.ExitCode, stdout, stderr); - } - private static void TryDelete(string? path) { if (path is not null) diff --git a/test/Dmon.Core.Tests/Dmon.Core.Tests.csproj b/test/Dmon.Core.Tests/Dmon.Core.Tests.csproj index b5490c42..35bde33b 100644 --- a/test/Dmon.Core.Tests/Dmon.Core.Tests.csproj +++ b/test/Dmon.Core.Tests/Dmon.Core.Tests.csproj @@ -24,6 +24,10 @@ + + + + diff --git a/test/Dmon.Core.Tests/Packaging/ToolPackTests.cs b/test/Dmon.Core.Tests/Packaging/ToolPackTests.cs index 6b5d11f4..a6def1a6 100644 --- a/test/Dmon.Core.Tests/Packaging/ToolPackTests.cs +++ b/test/Dmon.Core.Tests/Packaging/ToolPackTests.cs @@ -1,7 +1,7 @@ -using System.Diagnostics; using System.IO.Compression; using System.Reflection; using System.Text.RegularExpressions; +using Dmon.Tests.Shared; namespace Dmon.Core.Tests.Packaging; @@ -177,41 +177,17 @@ private static string LocateTerminalCsproj() private static async Task RunPackAsync(string csprojPath, string outputDir) { - ProcessStartInfo psi = new() - { - FileName = "dotnet", - Arguments = $"pack \"{csprojPath}\" -c Release -o \"{outputDir}\"", - WorkingDirectory = Path.GetDirectoryName(csprojPath)!, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }; - - using Process proc = new() { StartInfo = psi }; - proc.Start(); - - Task stdoutTask = proc.StandardOutput.ReadToEndAsync(); - Task stderrTask = proc.StandardError.ReadToEndAsync(); - - using CancellationTokenSource cts = new(TimeSpan.FromMinutes(3)); - try - { - await proc.WaitForExitAsync(cts.Token); - } - catch (OperationCanceledException) - { - try { proc.Kill(entireProcessTree: true); } catch { /* best effort */ } - throw new TimeoutException("dotnet pack timed out after 3 minutes."); - } - - string output = await stdoutTask; - string errors = await stderrTask; + ProcessResult result = await ProcessRunner.RunAsync( + "dotnet", + $"pack \"{csprojPath}\" -c Release -o \"{outputDir}\"", + Path.GetDirectoryName(csprojPath)!, + TimeSpan.FromMinutes(3)); - if (proc.ExitCode != 0) + if (result.ExitCode != 0) { throw new InvalidOperationException( - $"dotnet pack failed (exit {proc.ExitCode}).\nstdout: {output}\nstderr: {errors}"); + $"dotnet pack failed (exit {result.ExitCode}){result.TruncatedNote}.\n" + + $"stdout: {result.StandardOutput}\nstderr: {result.StandardError}"); } } diff --git a/test/Dmon.Terminal.Tests/Dmon.Terminal.Tests.csproj b/test/Dmon.Terminal.Tests/Dmon.Terminal.Tests.csproj index 31125c5b..fa7df37a 100644 --- a/test/Dmon.Terminal.Tests/Dmon.Terminal.Tests.csproj +++ b/test/Dmon.Terminal.Tests/Dmon.Terminal.Tests.csproj @@ -18,6 +18,10 @@ + + + + diff --git a/test/Dmon.Terminal.Tests/InitCommandTests.cs b/test/Dmon.Terminal.Tests/InitCommandTests.cs index 227ff4d2..ee87cc2f 100644 --- a/test/Dmon.Terminal.Tests/InitCommandTests.cs +++ b/test/Dmon.Terminal.Tests/InitCommandTests.cs @@ -3,6 +3,7 @@ using System.Text.Json; using Dmon.Protocol; using Dmon.Terminal; +using Dmon.Tests.Shared; namespace Dmon.Terminal.Tests; @@ -115,42 +116,14 @@ private static async Task RunDotnetAsync( string workingDirectory, int timeoutSeconds) { - ProcessStartInfo psi = new() - { - FileName = "dotnet", - Arguments = $"{verb} \"{targetPath}\"", - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }; - - using Process proc = new() { StartInfo = psi }; - proc.Start(); - - // Read stdout and stderr concurrently to avoid the pipe-buffer deadlock. - Task stdoutTask = proc.StandardOutput.ReadToEndAsync(); - Task stderrTask = proc.StandardError.ReadToEndAsync(); - - using CancellationTokenSource cts = new(TimeSpan.FromSeconds(timeoutSeconds)); - try - { - await proc.WaitForExitAsync(cts.Token); - } - catch (OperationCanceledException) - { - try { proc.Kill(entireProcessTree: true); } catch { /* best effort */ } - throw new TimeoutException($"dotnet {verb} timed out after {timeoutSeconds}s."); - } - - string output = await stdoutTask; - string errors = await stderrTask; + ProcessResult result = await ProcessRunner.RunAsync( + "dotnet", $"{verb} \"{targetPath}\"", workingDirectory, TimeSpan.FromSeconds(timeoutSeconds)); - if (proc.ExitCode != 0) + if (result.ExitCode != 0) { throw new InvalidOperationException( - $"dotnet {verb} failed (exit {proc.ExitCode}).\nstdout: {output}\nstderr: {errors}"); + $"dotnet {verb} failed (exit {result.ExitCode}){result.TruncatedNote}.\n" + + $"stdout: {result.StandardOutput}\nstderr: {result.StandardError}"); } } diff --git a/test/Dmon.Terminal.Tests/InitFeedFixture.cs b/test/Dmon.Terminal.Tests/InitFeedFixture.cs index 7d2980fb..2295d7b4 100644 --- a/test/Dmon.Terminal.Tests/InitFeedFixture.cs +++ b/test/Dmon.Terminal.Tests/InitFeedFixture.cs @@ -1,5 +1,5 @@ -using System.Diagnostics; using System.Reflection; +using Dmon.Tests.Shared; namespace Dmon.Terminal.Tests; @@ -53,41 +53,14 @@ private static string LocateRepoRoot() private static async Task RunAsync(string fileName, string arguments, string workingDirectory) { - ProcessStartInfo psi = new() - { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }; - - using Process proc = new() { StartInfo = psi }; - proc.Start(); - - Task stdoutTask = proc.StandardOutput.ReadToEndAsync(); - Task stderrTask = proc.StandardError.ReadToEndAsync(); - - using CancellationTokenSource cts = new(TimeSpan.FromMinutes(5)); - try - { - await proc.WaitForExitAsync(cts.Token); - } - catch (OperationCanceledException) - { - try { proc.Kill(entireProcessTree: true); } catch { /* best effort */ } - throw new TimeoutException($"pack-core.sh timed out after 5 minutes."); - } - - string output = await stdoutTask; - string errors = await stderrTask; + ProcessResult result = await ProcessRunner.RunAsync( + fileName, arguments, workingDirectory, TimeSpan.FromMinutes(5)); - if (proc.ExitCode != 0) + if (result.ExitCode != 0) { throw new InvalidOperationException( - $"pack-core.sh failed (exit {proc.ExitCode}).\nstdout: {output}\nstderr: {errors}"); + $"pack-core.sh failed (exit {result.ExitCode}){result.TruncatedNote}.\n" + + $"stdout: {result.StandardOutput}\nstderr: {result.StandardError}"); } } } diff --git a/test/Shared/ProcessRunner.cs b/test/Shared/ProcessRunner.cs new file mode 100644 index 00000000..6fe473b2 --- /dev/null +++ b/test/Shared/ProcessRunner.cs @@ -0,0 +1,153 @@ +using System.Diagnostics; +using System.Text; + +namespace Dmon.Tests.Shared; + +/// +/// Result of a run. is true when +/// a descendant process was still holding the output pipes after the drain grace elapsed — +/// and then hold only what arrived +/// before the grace ran out, not the full output. +/// +internal sealed record ProcessResult(int ExitCode, string StandardOutput, string StandardError, bool OutputTruncated) +{ + /// + /// " (output truncated — a descendant still held the pipe)" when + /// is true, otherwise empty — for appending to a failure message without a ternary at + /// every call site. + /// + public string TruncatedNote => OutputTruncated ? " (output truncated — a descendant still held the pipe)" : ""; +} + +/// +/// Runs a child process with a bounded wait for exit and a bounded grace for the output +/// pumps to drain, so a descendant that outlives the process it was spawned by can never +/// block the caller indefinitely. dotnet build/pack/restore spawn +/// reusable MSBuild worker nodes (/nodeReuse:true, the default) that inherit the +/// redirected pipes; when the parent exits but a node stays alive, plain EOF-based reads +/// (ReadToEndAsync awaited with no timeout) never complete. +/// +internal static class ProcessRunner +{ + private static readonly TimeSpan DefaultDrainGrace = TimeSpan.FromSeconds(10); + + public static Task RunAsync( + string fileName, + string arguments, + string workingDirectory, + TimeSpan timeout, + CancellationToken cancellationToken = default) + => RunAsync(fileName, arguments, workingDirectory, timeout, DefaultDrainGrace, cancellationToken); + + public static async Task RunAsync( + string fileName, + string arguments, + string workingDirectory, + TimeSpan timeout, + TimeSpan drainGrace, + CancellationToken cancellationToken = default) + { + ProcessStartInfo psi = new() + { + FileName = fileName, + Arguments = arguments, + WorkingDirectory = workingDirectory, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true, + }; + // Propagates through `bash pack-core.sh` to every `dotnet pack` it runs — stops a + // worker node from surviving the script and holding the pipe open behind it. + psi.Environment["MSBUILDDISABLENODEREUSE"] = "1"; + + using Process process = new() { StartInfo = psi }; + process.Start(); + + // Pump stdout/stderr into a thread-safe snapshot instead of awaiting ReadToEndAsync, + // so a snapshot is always available even if EOF never arrives. Do NOT switch to + // BeginOutputReadLine/OutputDataReceived: since .NET 5, WaitForExitAsync (and the + // parameterless WaitForExit()) also waits for EOF on async-mode redirected streams, + // which reproduces the exact hang this helper exists to avoid. + OutputPump stdoutPump = new(process.StandardOutput); + OutputPump stderrPump = new(process.StandardError); + Task stdoutPumpTask = stdoutPump.RunAsync(); + Task stderrPumpTask = stderrPump.RunAsync(); + + using CancellationTokenSource timeoutCts = + CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(timeout); + + try + { + await process.WaitForExitAsync(timeoutCts.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + TryKill(process); + if (cancellationToken.IsCancellationRequested) throw; + + throw new TimeoutException( + $"{fileName} {arguments} timed out after {timeout}.\n" + + $"stdout so far: {stdoutPump.Snapshot()}\nstderr so far: {stderrPump.Snapshot()}"); + } + + using CancellationTokenSource graceCts = new(drainGrace); + bool outputTruncated = false; + try + { + await Task.WhenAll(stdoutPumpTask, stderrPumpTask).WaitAsync(graceCts.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + // A descendant still holds a pipe open (the same node-reuse shape as the exit + // wait above). The pump tasks keep running in the background and catch every + // exception internally, so returning here leaves nothing unobserved. + outputTruncated = true; + } + + return new ProcessResult(process.ExitCode, stdoutPump.Snapshot(), stderrPump.Snapshot(), outputTruncated); + } + + private static void TryKill(Process process) + { + try { process.Kill(entireProcessTree: true); } catch { /* best effort */ } + } + + private sealed class OutputPump(StreamReader reader) + { + private readonly StringBuilder _buffer = new(); + private readonly object _gate = new(); + + public async Task RunAsync() + { + try + { + char[] chunk = new char[4096]; + while (true) + { + int read = await reader.ReadAsync(chunk).ConfigureAwait(false); + if (read == 0) break; + + lock (_gate) + { + _buffer.Append(chunk, 0, read); + } + } + } + catch + { + // May run long after RunAsync returns and the caller disposes the Process + // (and this reader with it); must never surface as an unobserved fault. + } + } + + public string Snapshot() + { + lock (_gate) + { + return _buffer.ToString(); + } + } + } +} From 64cd0ab9b73d9c5a408f7b4813f27d97f6a1adb4 Mon Sep 17 00:00:00 2001 From: Rendle Date: Fri, 11 Sep 2026 15:57:48 +0100 Subject: [PATCH 2/2] docs(tech-debt): resolve terminal-tests-hang Record the fix, the forcing runs in both directions, and a correction: OutputDataReceived-mode reads do not escape the hang, because WaitForExitAsync waits for EOF on them. Note that production's BuildFileBasedProgramAsync has the same shape but did not reproduce, and that the suspected Core-hang mechanism is now closed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015CJu5CxqY5B6o3GfMVAfsu --- tech-debt/README.md | 2 +- .../dmon-core-tests-intermittent-failure.md | 7 +++ tech-debt/terminal-tests-hang.md | 56 ++++++++++++++++++- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/tech-debt/README.md b/tech-debt/README.md index 39e573c2..cc431511 100644 --- a/tech-debt/README.md +++ b/tech-debt/README.md @@ -32,7 +32,7 @@ Debt that outlives a change needs somewhere that outlives a change. ### Tests - [`Dmon.Core.Tests` has a recurring intermittent failure](dmon-core-tests-intermittent-failure.md) — ~1 red run in 8; the failing test was not captured, so catch it with its name first. - [`WizardEngineTests` intermittent failure](wizard-engine-intermittent-failure.md) — the one *identified* sighting. -- [Three `Dmon.Terminal.Tests` tests hang, intermittently](terminal-tests-hang.md) — the `InitCommandTests`: `InitFeedFixture` waits for stdout EOF with no timeout, and a reusable MSBuild node started by `pack-core.sh` holds the pipe open until it idles out (~15 min). Verified 2026-09-11. Core's `ComposedCoreFeedFixture` is a twin (unforced lead for the Core hang). Workaround: `MSBUILDDISABLENODEREUSE=1`. +- ~~[Three `Dmon.Terminal.Tests` tests hang, intermittently](terminal-tests-hang.md)~~ — **resolved** on `fix/terminal-tests-hang` (2026-09-11). All six copies of the wait-for-EOF shape, Terminal and Core, now use one shared `test/Shared/ProcessRunner.cs`: node reuse is off, and the post-exit drain is bounded. Forced both ways. The workaround variable is no longer needed. Production's `BuildFileBasedProgramAsync` has the same shape, but it did not reproduce; see the note. - [`BootstrapService` is untested and duplicates the resolver's root walk](bootstrap-service-untested-and-duplicates-root-walk.md) — the spec's bootstrap scenario has no test, and its own copy of the marker rule can drift from the resolver's. - [No test harness exercises a real gateway over a real spawned core](no-full-stack-gateway-test-harness.md) — `Dmon.Network.Tests` fakes the core with a scripted-stdout replayer, so every gateway task must test one level down and say so. - ~~[The live e2e test writes into the user's home session store](live-e2e-test-writes-into-home-session-store.md)~~ — **resolved** by `session-root-resolution` (#116, `90eed64`). There were three writers (the live test, `CoreProcessFixture`, and `CoreProcessManagerRestartTests`); all now pin `sessionStore: local` in a temp-root `config.yaml`. The accumulated test sessions are not yet pruned. diff --git a/tech-debt/dmon-core-tests-intermittent-failure.md b/tech-debt/dmon-core-tests-intermittent-failure.md index 90297a27..959172dd 100644 --- a/tech-debt/dmon-core-tests-intermittent-failure.md +++ b/tech-debt/dmon-core-tests-intermittent-failure.md @@ -82,6 +82,13 @@ that fixture, shared by `CompositionRootTests`, `FileBasedProgramLaunchTests` an shape. That is a **lead** for the hang only. It says nothing about this note's one-off `Failed: 1`, which is a different symptom. See [the Terminal hang](terminal-tests-hang.md). +**Update (2026-09-11):** the Terminal fix converted all of those Core sites too, so the +suspected mechanism is now closed in this assembly. Two full runs with node reuse **on** +then passed (629/630, 1 skip, 57-59 s). The Core hang itself was **never forced**, so +treat it as "probably the same cause, now fixed" rather than "verified fixed". A Core hang +seen after this merge is a new finding. The `Failed: 1` above is untouched: still open, +still unnamed. + ## Provenance Observed and measured by the Architect during section 6 of `dmon-home-foundations` diff --git a/tech-debt/terminal-tests-hang.md b/tech-debt/terminal-tests-hang.md index 23167462..030d4557 100644 --- a/tech-debt/terminal-tests-hang.md +++ b/tech-debt/terminal-tests-hang.md @@ -1,6 +1,7 @@ # Three `Dmon.Terminal.Tests` tests hang -**Status:** open. Tests named and cause verified (2026-09-11); not yet fixed. +**Status:** resolved on branch `fix/terminal-tests-hang` (2026-09-11); the merge commit is +recorded here once merged. **Where:** `test/Dmon.Terminal.Tests/InitFeedFixture.cs:54-92` (`RunAsync`), shared by the three `InitCommandTests` **Surfaced:** 2026-09-10, while measuring which tests write into `~/.dmon/sessions` @@ -8,6 +9,54 @@ three `InitCommandTests` **Severity:** medium. It blocks `make test` from completing reliably, and it hides any failure in the tests that hang. +## Resolution (2026-09-11) + +All six copies of the shape now call one helper, `test/Shared/ProcessRunner.cs`, which is +compiled into both `Dmon.Core.Tests` and `Dmon.Terminal.Tests` as a linked file. It applies +both measures from the fix list below, and each one works on its own: + +1. It sets `MSBUILDDISABLENODEREUSE=1` on every process it starts, so no worker node + outlives `pack-core.sh`. +2. It never waits unboundedly for EOF. Output is pumped into a snapshot; after the process + exits, the drain gets a 10 s grace. If a descendant still holds the pipe, the helper + returns what it has with `OutputTruncated = true`: a zero exit still passes, and a + non-zero one fails with a note that the output was cut short. + `ProcessRunnerTests` proves this in isolation with `bash -c 'sleep 30 & echo $!'`. + +**Forced both ways (verified 2026-09-11)**, from zero MSBuild nodes each time, node reuse on +in the outer environment, `--filter InitCommandTests`: + +| Tree | Result | +|---|---| +| `main` @ `ac1fa98` (before) | hung; aborted at the 2 min hang timeout; a `nodeReuse:true` MSBuild node was alive afterwards | +| the fix | **3/3 in 7 s**; no MSBuild node afterwards | +| the fix with measure 1 disabled (measure 2 alone) | **3/3**, ~12 s slower (one drain grace used up); a `nodeReuse:true` node **was** alive afterwards, so the hang condition was present and was survived | + +Measure 1 alone is the `MSBUILDDISABLENODEREUSE=1` row of the forcing table below. Two +full `dotnet test Everything.slnx` runs **without** the workaround variable then passed +every assembly, about 70 s each (Terminal 194/194 in 24-25 s, Core 629/630 with 1 skip). + +**A correction to the fix list below.** Option 2's "read with `OutputDataReceived` and stop +reading once the process has exited" does **not** work. Since .NET 5, +`Process.WaitForExitAsync` (and parameterless `WaitForExit()`) also waits for EOF on streams +read in that async mode, so it reproduces the hang. The helper pumps with `ReadAsync` +instead, and says so in a comment. + +**Left alone, deliberately:** +- The long-running `dotnet run`/`dotnet exec` readers (`RunAndReadAgentReadyAsync` in + `InitCommandTests` and `CompositionRootTests`, `FileBasedProgramLaunchTests`, + `CoreProcessFixture`, `LegacyExtensionsListIgnoredIntegrationTest`). They read lines under a + token and kill the tree, which is a different shape, and none of them builds. +- **Production has the same shape, not reproduced.** `CoreProcessManager.BuildFileBasedProgramAsync` + (`core/Dmon.Runtime/CoreProcessManager.cs:186-192`) runs `dotnet build ` and then + awaits `ReadToEndAsync` bounded only by the caller's token, and the Terminal host's token + has no deadline (`frontends/Dmon.Terminal/Program.cs:30-37`). One forcing attempt + (`FileBasedProgramLaunchTests`, zero nodes, reuse on) passed 2/2 in 13 s with **no** MSBuild + node left behind. The inference, not verified: a single-project file-based build does not + start an out-of-process worker node, while `pack-core.sh`'s multi-project packs do. If a + `Dmon.cs` ever gains project references, or MSBuild changes that behaviour, this becomes + the same hang at host startup. + ## Cause (verified 2026-09-11) The three tests are the `InitCommandTests`: @@ -43,7 +92,7 @@ standalone `--filter InitCommandTests` control, from zero nodes. | `--filter InitCommandTests`, reuse on (the control) | hung, aborted at 2 min; an `MSBuild.dll` node started **during the fixture** was still alive afterwards | | two full suites with `MSBUILDDISABLENODEREUSE=1` | Terminal **194/194 in 21-22 s** both times | -## Fix (not applied) +## Fix (as recorded before the resolution; see the correction above) It needs a test-code change, which was out of scope for `session-root-resolution`. Either measure works alone; doing both is more robust: @@ -67,7 +116,8 @@ Apply the same fix to every copy of the shape: - `Packaging/ToolPackTests.cs` and `Composition/VersionRangeRestoreTests.cs`, which also run `dotnet` with redirected output and `ReadToEndAsync`; check each one. -**Workaround until then:** run the suite with `MSBUILDDISABLENODEREUSE=1` exported. +**Workaround until then:** run the suite with `MSBUILDDISABLENODEREUSE=1` exported. No +longer needed once the resolution above is merged. ## History