diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 0c84c45261..0e5b364234 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -6,10 +6,9 @@ using System.Diagnostics; using System.Globalization; using System.IO; +using System.Runtime.CompilerServices; using System.Threading; -#if !NET using System.Threading.Tasks; -#endif using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces; @@ -23,6 +22,26 @@ public partial class ProcessHelper : IProcessHelper private static readonly string Arm = "arm"; private readonly Process _currentProcess = Process.GetCurrentProcess(); + // Bounded time (ms) we wait for a crashed process's redirected stderr to reach EOF before reading it, + // so a late-delivered crash callstack (e.g. "Stack overflow.") is not dropped. See the Exited handler. + private const int CrashErrorDrainTimeout = 5000; + + // Bounded time (ms) we wait for stderr to drain after an exit we are not interested in diagnosing, kept + // short so neither the common case nor the rare grandchild-keeps-the-pipe-open case adds latency. + private const int NonCrashErrorDrainTimeout = 500; + + // Bounded time (ms) we still give stderr once we decide to tear the process down. Not zero, because the + // output is often already sitting in the pipe and costs nothing to pick up, but short enough that an abort + // stays responsive even when a grandchild process (e.g. a browser driver) keeps the pipe open forever. + private const int TearDownErrorDrainTimeout = 100; + + // Per-process signal that we are deliberately tearing the process down (aborting or cleaning up a run), + // rather than observing it die on its own. Cancelling it cuts the stderr drain short - including a drain + // that is already in flight, which is what keeps aborting a run from an IDE responsive. ConditionalWeakTable + // holds only weak references to the processes, so entries disappear when a process is collected and nothing + // has to be removed explicitly. + private readonly ConditionalWeakTable _tearDownSignals = new(); + #if !NET private readonly IEnvironment _environment; #endif @@ -127,23 +146,28 @@ void InitializeAndStart() process.OutputDataReceived += (sender, args) => outputCallBack(sender as Process, args.Data); } - // Set once the redirected stderr stream reaches EOF (signaled by a null Data event, + // Completed once the redirected stderr stream reaches EOF (signaled by a null Data event, // which is raised after all stderr lines have been handed to errorCallback). This is // the only reliable signal that the asynchronously-collected error output is complete: - // neither WaitForExit(timeout) nor WaitForExitAsync guarantees the ErrorDataReceived - // callbacks have run. The exit handler below waits (bounded) on this before reading. - ManualResetEventSlim? errorStreamClosed = null; + // neither WaitForExit(timeout) nor WaitForExitAsync(token) is guaranteed to observe EOF, because + // the latter stops waiting for it as soon as its token is cancelled. The exit handler below awaits + // (bounded) on this before reading. + TaskCompletionSource? errorStreamClosed = null; if (errorCallback != null) { - errorStreamClosed = new ManualResetEventSlim(initialState: false); + // RunContinuationsAsynchronously so completing this from the ErrorDataReceived callback does not + // inline the exit handler's continuation onto the stderr-reader thread. + errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); process.ErrorDataReceived += (sender, args) => { + errorCallback(sender as Process, args.Data); + + // Signal EOF only after the last callback has been delivered, so anyone who observes this + // is guaranteed to see the complete error output. if (args.Data is null) { - errorStreamClosed.Set(); + errorStreamClosed.TrySetResult(true); } - - errorCallback(sender as Process, args.Data); }; } @@ -151,8 +175,8 @@ void InitializeAndStart() { process.Exited += async (sender, args) => { - const int timeout = 500; - var stopwatch = Stopwatch.StartNew(); + // Bounded time we give the process to fully exit after we are notified of its exit. + const int processExitTimeout = 500; if (sender is Process p) { @@ -165,14 +189,17 @@ void InitializeAndStart() // issues, discussions and documentations. // // On .NET 5 and later we use WaitForExitAsync to give the child process (and any - // grandchild) some time to exit. NOTE: WaitForExitAsync only waits for the process - // to exit; it does NOT guarantee that the asynchronous Output/ErrorDataReceived - // callbacks have finished delivering. The bounded stderr drain after this block - // ensures the captured error output is complete before exitCallBack reads it. + // grandchild) some time to exit. NOTE: WaitForExitAsync does wait for the redirected + // Output/Error streams to reach EOF, but only for as long as its token allows - once + // the token is cancelled it stops waiting for them. The bounded stderr drain after + // this block is what gives a crashed process a longer, separate budget to deliver its + // callstack. // // For older frameworks, the solution is more tricky but it seems we can get the expected // behavior using the parameterless 'WaitForExit()' combined with an awaited Task.Run call. - var cts = new CancellationTokenSource(timeout); + // 'using' so the timer the timeout allocates is released as soon as we are done waiting, + // instead of leaking one per process exit when many test hosts are spawned. + using var cts = new CancellationTokenSource(processExitTimeout); #if NET await p.WaitForExitAsync(cts.Token); #else @@ -191,7 +218,7 @@ void InitializeAndStart() // the testhost to become a zombie process in the first place. if (_environment.OperatingSystem is PlatformOperatingSystem.Windows) { - p.WaitForExit(timeout); + p.WaitForExit(processExitTimeout); } else { @@ -201,6 +228,12 @@ void InitializeAndStart() { if (!p.HasExited) { + // We are force-killing a process that overran the exit budget (e.g. a + // grandchild keeps it hanging). Signal the teardown - exactly like + // TerminateProcess does - BEFORE killing, so the stderr drain below + // uses the short teardown budget instead of treating our own kill as + // a crash and waiting the generous budget unnecessarily. + SignalTearDown(p); p.Kill(); } } @@ -222,12 +255,28 @@ void InitializeAndStart() // the exit) or InvalidOperationException. } - // The process has exited. Within the SAME bounded budget used above, wait for the - // redirected stderr to reach EOF so that asynchronously-collected error output - // (e.g. a testhost crash callstack such as "Stack overflow.") is complete before the - // exit callback consumes it. WaitForExit(timeout)/WaitForExitAsync do not guarantee - // the ErrorDataReceived callbacks have run. - WaitForErrorStreamToDrain(errorStreamClosed, timeout, stopwatch.ElapsedMilliseconds); + // The process has exited. Asynchronously wait (bounded) for the redirected stderr to reach + // EOF so that asynchronously-collected error output (e.g. a testhost crash callstack such as + // "Stack overflow.") is complete before the exit callback consumes it. + // + // We await rather than block here on purpose: the crash callstack can be delivered to + // ErrorDataReceived noticeably late under load (e.g. thread-pool starvation while many test + // hosts run in parallel on CI), and blocking a thread-pool thread for the whole drain budget + // would compete with the very ErrorDataReceived callback we are waiting for and could starve + // it out. Dropping that output both produces a misleading error message and makes + // RunTestsShouldThrowOnStackOverflowException flaky. + // + // This drain budget is intentionally separate from (and far more generous than) the + // process-exit budget above, and the generous part is only spent on a crash - an abnormal + // exit of a process we were not already tearing down. A clean exit gets a short grace + // period, and a process we are tearing down (aborting or cleaning up a run) gets less + // still, because there the priority is to get out of the way rather than to diagnose. + // The teardown signal is a cancellation, so asking to tear down also cuts short a drain + // that is already in flight. In every case the wait returns as soon as EOF is observed, so + // a process that exits and drains promptly pays almost nothing. + var tearDown = GetTearDownToken(p); + var errorDrainTimeout = GetErrorDrainTimeout(exitedCleanly: ExitedCleanly(p), tearingDown: tearDown.IsCancellationRequested); + await WaitForErrorStreamToDrainAsync(errorStreamClosed, errorDrainTimeout, tearDown, TearDownErrorDrainTimeout).ConfigureAwait(false); } // If exit callback has code that access Process object, ensure that the exceptions handling should be done properly. @@ -252,27 +301,124 @@ void InitializeAndStart() } /// - /// Waits, bounded by the time remaining in , for the redirected - /// standard error stream to reach EOF (signaled via ). This ensures - /// all callbacks have completed - and therefore the captured - /// error output is complete - before it is consumed by the exit callback. It returns immediately when - /// there is no redirected error stream, when it has already drained, or when the budget is already - /// exhausted (e.g. a grandchild process keeps the pipe open), so the caller can never hang. + /// Asynchronously waits, bounded by , for the redirected standard + /// error stream to reach EOF (signaled by completing ). This ensures all + /// callbacks have completed - and therefore the captured error + /// output is complete - before it is consumed by the exit callback. It returns immediately when there is + /// no redirected error stream, when the timeout is not positive, or when the stream has already drained + /// (the common case), and is otherwise bounded by the timeout (e.g. a grandchild process keeps the pipe + /// open), so the caller can never hang. It deliberately does not block the calling thread while waiting, + /// so it does not consume a thread-pool thread that the pending + /// callback may itself need in order to deliver EOF under thread-pool starvation. + /// + /// When is signaled we are no longer diagnosing the process but getting out of + /// its way (aborting or cleaning up a run), so the remaining wait collapses to + /// . That applies to a wait that is already in flight too, + /// which is what keeps an abort responsive when a long crash budget is already being spent. + /// /// - internal static void WaitForErrorStreamToDrain(ManualResetEventSlim? errorStreamClosed, int budgetMilliseconds, long elapsedMilliseconds) + internal static async Task WaitForErrorStreamToDrainAsync( + TaskCompletionSource? errorStreamClosed, + int timeoutMilliseconds, + CancellationToken tearDown = default, + int tearDownTimeoutMilliseconds = 0) { - if (errorStreamClosed is null) + if (errorStreamClosed is null || timeoutMilliseconds <= 0 || errorStreamClosed.Task.IsCompleted) { return; } - var remainingMilliseconds = budgetMilliseconds - (int)elapsedMilliseconds; - if (remainingMilliseconds > 0) + using var timeoutCancellation = new CancellationTokenSource(); + + // Registered rather than awaited alongside the others, so a teardown that arrives mid-wait shortens the + // budget instead of being noticed only after the original one has been spent. Disposed before + // timeoutCancellation (reverse declaration order), so the callback cannot run against a disposed source. + using var tearDownRegistration = tearDown.CanBeCanceled + ? tearDown.Register(() => + { + try + { + timeoutCancellation.CancelAfter(tearDownTimeoutMilliseconds); + } + catch (ObjectDisposedException) + { + // The wait already finished and disposed its timeout; there is nothing left to shorten. + } + }) + : default; + + // Cancelling the delay leaves it in the canceled - not faulted - state, so it never needs to be observed. + var delayTask = Task.Delay(timeoutMilliseconds, timeoutCancellation.Token); + var completedTask = await Task.WhenAny(errorStreamClosed.Task, delayTask).ConfigureAwait(false); + + // Stop the timer as soon as the stream drains so we don't leave it pending for the whole timeout. + if (completedTask != delayTask) + { + timeoutCancellation.Cancel(); + } + } + + /// + /// Returns whether a process exited with code 0. A process whose exit code cannot be retrieved (e.g. it was + /// disposed while the exit was being handled) is reported as not clean, so a possible crash keeps the + /// generous stderr budget rather than being cut short. Deciding that we are tearing the process down is a + /// separate, explicit signal, so this does not have to guess at intent. + /// + private static bool ExitedCleanly(Process process) + { + try + { + return process.HasExited && process.ExitCode == 0; + } + catch + { + return false; + } + } + + /// + /// Picks the bounded time we are willing to wait for the redirected stderr to reach EOF. + /// + /// A process we are tearing down (aborting or cleaning up a run) gets the shortest budget: we are no + /// longer diagnosing it, we are getting out of its way, and a grandchild process (e.g. a browser driver) + /// that keeps the pipe open must not be able to stall the abort. + /// A clean exit gets a short grace period, because there is normally nothing left to collect. + /// A crash - an abnormal exit of a process we were not tearing down - gets the generous budget, so a + /// late-delivered crash callstack such as "Stack overflow." is captured rather than truncated. + /// + /// + internal static int GetErrorDrainTimeout(bool exitedCleanly, bool tearingDown) + => tearingDown ? TearDownErrorDrainTimeout + : exitedCleanly ? NonCrashErrorDrainTimeout + : CrashErrorDrainTimeout; + + /// + /// Signals that we are deliberately tearing down, so its stderr drain is cut + /// short. Safe to call more than once, and safe to call for a process this helper did not launch. + /// + private void SignalTearDown(Process process) + { + try + { + GetTearDownSource(process).Cancel(); + } + catch { - errorStreamClosed.Wait(remainingMilliseconds); + // Cancel surfaces whatever the registered callbacks threw. Failing to shorten a drain is not worth + // failing the teardown the caller actually asked for. (EqtTrace is not available in this assembly.) } } + /// + /// Returns the teardown token for , already signaled when we asked to tear the + /// process down before it exited. + /// + private CancellationToken GetTearDownToken(Process process) + => GetTearDownSource(process).Token; + + private CancellationTokenSource GetTearDownSource(Process process) + => _tearDownSignals.GetValue(process, static _ => new CancellationTokenSource()); + /// public string? GetCurrentProcessFileName() { @@ -342,9 +488,20 @@ public void SetExitCallback(int processId, Action? callbackAction) /// public void TerminateProcess(object? process) { + if (process is not Process proc) + { + return; + } + + // We are tearing this process down on purpose (abort/cleanup), so we are no longer interested in + // diagnosing it. Signal that BEFORE the kill, so the exit handler - which can run at any moment from + // here on - cannot miss it, and signal it even when the process has already exited, so a stderr drain + // that is already in flight for an earlier crash is cut short instead of holding up the abort. + SignalTearDown(proc); + try { - if (process is Process proc && !proc.HasExited) + if (!proc.HasExited) { proc.Kill(); } diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index d9a00cff82..fa69e81312 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Threading; +using System.Threading.Tasks; using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions; @@ -11,7 +12,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommandLine.UnitTests; /// -/// Tests for , the bounded wait that lets the process +/// Tests for , the bounded wait that lets the process /// exit callback observe the complete standard error output of a crashed test host. Without it, the exit /// callback could read the asynchronously-collected stderr before all ErrorDataReceived callbacks had run, /// dropping a crash callstack such as "Stack overflow." (the cause of the flaky @@ -22,47 +23,73 @@ public class ProcessHelperTests { private const int BudgetMs = 500; + // The drain's token means "we are tearing this process down", which is unrelated to test cancellation, so + // tests that do not exercise teardown pass a token that is never signaled rather than + // TestContext.CancellationToken. + private static readonly CancellationTokenSource NoTearDownSource = new(); + private static CancellationToken NoTearDown => NoTearDownSource.Token; + [TestMethod] - public void WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses() + public async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses() { - using var errorStreamClosed = new ManualResetEventSlim(initialState: false); + // EOF arrives AFTER we start waiting, mimicking a slow ErrorDataReceived delivery that lands just after + // the exit handler begins draining. Start the wait first and assert it is still in progress, then signal + // EOF: the wait must observe that late completion and return promptly - not return early (dropping the + // crash callstack) and not block to the timeout. + var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var drainTask = ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000, NoTearDown); + Assert.IsFalse(drainTask.IsCompleted, "The wait must still be in progress before the error stream drains."); + + // The late ErrorDataReceived EOF finally arrives; the wait must observe it and return promptly. The + // budget above is far larger than this should take, so if the wait ignored EOF and always ran to the + // timeout the elapsed-time assertion below would catch it (the await would take ~5s, not a few ms). + var stopwatch = Stopwatch.StartNew(); + errorStreamClosed.TrySetResult(true); - // The stream reaches EOF a little later, mimicking a slow ErrorDataReceived delivery. - var setter = new Thread(() => - { - Thread.Sleep(150); - errorStreamClosed.Set(); - }) - { IsBackground = true }; + await drainTask; + stopwatch.Stop(); + + Assert.IsTrue(errorStreamClosed.Task.IsCompleted, "The method must wait until the error stream is drained."); + Assert.IsLessThan( + 2000L, + stopwatch.ElapsedMilliseconds, + $"The wait must return as soon as the late EOF is observed, not run to the timeout (took {stopwatch.ElapsedMilliseconds} ms)."); + } + + [TestMethod] + public async Task WaitForErrorStreamToDrainShouldReturnImmediatelyWhenAlreadyDrained() + { + // The stream has already reached EOF (all ErrorDataReceived callbacks have been delivered) before we + // start waiting, so the common fast path must not add any latency. + var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + errorStreamClosed.TrySetResult(true); var stopwatch = Stopwatch.StartNew(); - setter.Start(); - ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed, budgetMilliseconds: 5000, elapsedMilliseconds: 0); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000, NoTearDown); stopwatch.Stop(); - setter.Join(); - Assert.IsTrue(errorStreamClosed.IsSet, "The method must wait until the error stream is drained."); Assert.IsLessThan( - 3000L, + 250L, stopwatch.ElapsedMilliseconds, - $"The method should return shortly after the stream closes, not at the budget timeout (took {stopwatch.ElapsedMilliseconds} ms)."); + $"When the stream is already drained the method must return immediately (took {stopwatch.ElapsedMilliseconds} ms)."); } [TestMethod] - public void WaitForErrorStreamToDrainShouldBeBoundedWhenTheErrorStreamNeverCloses() + public async Task WaitForErrorStreamToDrainShouldBeBoundedWhenTheErrorStreamNeverCloses() { // Models a grandchild process keeping the pipe open: EOF never arrives. - using var errorStreamClosed = new ManualResetEventSlim(initialState: false); + var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var stopwatch = Stopwatch.StartNew(); - ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed, BudgetMs, elapsedMilliseconds: 0); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, BudgetMs, NoTearDown); stopwatch.Stop(); - Assert.IsFalse(errorStreamClosed.IsSet, "Precondition: the stream never closes in this test."); + Assert.IsFalse(errorStreamClosed.Task.IsCompleted, "Precondition: the stream never closes in this test."); Assert.IsGreaterThanOrEqualTo( 150L, stopwatch.ElapsedMilliseconds, - $"The method should wait roughly the budget for the stream (waited only {stopwatch.ElapsedMilliseconds} ms)."); + $"The method should wait roughly the timeout for the stream (waited only {stopwatch.ElapsedMilliseconds} ms)."); Assert.IsLessThan( 5000L, stopwatch.ElapsedMilliseconds, @@ -70,28 +97,28 @@ public void WaitForErrorStreamToDrainShouldBeBoundedWhenTheErrorStreamNeverClose } [TestMethod] - public void WaitForErrorStreamToDrainShouldNotWaitWhenTheBudgetIsAlreadyExhausted() + public async Task WaitForErrorStreamToDrainShouldNotWaitWhenTimeoutIsNotPositive() { - // The exit wait above already consumed the whole budget (e.g. a slow grandchild), so there is no - // time left to wait for stderr - we must not add any latency on top. - using var errorStreamClosed = new ManualResetEventSlim(initialState: false); + // A non-positive timeout means there is no time budget left to wait for stderr - we must not add + // any latency on top. + var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var stopwatch = Stopwatch.StartNew(); - ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed, BudgetMs, elapsedMilliseconds: BudgetMs + 100); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 0, NoTearDown); stopwatch.Stop(); - Assert.IsFalse(errorStreamClosed.IsSet); + Assert.IsFalse(errorStreamClosed.Task.IsCompleted); Assert.IsLessThan( 250L, stopwatch.ElapsedMilliseconds, - $"With the budget exhausted the method must return immediately (took {stopwatch.ElapsedMilliseconds} ms)."); + $"With a non-positive timeout the method must return immediately (took {stopwatch.ElapsedMilliseconds} ms)."); } [TestMethod] - public void WaitForErrorStreamToDrainShouldReturnImmediatelyWhenThereIsNoErrorStream() + public async Task WaitForErrorStreamToDrainShouldReturnImmediatelyWhenThereIsNoErrorStream() { var stopwatch = Stopwatch.StartNew(); - ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed: null, BudgetMs, elapsedMilliseconds: 0); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed: null, BudgetMs, NoTearDown); stopwatch.Stop(); Assert.IsLessThan( @@ -99,4 +126,104 @@ public void WaitForErrorStreamToDrainShouldReturnImmediatelyWhenThereIsNoErrorSt stopwatch.ElapsedMilliseconds, $"With no redirected error stream the method must be a no-op (took {stopwatch.ElapsedMilliseconds} ms)."); } + + [TestMethod] + public void GetErrorDrainTimeoutShouldUseTheGenerousBudgetOnlyForACrash() + { + // A crash is an abnormal exit of a process we were not tearing down. + var crash = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, tearingDown: false); + var cleanExit = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: true, tearingDown: false); + var tearDown = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, tearingDown: true); + var cleanTearDown = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: true, tearingDown: true); + + Assert.IsGreaterThan( + cleanExit, + crash, + "A crash must wait longer for stderr to drain than a clean exit, so a late crash callstack is captured."); + + // A process we are tearing down (e.g. aborting from an IDE) must drain fastest of all, so an abort never + // stalls for seconds when a grandchild keeps the stderr pipe open. + Assert.IsLessThan( + cleanExit, + tearDown, + "A process we are tearing down must use a shorter budget than even a clean exit."); + Assert.AreEqual( + tearDown, + cleanTearDown, + "Tearing down decides the budget on its own; the exit code cannot make it wait longer."); + } + + [TestMethod] + public async Task WaitForErrorStreamToDrainShouldBeCutShortWhenTearDownIsSignaledUpFront() + { + // We already asked to tear the process down before its exit was handled (the abort/cleanup case). EOF + // never arrives because a grandchild keeps the pipe open, so only the short teardown budget may be spent + // even though the caller passed the generous crash budget. + var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var tearDown = new CancellationTokenSource(); + tearDown.Cancel(); + + var stopwatch = Stopwatch.StartNew(); + await ProcessHelper.WaitForErrorStreamToDrainAsync( + errorStreamClosed, + timeoutMilliseconds: 30000, + tearDown.Token, + tearDownTimeoutMilliseconds: 100); + stopwatch.Stop(); + + Assert.IsFalse(errorStreamClosed.Task.IsCompleted, "Precondition: the stream never closes in this test."); + Assert.IsLessThan( + 5000L, + stopwatch.ElapsedMilliseconds, + $"An already-signaled teardown must collapse the budget, not spend it (took {stopwatch.ElapsedMilliseconds} ms)."); + } + + [TestMethod] + public async Task WaitForErrorStreamToDrainShouldBeCutShortWhenTearDownArrivesWhileWaiting() + { + // The process crashed, so we started spending the generous budget, and only then did the user abort. + // The wait must react to that instead of running the crash budget to completion - this is what makes + // aborting a run from an IDE responsive. + var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var tearDown = new CancellationTokenSource(); + + var stopwatch = Stopwatch.StartNew(); + var drainTask = ProcessHelper.WaitForErrorStreamToDrainAsync( + errorStreamClosed, + timeoutMilliseconds: 30000, + tearDown.Token, + tearDownTimeoutMilliseconds: 100); + Assert.IsFalse(drainTask.IsCompleted, "The wait must be in progress before the teardown is signaled."); + + tearDown.Cancel(); + await drainTask; + stopwatch.Stop(); + + Assert.IsFalse(errorStreamClosed.Task.IsCompleted, "Precondition: the stream never closes in this test."); + Assert.IsLessThan( + 5000L, + stopwatch.ElapsedMilliseconds, + $"A teardown signaled mid-wait must cut the wait short (took {stopwatch.ElapsedMilliseconds} ms)."); + } + + [TestMethod] + public async Task WaitForErrorStreamToDrainShouldStillCaptureOutputThatArrivesDuringTearDown() + { + // Tearing down shortens the wait but does not make it give up instantly: stderr that is already sitting + // in the pipe costs nothing to pick up, and dropping it would replace a real error with a blank one. + var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var tearDown = new CancellationTokenSource(); + tearDown.Cancel(); + + var drainTask = ProcessHelper.WaitForErrorStreamToDrainAsync( + errorStreamClosed, + timeoutMilliseconds: 30000, + tearDown.Token, + tearDownTimeoutMilliseconds: 5000); + + errorStreamClosed.TrySetResult(true); + await drainTask; + + Assert.IsTrue(errorStreamClosed.Task.IsCompleted, "EOF that arrives within the teardown budget must still be observed."); + } }