Skip to content

Bound local test partitions and collect hang dumps across platforms - #85849

Open
jaredpar wants to merge 25 commits into
dotnet:mainfrom
jaredpar:jaredpar-local-test-timeouts
Open

jaredpar wants to merge 25 commits into
dotnet:mainfrom
jaredpar:jaredpar-local-test-timeouts

Conversation

@jaredpar

@jaredpar jaredpar commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Preserve the fixed 25-minute local VSTest inactivity timeout and the 90-minute global timeout trigger. Local work items remain whole assemblies without a separate deadline; Helix retains its own infrastructure deadlines and 15-minute VSTest setting.
  • On global timeout, write failure results, collect dumps of active launcher trees on Windows and Linux, then terminate them. Ctrl+C stops tests without initiating dumps.
  • Dump APIs run in helper processes to isolate native failures and concurrent DbgHelp calls. Collection has no timeout; a stuck helper still requires external termination.
  • Keep diagnostics scoped to each work-item invocation. Preserve early synthetic failures rather than overwriting them. Capture screenshots only in the integration-test script.
  • No new timeout options, process-tracking abstraction, checked-in test project, or permanent hang probe.

Review updates and local validation

fad07f9c2d7 addresses all seven comments from @dibarbet: shorter README, removal of unused ProcessRunner cancellation plumbing, WaitAsync for executor cancellation, required diagnostics directory, no duplicate synthetic-result writes, distinct user cancellation, and documentation of the helper-process rationale.

  • Analyzer build: zero warnings/errors; formatting and diff checks passed.
  • Existing MetadataReferencePropertiesTests passed through RunTests on .NET and .NET Framework.
  • A rejected VSTest platform produced the expected synthetic failed result.
  • A session-only harness exercised the actual runner with short cancellation/deadline values, without editing test sources. User cancellation exited 1 with zero dumps. A five-second global deadline against an active assembly exited 1 with five dumps, including a debugger-readable testhost dump. The initial failure message was preserved.
  • An initial shorter probe caught a testhost during startup/exit and that dump failed; the remaining four processes were dumped. The active-assembly probe above verified testhost capture.

Fresh CI for this revision has not been verified. Earlier end-to-end artifact proof is preserved below.

Preserved CI artifact evidence

Intentional-hang build 1618314 — browse/download artifacts, commit 04635fe7f9e. All three testhost dumps were downloaded and opened with dotnet-dump analyze; their stacks showed the intentional sleep.

Platform/runtime Artifact Testhost dump path within artifact
Linux .NET 10 x64 Test_Linux_Debug Attempt 1 Logs artifacts/TestResults/Debug/WorkItem_0_x64/8a6fc3c1e2c944ecb19e55ff5fb963ec/dotnet-94725-hangdump.dmp
Windows .NET 10 x64 Test_Windows_CoreClr_Debug_Single_Machine Attempt 1 Logs artifacts/TestResults/Debug/WorkItem_0_x64/4a25842e4dbf4723af8c8cb7c3001af9/testhost-5368-hangdump.dmp
Windows .NET Framework 4.7.2 x64 Test_Windows_CoreClr_Debug_Single_Machine Attempt 1 Logs artifacts/TestResults/Debug/WorkItem_1_x64/6d31d7be3fe040fbb965592037d9921f/testhost.net472-8628-hangdump.dmp

Linux VSTest-inactivity evidence build 1618278: artifact Test_Linux_Debug_Spanish_Single_Machine Attempt 1 Logs, dump artifacts/TestResults/Debug/WorkItem_0_x64/0de1c48e228c443783523539f88e85d7/b70c6072-25f4-4102-b09e-bb26070bd161/dotnet_94701_20260930T174048_hangdump.dmp was also downloaded and debugger-verified.

These are historical intentional-failure builds with shortened timeouts. All temporary CI test/pipeline edits were removed in 848f3c1eb06; subsequent local probes were removed before their commits. Earlier one-minute concurrent global-timeout evidence is also preserved; its superseded option descriptions do not apply to the current implementation.

Assembly timing and retention

Isolation investigation: exact CI-built LSP, Semantic and Features assemblies passed individually on Linux in 11m21s, 11m53s and 22m08s. Successful main builds showed normal CI times of 30–45 minutes, so no absolute assembly deadline or repartitioning is introduced. Earlier integration CI 1618397 passed.

Existing approximately 8-GB aggregate dump pruning remains unchanged and can remove dumps before upload. The retained LSP dump from 1618396 artifacts is in Test_Linux_Debug Attempt 1 Logs, path artifacts/TestResults/Debug/WorkItem_37_x64/6e0dc61b5c034d4b8c69d4b6b2843d47/dotnet-119348-hangdump.dmp. Collection validation does not remove that retention limitation. x86/ARM dump inspection was not performed.

Microsoft Reviewers: Open in CodeFlow

jaredpar and others added 2 commits September 30, 2026 10:26
Separate inactivity and work-item deadlines, isolate dump collection in a helper process, and terminate owned processes before reporting timeout failures.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise Windows Framework/Core partition timeouts and Linux partition/inactivity timeouts in single-machine PR jobs. Revert this commit after preserving downloadable artifact evidence.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 17:28
@jaredpar
jaredpar requested review from a team as code owners September 30, 2026 17:28
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The intentional hang fixture and temporary CI overrides remain, and clean-head validation is still pending.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Adds bounded local test deadlines, cross-platform hang-dump collection, owned-process cleanup, and synthetic timeout failures.

Changes:

  • Adds configurable inactivity, work-item, and dump deadlines.
  • Tracks and terminates owned process trees while preserving diagnostics.
  • Temporarily enables intentional CI hangs for dump validation.

Holistic assessment: The design is coherent, but the intentional hang probe and pipeline overrides must be reverted before merge.

File Description
src/​Tools/​RunTests/​TestRunner.cs Waits for bounded cleanup on cancellation.
src/​Tools/​RunTests/​README.md Documents timeout behavior and options.
src/​Tools/​RunTests/​Program.cs Coordinates global timeout cleanup and screenshots.
src/​Tools/​RunTests/​ProcessUtil.cs Adds process-tree termination and ancestry snapshots.
src/​Tools/​RunTests/​ProcessTestExecutor.cs Enforces deadlines and emits synthetic failures.
src/​Tools/​RunTests/​ProcessRunner.cs Drains output before reporting cancellation.
src/​Tools/​RunTests/​OwnedProcessTree.cs Tracks launcher descendants.
src/​Tools/​RunTests/​Options.cs Adds timeout and integration options.
src/​Tools/​RunTests/​DumpCollector.cs Adds bounded cross-platform dump helpers.
src/​Compilers/​Core/​CodeAnalysisTest/​MetadataReferences/​MetadataReferencePropertiesTests.cs Adds the temporary intentional hang.
eng/​test-vsi.ps1 Selects integration-test timeout allowances.
azure-pipelines.yml Temporarily runs targeted hang validation.
.github/​memory/​TESTING_STRATEGY.md Records the new RunTests behavior.

Validate helper targets using kernel process start ticks rather than per-process boot-time estimates. Ensure failed partitions can write logs before artifact publication on clean agents.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 17:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The intentional hang and temporary pipeline overrides remain, CI is incomplete, and Ctrl+C is misreported as a global timeout.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Distinguish user cancellation from global deadline expiry

src/​Tools/​RunTests/​Program.cs:109

A Ctrl+C now sets e.Cancel = true, which cancels the token used by timeoutTask; this branch therefore also handles user cancellation, but reports it as a global deadline expiry. Distinguish cancellation in the diagnostic so interrupted local runs are not misreported as watchdog failures.

Revert probe 918ee4d. Windows Core/Framework and Linux partition dump artifacts from build 1618314 were downloaded and their blocked test stacks verified. Build/artifact paths remain in PR dotnet#85849; Linux inactivity evidence is in build 1618278.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 18:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Helper-budget expiry can leave large partial dumps outside the size cap, and final CI remains incomplete.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Partial dumps can remain locked and exhaust agent disk

src/​Tools/​RunTests/​DumpCollector.cs:115

When the budget expires, WaitAsync(budget.Token) can return before the helper has exited. ProcessRunner responds by calling Process.Kill, but that API is asynchronous; on Windows the helper may therefore still hold this .partial file open with FileShare.None, causing this deletion to fail. Since the exception is swallowed and CheckTotalDumpFilesSize only scans *.dmp, repeated timed-out work items can leave multi-GB partial dumps and exhaust the agent disk. Please wait for helper termination within a bounded cleanup window before deleting the partial file (and/or include partial dumps in the size cleanup).

Medium severity Ctrl+C is misreported as a global timeout

src/​Tools/​RunTests/​Program.cs:111

Ctrl+C also completes timeoutTask because it was created with the caller's cancellation token, and the new handler now sets e.Cancel = true. As a result, an explicit user cancellation is reported as a global timeout and captures a timeout screenshot. Keep the bounded cleanup, but distinguish caller cancellation before logging timeout-specific diagnostics.

@jaredpar

Copy link
Copy Markdown
Member Author

Timeout investigation: conclusion

The 25-minute deadline is being applied to full assemblies whose normal CI execution exceeds 25 minutes. These failures do not establish hung tests. All three exact failing-build assemblies complete successfully in isolation, and the same three assemblies passed in two preceding main-branch CI builds after 30-45 minutes.

Exact work items

Build 1618396, Test_Linux_Debug, Debug/net10.0/x64:

Work item (Microsoft.CodeAnalysis. prefix omitted) Isolated replay Passed / skipped / failed Passing main 1616235 Passing main 1616146
LanguageServer.Protocol.UnitTests_37 11:20.894 1,745 / 17 / 0 38:41.849 40:15.101
CSharp.Semantic.UnitTests_26 11:53.152 19,820 / 104 / 0 30:29.679 30:42.196
CSharp.Features.UnitTests_22 22:07.627 20,771 / 179 / 0 43:42.850 45:12.094

Both normal and Spanish Linux jobs in the PR build timed out on these same three assemblies.

Reproduction

  • Downloaded Transport_Artifacts_Unix_Debug-Attempt1 directly from failing build 1618396 and rehydrated its binaries. No rebuild or source substitutions.
  • Linux/WSL Ubuntu 22.04, local Linux filesystem (not Windows-mounted test binaries).
  • SDK 11.0.100-rc.1.26425.128, runtime 10.0.12, matching the failing CI job.
  • Ran one assembly at a time with the CI-built RunTests executable, --ci --testConfiguration Debug --testFramework core --sequential, exact assembly include regex, no test-case filter.
  • Restricted CPU affinity to four CPUs and set DOTNET_PROCESSOR_COUNT=4. CI ran six assemblies concurrently (the runner's 1.5 x processor count setting).
  • Used a diagnostic-only --workItemTimeout 3600 --timeout 65 override to allow an isolated run to finish if it exceeded 25 minutes. Retained --testInactivityTimeout 600. No repository defaults changed.
  • All three exited 0. Verified full xUnit result XML: 42,336 passed, 300 skipped, zero failures.
  • Longest reported individual test durations: LSP 89.96s, Semantic 179.13s, Features 11.21s. xUnit per-test durations can exclude async fixture/setup/teardown time, so these are not a substitute for wall-clock measurements.
  • Local and CI machines differ; the timing ratio cannot be attributed solely to contention. The main-branch results independently establish healthy CI execution beyond the new limit.

Artifact sufficiency

The published xUnitFailure-Microsoft.CodeAnalysis.*.log files contain the full VSTest response arguments: assembly path, framework directory, architecture, timeout configuration, and any test filter. There was no test filter in these work items. The numeric suffix is an index into the full-assembly work-item list, not a subdivision of that assembly.

The uploaded transport payload provides the original binaries, allowing exact-input replay. No additional artifact publication changes are needed to identify or replay these full-assembly work items.

The retained LSP dump showed MEF discovery workers active and a test awaiting catalog creation. Logs showed LSP and Features test activity around 24m20s, shortly before the deadline. This agrees with long-running test suites rather than a demonstrated deadlock.

Resolution direction

Keep the requested 25-minute deadline for actual bounded partitions, but split large local assemblies into smaller work items. Local TestRunner.CreateWorkItemsForFullAssemblies currently puts every assembly into one work item with an empty method filter; the finer AssemblyScheduler is used by Helix, not this path. Raising the deadline would mask this scheduling mismatch.

The separate existing approximately 8-GB dump-pruning problem remains unresolved; identifying the partition did not require its deleted dumps. This investigation changes no product code or pipeline configuration.

Evidence links

Local evidence is retained in session files/ci1618396: *-isolated.xml, *-isolated.log, *-isolated.time.txt, isolation-environment.txt, baseline-1616235-linux.log, and baseline-1616146-linux.log. The replay script is files/replay-linux-timeouts.sh.

Keep the original 90-minute whole-run deadline and make the separate work-item deadline opt-in. Retain bounded global-timeout diagnostics and the 10-minute inactivity default.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 21:15
@jaredpar

Copy link
Copy Markdown
Member Author

Original assembly behavior restored; concurrent global timeout verified

Before this PR there was no 50-minute assembly deadline. There was a 25-minute VSTest inactivity timeout and a 90-minute whole-run timeout. Restored the absence of a separate assembly deadline by making --workItemTimeout opt-in. The new 10-minute inactivity default and original 90-minute global default remain. --integration keeps its 25-minute inactivity allowance without introducing a separate 45-minute assembly limit.

Fresh one-minute global-timeout experiment

Temporarily placed an environment-gated infinite sleep in the existing MetadataReferencePropertiesTests.Constructor, built both net10.0 and net472, and ran both concurrently on Windows x64:

dotnet artifacts\bin\RunTests\Debug\net10.0\RunTests.dll `
  --include '^Microsoft\.CodeAnalysis\.UnitTests$' `
  --testfilter 'FullyQualifiedName=Microsoft.CodeAnalysis.UnitTests.MetadataReferencePropertiesTests.Constructor' `
  --timeout 1 --out <probe-directory> --logs <probe-directory> `
  --env:RUNTESTS_GLOBAL_TIMEOUT_PROBE=<probe-directory>

There was no work-item override and no inactivity override. Thus the global deadline fired first, exercising the same cancellation path used by the default 90-minute timeout.

At 45 seconds, inventoried the process tree and confirmed both testhosts had entered the hang. At the global deadline, all six live eligible owned processes received full dumps:

Work item Process PID Dump bytes
.NET VSTest launcher 17920 148,248,490
.NET Data collector 29700 135,471,226
.NET Testhost 31548 443,979,056
.NET Framework VSTest launcher 34784 148,174,830
.NET Framework Data collector 35692 136,458,518
.NET Framework Testhost 23820 659,135,018
  • Exited with code 1 after 64.076 seconds, including dump collection and cleanup.
  • Two synthetic failed xUnit results, one for each active work item.
  • All six dumps opened successfully with dotnet-dump analyze; both testhost stacks show MetadataReferencePropertiesTests.Constructor() blocked in Thread.Sleep.
  • All six owned target processes and all six dump-helper processes exited.
  • An unrelated sentinel remained alive; the validation controller stopped it afterward.
  • Collection is scoped to owned test processes, not arbitrary unrelated processes on the machine. Budgets/failures can prevent individual dumps; this probe verified complete coverage of its six eligible live targets.

Also verified the optional --workItemTimeout 30 still fires independently and collects dumps (failed as expected after 32.04 seconds).

The temporary source modification was removed before committing. Rebuilt the original test project and verified all three selected tests pass on each of net10.0 and net472. No new test project or permanent probe was added. RunTests built with analyzers with zero warnings/errors; formatter verification and git diff --check passed.

The existing approximately 8-GB aggregate dump-pruning policy is a separate remaining limitation for larger CI failures. This new local experiment produced about 1.56 GiB of complete dumps; it does not resolve that retention issue.

Evidence retained in session files/global-timeout-concurrent: verification.json, processes-before-timeout.json, runner logs, two synthetic failure XML files, and six debugger stack captures. Earlier uploaded cross-platform intentional-hang evidence remains at https://dev.azure.com/dnceng-public/public/_build/results?buildId=1618314&view=artifacts.

jaredpar and others added 2 commits September 30, 2026 14:19
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Process identity validation must prevent recycled PIDs from being treated as owned descendants.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread src/Tools/RunTests/OwnedProcessTree.cs Outdated
Copilot AI balanced review requested due to automatic review settings September 30, 2026 21:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Process ownership and partial-dump cleanup remain unsafe, and the new lifecycle lacks permanent regression coverage.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Await helper termination before deleting partial files

src/​Tools/​RunTests/​DumpCollector.cs:107

The prior partial-dump concern remains on the current head: canceling this WaitAsync does not wait for the helper to exit. On Windows the helper can still hold the .partial file with FileShare.None when the catch block tries to delete it; that failure is swallowed, and the size cleanup ignores *.partial, so repeated budget expirations can exhaust the agent disk. Please bound and await helper termination before deleting, and include any leftover partial files in retention cleanup.

Low severity Add regression tests for timeout and process lifecycle paths

src/​Tools/​RunTests/​ProcessTestExecutor.cs:154

This new timeout/process lifecycle has no permanent automated regression coverage; the temporary hang probes were removed. That leaves global cancellation, queued continuation, helper-budget expiry, synthetic XML, and unrelated-process isolation unprotected. Please add a sibling RunTests test project using controllable child processes and cover normal completion plus the timeout/cancellation paths.

Use fixed inactivity settings and leave work-item deadlines to infrastructure. Continue awaiting dump helper processes without a timeout after global cancellation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 21:30
Copilot AI balanced review requested due to automatic review settings September 30, 2026 23:09
jaredpar and others added 3 commits September 30, 2026 16:10
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The timeout split remains unimplemented, PID reuse can target unrelated processes, descendant discovery can skip root dumps, and cancellation can leave WER settings enabled.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity · 1 Low severity

Open (5)

Comment thread src/Tools/RunTests/Program.cs
Copilot AI balanced review requested due to automatic review settings September 30, 2026 23:16
@jaredpar

Copy link
Copy Markdown
Member Author

@dibarbet PTAL

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The unresolved PID-reuse race can cause dump or termination actions to target a process the runner does not own.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (4)

Comment thread src/Tools/RunTests/README.md Outdated
Comment thread src/Tools/RunTests/ProcessRunner.cs Outdated
Comment thread src/Tools/RunTests/ProcessTestExecutor.cs
Comment thread src/Tools/RunTests/ProcessTestExecutor.cs Outdated
Comment thread src/Tools/RunTests/ProcessTestExecutor.cs Outdated
Comment thread src/Tools/RunTests/ProcessTestExecutor.cs Outdated
Comment thread src/Tools/RunTests/DumpCollector.cs
Simplify wait and diagnostic APIs, preserve initial failure results, and skip dump collection on user cancellation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 1, 2026 17:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Cross-platform process termination and dump collection are high-risk, and the latest CI validation is still pending.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@jaredpar

jaredpar commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

/azp run roslyn-integration-CI

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Comment thread .github/memory/TESTING_STRATEGY.md Outdated
Comment on lines +58 to +59
/// Separate helpers isolate native failures and concurrent DbgHelp calls from different
/// work items; DbgHelp is single-threaded. They do not bound collection time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these lines relevant?

Comment thread src/Tools/RunTests/DumpCollector.cs Outdated
{ "logs=", "Log file directory (when running on Helix, this is relative to the Helix work item directory)", s => logFileDirectory = s },
{ "artifactspath=", "Path to the artifacts directory (auto-detected from binary location if not set)", s => artifactsPath = s },
{ "collectdumps", "Gather dumps on timeouts and crashes (process executor only, not supported with --helix)", o => collectDumps = o is object },
{ "collectdumps", "Enable additional Windows WER crash dumps when elevated (local only; timeout dumps are always attempted)", o => collectDumps = o is object },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does elevated mean?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you're admin.

Comment thread src/Tools/RunTests/Program.cs Outdated
Comment thread src/Tools/RunTests/README.md Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 2, 2026 19:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Fresh full and integration CI remains pending for high-risk cross-platform timeout and process-management behavior.

Review effort: Balanced
Findings: None

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 2, 2026 21:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unix testhosts are not actually prioritized, risking loss of the most actionable dump during interrupted collection.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

}

// Prioritize test hosts over the launcher and other owned .NET processes.
foreach (var (process, name) in candidates.OrderByDescending(p => p.Name.StartsWith("testhost", StringComparison.Ordinal)))
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 2, 2026 22:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Process-tree discovery failures can suppress all dump collection, and Unix testhosts remain incorrectly deprioritized.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Separate parent discovery failures from dump collection

src/​Tools/​RunTests/​ProcessTestExecutor.cs:231

The earlier snapshot-failure gap is still present after this refactor, and the new macOS path makes it concrete: GetParentProcessIdsMacAsync throws when ps fails or has unexpected output. Because descendant discovery and CollectAsync share this try, any such failure skips collection even for the launcher already in processes, then the finally kills it. Catch discovery failures separately so dump collection is still attempted for every candidate obtained so far.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants