Deadline-aware cancellation (prototype) - #10018
Conversation
CI systems (AzDO, GitHub Actions) hard-cancel a job at a fixed wall-clock time. When that happens the runner kills the test process, so we lose the TRX/HTML/AzDO reports and get no dump for a hanging test. This teaches MTP about that deadline through an environment variable and lets it react a bit early, while it still controls its own shutdown: - TESTINGPLATFORM_DEADLINE is an absolute instant (ISO 8601, parsed to UTC). - At deadline minus stop margin (default 60s) an in-process extension asks the framework to gracefully stop scheduling new tests, so the session ends normally and every reporter finalizes. - At deadline minus dump margin (default 30s) the out-of-process HangDump controller takes a dump of the process tree and kills the host, for the case where the host is wedged and never reaches the graceful stop. The deadline comes from the environment, so there is no hardcoded timeout in MTP. The margins are MTP side policy and are env overridable. Wiring the real deadline from the CI timeout is a small bit of YAML, left for a follow-up. It is opt-in: with no deadline set both timers stay unarmed and there is no behavior change. The graceful stop also degrades to a no-op when the framework does not expose IGracefulStopTestExecutionCapability. Verified: build.cmd -pack passes 0/0, and the new acceptance tests pass (AbortAtDeadlineTests 5/5, HangDumpTests 30/30 including the deadline dump). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a prototype “deadline-aware cancellation” mechanism to Microsoft.Testing.Platform (MTP) so CI can provide an absolute wall-clock deadline and MTP can proactively (a) request a graceful stop before the runner hard-kills the process, and (b) trigger HangDump as a fallback before the deadline.
Changes:
- Introduces
DeadlineHelper+ new env vars (TESTINGPLATFORM_DEADLINE,*_STOP_MARGIN,*_DUMP_MARGIN) for parsing an absolute UTC deadline and margins. - Registers a new in-proc
AbortAtDeadlineExtensionthat schedules a timer to requestIGracefulStopTestExecutionCapability. - Extends HangDump to arm an additional one-shot timer for the absolute deadline and adds acceptance coverage for both graceful stop and deadline-driven dump.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HangDumpTests.cs | Adds acceptance coverage ensuring HangDump can be triggered via absolute deadline env var. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/AbortAtDeadlineTests.cs | New acceptance suite validating the graceful-stop behavior around deadlines/margins and missing capability. |
| src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt | Tracks newly added internal APIs/constants for PublicAPIAnalyzers. |
| src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.Framework.cs | Registers AbortAtDeadlineExtension into the message bus when enabled. |
| src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs | Adds constants for the new deadline-related environment variables. |
| src/Platform/Microsoft.Testing.Platform/Helpers/DeadlineHelper.cs | New helper to read/parse deadline + margins from environment. |
| src/Platform/Microsoft.Testing.Platform/Extensions/AbortAtDeadlineExtension.cs | New extension that arms a timer to request graceful stop before the deadline. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/InternalAPI/InternalAPI.Unshipped.txt | Tracks newly added env-var constants due to shared-source inclusion. |
| src/Platform/Microsoft.Testing.Extensions.Retry/InternalAPI/InternalAPI.Unshipped.txt | Tracks newly added env-var constants due to shared-source inclusion. |
| src/Platform/Microsoft.Testing.Extensions.MSBuild/InternalAPI.Unshipped.txt | Tracks newly added env-var constants due to shared-source inclusion. |
| src/Platform/Microsoft.Testing.Extensions.HotReload/InternalAPI/InternalAPI.Unshipped.txt | Tracks newly added env-var constants due to shared-source inclusion. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Microsoft.Testing.Extensions.HangDump.csproj | Links DeadlineHelper.cs into HangDump extension for shared deadline parsing. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/InternalAPI/InternalAPI.Unshipped.txt | Tracks DeadlineHelper + env-var constants for this assembly. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs | Arms a new deadline-driven dump timer and prevents double dump via _dumpTaken. |
| @@ -191,6 +203,23 @@ | |||
| null, | |||
| _activityTimerValue!.Value, | |||
| TimeSpan.FromMilliseconds(-1)); | |||
There was a problem hiding this comment.
Fixed in 1ddfd5a. Both timer callbacks now go through a TriggerDumpOnce trampoline that owns the Interlocked one-shot gate and assigns _activityIndicatorTask only for the winning caller. The loser returns without touching it, so it can no longer overwrite the real dump task, and Dispose keeps awaiting the right one.
| _deadlineTimer = new Timer( | ||
| _ => _activityIndicatorTask = TakeDumpOfTreeAsync(cancellationToken), | ||
| null, | ||
| dueTime, | ||
| TimeSpan.FromMilliseconds(-1)); |
There was a problem hiding this comment.
Fixed in 1ddfd5a, same TriggerDumpOnce trampoline. The deadline timer no longer assigns _activityIndicatorTask directly; only the winner of the one-shot gate does.
| // The inactivity timer and the deadline timer can both fire; only dump once. | ||
| if (Interlocked.Exchange(ref _dumpTaken, 1) != 0) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fixed in 1ddfd5a. The deadline path now carries its own reason and its own output message (new HangDumpDeadlineReached resource, xlf regenerated) instead of reusing the inactivity "timeout expired" text.
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. |
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. |
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…-cancellation # Conflicts: # src/Platform/Microsoft.Testing.Extensions.TrxReport/InternalAPI/InternalAPI.Unshipped.txt
Fixes from the review of #10018: - HangDump: the deadline timer and the inactivity timer both wrote _activityIndicatorTask, so the losing one could overwrite the winner's real dump task with a completed no-op, and Dispose would stop waiting for the dump mid-flight. Move the one-shot guard into a TriggerDumpOnce trampoline so only the winning timer assigns _activityIndicatorTask. - HangDump: the deadline path logged "Hang dump timeout expired", which is misleading because no inactivity timeout expired. Give the deadline case its own reason and its own output message (new HangDumpDeadlineReached resource + regenerated xlf). - AbortAtDeadlineExtension: compute a local non-null stopAt instead of dereferencing _stopAt.Value, so there is no nullable deref. - Add the UTF-8 BOM to the three new source files to satisfy the charset=utf-8-bom editorconfig rule. Verified: build 0/0, AbortAtDeadline 5/5, full HangDump suite 30/30. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| _deadlineTimer = new Timer( | ||
| _ => TriggerDumpOnce(cancellationToken, triggeredByDeadline: true), | ||
| null, | ||
| dueTime, | ||
| TimeSpan.FromMilliseconds(-1)); |
There was a problem hiding this comment.
Good catch. I moved the deadline timer so it is armed right after we have the test host process info, before the pipe handshake. Now if the host wedges during startup and never connects back over the pipe, the deadline dump and kill are still armed, which is the case the deadline is meant for. The in-progress-test list needs the consumer pipe, so I made that part best-effort and skip it when the pipe never connected.
🤖
| if (Interlocked.Exchange(ref _dumpTaken, 1) != 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _activityIndicatorTask = TakeDumpOfTreeAsync(cancellationToken, triggeredByDeadline); |
There was a problem hiding this comment.
Right. The Interlocked.Exchange claimed the gate but did not publish _activityIndicatorTask atomically, so disposal could read it as null and tear the pipes down while a dump the timer just started was still running. I now take the dump-once gate and publish the task under one lock, and Dispose/DisposeAsync take the same lock, claim the gate so no new dump can start, and capture the in-flight task to wait on outside the lock.
🤖
|
|
||
| if (DeadlineHelper.TryGetDeadline(environment, out DateTimeOffset deadline) && capability is not null) | ||
| { | ||
| DateTimeOffset stopAt = deadline - DeadlineHelper.GetStopMargin(environment); |
There was a problem hiding this comment.
Agreed. deadline - margin can underflow DateTimeOffset for a very old (but valid) deadline or a large margin. I added DeadlineHelper.SubtractSaturating, which clamps at DateTimeOffset.MinValue, so underflow just means the stop instant is already in the past and we act immediately.
🤖
| // has a chance to complete before the CI runner hard-kills the process. | ||
| if (DeadlineHelper.TryGetDeadline(_environment, out DateTimeOffset deadline)) | ||
| { | ||
| _deadlineDumpAt = deadline - DeadlineHelper.GetDumpMargin(_environment); |
There was a problem hiding this comment.
Same underflow class here, so this path now uses DeadlineHelper.SubtractSaturating too.
🤖
…-cancellation # Conflicts: # src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt
More fixes from the review of #10018: - HangDump: the absolute deadline timer was armed only after the pipe handshake in OnTestHostProcessStartedAsync. A test host that wedges during startup never connects back over the pipe, so those waits blocked past the deadline and the deadline dump/kill was never armed, which is exactly the case the deadline is for. Arm the deadline timer right after we have the test host process info (before the handshake). The dump path only needs the PID; the in-progress-test list needs the consumer pipe, so I make it best-effort and skip it when the pipe never connected. - HangDump: the winning timer published _activityIndicatorTask without any ordering against disposal. Disposal could read the field as null, release, and tear the pipes down while a dump the timer just started was still running. Guard the "take the dump once" gate and the task publish under one lock, and have Dispose/DisposeAsync take that lock, claim the gate so no new dump can start, and capture the in-flight task to wait on outside the lock. The lock is a System.Threading.Lock on net9.0 and an object below it, so it still compiles on netstandard2.0. - AbortAtDeadline and HangDump: deadline - margin on DateTimeOffset throws for a very old (but valid) deadline or a large margin. Add a shared DeadlineHelper.SubtractSaturating that clamps at DateTimeOffset.MinValue, so underflow means "already in the past" -> act immediately. Verified: build 0/0, AbortAtDeadline 5/5, full HangDump suite 30/30. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| } | ||
| } | ||
|
|
||
| public Type[] DataTypesConsumed { get; } = [typeof(TestNodeUpdateMessage)]; |
There was a problem hiding this comment.
Good catch. DataTypesConsumed is now an empty array, so the bus keeps the reference for the run without routing every test result into a no-op ConsumeAsync.
🤖
| return; | ||
| } | ||
|
|
||
| _ = Task.Run(HandleDeadlineAsync); |
There was a problem hiding this comment.
Switched to calling HandleDeadlineAsync directly instead of Task.Run, so single-threaded runtimes (browser/WASI) don't queue work that never runs. The method is async and yields at the first await, so it doesn't block the timer thread.
🤖
| dueTime = TimeSpan.Zero; | ||
| } | ||
|
|
||
| _timer = new Timer(static state => ((AbortAtDeadlineExtension)state!).OnDeadlineReached(), this, dueTime, Timeout.InfiniteTimeSpan); |
There was a problem hiding this comment.
Added a clamp to the Timer maximum (~49.7 days) so the ctor can't throw for a far-future deadline. The run is disposed long before that, so the timer never fires early in practice.
🤖
| _deadlineTimer = new Timer( | ||
| _ => TriggerDumpOnce(cancellationToken, triggeredByDeadline: true), | ||
| null, | ||
| dueTime, | ||
| TimeSpan.FromMilliseconds(-1)); |
| // The consumer pipe is only present once the test host connected back over it. When the host | ||
| // wedged during startup and the deadline dump path fired, there is no pipe to ask for the | ||
| // in-progress tests, so we skip that best-effort list and still take the dump and kill the tree. | ||
| if (_namedPipeClient is not null) |
There was a problem hiding this comment.
Right, a non-null client isn't necessarily connected: it's created when the host sends its pipe name but connected later. I wrapped the in-progress query in try/catch so a deadline dump firing in that window can't block taking the dump and killing the tree.
🤖
| <value>Hang dump file</value> | ||
| </data> | ||
| <data name="HangDumpDeadlineReached" xml:space="preserve"> | ||
| <value>CI deadline reached: taking a hang dump before the run is hard-cancelled.</value> |
There was a problem hiding this comment.
Renamed the resource to HangDumpDeadlineApproaching and reworded the text and log reason. The dump fires at deadline minus the dump margin, so "approaching" is accurate. Regenerated the xlf files.
🤖
| await _logger.LogInformationAsync($"Deadline approaching (stop scheduled at {_stopAt:o}). Requesting graceful stop of test execution.").ConfigureAwait(false); | ||
|
|
||
| CancellationToken cancellationToken = _cancellationTokenSource.CancellationToken; | ||
| await _outputDevice.DisplayAsync( | ||
| this, | ||
| new FormattedTextOutputDeviceData("Deadline approaching: gracefully stopping the test run so reports can be finalized before the CI hard-cancel."), | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| await capability.StopTestExecutionAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Moved the graceful stop into its own try/catch, separate from the best-effort diagnostics, so a logging or output-device failure can't skip the stop. The diagnostics failure log is itself swallowed so it can't re-throw and skip the stop either.
🤖
The automated re-review flagged seven spots after the last push. All are in the two deadline timers and the hang dump resource text. AbortAtDeadlineExtension: - DataTypesConsumed is now empty. The extension only implements IDataConsumer so the message bus keeps a live reference to it (which keeps its timer alive). Returning [TestNodeUpdateMessage] made the bus route every test result to a no-op ConsumeAsync, which is O(test-count) for nothing. - The timer callback calls HandleDeadlineAsync directly instead of Task.Run. On single-threaded runtimes (browser/WASI) Task.Run can queue work that never runs; the method is already async and yields at the first await. - Arming the timer clamps a far-future due time to the Timer maximum (~49.7 days) instead of throwing. The run is disposed long before that, so the timer never fires early in practice. - The graceful stop now runs in its own try/catch, separate from the best-effort diagnostics. A logging or output-device failure can no longer skip the stop, and the diagnostics failure log is itself swallowed so it cannot re-throw and skip the stop either. HangDumpProcessLifetimeHandler: - Same far-future clamp on the deadline dump timer. - The in-progress-test query before dumping is wrapped in try/catch. A non-null pipe client is not necessarily connected (it is created when the host sends its pipe name but connected later), so a deadline dump firing in that window could hit an unconnected pipe. Any failure is logged and swallowed so it cannot block taking the dump and killing the tree. - Renamed the resource HangDumpDeadlineReached to HangDumpDeadlineApproaching and reworded the text and log reason to "approaching". The dump fires at deadline minus the dump margin, so the deadline has not been reached yet. Regenerated the xlf files. Local Debug pack is 0/0. AbortAtDeadline acceptance tests 5/5 and the full HangDump acceptance suite 30/30. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs:459
- This request is not actually best-effort when the host is wedged.
NamedPipeClient.RequestReplyAsyncwaits for a response until the supplied run token is canceled (NamedPipeClient.cs:103-105), so a connected host whose control-pipe callback no longer runs can block here indefinitely and prevent both dump creation and process-tree termination. Skip this query for deadline-triggered dumps or bound it to a short deadline-specific budget.
GetInProgressTestsResponse tests = await _namedPipeClient.RequestReplyAsync<GetInProgressTestsRequest, GetInProgressTestsResponse>(new GetInProgressTestsRequest(), cancellationToken).ConfigureAwait(false);
| } | ||
| } | ||
|
|
||
| public void Dispose() => _timer?.Dispose(); |
There was a problem hiding this comment.
Done in e7786b2 — the handler task is drained on disposal now (bounded blocking wait on netstandard2.0, async drain in DisposeAsync on net), so the deadline callback is not left running after the extension is torn down.
🤖
| await _outputDevice.DisplayAsync( | ||
| this, | ||
| new FormattedTextOutputDeviceData("Deadline approaching: gracefully stopping the test run so reports can be finalized before the CI hard-cancel."), | ||
| _cancellationTokenSource.CancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Done in e7786b2. The message and description are in PlatformResources now (AbortAtDeadlineMessage, AbortAtDeadlineDescription) and regenerated into the locale xlf files.
🤖
| // Prototype defaults. stopMargin > dumpMargin so the graceful stop is attempted first and the | ||
| // hang dump is the fallback for a host that did not stop in time. | ||
| private static readonly TimeSpan DefaultStopMargin = TimeSpan.FromSeconds(60); | ||
| private static readonly TimeSpan DefaultDumpMargin = TimeSpan.FromSeconds(30); |
There was a problem hiding this comment.
Done in e7786b2. DeadlineHelperTests covers the parsing, timezone handling, margin defaults and fallback, and the saturating subtraction.
🤖
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Review — deadline-aware cancellation
I ran this through several review passes (expert MTP reviewer + a bug-focused diff reviewer) and then validated every finding against the code on 14d6b88. Skipping everything the earlier Copilot round already covered and you already fixed.
The shape is good and the concurrency work after the last round (the _dumpLock gate + TriggerDumpOnce trampoline, SubtractSaturating, the timer clamp) reads correctly to me. What follows is what survived validation, roughly in priority order. The first three are the ones I'd want settled before this stops being a prototype.
Things I checked and found clean, so you don't have to re-litigate them: the empty DataTypesConsumed is legal (AsynchronousMessageBus.InitAsync just creates no processor, and the consumer is still disposed via CommonTestHost line 401's messageBus.DataConsumerServices loop); the InternalAPI.Unshipped.txt entries are exact across all five projects that link-compile EnvironmentVariableConstants.cs; the .xlf files are genuinely generated (state="new", source==target, alphabetical); [Embedded] on DeadlineHelper matches every neighbour in that folder; and no new test hard-codes a \(\d+ms\) duration pattern.
|
|
||
| try | ||
| { | ||
| await capability.StopTestExecutionAsync(_cancellationTokenSource.CancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Major — a run truncated by the deadline still exits with code 0.
TestApplicationResult.GetProcessExitCode() derives a non-success code from exactly four signals:
// TestApplicationResult.cs:142-145
exitCode = exitCode == ExitCode.Success && _policiesService.IsMaxFailedTestsTriggered ? ExitCode.TestExecutionStoppedForMaxFailedTests : exitCode;
exitCode = exitCode == ExitCode.Success && _testAdapterTestSessionFailure ? ExitCode.TestAdapterTestSessionFailure : exitCode;
exitCode = exitCode == ExitCode.Success && _failedTestsCount > 0 ? ExitCode.AtLeastOneTestFailed : exitCode;
exitCode = exitCode == ExitCode.Success && _policiesService.IsAbortTriggered ? ExitCode.TestSessionAborted : exitCode;AbortForMaxFailedTestsExtension covers itself by calling _policiesService.ExecuteMaxFailedTestsCallbacksAsync right after StopTestExecutionAsync. This extension never touches IStopPoliciesService, so a 3000-test suite that gets cut off after 400 tests returns 0.
That inverts the value proposition: today the CI hard-cancel at least fails the job. After this change the job goes green on a run that never finished, and the only trace is one console line. AbortAtDeadlineTests currently enshrines it — all five methods assert ExitCode.Success.
Suggest a dedicated stop-policy flag plus a distinct ExitCode (mirroring TestExecutionStoppedForMaxFailedTests), mapped in GetProcessExitCode. If green-on-truncation is a deliberate prototype choice, it needs to be explicit in the XML doc, because nothing in the code hints at it.
There was a problem hiding this comment.
Fixed in e7786b2. A deadline-truncated run no longer returns success. StopPoliciesService now exposes IsDeadlineTriggered, the extension sets it before it asks for the graceful stop, and TestApplicationResult maps that to the new ExitCode.TestExecutionStoppedAtDeadline (15) when the run would otherwise be 0 (TestApplicationResult.cs:183). The acceptance tests assert the 15 now.
🤖
| dataConsumersBuilder.Add(abortForMaxFailedTestsExtension); | ||
| } | ||
|
|
||
| var abortAtDeadlineExtension = new AbortAtDeadlineExtension( |
There was a problem hiding this comment.
Major — in server mode the deadline is sticky and silently zeroes out every subsequent request.
BuildTestFrameworkAsync is invoked per JSON-RPC request, not once per process — see ServerTestHost.RequestExecution.cs:111. So every testing/runTests and testing/discoverTests constructs a fresh AbortAtDeadlineExtension and arms a fresh Timer, all reading the same absolute instant the server process inherited at startup.
Concrete failure: a VS / VS Code test-explorer session (or dotnet test in server mode) launched from a shell that happens to have TESTINGPLATFORM_DEADLINE exported. Once wall-clock passes the deadline, every later request computes dueTime = 0, fires immediately, and requests a graceful stop before anything is scheduled. The user gets "0 tests, success" forever with no way to recover short of restarting the IDE.
Disposal is fine (line 159 of RequestExecution.cs disposes the per-request provider, which walks DataConsumerServices), so this is behavioural rather than a leak.
Also worth noting the same path runs for discovery / --list-tests, where "gracefully stop test execution" doesn't really mean anything.
Suggest gating registration on a single-shot console run, and/or treating a deadline that is already in the past at construction time as "not applicable" (logged) rather than "fire now".
There was a problem hiding this comment.
Fixed in e7786b2. The extension is only constructed for a console run now: the registration is gated on pushOnlyProtocol?.IsServerMode != true && !testFrameworkBuilderData.IsForDiscoveryRequest (TestHostBuilder.Framework.cs:120). Server mode builds the framework per request, so arming a timer against the same absolute instant on every request was exactly the problem you describe.
🤖
| dueTime = MaxTimerDueTime; | ||
| } | ||
|
|
||
| _deadlineTimer = new Timer( |
There was a problem hiding this comment.
Major — arming the timer here means the deadline dump can be taken and then thrown away.
Moving the arming above the handshake is right for the reason in the comment, but nothing downstream was adjusted for the case where the timer actually wins. Sequence:
- Test host wedges during startup, never connects;
_waitConnectionTasknever completes. _deadlineTimerfires.TakeDumpOfTreeAsyncwrites the dumps into_dumpFiles, then itsfinallykills the process tree.- Killing the host does not complete
WaitConnectionAsync, so line 236 keeps waiting the fullTimeoutHelper.DefaultHangTimeSpanTimeoutand then throwsTimeoutException. (TimeoutAfterAsyncthere takes no cancellation token, so nothing can shorten it.) If the connection landed but the consumer pipe name didn't, line 239 / theApplicationStateGuardat 240 throw instead. TestHostControllersTestHost.cs:319awaitsOnTestHostProcessStartedAsyncwith no try/catch, so that exception unwinds straight past theOnTestHostProcessExitedAsyncloop.
Net result in exactly the scenario this feature exists for: the dump files are on disk but never published as FileArtifacts (publication lives only in OnTestHostProcessExitedAsync, lines 299-302), and the process burns another 5+ minutes past the deadline — so CI hard-cancels it anyway.
Two options, not mutually exclusive: publish _dumpFiles from the dump path itself, and/or wrap 235-242 with catch (Exception ex) when (Volatile.Read(ref _dumpTaken) != 0) → log and return, so the controller proceeds to WaitForExitAsync / OnTestHostProcessExitedAsync normally.
There was a problem hiding this comment.
Fixed in e7786b2. When the deadline timer has already taken the dump, OnTestHostProcessStartedAsync now returns instead of throwing: catch (Exception ex) when (Volatile.Read(ref _dumpTaken) != 0) (HangDumpProcessLifetimeHandler.cs:259). That lets the lifetime handler continue to OnTestHostProcessExitedAsync, which is what publishes the artifact, so the deadline dump is no longer collected and then dropped.
🤖
| // Prototype defaults. stopMargin > dumpMargin so the graceful stop is attempted first and the | ||
| // hang dump is the fallback for a host that did not stop in time. | ||
| private static readonly TimeSpan DefaultStopMargin = TimeSpan.FromSeconds(60); | ||
| private static readonly TimeSpan DefaultDumpMargin = TimeSpan.FromSeconds(30); |
There was a problem hiding this comment.
Major — stopMargin > dumpMargin is the invariant the whole design rests on, and it's only a comment about the defaults.
GetStopMargin and GetDumpMargin are read independently, in two different processes, with no cross-check and no warning anywhere.
Set TESTINGPLATFORM_DEADLINE_DUMP_MARGIN=120 (wanting headroom to write a big dump) and leave the stop margin at its 60s default, and the ordering inverts: at deadline-120s the HangDump controller dumps the whole process tree and kills a perfectly healthy run, 60 seconds before the graceful stop would have fired. All reports are lost. TESTINGPLATFORM_DEADLINE_STOP_MARGIN=10 does the same thing from the other side. Neither produces any diagnostic.
Cheapest fix that keeps this honest: resolve both margins in one helper and emit a warning to the output device when dumpMargin >= stopMargin. At minimum, log both resolved values in both processes so it's diagnosable after the fact.
There was a problem hiding this comment.
Fixed in e7786b2. The invariant is checked at runtime now: if the resolved dump margin is not smaller than the stop margin the extension logs a warning, and it also logs the resolved deadline and both margins so the ordering is visible. I kept it a warning rather than a hard error because the run can still proceed; it just means the dump path may fire before the graceful stop.
🤖
| return false; | ||
| } | ||
|
|
||
| if (!DateTimeOffset.TryParse(raw, CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal, out DateTimeOffset parsed)) |
There was a problem hiding this comment.
Major — every misconfiguration is a silent no-op, and an offset-less timestamp is silently reinterpreted as UTC.
Three separate silent failures land here:
- Offset-less input.
AssumeUniversalmeans2026-03-01T14:50:00(no offset, noZ) is taken as UTC. A UTC+2 pipeline doing[DateTime]::Now.AddMinutes(50).ToString("s")therefore hands you an instant that is already two hours in the past.dueTimeclamps to zero, the extension requests a graceful stop during startup, the suite runs ~nothing, and (per the exit-code comment) reports success. Every acceptance test usesDateTimeOffset.UtcNow.ToString("o"), which always carries+00:00, so none of them can catch this. - Typo / malformed date.
TESTINGPLATFORM_DEADLIINE, or a valueTryParserejects, disables the entire safety net with zero output. - Unparsable margin.
GetMarginsilently reverts to the default, soTESTINGPLATFORM_DEADLINE_DUMP_MARGIN=2min(unsupported syntax) quietly becomes 30s.
And a fourth, one level up at line 56: when the framework doesn't expose IGracefulStopTestExecutionCapability — which is every VSTestBridge-based framework — the extension disables itself with no message at all. Compare --maximum-failed-tests, which produces an explicit validation error when the capability is absent.
Suggest: require an explicit offset/Z (or at least log the resolved instant), warn on an unparsable deadline or margin, and log once when a deadline is configured but the framework can't honour it. Right now "nothing happened" and "it's not supported here" are indistinguishable from CI output.
There was a problem hiding this comment.
Improved in e7786b2. A deadline that is set but unparsable now logs a warning instead of being ignored in silence, the case where the framework has no graceful-stop capability logs a warning, and the resolved deadline plus margins are logged. The offset-less string is at least visible now: I parse with AssumeUniversal | AdjustToUniversal and log the resolved instant, so an operator can see whether it landed where they meant. I kept the margin fallback silent by design (a bad margin is not fatal), but its resolved value is in the log too.
🤖
| catch (Exception ex) | ||
| { | ||
| // Best-effort: never let the timer callback crash the process during teardown. | ||
| await _logger.LogErrorAsync("Failed to request graceful stop at deadline.", ex).ConfigureAwait(false); |
There was a problem hiding this comment.
Minor — this is the one logging call in the method that isn't double-guarded, and it can FailFast the process.
Lines 139-146 carefully wrap the recovery log in a nested try/catch, with a comment explaining exactly why. The identical hazard here is bare. StopTestExecutionAsync is a public [TPEXP] extensibility point (a third-party framework can throw anything, and ObjectDisposedException is plausible if the timer fires during teardown); if LogErrorAsync then also throws — the FileLoggerProvider is disposed at CommonTestHost.cs:93, before process end — the exception escapes HandleDeadlineAsync.
Because line 114 discards the task, nobody observes it. With PlatformExitProcessOnUnhandledException / TESTINGPLATFORM_EXIT_PROCESS_ON_UNHANDLED_EXCEPTION=1 set, UnhandledExceptionHandler.OnTaskSchedulerUnobservedTaskException (line 45) calls LogErrorAndExit(..., forceClose: true) → _environment.FailFast(...). So the finalizer thread hard-crashes the process and every result is lost — the exact opposite of what this feature is for.
Two-line fix: wrap this in the same nested try/catch as above. Separately, it'd be worth storing the task in a field so Dispose() can drain it with a bounded wait — Timer.Dispose() doesn't wait for an in-flight callback, so today HandleDeadlineAsync can still be touching _logger / _outputDevice / the capability after their providers are gone.
There was a problem hiding this comment.
Fixed in e7786b2. The stop call and its logging are wrapped the same way as the rest of the handler now, so a throwing logger cannot escape the timer callback and FailFast the process. The handler task is also drained with a bounded wait on disposal (blocking on netstandard2.0, async in DisposeAsync on net) so it is not still touching the logger and output device after teardown.
🤖
| // Claim the gate so no timer callback can start a new dump once we begin tearing down the | ||
| // pipes, and capture any dump already in flight so we wait for it below. | ||
| _dumpTaken = 1; | ||
| activityIndicatorTask = _activityIndicatorTask; |
There was a problem hiding this comment.
Minor — neither Dispose() nor DisposeAsync() disposes _activityTimer / _deadlineTimer.
Both dispose paths claim the _dumpTaken gate (so a late callback is a no-op — good) and dispose the pipes, but the two Timer objects are only disposed on the happy path in OnTestHostProcessExitedAsync (lines 276-292). That method is skipped in at least two reachable cases: its own cancellationToken.ThrowIfCancellationRequested() at line 274 on Ctrl+C, and the exception path I described on the arming block above.
Timer.Dispose() is idempotent, so this is a two-line addition here and in DisposeAsync without touching the existing disposal:
_deadlineTimer?.Dispose();
_activityTimer?.Dispose();There was a problem hiding this comment.
Fixed in e7786b2. Both timers are disposed on every teardown path now — _deadlineTimer?.Dispose(); _activityTimer?.Dispose(); in Dispose (HangDumpProcessLifetimeHandler.cs:584) and again in DisposeAsync (:627), not only on the clean exit path.
🤖
| } | ||
|
|
||
| _dumpTaken = 1; | ||
| _activityIndicatorTask = TakeDumpOfTreeAsync(cancellationToken, triggeredByDeadline); |
There was a problem hiding this comment.
Minor — the dump is invoked under the lock, not just scheduled.
TakeDumpOfTreeAsync(...) runs synchronously up to its first incomplete await. _logger.LogInformationAsync and _outputDisplay.DisplayAsync both return completed tasks in ordinary configurations (no file logger, buffered output device), in which case execution continues under _dumpLock into GetProcessById / GetProcessTreeAsync — real work, on the timer's thread-pool thread.
Meanwhile Dispose() (560) and DisposeAsync() (596) both block on that same lock, on whatever thread the platform tears down on. Not a deadlock, but an unbounded stall on a path that already has its own timeout semantics.
Keep the decision under the lock and start the task outside it — set _dumpTaken inside, then assign _activityIndicatorTask after releasing.
There was a problem hiding this comment.
Fixed in e7786b2. The lock now only claims the gate (sets the flag and stores the task); the dump work itself starts with await Task.Yield() (HangDumpProcessLifetimeHandler.cs:348) so it runs off the lock. Only the one-shot bookkeeping happens under the lock now.
🤖
| /// the stop/dump instant. Saturating means "this instant is already in the past", which for both | ||
| /// callers translates to "act immediately". | ||
| /// </summary> | ||
| public static DateTimeOffset SubtractSaturating(DateTimeOffset instant, TimeSpan margin) |
There was a problem hiding this comment.
Minor — this class has no unit tests, and it's where all the subtle logic lives.
TryGetDeadline (parsing + timezone semantics), GetMargin (fallback behaviour) and SubtractSaturating (underflow clamping) are pure, static, and trivially testable — and test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/ already has the exact precedent in TimeSpanParserTests.cs.
Today they're only exercised indirectly by timing-sensitive acceptance tests that always feed a well-formed DateTimeOffset.UtcNow.ToString("o"), so both the offset-less footgun and the saturation clamp are invisible to CI.
A DeadlineHelperTests.cs with [DataRow] coverage for Z-suffixed / +02:00 / offset-less / empty / garbage deadlines, margins "30" "30s" "1m" "" "abc", and SubtractSaturating(DateTimeOffset.MinValue + 1s, 1h) == MinValue would pin all of it down for a few minutes' work.
(Small aside while you're in here: the parsed >= TimeSpan.Zero guard in GetMargin is dead — TimeSpanParser's pattern is \d+, so it can never produce a negative. Harmless, but it reads as protection that isn't there.)
There was a problem hiding this comment.
Both done in e7786b2. I added DeadlineHelperTests that cover the ISO 8601 parsing, the UTC and offset handling, the margin defaults and the unparsable fallback, and the saturating subtraction. I also dropped the >= TimeSpan.Zero check: TimeSpanParser's regex has no sign, so a parsed margin is always non-negative and the branch was dead. There is a comment saying so now.
🤖
| // Stop margin 0 means the graceful stop is scheduled for the deadline itself, a few | ||
| // seconds out. The framework blocks until the stop is requested, so this proves the | ||
| // timer fires on schedule (not only when the deadline is already past). | ||
| ["TESTINGPLATFORM_DEADLINE"] = DateTimeOffset.UtcNow.AddSeconds(6).ToString("o"), |
There was a problem hiding this comment.
Minor — this test can't fail for the reason it exists.
UtcNow.AddSeconds(6) is evaluated in the test process, but the timer is armed in the child after process launch + runtime startup + the MTP builder runs. On a loaded CI agent that easily exceeds 6s, at which point dueTime clamps to zero and the run takes the "already past" path — which is what WhenDeadlineIsInThePast_GracefullyStopsImmediately already covers. The assertions still pass, so the test silently stops testing "the timer fires on schedule".
Same applies to WhenStopMarginIsSubtracted (60s out, 60s margin → stop instant ≈ now).
If you want this to actually pin the timer down, have the test asset report when the stop arrived (e.g. print elapsed-since-start from StopTestExecutionAsync) and assert it's non-trivially greater than zero.
Also: await GracefulStop.Instance.TCS.Task in the asset has no timeout, so a regression in the extension turns these into hangs rather than failures — the acceptance harness timeout is what would eventually catch it. A WaitAsync/Task.WhenAny with a generous cap would fail fast with a readable message instead.
There was a problem hiding this comment.
Half fixed in e7786b2, and I want to be honest about the other half. The unbounded wait is gone: the asset now waits on Task.WhenAny(tcs.Task, Task.Delay(2min)), so a broken stop path fails the assertions instead of hanging until the harness kills it. I did not add an elapsed-time assertion. The tests already assert the new exit code (15) and the stop message, which is what catches a stop that never happened; a wall-clock timing assertion on top of that is the kind of check this repo warns is flaky, so for the prototype I left it out. Happy to add a generous lower bound if you would rather have it.
🤖
…-cancellation # Conflicts: # src/Platform/Microsoft.Testing.Extensions.HangDump/InternalAPI/InternalAPI.Unshipped.txt # src/Platform/Microsoft.Testing.Extensions.MSBuild/InternalAPI.Unshipped.txt # src/Platform/Microsoft.Testing.Extensions.Retry/InternalAPI/InternalAPI.Unshipped.txt
…ocalization, disposal) This is the review round on the deadline-aware cancellation prototype. The big behavioural change is the exit code: a run that the deadline cuts short no longer reports success. - A deadline-truncated run now returns ExitCode.TestExecutionStoppedAtDeadline (15) instead of 0. The stop policy service gained IsDeadlineTriggered and a deadline callback, mirroring the max-failed-tests path, and the extension sets the flag before it asks for the graceful stop so the exit code cannot be raced by session finalization. Without this a suite that never finished would go green on CI, which is the opposite of what the feature is for. - The in-proc extension is only registered for a console run. Server mode builds the framework per request, so it would re-arm the timer against the same absolute instant on every request and fire immediately once the deadline passed. Discovery requests are skipped too. - The operator-facing description and console message moved into PlatformResources and are regenerated into the 13 locales, matching the HangDump side. - The graceful-stop logging is now wrapped the same way as the rest of the handler so a throwing logger cannot escape the timer callback and FailFast the process, and the handler task is drained with a bounded wait on disposal so it is not still touching the logger and output device after teardown. - DeadlineHelper now logs the resolved deadline and margins, warns when the deadline is set but malformed, warns when the framework has no graceful-stop capability, and warns when the dump margin is not smaller than the stop margin. The offset-less footgun is at least visible in the log now. I removed the dead non-negative margin guard. - HangDump disposes both timers on every teardown path (not only the clean exit), takes the dump outside the dump lock (claim the gate under the lock, yield before the work), and lets the deadline dump be published through the normal exited path by returning from the failed handshake when a dump is already in progress. - Added DeadlineHelperTests covering parsing, timezone handling, margin fallback, and the saturating subtraction, which were only exercised indirectly before. Capped the acceptance asset's wait so a broken stop path fails the assertions fast instead of hanging until the harness times out. Local Debug pack is 0/0. DeadlineHelper unit tests 28/28 and AbortAtDeadline acceptance tests 5/5. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0147c6f-0cfb-4dd6-b420-582b3c7988de
…-cancellation # Conflicts: # src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/InternalAPI/InternalAPI.Unshipped.txt # src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/PACKAGE.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/DeadlineHelperTests.cs:1
- This new C# file is missing the UTF-8 BOM required by
.editorconfig:66-67. Add the BOM so repository encoding checks and future rewrites preserve the required format.
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs:246 - For the startup-wedge case described above, this still blocks until the five-minute default handshake timeout. Killing a host that never connected does not complete the server's
WaitForConnectionAsync, and the controller cannot reachWaitForExitAsync/OnTestHostProcessExitedAsyncto publish the dump before the CI hard deadline. Race or cancel the handshake when the deadline dump wins, then await the dump task before continuing.
await _logger.LogDebugAsync($"Wait for test host connection to the server pipe '{_singleConnectionNamedPipeServer.PipeName.Name}'").ConfigureAwait(false);
await _waitConnectionTask.TimeoutAfterAsync(TimeoutHelper.DefaultHangTimeSpanTimeout).ConfigureAwait(false);
using CancellationTokenSource timeout = new(TimeoutHelper.DefaultHangTimeSpanTimeout);
using var linkedCancellationToken = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeout.Token);
_waitConsumerPipeName.Wait(linkedCancellationToken.Token);
| static Microsoft.Testing.Platform.Resources.PlatformResources.NamedPipeDirectoryNotWritableErrorMessage.get -> string! | ||
| static Microsoft.Testing.Platform.Resources.PlatformResources.NamedPipePathTooLongErrorMessage.get -> string! | ||
| Microsoft.Testing.Platform.Helpers.ExitCode.CoverageThresholdFailed = 14 -> Microsoft.Testing.Platform.Helpers.ExitCode | ||
| Microsoft.Testing.Platform.Helpers.ExitCode.TestExecutionStoppedAtDeadline = 15 -> Microsoft.Testing.Platform.Helpers.ExitCode |
There was a problem hiding this comment.
CtrfReport doesn't track EnvironmentVariableConstants in its InternalAPI the way HangDump/Retry/TrxReport do. Each of those three declares all 33 of that class's members; CtrfReport declares 0 of them and still builds clean -- the linked file was added by #10343 without any of the consts being declared. I verified with a clean -t:Rebuild --no-incremental: 0 warnings, and even removing an already-declared entry (the exit code) produces no RS0016. So the analyzer isn't enforcing that linked class here, and adding just the 3 deadline consts would be inconsistent (3 of 33) without fixing any real break. Happy to add them defensively if you'd prefer the symmetry, but they aren't required.
🤖
| // Task.Run can queue work that never executes. Invoke the async method directly; it is | ||
| // already asynchronous, so it yields at the first await without blocking the timer thread. | ||
| // Store the task so disposal can drain it instead of racing teardown. | ||
| _handleDeadlineTask = HandleDeadlineAsync(); |
There was a problem hiding this comment.
Good catch -- this ordering is real. The timer callback could pass the _disposed check, Dispose could then run to completion and read _handleDeadlineTask while it was still null (so the drain no-ops), and only afterwards would the callback publish the task and run the handler against torn-down services. Fixed in f110c3d: publishing the task and setting/reading _disposed now happen under a shared lock, so the two can't interleave -- either the callback publishes before Dispose captures (and Dispose drains it), or Dispose sets _disposed first and the callback observes it and never starts the handler.
🤖
| // are published; otherwise a deadline dump would be taken but never surfaced as an artifact. | ||
| try | ||
| { | ||
| await _logger.LogDebugAsync($"Wait for test host connection to the server pipe '{_singleConnectionNamedPipeServer.PipeName.Name}'").ConfigureAwait(false); |
There was a problem hiding this comment.
This one is a false positive. _singleConnectionNamedPipeServer is assigned once (line 160) and from then on is only ever ?.Dispose()d (lines 617/654) -- it's never set back to null. There's also no await between the ApplicationStateGuard.Ensure(... is not null) on line 204 and the use on line 242, so nothing can null it in between. The compiler agrees (clean build, no CS8602 here), and lines 168-169 already dereference the same field directly after a similar guard. So there's no reachable null to capture into a local.
🤖
The timer callback could pass the _disposed check, then Dispose could run to completion and read _handleDeadlineTask while it was still null (draining nothing), and only afterwards would the callback publish the task and run the handler against torn-down services. Publish the task and set/read _disposed under a shared lock so the two cannot interleave: either the callback publishes before Dispose captures it (Dispose then drains it), or Dispose sets _disposed first and the callback observes it and never starts the handler. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0147c6f-0cfb-4dd6-b420-582b3c7988de
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/DeadlineHelperTests.cs:1
- This new C# file is missing the repository-required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor*.cs; save the file with that encoding so formatting/encoding checks do not fail.
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.Framework.cs:120 - This condition disables graceful deadline handling for every
dotnet testserver-mode execution (DotnetTestConnection.IsServerModeis true once that transport connects). That is the normal SDK/CI path described by this PR, yet the acceptance coverage only launches the apphost directly, so those runs silently ignoreTESTINGPLATFORM_DEADLINE. The timer needs once-per-process/server coordination rather than skipping server mode altogether, with adotnet testacceptance case.
src/Platform/Microsoft.Testing.Platform/Extensions/AbortAtDeadlineExtension.cs:254 IsDeadlineTriggeredis committed before the framework accepts the stop request. If a framework capability throws, the catch deliberately lets execution continue, but the eventual result is still exit code 15 even though execution was never stopped at the deadline. Only commit the deadline verdict after a successful stop request, or add rollback/atomic state handling that preserves the finalization race described here without reporting a false truncation.
| if (_namedPipeClient is not null) | ||
| { | ||
| string hangTestsFileName = Path.ChangeExtension(finalDumpFileName, ".log"); | ||
| using (FileStream fs = File.OpenWrite(hangTestsFileName)) | ||
| using (StreamWriter sw = new(fs)) | ||
| try | ||
| { | ||
| await _outputDisplay.DisplayAsync(new ErrorMessageOutputDeviceData(ExtensionResources.RunningTestsWhileDumping), cancellationToken).ConfigureAwait(false); | ||
| foreach ((string testName, int seconds) in tests.Tests) | ||
| GetInProgressTestsResponse tests = await _namedPipeClient.RequestReplyAsync<GetInProgressTestsRequest, GetInProgressTestsResponse>(new GetInProgressTestsRequest(), cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Real catch, and it's the one path that matters most -- a connected-but-wedged host is exactly the scenario the deadline dump exists for. RequestReplyAsync was waiting on the application token, which isn't cancelled while the run is still "in progress", so a host that connected its pipe then stopped replying would block the dump and kill indefinitely and eat the whole (default 30s) dump margin. Fixed in dba2383: the query now runs under a linked CTS bounded to 5s (CancelAfter), so on a wedged host it cancels out fast, surfaces as OperationCanceledException, and is swallowed like any other query failure -- the dump and tree-kill proceed. The healthy path answers in milliseconds, well under the bound, so the in-progress list is still collected when the host is responsive.
🤖
The in-progress-test list gathered before a hang dump was only best-effort against failures, not against a connected-but-wedged host. RequestReplyAsync waited on the application token, which is not cancelled while the run is still "in progress" -- exactly when the deadline dump fires -- so a host that connected its pipe but then stopped replying would block the dump and kill indefinitely and consume the whole (default 30s) dump margin, producing no dump. Wrap the query in a linked CTS bounded to 5s so the metadata collection can never eat the margin; the timeout surfaces as OperationCanceledException and is swallowed like any other query failure, letting the dump and tree-kill proceed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0147c6f-0cfb-4dd6-b420-582b3c7988de
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/DeadlineHelperTests.cs:1
- This file is missing the UTF-8 BOM required for C# files by
.editorconfig:66-67. Add the BOM so the new test file complies with the repository encoding rule.
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs:494 - The in-progress-test query is inside
TakeDumpAsync, which is called once per process in the tree. For a connected but wedged host, every process therefore incurs this 5-second timeout; with six processes the default 30-second dump margin is exhausted before the root test host (dumped last) is reached. Query the host once per dump operation and reuse the result when writing any test-list artifact so optional metadata consumes at most one timeout.
using var queryCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
queryCts.CancelAfter(InProgressTestsQueryTimeout);
GetInProgressTestsResponse tests = await _namedPipeClient.RequestReplyAsync<GetInProgressTestsRequest, GetInProgressTestsResponse>(new GetInProgressTestsRequest(), queryCts.Token).ConfigureAwait(false);
| await _policiesService.ExecuteDeadlineCallbacksAsync().ConfigureAwait(false); | ||
|
|
||
| await capability.StopTestExecutionAsync(_cancellationTokenSource.CancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Good catch, this was a real hole. Fixed in c693499.
AbortAtDeadlineExtension now also implements ITestSessionLifetimeHandler. OnTestSessionFinishingAsync runs right after the framework invoker returns and before the reporters render, and it sets an _executionCompleted flag under the same lock the timer callback and Dispose use. OnDeadlineReached checks that flag on its fast-path and again inside the lock, so a timer that fires while TRX/HTML/AzDO are finalizing an already-finished run is a no-op instead of forcing exit code 15.
It never un-marks a genuine mid-execution truncation: if the timer already fired during execution, IsDeadlineTriggered is already set and flipping _executionCompleted afterwards is harmless.
Wiring-wise the extension is added to testSessionLifetimeHandlers in TestHostBuilder, and the TestSessionLifetimeHandlersContainer registration moved below that so the container includes it.
🤖
The deadline timer was armed at construction and only disarmed on Dispose, so if it fired after the run had already finished -- while the reporters were still finalizing TRX/HTML/AzDO output -- a fully-completed run wrongly exited with code 15 and reported that tests stopped early. AbortAtDeadlineExtension now also implements ITestSessionLifetimeHandler. When OnTestSessionFinishingAsync fires (right after the framework invoker returns, before the reporters render), it sets an _executionCompleted flag under the same lock the timer callback and Dispose use. OnDeadlineReached checks that flag on its fast-path and again inside the lock, so a late timer fire during reporting is a no-op. It never un-marks a real mid-execution truncation: if the timer already fired during execution, IsDeadlineTriggered is already set and setting the flag afterwards is harmless. Wire the extension into testSessionLifetimeHandlers in TestHostBuilder and move the TestSessionLifetimeHandlersContainer registration below that so the container includes it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0147c6f-0cfb-4dd6-b420-582b3c7988de
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/Platform/Microsoft.Testing.Platform/Extensions/AbortAtDeadlineExtension.cs:292
- The deadline flag is committed before the graceful-stop request succeeds. If
StopTestExecutionAsyncthrows (an error this method explicitly catches), execution can continue to completion, butTestApplicationResultwill still returnTestExecutionStoppedAtDeadlineand reporters will claim execution stopped early. Record the deadline outcome only once the capability has accepted the stop, or add rollback/atomic coordination so a failed request cannot leave the truncated-run verdict set.
| private async Task HandleDeadlineAsync() | ||
| { | ||
| if (_capability is not { } capability) |
There was a problem hiding this comment.
Good catch. You're right: if every await is already completed (empty deadline-callback queue, no-op logger/output device, a synchronous graceful-stop implementation) the async method runs to completion synchronously, and here that means under _lock -- which can deadlock against OnTestSessionFinishingAsync/Dispose, both of which take the same lock.
Fixed by forcing the boundary: HandleDeadlineAsync now starts with await Task.Yield(), so it always hands an incomplete task back to OnDeadlineReached and _lock is released before any handler work runs -- the same pattern TakeDumpOfTreeAsync uses. I also corrected the comment in OnDeadlineReached that claimed the method "already yields at the first await", which was exactly the wrong assumption you flagged.
Task.Yield posts the continuation back to the current context, so it stays cooperative on a single-threaded runtime (browser/WASI) -- that's why I kept away from Task.Run here.
🤖
HandleDeadlineAsync is started synchronously inside _lock and its returned task is published to _handleDeadlineTask. An async method does not necessarily yield at its first await: the logger, output device, the empty deadline-callback queue and the framework graceful-stop capability can all return already-completed tasks, which would run the whole handler synchronously while _lock is held. If the graceful stop then synchronously waited on session finishing or disposal (both take _lock), that would deadlock. Yield immediately (await Task.Yield()) at the top of HandleDeadlineAsync, matching HangDumpProcessLifetimeHandler.TakeDumpOfTreeAsync, so control returns to the caller, the task is observed, and _lock is released before any handler work runs. Task.Yield posts the continuation back to the current context, so it is cooperative and safe on a single-threaded runtime (browser/WASI), unlike Task.Run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0147c6f-0cfb-4dd6-b420-582b3c7988de
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/Platform/Microsoft.Testing.Platform/Extensions/AbortAtDeadlineExtension.cs:196
- This completion signal is too late to prevent false deadline failures.
CommonTestHost.NotifyTestSessionEndAsyncfirst drains the message bus (CommonTestHost.cs:399-402), and because this handler is anIDataConsumer, it is not invoked until the consumer pass (CommonTestHost.cs:450-461). The framework can therefore finish all tests, then the timer can fire during result/report draining and force exit code 15. Signal execution completion immediately after the framework invoker returns instead.
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/DeadlineHelperTests.cs:1 - This new C# file is missing the UTF-8 BOM required by
.editorconfig:66-67; the other new C# files preserve it. Add the BOM so encoding checks and future rewrites follow the repository convention.
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs:494 - The five-second limit is applied once per process because
TakeDumpAsyncis called sequentially for every process inbottomUpTree(lines 401-405). For a connected-but-wedged host with six processes, these repeated queries can consume the entire default 30-second margin before reaching the parent-host dump and kill. Query the host once before the process loop, or enforce one shared overall query budget.
using var queryCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
queryCts.CancelAfter(InProgressTestsQueryTimeout);
GetInProgressTestsResponse tests = await _namedPipeClient.RequestReplyAsync<GetInProgressTestsRequest, GetInProgressTestsResponse>(new GetInProgressTestsRequest(), queryCts.Token).ConfigureAwait(false);
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs:370
- These diagnostics execute before the dump path's
try/finally. If either the logger or output provider throws, the task faults before acquiring the process tree and before the kill cleanup, so the deadline fallback produces neither a dump nor an early kill. Make diagnostics best-effort and ensure the kill fallback still runs when reporting fails.
await _logger.LogInformationAsync($"{dumpReason}. Taking hang dump.").ConfigureAwait(false);
await _outputDisplay.DisplayAsync(
new ErrorMessageOutputDeviceData(triggeredByDeadline
? ExtensionResources.HangDumpDeadlineApproaching
: string.Format(CultureInfo.InvariantCulture, ExtensionResources.HangDumpTimeoutExpired, _activityTimerValue)),
src/Platform/Microsoft.Testing.Platform/Services/StopPoliciesService.cs:71
- The new deadline callback path has no matching unit coverage, although
StopPoliciesServiceTestsdirectly covers the equivalent max-failed-tests and abort state/callback/late-registration behavior. Add deadline-policy tests so callback invocation andIsDeadlineTriggeredsemantics are protected independently of the acceptance test's empty callback queue.
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsExitCode.cs:63 - This new reason mapping is not exercised by
GitHubActionsExitCodeTests, which already verifies the neighboring known mappings for exit codes 9, 13, and 14. Add exit code 15 to those tests so a missing resource mapping cannot silently fall back to the generic reason.
(int)ExitCode.TestExecutionStoppedAtDeadline => GitHubActionsResources.ExitCodeReasonTestExecutionStoppedAtDeadline,
CI hard-cancels a job at a fixed wall-clock time. When it fires the runner kills the test process, and we lose the TRX/HTML/AzDO reports and get no dump for a hanging test. This is a prototype that tells MTP when that deadline is, so it can react a little early while it still owns its shutdown.
What it does
The deadline arrives as an environment variable, an absolute instant:
TESTINGPLATFORM_DEADLINE: ISO 8601, parsed to UTC.TESTINGPLATFORM_DEADLINE_STOP_MARGIN: lead time for the graceful stop. Default 60s.TESTINGPLATFORM_DEADLINE_DUMP_MARGIN: lead time for the hang dump. Default 30s.Two reactions, armed off the same instant:
deadline - stopMarginan in-process extension asks the framework to gracefully stop scheduling new tests (IGracefulStopTestExecutionCapability). In-flight tests finish, the session ends normally, and every reporter gets to finalize.deadline - dumpMarginthe out-of-process HangDump controller dumps the process tree and kills the host. This is the fallback for a wedged host that never reaches the graceful stop. It reuses the controller that HangDump already runs, so it costs nothing extra when--hangdumpis on.stopMargin > dumpMarginon purpose: try the clean stop first, dump only if that did not happen in time.It is opt-in. No deadline set, both timers stay unarmed, no behavior change. The graceful stop also degrades to a no-op if the framework does not expose the capability.
Where the deadline comes from
Out of scope here, that is the "solve timing later" part. MTP has no hardcoded timeout, it only reacts to whatever instant the environment gives it. The CI side is a few lines of YAML that compute
job start + timeoutand export it. Rough sketches:GitHub Actions (job has
timeout-minutes: 60):Azure DevOps (job has
timeoutInMinutes: 60):Both approximate "now" as the job start, which is close enough. The nicer version is Arcade computing the real deadline once and exporting it for everyone, so nobody has to copy YAML around. Left for a follow-up.
Verified
build.cmd -packpasses 0/0. New acceptance tests:AbortAtDeadlineTests(5): past deadline stops immediately, future deadline stops when the timer fires, the stop margin is subtracted from the deadline, no deadline stays silent, missing capability is a no-op.HangDumpTests.HangDump_AbsoluteDeadline_CreateDump(3 tfms): 30 minute inactivity timeout so only the deadline path can fire, and it produces a dump. FullHangDumpTestsstill 30/30.This is a prototype, so LMK what you think about the shape before I polish it. Open questions: