Add HangDump disposal path tests

[Amaury Levé (Evangelink)]

HangDumpProcessLifetimeHandler teardown owns timers, dump coordination, and named-pipe resources, but its synchronous and asynchronous disposal paths had no direct unit coverage.

This adds deterministic coverage for cleanup and gate claiming, waiting for in-flight dumps, timeout and fault propagation/reporting, repeated mixed disposal, and late timer callbacks. A constructor-injected internal timeout keeps timeout tests immediate while retaining the existing production default.

Fixes #11024

[Copilot]

One or more custom setup steps configured for this repository failed during this Copilot code review run:

Build the repository

Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review.

Note

You can configure setup steps for Copilot code review separately from Copilot cloud agent with a copilot-code-review.yml file. Read the docs for details.

[Copilot]

Copilot review overview

🟡 Changes recommended

The tracked constructor API baseline is missing, and named-pipe disposal remains unverified.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 1

New issues introduced by this change (2)

Severity	Finding
	src/​Platform/​Microsoft.Testing.Extensions.HangDump/​HangDumpProcessLifetimeHandler.cs — This changes a tracked internal constructor signature without updating the API baselines.…
	test/​UnitTests/​Microsoft.Testing.Extensions.UnitTests/​HangDumpTests.cs — The happy-path test leaves _namedPipeClient and _singleConnectionNamedPipeServer null, so it…

What changed in this PR

Adds disposal-path coverage for HangDumpProcessLifetimeHandler, addressing #11024.

Changes:

• Adds configurable disposal timeout.
• Tests synchronous/asynchronous cleanup, failures, timeouts, idempotency, and timer callbacks.

File	Description
HangDumpTests.cs	Adds disposal tests and helpers.
HangDumpProcessLifetimeHandler.Disposal.cs	Uses the injected timeout.
HangDumpProcessLifetimeHandler.cs	Adds timeout constructor parameter.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

#	Dimension	Verdict
1	Algorithmic Correctness	✅ Clean
2	Threading & Concurrency	✅ Clean
3	Public API	✅ Clean — optional parameter with default null preserves all existing call sites
4	Cross-TFM	✅ Clean — #if NETCOREAPP guard on the using is correct (only TimeoutAfterAsync needs it)
5	Performance	✅ N/A
6	Error Handling	✅ Clean
7	IPC & Serialization	N/A
8	Localization	N/A
9	Test Quality	✅ Clean — good coverage of happy path, timeout, faulted, idempotent, and race-after-gate scenarios
10	Security	N/A
11	Naming & Readability	✅ Clean
12	Disposable & Lifetime	✅ Clean
13	Configuration	N/A
14	Logging & Telemetry	N/A
15	Documentation	✅ Clean
16	Backward Compatibility	✅ Clean — optional constructor parameter preserves binary compat for internal type
17	Build & CI	N/A
18	Analyzers & Code Fixes	N/A
19	Dependency Management	N/A
20	PublicAPI.Unshipped.txt	✅ N/A — type is internal
21	Scope Discipline	✅ Clean — single concern (testable dispose timeout)
22	init Accessor Ban	✅ Clean

Overall: No blocking or major issues found. The change is well-scoped: it makes HangDumpProcessLifetimeHandler's dispose timeout injectable for testing while keeping the production default unchanged, and adds thorough unit tests for every disposal code path.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The asynchronous happy path does not verify resource disposal on a fresh handler.

Review tier: Balanced
Findings: None

Issues resolved since last review (2)

Severity	Finding
	test/​UnitTests/​Microsoft.Testing.Extensions.UnitTests/​HangDumpTests.cs — The happy-path test leaves _namedPipeClient and _singleConnectionNamedPipeServer null, so it… View resolved comment
	src/​Platform/​Microsoft.Testing.Extensions.HangDump/​HangDumpProcessLifetimeHandler.cs — This changes a tracked internal constructor signature without updating the API baselines.… View resolved comment

Suppressed comments (1)

test/UnitTests/Microsoft.Testing.Extensions.UnitTests/HangDumpTests.cs:686

• This DisposeAsync call runs only after Dispose has already torn down every resource, while the other async tests exit through exception paths before cleanup. Consequently, removing the async named-pipe and wait-handle disposal at HangDumpProcessLifetimeHandler.Disposal.cs:60-62 would leave every new test passing. Please add a fresh-handler async happy-path test that populates those resources and verifies they are disposed before using this mixed-call test for idempotency.

        await handler.DisposeAsync();

[Copilot]

One or more custom setup steps configured for this repository failed during this Copilot code review run:

Build the repository

Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review.

Note

You can configure setup steps for Copilot code review separately from Copilot cloud agent with a copilot-code-review.yml file. Read the docs for details.

[github-actions]

🧪 Expert test review — PR #11039

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new HangDumpTests. Dispose_ RepeatedAndMixedSyncAsyncCalls_ AreIdempotent	1/2 killed	Only checks _dumpTaken; doesn't assert pipe/timer fields stay safely disposed across repeats.	Also assert repeated calls don't throw ObjectDisposedException and that _activityIndicatorTask state is stable.
A (90–100)	new HangDumpTests. Dispose_ CompletedDump_ StopsTimersClaimsGateAndDisposesResources	5/5 killed	Verifies gate claim, both timers disposed, and both pipe endpoints actually torn down.	—
A (90–100)	new HangDumpTests. Dispose_ DumpInFlight_ WaitsForCompletion	3/3 killed	Proves Dispose blocks on an in-flight dump task and only returns after it completes.	—
A (90–100)	new HangDumpTests. Dispose_ FaultedDump_ ReportsAndRethrowsAggregateException	3/3 killed	Confirms the sync path reports via output device and rethrows the wrapping AggregateException.	—
A (90–100)	new HangDumpTests. Dispose_ InFlightDumpExceedsTimeout_ ThrowsWithoutProductionDelay	3/3 killed	Uses a zero dispose timeout for a deterministic, non-flaky timeout assertion with exact message text.	—
A (90–100)	new HangDumpTests. Dispose_ TimerCallbackArrivingAfterGateClaim_ DoesNotStartDump	2/2 killed	Directly exercises the race-guard gate via reflection and asserts no new dump task is published.	—
A (90–100)	new HangDumpTests. DisposeAsync_ FaultedDump_ ReportsAndRethrowsOriginalException	3/3 killed	Verifies the async path rethrows the original (unwrapped) exception, unlike the sync AggregateException path.	—
A (90–100)	new HangDumpTests. DisposeAsync_ InFlightDumpExceedsTimeout_ ReportsAndThrowsWithoutProductionDelay	3/3 killed	Deterministically times out via zero dispose timeout and checks both reported and thrown exception content.	—

_{This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Suggestions on the Files
changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 76.7 AIC · ⌖ 2.72 AIC · ⊞ 16.9K · [◷]( · ◷)

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 76.7 AIC · ⌖ 2.72 AIC · ⊞ 16.9K · ◷

[Copilot]

Copilot review overview

🟢 Approval recommended

The disposal coverage is comprehensive and deterministic; only minor error-message wording remains.

Review tier: Balanced
Findings: None

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.Disposal.cs:33

• TimeSpan interpolation already renders a duration (for example, 00:00:00), so appending seconds produces the confusing message “didn't exit in 00:00:00 seconds”; the new assertion also locks this wording in. Format TotalSeconds with invariant culture, or remove the seconds suffix, and update the corresponding test expectation.

[Copilot]

Copilot review overview

🟢 Approval recommended

The changes provide comprehensive deterministic coverage without altering the public or tracked internal API.

Review tier: Balanced
Findings: None

[github-actions]

🧪 Expert test review — PR #11039

Grade	Test	Mutation	Notes	How to improve
A (90–100)	new HangDumpTests. Dispose_TimerCallbackArrivingAfterGateClaim_ DoesNotStartDump	2/2 killed	Verifies the post-dispose gate short-circuit: dump count stays claimed and no task is published.	—
A (90–100)	new HangDumpTests. Dispose_FaultedDump_ ReportsAndRethrowsAggregateException	3/3 killed	Confirms the sync path wraps the faulted task in AggregateException and reports the inner message.	—
A (90–100)	new HangDumpTests. DisposeAsync_FaultedDump_ ReportsAndRethrowsOriginalException	3/3 killed	Confirms the async path rethrows the original (unwrapped) exception and reports it.	—
A (90–100)	new HangDumpTests. Dispose_InFlightDumpExceedsTimeout_ ThrowsWithoutProductionDelay	2/2 killed	Injects TimeSpan.Zero via the new _disposeTimeout seam to assert timeout message deterministically and fast.	—
A (90–100)	new HangDumpTests. DisposeAsync_InFlightDumpExceedsTimeout_ ReportsAndThrowsWithoutProductionDelay	3/3 killed	Verifies async path throws TimeoutException (vs. sync's InvalidOperationException) and reports it, matching TimeoutAfterAsync's real behavior.	—
A (90–100)	new HangDumpTests. Dispose_DumpInFlight_ WaitsForCompletion	2/2 killed	Uses a real background thread and SpinWait to prove Dispose() blocks until the in-flight dump completes.	—
A (90–100)	new HangDumpTests. Dispose_CompletedDump_ StopsTimersClaimsGateAndDisposesResources	4/4 killed	Covers the full happy-path contract: gate claimed, both timers disposed, and both named-pipe endpoints disposed.	—
A (90–100)	new HangDumpTests. DisposeAsync_CompletedDump_ DisposesResources	3/3 killed	Async counterpart of the happy-path test; asserts both pipes are disposed after DisposeAsync().	—
A (90–100)	new HangDumpTests. Dispose_RepeatedAndMixedSyncAsyncCalls_ AreIdempotent	3/3 killed	Strengthened after earlier review round to use real pipes/timers and assert the original completed task is preserved across mixed sync/async re-entry.	—

All 9 new tests target the _disposeTimeout seam and the _dumpLock/_dumpTaken gate this PR touches, and each asserts a distinct, meaningful facet of the disposal contract (gate claim, timer/pipe teardown, exception wrapping vs. rethrow, timeout wording, and idempotency). Three high-confidence findings from earlier automated review rounds on this PR (missing pipe-disposal assertions in the happy-path test, a weaker idempotency assertion, and the API-baseline/readonly production concerns) are already resolved in the current revision — no new actionable findings identified in this pass, so no inline suggestions were posted.

_{This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Suggestions on the Files
changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 208.3 AIC · ⌖ 2.52 AIC · ⊞ 16.9K · [◷]( · ◷)

[github-actions]

Pipeline Test Triage — Final Resolution

Build: [1582858]((dev.azure.com/redacted) (completed, result: failed) · Check: microsoft.testfx · PR: #11039

Summary

Across 99 candidate test results (all matrix legs: Linux/macOS/Windows × Debug/Release × net8.0/net462), exactly one test failure was found:

• Test: Dispose_FromANotificationHandler_DoesNotSelfWaitOnTheReadLoop
  (Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests)
• Failed leg: Windows Release, net462 only
• All other legs passed, including the same test on Windows Release net8.0, Windows Debug net462/net8.0, Linux, and macOS (durations 11–136ms).
• Failure detail: Assert.IsLessThan(TimeSpan.FromSeconds(4), elapsed) — dispose-from-notification-handler took 6969ms, exceeding the 4000ms upper bound.
• No retry attempts recorded for this failure (CTRF retryAttempts: [], flaky: null), so this is not proven flaky (no observed fail-then-pass in the same run).

Historical correlation

history.json covers up to 12 prior main/scheduled builds in the last 30 days (incomplete: true, so absence elsewhere is not conclusive). This same test:

• Failed once previously in build 1582671 (same net462/Windows-Release leg) but passed in 10 of the other 11 sampled builds, including immediately before/after.

This pattern (isolated timing-margin failure on a single matrix leg, no reproduction on the same PR's other legs, occasional historical occurrence with mostly-passing history) does not meet the durable issue threshold: it hasn't shown fail-then-pass recovery within the same run, and it hasn't recurred across ≥2 independent main/scheduled builds in immediate succession — it's an intermittent timing-sensitive assertion (6969ms vs a 4000ms bound) most consistent with an environment/agent timing variance on Windows/net462 rather than a deterministic product regression.

Conclusion

No durable defect signature established. Not creating an issue at this time — if this failure recurs on additional unrelated builds/commits, or a fail→pass retry pattern emerges, it would meet the flaky-test threshold and warrant an issue with type/flaky-test.

Next step if it recurs: consider loosening the timing assertion margin or adding CTRF retry to distinguish transient scheduler delay from a genuine regression in MtpServerClientInProcessTests.Dispose_FromANotificationHandler_DoesNotSelfWaitOnTheReadLoop.

🤖 Automated content by GitHub Copilot. Generated by the Pipeline Test Triage workflow. · auto · 61.5 AIC · ⌖ 1.84 AIC · ⊞ 9.2K · [◷]( · ◷)

[github-actions]

🧵 Parallel-safety audit — PR #11039

Parallelization — assembly audited:

Test assembly	Scope	Workers	Analyzer coverage
Microsoft.Testing.Extensions.UnitTests	MethodLevel	0 (CPU count)	coverable once the parallel-safety analyzers ship (assembly attribute in Program.cs)

Under MethodLevel, every test method is its own concurrency chunk — intra-class and cross-class conflicts are both live, so this audit judged the new tests at full severity, not readiness-only.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found. The new Dispose_* / DisposeAsync_* tests in HangDumpTests.cs:

• Each construct their own HangDumpProcessLifetimeHandler instance via CreateHandler(...), with all dependencies mocked (ILoggerFactory, IOutputDevice, IClock, IProcessHandler, ITask, IEnvironment) — no shared production singleton is touched.
• Use NamedPipeServerEndpoint($"hang_{Guid.NewGuid():N}") for every pipe, so no two tests (or concurrent runs) can collide on the same named-pipe OS resource.
• Mutate only private instance fields (_activityIndicatorTask, _deadlineTimer, _disposeTimeout, ...) on their own handler via reflection helpers (SetHandlerField/GetHandlerField) — never a static field, environment variable, current-directory, console stream, or culture setting.
• Do not write to any shared filesystem path; no Path.Combine/File.*/Directory.* calls appear in the diff.
• Add no [ResourceLock] or [DoNotParallelize] declarations, and none pre-exist on this class, so there is nothing to reconcile (category C) and no over-serialization (category D) to report.

The only pre-existing shared state in the production type (HangDumpProcessLifetimeHandler.InProgressTestsQueryTimeout / BestEffortDiagnosticsTimeout) are static readonly TimeSpan constants, never mutated — not a hazard.

Nothing to flag for parallel-safety in this PR.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 73.2 AIC · ⌖ 3.3 AIC · ⊞ 24.8K · [◷]( · ◷)