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. |
![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>
…-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>
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);
Amaury Levé (Evangelink)
left a comment
Amaury Levé (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.
…-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);
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
This comment has been minimized.
This comment has been minimized.
Amaury Levé (Evangelink)
left a comment
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Reviewed the current head after the latest feedback fixes. The deadline, HangDump, server/hot-reload, configuration propagation, API baseline, localization, and regression coverage all look sound.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686 🤖
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 112.1 AIC · ⌖ 2.42 AIC · ⊞ 16.9K · ◷
This comment has been minimized.
This comment has been minimized.
The previous required build failed while Azure DevOps was unavailable during coverage publication. Retry validation on an unchanged tree. 🤖 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Require the exit-reason regression test to identify the deadline explicitly instead of accepting any distinct non-empty text. 🤖 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686
Added this in 450e32143. The regression test now requires the reason to contain 🤖 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 95 out of 95 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/UnitTests/MSTestAdapter.UnitTests/MSTestGracefulStopTestExecutionCapabilityTests.cs:44
- Always complete the current capability in
finally. This test mutates process-wide static counters; if any assertion before the explicit completion fails, the leaked active/pending owner can contaminate later tests and cause cascading failures.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686 🤖
This comment has been minimized.
This comment has been minimized.
Fixed in 3e2bc90a4. The test now completes whichever capability is current from 🤖 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 95 out of 95 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/AbortAtDeadlineTests.cs:32
- These acceptance assertions cover the console summary, but not the PR's primary promise that deadline-driven graceful shutdown lets TRX/HTML/Azure DevOps reporters finalize. A regression that stops tests correctly but tears down before a reporter writes or publishes its final artifact would still pass. Please add an acceptance case that enables at least the relevant file reporters and verifies their completed artifacts after the deadline stop (and cover the Azure DevOps finalization path with its existing test infrastructure).
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 224.1 AIC · ⌖ 1.36 AIC · ⊞ 16.9K · ◷
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686 🤖
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686 🤖
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686 🤖
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aed1b93b-c485-4242-acb5-ffda4d6c7686 🤖
Added in cb606ef04. The Hot Reload deadline test now enables the HTML reporter, verifies the report exists after shutdown, and checks the completed HTML structure and embedded report data. The focused acceptance test passes. The same commit also isolates 🤖 |
🧪 Expert test review — PR #10018No new or modified test methods were identified in the changed regions of this PR (the deterministic file-extraction step produced no changed test files to analyze). Re-run with
|
🧵 Parallel-safety audit — PR #10018Parallelization — one row per test assembly audited:
Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 1 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 1. I audited all 12 changed/added test files in this PR ( Highlights of what I checked and ruled out:
Info
Advisory only — heuristic, non-blocking. Re-run with
|
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 environment variables:
TESTINGPLATFORM_DEADLINE: the CI hard-cancel instant in ISO 8601 format, 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
The CI producer must export the actual hard-cancel instant. MTP does not derive the job timeout, and the producer must not subtract the stop or dump margin. MTP applies both margins after reading the deadline.
The preferred version is Arcade or the job orchestrator computing the hard-cancel instant once, before the job starts, and exporting it for every test process.
If the CI system does not expose that instant, the fallback is to compute
now + the full job timeoutin the first job step, before checkout, restore, build, or test work. For a 60-minute timeout:GitHub Actions:
Azure DevOps:
These fallback examples approximate the job start with the first step. Any delay before the first step reduces the real time available, so production CI should prefer the orchestrator-provided instant or subtract a small orchestration buffer. The 60s/30s MTP margins remain separate and provide the graceful-stop and dump lead time.
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:
🤖