From 4558f22e3ac23c2e2a6e1096dfef9bdad824e54c Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Mon, 29 Jun 2026 16:43:00 +0200 Subject: [PATCH 1/8] Drain crashed testhost stderr without blocking a thread-pool thread RunTestsShouldThrowOnStackOverflowException is still flaky. When a testhost crashes, the "Stack overflow." line can reach ErrorDataReceived late under thread-pool starvation (many hosts running in parallel on CI). The exit handler shared one 500ms budget with the process-exit wait and then blocked a thread-pool thread on it, so it both ran low on time and competed for the very thread that delivers the output it was waiting for. The callstack got dropped and the abort message was truncated to just the process path. Decouple the stderr drain from the exit-wait budget, await the EOF signal instead of blocking it, and only spend the generous budget when the process exited abnormally. Clean exits keep a short grace period and the wait returns as soon as EOF arrives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/System/ProcessHelper.cs | 105 +++++++++++++----- .../ProcessHelperTests.cs | 54 ++++----- 2 files changed, 100 insertions(+), 59 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 0c84c45261..77455d2560 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -7,9 +7,7 @@ using System.Globalization; using System.IO; using System.Threading; -#if !NET using System.Threading.Tasks; -#endif using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces; @@ -23,6 +21,14 @@ 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 a clean exit, kept short so neither the common + // case nor the rare grandchild-keeps-the-pipe-open case adds latency. See the Exited handler. + private const int CleanExitErrorDrainTimeout = 500; + #if !NET private readonly IEnvironment _environment; #endif @@ -127,20 +133,22 @@ 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; + // callbacks have run. 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) => { if (args.Data is null) { - errorStreamClosed.Set(); + errorStreamClosed.TrySetResult(true); } errorCallback(sender as Process, args.Data); @@ -151,8 +159,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) { @@ -172,7 +180,7 @@ void InitializeAndStart() // // 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); + var cts = new CancellationTokenSource(processExitTimeout); #if NET await p.WaitForExitAsync(cts.Token); #else @@ -191,7 +199,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 { @@ -222,12 +230,27 @@ 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. + // WaitForExit(timeout)/WaitForExitAsync do not guarantee the ErrorDataReceived callbacks + // have run. + // + // 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 when the process exited + // abnormally. A clean exit gets only a short grace period so we never add latency to the + // common case (e.g. when a grandchild process keeps the stderr pipe open and EOF never + // arrives). In every case the wait returns as soon as EOF is observed, so a process that + // exits and drains promptly pays almost nothing. + var errorDrainTimeout = DidProcessExitCleanly(p) ? CleanExitErrorDrainTimeout : CrashErrorDrainTimeout; + await WaitForErrorStreamToDrainAsync(errorStreamClosed, errorDrainTimeout).ConfigureAwait(false); } // If exit callback has code that access Process object, ensure that the exceptions handling should be done properly. @@ -252,24 +275,50 @@ 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 or when the timeout is not positive, 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. /// - internal static void WaitForErrorStreamToDrain(ManualResetEventSlim? errorStreamClosed, int budgetMilliseconds, long elapsedMilliseconds) + internal static async Task WaitForErrorStreamToDrainAsync(TaskCompletionSource? errorStreamClosed, int timeoutMilliseconds) { - if (errorStreamClosed is null) + if (errorStreamClosed is null || timeoutMilliseconds <= 0) { return; } - var remainingMilliseconds = budgetMilliseconds - (int)elapsedMilliseconds; - if (remainingMilliseconds > 0) + using var timeoutCancellation = new CancellationTokenSource(); + 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 when the process has exited with a zero exit code. A non-zero exit code + /// (a crash) - or an exit code that cannot be retrieved - is treated as not-clean so the caller waits the + /// longer stderr drain budget and does not truncate potentially important crash output. + /// + private static bool DidProcessExitCleanly(Process process) + { + try + { + return process.HasExited && process.ExitCode == 0; + } + catch { - errorStreamClosed.Wait(remainingMilliseconds); + // If the exit code is not retrievable (e.g. the process handle is gone), assume a crash so we + // give the redirected stderr the longer budget to drain. + return false; } } diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index d9a00cff82..bca61e7238 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -2,7 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Diagnostics; -using System.Threading; +using System.Threading.Tasks; using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions; @@ -11,7 +11,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 @@ -23,46 +23,38 @@ public class ProcessHelperTests private const int BudgetMs = 500; [TestMethod] - public void WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses() + public async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses() { - using var errorStreamClosed = new ManualResetEventSlim(initialState: false); - - // The stream reaches EOF a little later, mimicking a slow ErrorDataReceived delivery. - var setter = new Thread(() => - { - Thread.Sleep(150); - errorStreamClosed.Set(); - }) - { IsBackground = true }; + // The stream has already reached EOF (all ErrorDataReceived callbacks have been delivered). + 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); stopwatch.Stop(); - setter.Join(); - Assert.IsTrue(errorStreamClosed.IsSet, "The method must wait until the error stream is drained."); + Assert.IsTrue(errorStreamClosed.Task.IsCompleted, "The method must wait until the error stream is drained."); Assert.IsLessThan( 3000L, stopwatch.ElapsedMilliseconds, - $"The method should return shortly after the stream closes, not at the budget timeout (took {stopwatch.ElapsedMilliseconds} ms)."); + $"The method should return as soon as the stream is drained, not at the timeout (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); 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 +62,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); 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); stopwatch.Stop(); Assert.IsLessThan( From 27e742ab9a7111399f2bd1ef9823d150c816cda7 Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Mon, 29 Jun 2026 17:26:43 +0200 Subject: [PATCH 2/8] Keep IDE abort fast when we kill the test host The generous stderr-drain budget is only meant for a genuine crash. When we kill a still-running test host on purpose - aborting or cleaning up a run from an IDE - it exits with a non-zero code that is indistinguishable from a crash, so it would wait the full crash budget. If a grandchild process (e.g. a browser driver) inherited the stderr handle and keeps the pipe open, EOF never arrives and the abort hangs for seconds. Record the processes we kill in TerminateProcess and treat their exit as an abort (short drain), not a crash. A process that exited on its own is never recorded, so a real crash still gets the generous budget needed to capture a late callstack. Added a unit test for the budget selection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/System/ProcessHelper.cs | 56 +++++++++++++++++-- .../ProcessHelperTests.cs | 20 +++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 77455d2560..68afa0fd79 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -6,6 +6,7 @@ using System.Diagnostics; using System.Globalization; using System.IO; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; @@ -29,6 +30,15 @@ public partial class ProcessHelper : IProcessHelper // case nor the rare grandchild-keeps-the-pipe-open case adds latency. See the Exited handler. private const int CleanExitErrorDrainTimeout = 500; + // Processes we deliberately killed (e.g. when aborting or cleaning up a run). Their abnormal exit code is + // expected and is not a crash, so the exit handler must not spend the long stderr-drain budget on them - + // that would make aborting a run from an IDE slow whenever a grandchild process (e.g. a browser driver) + // keeps the stderr pipe open. 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 _deliberatelyTerminatedProcesses = new(); + private readonly object _deliberatelyTerminatedProcessesLock = new(); + private static readonly object DeliberateTerminationMarker = new(); + #if !NET private readonly IEnvironment _environment; #endif @@ -244,12 +254,13 @@ void InitializeAndStart() // 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 when the process exited - // abnormally. A clean exit gets only a short grace period so we never add latency to the - // common case (e.g. when a grandchild process keeps the stderr pipe open and EOF never - // arrives). In every case the wait returns as soon as EOF is observed, so a process that - // exits and drains promptly pays almost nothing. - var errorDrainTimeout = DidProcessExitCleanly(p) ? CleanExitErrorDrainTimeout : CrashErrorDrainTimeout; + // process-exit budget above, and the generous part is only spent when the process crashed - + // i.e. it exited abnormally on its own. A clean exit, or a process we deliberately killed + // (e.g. aborting a run from an IDE), gets only a short grace period so we never add latency + // to those cases - in particular we must not hang for seconds on abort when a grandchild + // process keeps the stderr pipe open and EOF never arrives. In every case the wait returns + // as soon as EOF is observed, so a process that exits and drains promptly pays almost nothing. + var errorDrainTimeout = GetErrorDrainTimeout(DidProcessExitCleanly(p), WasDeliberatelyTerminated(p)); await WaitForErrorStreamToDrainAsync(errorStreamClosed, errorDrainTimeout).ConfigureAwait(false); } @@ -322,6 +333,34 @@ private static bool DidProcessExitCleanly(Process process) } } + /// + /// Picks the bounded time we are willing to wait for the redirected stderr to reach EOF. A genuine crash + /// (an abnormal exit we did not cause) gets the generous budget so a late-delivered crash callstack is + /// captured; a clean exit, or a process we deliberately killed (an abort/cleanup), gets only the short + /// budget so we never add latency to those cases. + /// + internal static int GetErrorDrainTimeout(bool exitedCleanly, bool deliberatelyTerminated) + => exitedCleanly || deliberatelyTerminated ? CleanExitErrorDrainTimeout : CrashErrorDrainTimeout; + + private void MarkDeliberatelyTerminated(Process process) + { + lock (_deliberatelyTerminatedProcessesLock) + { + if (!_deliberatelyTerminatedProcesses.TryGetValue(process, out _)) + { + _deliberatelyTerminatedProcesses.Add(process, DeliberateTerminationMarker); + } + } + } + + private bool WasDeliberatelyTerminated(Process process) + { + lock (_deliberatelyTerminatedProcessesLock) + { + return _deliberatelyTerminatedProcesses.TryGetValue(process, out _); + } + } + /// public string? GetCurrentProcessFileName() { @@ -395,6 +434,11 @@ public void TerminateProcess(object? process) { if (process is Process proc && !proc.HasExited) { + // Killing a still-running process on purpose (abort/cleanup): record it so the exit handler + // treats the resulting abnormal exit as an abort (short stderr drain), not a crash (long + // stderr drain). A process that exits on its own is never recorded here, so a genuine crash + // still gets the generous budget. + MarkDeliberatelyTerminated(proc); proc.Kill(); } } diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index bca61e7238..461b446636 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -91,4 +91,24 @@ public async Task WaitForErrorStreamToDrainShouldReturnImmediatelyWhenThereIsNoE 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 we did not cause: not a clean exit and not something we killed. + var crash = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, deliberatelyTerminated: false); + var cleanExit = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: true, deliberatelyTerminated: false); + var aborted = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, deliberatelyTerminated: true); + var cleanAndAborted = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: true, deliberatelyTerminated: 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 deliberately killed (e.g. aborting from an IDE) must drain as fast as a clean exit, so + // an abort never hangs for seconds when a grandchild keeps the stderr pipe open. + Assert.AreEqual(cleanExit, aborted, "A deliberately terminated process must use the short (clean-exit) budget."); + Assert.AreEqual(cleanExit, cleanAndAborted, "A clean, deliberately terminated process must use the short budget."); + } } From 03a5a76d80d6629e3ef48f412efd40878939ee87 Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Tue, 30 Jun 2026 15:23:12 +0200 Subject: [PATCH 3/8] Dispose the process-exit CancellationTokenSource and test late EOF The CancellationTokenSource that bounds the post-exit wait is created with a timeout, so it allocates a timer; dispose it via 'using' so we don't leak one per testhost exit when many hosts are spawned. The stderr-drain test had been reduced to the already-drained fast path, so it could not catch a regression where EOF arrives just after the exit handler starts waiting. Make EOF land ~150ms into the wait so the test exercises waiting for a late ErrorDataReceived delivery and returning promptly, and keep a separate fast-path test for the already-drained case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/System/ProcessHelper.cs | 4 ++- .../ProcessHelperTests.cs | 30 +++++++++++++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 68afa0fd79..a06773f5bc 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -190,7 +190,9 @@ void InitializeAndStart() // // 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(processExitTimeout); + // '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 diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index 461b446636..af18a7428a 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -25,9 +25,15 @@ public class ProcessHelperTests [TestMethod] public async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses() { - // The stream has already reached EOF (all ErrorDataReceived callbacks have been delivered). + // EOF arrives a little AFTER we start waiting, mimicking a slow ErrorDataReceived delivery that lands + // just after the exit handler begins draining. The wait must observe that late completion and return + // promptly once it arrives - not return early (dropping the crash callstack) and not block to the timeout. var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - errorStreamClosed.TrySetResult(true); + _ = Task.Run(async () => + { + await Task.Delay(150); + errorStreamClosed.TrySetResult(true); + }); var stopwatch = Stopwatch.StartNew(); await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000); @@ -37,7 +43,25 @@ public async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses( Assert.IsLessThan( 3000L, stopwatch.ElapsedMilliseconds, - $"The method should return as soon as the stream is drained, not at the timeout (took {stopwatch.ElapsedMilliseconds} ms)."); + $"The method should return shortly after the stream closes, not at 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(); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000); + stopwatch.Stop(); + + Assert.IsLessThan( + 250L, + stopwatch.ElapsedMilliseconds, + $"When the stream is already drained the method must return immediately (took {stopwatch.ElapsedMilliseconds} ms)."); } [TestMethod] From 31646a45e5756df6e4329f81760571ebf446bbe9 Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Wed, 1 Jul 2026 13:29:30 +0200 Subject: [PATCH 4/8] Make the late-EOF drain test deterministic and analyzer-clean The late-EOF test from the previous commit signaled EOF from a Task.Run with a Task.Delay, neither taking a CancellationToken. That trips MSTEST0049, which is only a warning in Debug but an error in the Release build CI runs - so the Windows leg failed at compile and every test was skipped. That is why the build went red without a test actually failing. Rewrite it without a timed background task: start the wait, assert it is still in progress, then signal EOF and await it. That exercises the same "EOF lands after the wait has started" path, but deterministically - no sleep, no timing assertion that could itself flake - and without the analyzer-tripping Task.Run/Task.Delay. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ProcessHelperTests.cs | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index af18a7428a..873138fa29 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -25,25 +25,20 @@ public class ProcessHelperTests [TestMethod] public async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses() { - // EOF arrives a little AFTER we start waiting, mimicking a slow ErrorDataReceived delivery that lands - // just after the exit handler begins draining. The wait must observe that late completion and return - // promptly once it arrives - not return early (dropping the crash callstack) and not block to the timeout. + // 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); - _ = Task.Run(async () => - { - await Task.Delay(150); - errorStreamClosed.TrySetResult(true); - }); - var stopwatch = Stopwatch.StartNew(); - await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000); - stopwatch.Stop(); + var drainTask = ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000); + Assert.IsFalse(drainTask.IsCompleted, "The wait must still be in progress before the error stream drains."); + // The late ErrorDataReceived EOF finally arrives. + errorStreamClosed.TrySetResult(true); + + await drainTask; Assert.IsTrue(errorStreamClosed.Task.IsCompleted, "The method must wait until the error stream is drained."); - Assert.IsLessThan( - 3000L, - stopwatch.ElapsedMilliseconds, - $"The method should return shortly after the stream closes, not at the timeout (took {stopwatch.ElapsedMilliseconds} ms)."); } [TestMethod] From d6a65a440c569f166598af3d37ed9818c28b3bdc Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Wed, 1 Jul 2026 14:38:31 +0200 Subject: [PATCH 5/8] Mark the non-Windows exit-timeout kill as deliberate termination When the process overruns the 500ms exit budget on the non-Windows !NET path, the CancellationToken callback force-kills it. That kill was not recorded via MarkDeliberatelyTerminated, so the exit handler saw a non-zero exit, treated it as a crash, and waited the generous 5s stderr drain - the exact abort latency this change avoids. Mark it before the kill, mirroring TerminateProcess, so the drain uses the short budget. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/System/ProcessHelper.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index a06773f5bc..35142b44f8 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -221,6 +221,12 @@ void InitializeAndStart() { if (!p.HasExited) { + // We are force-killing a process that overran the exit budget (e.g. a + // grandchild keeps it hanging). Record it as a deliberate termination - + // exactly like TerminateProcess does - BEFORE killing, so the stderr + // drain below uses the short abort budget instead of treating our own + // kill as a crash and waiting the generous 5s budget unnecessarily. + MarkDeliberatelyTerminated(p); p.Kill(); } } From b9cd9ff6362aa7d2d5ba5b7d17745d711c8a0378 Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Wed, 1 Jul 2026 14:50:57 +0200 Subject: [PATCH 6/8] Skip the drain timer when stderr already drained, and assert the wait returns promptly The already-drained case is the common one on a clean exit; short-circuit before allocating the CancellationTokenSource and Task.Delay timer. Also tighten the late-EOF test so it fails if the wait ever ignored EOF and ran to the full timeout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/System/ProcessHelper.cs | 7 ++++--- test/vstest.console.UnitTests/ProcessHelperTests.cs | 11 ++++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 35142b44f8..26cb18b9e3 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -298,15 +298,16 @@ void InitializeAndStart() /// 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 or when the timeout is not positive, 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 + /// 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. /// internal static async Task WaitForErrorStreamToDrainAsync(TaskCompletionSource? errorStreamClosed, int timeoutMilliseconds) { - if (errorStreamClosed is null || timeoutMilliseconds <= 0) + if (errorStreamClosed is null || timeoutMilliseconds <= 0 || errorStreamClosed.Task.IsCompleted) { return; } diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index 873138fa29..951162720d 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -34,11 +34,20 @@ public async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses( var drainTask = ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000); Assert.IsFalse(drainTask.IsCompleted, "The wait must still be in progress before the error stream drains."); - // The late ErrorDataReceived EOF finally arrives. + // 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); 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] From 9c4ab9300a2cf93c1842b3654d830c5c7d781edc Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Mon, 17 Aug 2026 21:07:56 +0200 Subject: [PATCH 7/8] Require the kill exit code before treating an exit as our own abort TerminateProcess checks HasExited and then calls Kill, and the process can crash on its own in between. The kill then never lands, on .NET it silently does nothing and on .NET Framework it throws, but we had already recorded the process as deliberately terminated, so the exit handler gave a real crash the 500 ms abort drain instead of the 5s crash drain, and could truncate the callstack that drain exists to capture. The request is still recorded before the kill, so the exit handler cannot miss it, but recording it no longer shortens the drain on its own. The exit counts as ours only when the exit code is the one Kill leaves behind, -1 on Windows and 128 + SIGKILL on Unix. A process that crashed in that window keeps its own exit code and so keeps the generous budget, and an exit code we cannot read is treated the same way. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../common/System/ProcessHelper.cs | 97 ++++++++++++------- .../ProcessHelperTests.cs | 67 +++++++++++++ 2 files changed, 129 insertions(+), 35 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 26cb18b9e3..3015765f9f 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -30,14 +30,21 @@ public partial class ProcessHelper : IProcessHelper // case nor the rare grandchild-keeps-the-pipe-open case adds latency. See the Exited handler. private const int CleanExitErrorDrainTimeout = 500; - // Processes we deliberately killed (e.g. when aborting or cleaning up a run). Their abnormal exit code is - // expected and is not a crash, so the exit handler must not spend the long stderr-drain budget on them - - // that would make aborting a run from an IDE slow whenever a grandchild process (e.g. a browser driver) - // keeps the stderr pipe open. 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 _deliberatelyTerminatedProcesses = new(); - private readonly object _deliberatelyTerminatedProcessesLock = new(); - private static readonly object DeliberateTerminationMarker = new(); + // Processes we asked to kill (e.g. when aborting or cleaning up a run). When such a kill lands, the abnormal + // exit code is expected and is not a crash, so the exit handler must not spend the long stderr-drain budget + // on it - that would make aborting a run from an IDE slow whenever a grandchild process (e.g. a browser + // driver) keeps the stderr pipe open. This records only that we asked; whether the kill actually terminated + // the process is decided from the exit code, see WasTerminatedByOurKill. 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 _killRequestedProcesses = new(); + private readonly object _killRequestedProcessesLock = new(); + private static readonly object KillRequestedMarker = new(); + + // Exit code a process is left with when Process.Kill() terminates it: Kill calls TerminateProcess(handle, -1) + // on Windows, and sends SIGKILL on Unix, which .NET reports as 128 + signal number. + private const int WindowsKillExitCode = -1; + private const int UnixSigKillExitCode = 128 + 9; #if !NET private readonly IEnvironment _environment; @@ -222,11 +229,12 @@ void InitializeAndStart() if (!p.HasExited) { // We are force-killing a process that overran the exit budget (e.g. a - // grandchild keeps it hanging). Record it as a deliberate termination - - // exactly like TerminateProcess does - BEFORE killing, so the stderr - // drain below uses the short abort budget instead of treating our own - // kill as a crash and waiting the generous 5s budget unnecessarily. - MarkDeliberatelyTerminated(p); + // grandchild keeps it hanging). Record the request - exactly like + // TerminateProcess does - BEFORE killing, so the stderr drain below uses + // the short abort budget instead of treating our own kill as a crash and + // waiting the generous 5s budget unnecessarily. Recording it does not by + // itself shorten the budget; see WasTerminatedByOurKill. + MarkKillRequested(p); p.Kill(); } } @@ -263,12 +271,15 @@ void InitializeAndStart() // // This drain budget is intentionally separate from (and far more generous than) the // process-exit budget above, and the generous part is only spent when the process crashed - - // i.e. it exited abnormally on its own. A clean exit, or a process we deliberately killed + // i.e. it exited abnormally on its own. A clean exit, or a process our own kill terminated // (e.g. aborting a run from an IDE), gets only a short grace period so we never add latency // to those cases - in particular we must not hang for seconds on abort when a grandchild // process keeps the stderr pipe open and EOF never arrives. In every case the wait returns // as soon as EOF is observed, so a process that exits and drains promptly pays almost nothing. - var errorDrainTimeout = GetErrorDrainTimeout(DidProcessExitCleanly(p), WasDeliberatelyTerminated(p)); + var exitCode = GetExitCodeOrNull(p); + var errorDrainTimeout = GetErrorDrainTimeout( + exitedCleanly: exitCode == 0, + deliberatelyTerminated: WasTerminatedByOurKill(WasKillRequested(p), exitCode)); await WaitForErrorStreamToDrainAsync(errorStreamClosed, errorDrainTimeout).ConfigureAwait(false); } @@ -324,21 +335,20 @@ internal static async Task WaitForErrorStreamToDrainAsync(TaskCompletionSource - /// Returns when the process has exited with a zero exit code. A non-zero exit code - /// (a crash) - or an exit code that cannot be retrieved - is treated as not-clean so the caller waits the - /// longer stderr drain budget and does not truncate potentially important crash output. + /// Returns the exit code of a process that has exited, or when it has not exited or the + /// exit code cannot be retrieved (e.g. the process handle is gone). Callers treat a exit + /// code as a crash, so the redirected stderr gets the longer budget to drain and potentially important crash + /// output is not truncated. /// - private static bool DidProcessExitCleanly(Process process) + private static int? GetExitCodeOrNull(Process process) { try { - return process.HasExited && process.ExitCode == 0; + return process.HasExited ? process.ExitCode : null; } catch { - // If the exit code is not retrievable (e.g. the process handle is gone), assume a crash so we - // give the redirected stderr the longer budget to drain. - return false; + return null; } } @@ -351,22 +361,37 @@ private static bool DidProcessExitCleanly(Process process) internal static int GetErrorDrainTimeout(bool exitedCleanly, bool deliberatelyTerminated) => exitedCleanly || deliberatelyTerminated ? CleanExitErrorDrainTimeout : CrashErrorDrainTimeout; - private void MarkDeliberatelyTerminated(Process process) + /// + /// Returns when an exit is the result of a kill we asked for, so the exit handler can + /// give it the short abort budget instead of the generous crash budget. + /// + /// Having asked is not enough on its own. We check and then call + /// , and the process can crash on its own in between - in which case the kill + /// never lands (on .NET it silently does nothing, on .NET Framework it throws) and the exit we are looking at + /// is a real crash. Requiring the exit code that our kill leaves behind means such a crash keeps the generous + /// budget and its late callstack is not truncated. An exit code we could not retrieve () + /// is treated the same way, for the same reason. + /// + /// + internal static bool WasTerminatedByOurKill(bool killRequested, int? exitCode) + => killRequested && exitCode is WindowsKillExitCode or UnixSigKillExitCode; + + private void MarkKillRequested(Process process) { - lock (_deliberatelyTerminatedProcessesLock) + lock (_killRequestedProcessesLock) { - if (!_deliberatelyTerminatedProcesses.TryGetValue(process, out _)) + if (!_killRequestedProcesses.TryGetValue(process, out _)) { - _deliberatelyTerminatedProcesses.Add(process, DeliberateTerminationMarker); + _killRequestedProcesses.Add(process, KillRequestedMarker); } } } - private bool WasDeliberatelyTerminated(Process process) + private bool WasKillRequested(Process process) { - lock (_deliberatelyTerminatedProcessesLock) + lock (_killRequestedProcessesLock) { - return _deliberatelyTerminatedProcesses.TryGetValue(process, out _); + return _killRequestedProcesses.TryGetValue(process, out _); } } @@ -443,11 +468,13 @@ public void TerminateProcess(object? process) { if (process is Process proc && !proc.HasExited) { - // Killing a still-running process on purpose (abort/cleanup): record it so the exit handler - // treats the resulting abnormal exit as an abort (short stderr drain), not a crash (long - // stderr drain). A process that exits on its own is never recorded here, so a genuine crash - // still gets the generous budget. - MarkDeliberatelyTerminated(proc); + // Killing a still-running process on purpose (abort/cleanup): record the request BEFORE the kill, + // so the exit handler - which can run at any moment from here on - cannot miss it. Recording it + // does not by itself shorten the stderr drain: the exit handler treats the exit as an abort only + // when the exit code is the one our kill leaves behind. So a process that crashes on its own + // between the check above and the kill is still treated as a crash and gets the generous budget, + // and a process that exits on its own without us asking never gets here at all. + MarkKillRequested(proc); proc.Kill(); } } diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index 951162720d..653d947963 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -139,4 +139,71 @@ public void GetErrorDrainTimeoutShouldUseTheGenerousBudgetOnlyForACrash() Assert.AreEqual(cleanExit, aborted, "A deliberately terminated process must use the short (clean-exit) budget."); Assert.AreEqual(cleanExit, cleanAndAborted, "A clean, deliberately terminated process must use the short budget."); } + + // Exit code a process is left with when Process.Kill() terminates it: TerminateProcess(handle, -1) on + // Windows, and 128 + SIGKILL on Unix. + private const int WindowsKillExitCode = -1; + private const int UnixSigKillExitCode = 137; + + // Windows STATUS_STACK_OVERFLOW - what a test host that blew its stack exits with. This is the crash whose + // callstack the drain budget exists to capture. + private const int StackOverflowExitCode = unchecked((int)0xC00000FD); + + [TestMethod] + public void WasTerminatedByOurKillShouldBeTrueWhenTheKillLanded() + { + // We asked to kill the process and the exit code is the one Process.Kill() leaves behind, so this exit + // is our abort and must get the short drain budget. + Assert.IsTrue( + ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: WindowsKillExitCode), + "A process killed on Windows exits with -1 and must count as deliberately terminated."); + Assert.IsTrue( + ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: UnixSigKillExitCode), + "A process killed on Unix exits with 128 + SIGKILL and must count as deliberately terminated."); + } + + [TestMethod] + public void WasTerminatedByOurKillShouldBeFalseWhenTheProcessCrashedInsideTheKillRaceWindow() + { + // TerminateProcess checks HasExited and then calls Kill, and the process can crash on its own in + // between. The kill then never lands - on .NET it silently does nothing, on .NET Framework it throws - + // and the exit we are looking at is a real crash carrying a real callstack. Having asked to kill must + // therefore not be enough to shorten the drain, or that callstack gets truncated. + Assert.IsFalse( + ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: StackOverflowExitCode), + "A process that crashed between the HasExited check and the kill must still count as a crash."); + + var timeout = ProcessHelper.GetErrorDrainTimeout( + exitedCleanly: false, + deliberatelyTerminated: ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: StackOverflowExitCode)); + var crashTimeout = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, deliberatelyTerminated: false); + + Assert.AreEqual( + crashTimeout, + timeout, + "A crash that happened while we were asking for a kill must keep the generous crash drain budget."); + } + + [TestMethod] + public void WasTerminatedByOurKillShouldBeFalseWhenTheExitCodeIsUnavailable() + { + // We could not read the exit code, so we cannot tell whether our kill landed. Assume it did not, so the + // stderr keeps the generous budget rather than being cut short. + Assert.IsFalse( + ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: null), + "An unretrievable exit code must not be treated as our kill."); + } + + [TestMethod] + public void WasTerminatedByOurKillShouldBeFalseWhenWeNeverAskedToKill() + { + // A process that exits with the kill exit code on its own was not killed by us - it crashed, or chose + // that exit code - and must get the generous budget. + Assert.IsFalse( + ProcessHelper.WasTerminatedByOurKill(killRequested: false, exitCode: WindowsKillExitCode), + "Without a kill request the exit is not ours, whatever the exit code is."); + Assert.IsFalse( + ProcessHelper.WasTerminatedByOurKill(killRequested: false, exitCode: StackOverflowExitCode), + "A crash we did not cause must count as a crash."); + } } From a7fd34d3716c4b4cb242b0034b9e8333e4591aee Mon Sep 17 00:00:00 2001 From: Jakub Jares Date: Tue, 18 Aug 2026 16:08:32 +0200 Subject: [PATCH 8/8] Cut the stderr drain short on teardown instead of guessing from exit codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit decided whether an exit was our own kill by checking the exit code Process.Kill() leaves behind. That does not hold up: - Both DefaultTestHostManager.CleanTestHostAsync and DotnetTestHostManager.CleanTestHostAsync call TerminateProcess and then dispose the Process immediately. After Dispose, HasExited and ExitCode throw InvalidOperationException, so the exit code came back null and the exit was treated as a crash - the 5s budget, on the abort path. - The budget was picked once, when the process exited. An abort arriving while a crash drain was already running could not shorten it. - A testhost that exits abnormally on its own while a grandchild (the Edge Driver case the surrounding code already describes) holds the stderr pipe was never covered by the kill tracking at all, so it went from 500ms to 5s. Replace it with a per-process cancellation signal. TerminateProcess signals it before killing, and also when the process has already exited, so a drain that is in flight is cut short too. Three budgets now: 5s for a crash, 500ms for a clean exit, 100ms once we are tearing down - short, but not zero, because output already sitting in the pipe costs nothing to pick up. Measured against a child whose grandchild holds the stderr pipe open, on the delay that TestRequestSender.GetAbortErrorMessage waits out: abort while the crash drain is in flight 5158ms -> 313ms kill a running process 1176ms -> 624ms crash with a late callstack 3192ms -> 3348ms, callstack still captured Also signal EOF after the last ErrorDataReceived callback rather than before it, and correct the comment about WaitForExitAsync - it does wait for the redirected streams to reach EOF, just only for as long as its token allows. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> 🤖 --- .../common/System/ProcessHelper.cs | 222 ++++++++++-------- .../ProcessHelperTests.cs | 154 ++++++------ 2 files changed, 212 insertions(+), 164 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 3015765f9f..0e5b364234 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -26,25 +26,21 @@ public partial class ProcessHelper : IProcessHelper // 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 a clean exit, kept short so neither the common - // case nor the rare grandchild-keeps-the-pipe-open case adds latency. See the Exited handler. - private const int CleanExitErrorDrainTimeout = 500; - - // Processes we asked to kill (e.g. when aborting or cleaning up a run). When such a kill lands, the abnormal - // exit code is expected and is not a crash, so the exit handler must not spend the long stderr-drain budget - // on it - that would make aborting a run from an IDE slow whenever a grandchild process (e.g. a browser - // driver) keeps the stderr pipe open. This records only that we asked; whether the kill actually terminated - // the process is decided from the exit code, see WasTerminatedByOurKill. 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 _killRequestedProcesses = new(); - private readonly object _killRequestedProcessesLock = new(); - private static readonly object KillRequestedMarker = new(); - - // Exit code a process is left with when Process.Kill() terminates it: Kill calls TerminateProcess(handle, -1) - // on Windows, and sends SIGKILL on Unix, which .NET reports as 128 + signal number. - private const int WindowsKillExitCode = -1; - private const int UnixSigKillExitCode = 128 + 9; + // 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; @@ -153,8 +149,9 @@ void InitializeAndStart() // 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 awaits (bounded) on this before reading. + // 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) { @@ -163,12 +160,14 @@ void InitializeAndStart() 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.TrySetResult(true); } - - errorCallback(sender as Process, args.Data); }; } @@ -190,10 +189,11 @@ 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. @@ -229,12 +229,11 @@ void InitializeAndStart() if (!p.HasExited) { // We are force-killing a process that overran the exit budget (e.g. a - // grandchild keeps it hanging). Record the request - exactly like - // TerminateProcess does - BEFORE killing, so the stderr drain below uses - // the short abort budget instead of treating our own kill as a crash and - // waiting the generous 5s budget unnecessarily. Recording it does not by - // itself shorten the budget; see WasTerminatedByOurKill. - MarkKillRequested(p); + // 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(); } } @@ -259,8 +258,6 @@ void InitializeAndStart() // 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. - // WaitForExit(timeout)/WaitForExitAsync do not guarantee the ErrorDataReceived callbacks - // have run. // // 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 @@ -270,17 +267,16 @@ void InitializeAndStart() // 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 when the process crashed - - // i.e. it exited abnormally on its own. A clean exit, or a process our own kill terminated - // (e.g. aborting a run from an IDE), gets only a short grace period so we never add latency - // to those cases - in particular we must not hang for seconds on abort when a grandchild - // process keeps the stderr pipe open and EOF never arrives. In every case the wait returns - // as soon as EOF is observed, so a process that exits and drains promptly pays almost nothing. - var exitCode = GetExitCodeOrNull(p); - var errorDrainTimeout = GetErrorDrainTimeout( - exitedCleanly: exitCode == 0, - deliberatelyTerminated: WasTerminatedByOurKill(WasKillRequested(p), exitCode)); - await WaitForErrorStreamToDrainAsync(errorStreamClosed, errorDrainTimeout).ConfigureAwait(false); + // 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. @@ -311,12 +307,21 @@ void InitializeAndStart() /// 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. + /// 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 async Task WaitForErrorStreamToDrainAsync(TaskCompletionSource? errorStreamClosed, int timeoutMilliseconds) + internal static async Task WaitForErrorStreamToDrainAsync( + TaskCompletionSource? errorStreamClosed, + int timeoutMilliseconds, + CancellationToken tearDown = default, + int tearDownTimeoutMilliseconds = 0) { if (errorStreamClosed is null || timeoutMilliseconds <= 0 || errorStreamClosed.Task.IsCompleted) { @@ -324,6 +329,25 @@ internal static async Task WaitForErrorStreamToDrainAsync(TaskCompletionSource + { + 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); @@ -335,66 +359,66 @@ internal static async Task WaitForErrorStreamToDrainAsync(TaskCompletionSource - /// Returns the exit code of a process that has exited, or when it has not exited or the - /// exit code cannot be retrieved (e.g. the process handle is gone). Callers treat a exit - /// code as a crash, so the redirected stderr gets the longer budget to drain and potentially important crash - /// output is not truncated. + /// 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 int? GetExitCodeOrNull(Process process) + private static bool ExitedCleanly(Process process) { try { - return process.HasExited ? process.ExitCode : null; + return process.HasExited && process.ExitCode == 0; } catch { - return null; + return false; } } /// - /// Picks the bounded time we are willing to wait for the redirected stderr to reach EOF. A genuine crash - /// (an abnormal exit we did not cause) gets the generous budget so a late-delivered crash callstack is - /// captured; a clean exit, or a process we deliberately killed (an abort/cleanup), gets only the short - /// budget so we never add latency to those cases. + /// 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 deliberatelyTerminated) - => exitedCleanly || deliberatelyTerminated ? CleanExitErrorDrainTimeout : CrashErrorDrainTimeout; + internal static int GetErrorDrainTimeout(bool exitedCleanly, bool tearingDown) + => tearingDown ? TearDownErrorDrainTimeout + : exitedCleanly ? NonCrashErrorDrainTimeout + : CrashErrorDrainTimeout; /// - /// Returns when an exit is the result of a kill we asked for, so the exit handler can - /// give it the short abort budget instead of the generous crash budget. - /// - /// Having asked is not enough on its own. We check and then call - /// , and the process can crash on its own in between - in which case the kill - /// never lands (on .NET it silently does nothing, on .NET Framework it throws) and the exit we are looking at - /// is a real crash. Requiring the exit code that our kill leaves behind means such a crash keeps the generous - /// budget and its late callstack is not truncated. An exit code we could not retrieve () - /// is treated the same way, for the same reason. - /// + /// 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. /// - internal static bool WasTerminatedByOurKill(bool killRequested, int? exitCode) - => killRequested && exitCode is WindowsKillExitCode or UnixSigKillExitCode; - - private void MarkKillRequested(Process process) + private void SignalTearDown(Process process) { - lock (_killRequestedProcessesLock) + try { - if (!_killRequestedProcesses.TryGetValue(process, out _)) - { - _killRequestedProcesses.Add(process, KillRequestedMarker); - } + GetTearDownSource(process).Cancel(); } - } - - private bool WasKillRequested(Process process) - { - lock (_killRequestedProcessesLock) + catch { - return _killRequestedProcesses.TryGetValue(process, out _); + // 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() { @@ -464,17 +488,21 @@ 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) { - // Killing a still-running process on purpose (abort/cleanup): record the request BEFORE the kill, - // so the exit handler - which can run at any moment from here on - cannot miss it. Recording it - // does not by itself shorten the stderr drain: the exit handler treats the exit as an abort only - // when the exit code is the one our kill leaves behind. So a process that crashes on its own - // between the check above and the kill is still treated as a crash and gets the generous budget, - // and a process that exits on its own without us asking never gets here at all. - MarkKillRequested(proc); proc.Kill(); } } diff --git a/test/vstest.console.UnitTests/ProcessHelperTests.cs b/test/vstest.console.UnitTests/ProcessHelperTests.cs index 653d947963..fa69e81312 100644 --- a/test/vstest.console.UnitTests/ProcessHelperTests.cs +++ b/test/vstest.console.UnitTests/ProcessHelperTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Diagnostics; +using System.Threading; using System.Threading.Tasks; using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions; @@ -22,6 +23,12 @@ 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 async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses() { @@ -31,7 +38,7 @@ public async Task WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses( // crash callstack) and not block to the timeout. var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - var drainTask = ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000); + 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 @@ -59,7 +66,7 @@ public async Task WaitForErrorStreamToDrainShouldReturnImmediatelyWhenAlreadyDra errorStreamClosed.TrySetResult(true); var stopwatch = Stopwatch.StartNew(); - await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 5000, NoTearDown); stopwatch.Stop(); Assert.IsLessThan( @@ -75,7 +82,7 @@ public async Task WaitForErrorStreamToDrainShouldBeBoundedWhenTheErrorStreamNeve var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var stopwatch = Stopwatch.StartNew(); - await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, BudgetMs); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, BudgetMs, NoTearDown); stopwatch.Stop(); Assert.IsFalse(errorStreamClosed.Task.IsCompleted, "Precondition: the stream never closes in this test."); @@ -97,7 +104,7 @@ public async Task WaitForErrorStreamToDrainShouldNotWaitWhenTimeoutIsNotPositive var errorStreamClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var stopwatch = Stopwatch.StartNew(); - await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 0); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed, timeoutMilliseconds: 0, NoTearDown); stopwatch.Stop(); Assert.IsFalse(errorStreamClosed.Task.IsCompleted); @@ -111,7 +118,7 @@ public async Task WaitForErrorStreamToDrainShouldNotWaitWhenTimeoutIsNotPositive public async Task WaitForErrorStreamToDrainShouldReturnImmediatelyWhenThereIsNoErrorStream() { var stopwatch = Stopwatch.StartNew(); - await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed: null, BudgetMs); + await ProcessHelper.WaitForErrorStreamToDrainAsync(errorStreamClosed: null, BudgetMs, NoTearDown); stopwatch.Stop(); Assert.IsLessThan( @@ -123,87 +130,100 @@ public async Task WaitForErrorStreamToDrainShouldReturnImmediatelyWhenThereIsNoE [TestMethod] public void GetErrorDrainTimeoutShouldUseTheGenerousBudgetOnlyForACrash() { - // A crash is an abnormal exit we did not cause: not a clean exit and not something we killed. - var crash = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, deliberatelyTerminated: false); - var cleanExit = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: true, deliberatelyTerminated: false); - var aborted = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, deliberatelyTerminated: true); - var cleanAndAborted = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: true, deliberatelyTerminated: true); + // 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 deliberately killed (e.g. aborting from an IDE) must drain as fast as a clean exit, so - // an abort never hangs for seconds when a grandchild keeps the stderr pipe open. - Assert.AreEqual(cleanExit, aborted, "A deliberately terminated process must use the short (clean-exit) budget."); - Assert.AreEqual(cleanExit, cleanAndAborted, "A clean, deliberately terminated process must use the short budget."); + // 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."); } - // Exit code a process is left with when Process.Kill() terminates it: TerminateProcess(handle, -1) on - // Windows, and 128 + SIGKILL on Unix. - private const int WindowsKillExitCode = -1; - private const int UnixSigKillExitCode = 137; - - // Windows STATUS_STACK_OVERFLOW - what a test host that blew its stack exits with. This is the crash whose - // callstack the drain budget exists to capture. - private const int StackOverflowExitCode = unchecked((int)0xC00000FD); - [TestMethod] - public void WasTerminatedByOurKillShouldBeTrueWhenTheKillLanded() + public async Task WaitForErrorStreamToDrainShouldBeCutShortWhenTearDownIsSignaledUpFront() { - // We asked to kill the process and the exit code is the one Process.Kill() leaves behind, so this exit - // is our abort and must get the short drain budget. - Assert.IsTrue( - ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: WindowsKillExitCode), - "A process killed on Windows exits with -1 and must count as deliberately terminated."); - Assert.IsTrue( - ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: UnixSigKillExitCode), - "A process killed on Unix exits with 128 + SIGKILL and must count as deliberately terminated."); - } + // 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(); - [TestMethod] - public void WasTerminatedByOurKillShouldBeFalseWhenTheProcessCrashedInsideTheKillRaceWindow() - { - // TerminateProcess checks HasExited and then calls Kill, and the process can crash on its own in - // between. The kill then never lands - on .NET it silently does nothing, on .NET Framework it throws - - // and the exit we are looking at is a real crash carrying a real callstack. Having asked to kill must - // therefore not be enough to shorten the drain, or that callstack gets truncated. - Assert.IsFalse( - ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: StackOverflowExitCode), - "A process that crashed between the HasExited check and the kill must still count as a crash."); - - var timeout = ProcessHelper.GetErrorDrainTimeout( - exitedCleanly: false, - deliberatelyTerminated: ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: StackOverflowExitCode)); - var crashTimeout = ProcessHelper.GetErrorDrainTimeout(exitedCleanly: false, deliberatelyTerminated: false); + var stopwatch = Stopwatch.StartNew(); + await ProcessHelper.WaitForErrorStreamToDrainAsync( + errorStreamClosed, + timeoutMilliseconds: 30000, + tearDown.Token, + tearDownTimeoutMilliseconds: 100); + stopwatch.Stop(); - Assert.AreEqual( - crashTimeout, - timeout, - "A crash that happened while we were asking for a kill must keep the generous crash drain budget."); + 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 void WasTerminatedByOurKillShouldBeFalseWhenTheExitCodeIsUnavailable() + public async Task WaitForErrorStreamToDrainShouldBeCutShortWhenTearDownArrivesWhileWaiting() { - // We could not read the exit code, so we cannot tell whether our kill landed. Assume it did not, so the - // stderr keeps the generous budget rather than being cut short. - Assert.IsFalse( - ProcessHelper.WasTerminatedByOurKill(killRequested: true, exitCode: null), - "An unretrievable exit code must not be treated as our kill."); + // 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 void WasTerminatedByOurKillShouldBeFalseWhenWeNeverAskedToKill() + public async Task WaitForErrorStreamToDrainShouldStillCaptureOutputThatArrivesDuringTearDown() { - // A process that exits with the kill exit code on its own was not killed by us - it crashed, or chose - // that exit code - and must get the generous budget. - Assert.IsFalse( - ProcessHelper.WasTerminatedByOurKill(killRequested: false, exitCode: WindowsKillExitCode), - "Without a kill request the exit is not ours, whatever the exit code is."); - Assert.IsFalse( - ProcessHelper.WasTerminatedByOurKill(killRequested: false, exitCode: StackOverflowExitCode), - "A crash we did not cause must count as a crash."); + // 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."); } }