Conversation
Focus and serialize Windows ProcessStartInfo tests, log startup and test ordering, and capture a WER dump before the Helix timeout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54c890f9-b409-4b7d-b810-3701e125fd96
|
/azp run runtime-libraries-coreclr outerloop-windows |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds additional diagnostics and run-configuration changes to help investigate hangs in System.Diagnostics.Process tests, primarily on Windows. The PR introduces both logging (per-test + targeted messages) and a watchdog/WER dump configuration mechanism.
Changes:
- Windows-only test execution configuration updates (disable parallelization, enable progress output, and currently filters to a single test class via
XUnitOptions). - New diagnostics helper with a module initializer, WER LocalDumps configuration attempt, and a FailFast watchdog.
- Adds assembly-level xUnit
BeforeAfterTestAttributehook and adds extra diagnostic logging in a couple ofProcessStartInfoTests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj | Adds Windows-only test runner settings (parallelization/progress and class filtering) and includes the new diagnostics source file. |
| src/libraries/System.Diagnostics.Process/tests/ProcessTestHangDiagnostics.cs | New diagnostics implementation: per-test logging hook, module initializer logging, WER LocalDumps setup, and watchdog FailFast timeout. |
| src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs | Adds targeted diagnostic logs around specific test execution points. |
| src/libraries/System.Diagnostics.Process/tests/AssemblyInfo.cs | Registers the diagnostics attribute at assembly level. |
Suppressed comments (1)
src/libraries/System.Diagnostics.Process/tests/ProcessTestHangDiagnostics.cs:83
- Configuring WER LocalDumps under HKLM requires admin and will typically fail in CI/Helix (and if it succeeds, it leaves a machine-wide setting behind). Using HKCU avoids the elevation requirement and limits scope to the current user, making it more likely the dump configuration actually takes effect.
{
using RegistryKey? key = Registry.LocalMachine.CreateSubKey(keyPath);
if (key is null)
{
Log($"Unable to create WER LocalDumps key HKLM\\{keyPath}.");
| <PropertyGroup Condition="'$(TargetPlatformIdentifier)' == 'windows'"> | ||
| <TestDisableParallelization>true</TestDisableParallelization> | ||
| <XUnitOptions>$(XUnitOptions) -class System.Diagnostics.Tests.ProcessStartInfoTests -parallel none</XUnitOptions> | ||
| <XUnitShowProgress>true</XUnitShowProgress> | ||
| </PropertyGroup> |
| ConfigureWindowsErrorReporting(); | ||
|
|
||
| var watchdog = new Thread(Watchdog) | ||
| { | ||
| IsBackground = true, | ||
| Name = "Process tests hang watchdog" | ||
| }; | ||
| watchdog.Start(); |
| [assembly: CollectionBehavior(CollectionBehavior.CollectionPerAssembly)] | ||
| [assembly: System.Diagnostics.Tests.ProcessTestHangDiagnosticsAttribute] | ||
|
|
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
Write BOM-less UTF-8 console output and place WER dumps in the Helix work-item upload directory. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54c890f9-b409-4b7d-b810-3701e125fd96
|
/azp run runtime-libraries-coreclr outerloop-windows |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj:17
- The hard-coded
-class System.Diagnostics.Tests.ProcessStartInfoTestsfilter inXUnitOptionsmeans Windows runs of this test project will only execute that single test class, skipping the rest of the Process test suite on Windows (significant test coverage reduction). This kind of focusing should be passed as a CI/test invocation parameter (e.g.,/p:XUnitClassName=...) rather than baked into the project file.
<PropertyGroup Condition="'$(TargetPlatformIdentifier)' == 'windows'">
<TestDisableParallelization>true</TestDisableParallelization>
<XUnitOptions>$(XUnitOptions) -class System.Diagnostics.Tests.ProcessStartInfoTests -parallel none</XUnitOptions>
<XUnitShowProgress>true</XUnitShowProgress>
src/libraries/System.Diagnostics.Process/tests/ProcessTestHangDiagnostics.cs:49
- The watchdog thread is started unconditionally from the module initializer and will
FailFastafter 3 minutes even for non-hung runs (e.g., slow machines or local debugging). Consider gating this to Helix runs (or an explicit opt-in env var) so it doesn’t introduce new crash behavior in normal Windows test execution.
var watchdog = new Thread(Watchdog)
{
IsBackground = true,
Name = "Process tests hang watchdog"
};
watchdog.Start();
src/libraries/System.Diagnostics.Process/tests/ProcessTestHangDiagnostics.cs:87
- Configuring WER LocalDumps via
Registry.LocalMachinewrites machine-wide state and will typically require elevated permissions; if it fails (UnauthorizedAccess), the dump capture won’t be configured at all. Prefer using a per-user key so the diagnostics can work under normal test permissions and avoid persisting HKLM changes.
using RegistryKey? key = Registry.LocalMachine.CreateSubKey(keyPath);
if (key is null)
{
Log($"Unable to create WER LocalDumps key HKLM\\{keyPath}.");
return;
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/azp run runtime-libraries-coreclr outerloop-windows |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
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.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| [PlatformSpecific(TestPlatforms.Windows)] | ||
| public void StartInfo_BadExe(bool useShellExecute) | ||
| { | ||
| if (useShellExecute && PlatformDetection.IsWindowsServer2019 && PlatformDetection.IsWindowsServerCore) |
There was a problem hiding this comment.
Assuming this isn't expected to stay disabled forever how about we add a comment that links the skip back to issue that tracks investigating and eventually re-enabling the test?
There was a problem hiding this comment.
Yeah, I need to create the infra issue and link it here.
adamsitnik
left a comment
There was a problem hiding this comment.
@steveisok Big thanks for investigating the hang! We have refactored the UseShellExecute code in #126314, I wonder if we have introduced a regression (cc @jkotas)
|
@adamsitnik I have this issue draft from the investigation - https://gist.github.com/steveisok/2f9b48f8833a7708b061478ffb4bedb8 It seems like it's a windows bug and something we should give to dnceng |
|
Does not repro anymore #131389 (comment) |
Restore bad-executable coverage, enable the existing verbose xUnit progress reporter, and capture a full test-host dump before the Helix timeout using the matching Windows createdump tool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Opt PR 131907 into the existing build and Helix conditions without changing test scope. Exclude only its diagnostic runtime.yml edit from path triggers, leaving other PRs and genuine CoreCLR changes unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Keep the eight-minute full-memory snapshot and original Helix deadline, log capture failures without changing test status, and reap an in-flight dump child when the host completes naturally. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Generate six isolated console-runner archives using existing script and ZIP tasks, preserving all class/theory coverage and full local/static-runner semantics. Remove temporary dump instrumentation; retain the narrow PR CI opt-in for elapsed-time validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Superseded by #135160, which carries the validated Process-suite sharding change in a clean, focused history without temporary diagnostic CI routing. Closing this investigation vehicle; its evidence remains available here. Note This comment was generated with GitHub Copilot. |
Summary
Mitigate #135001's Process-suite work-item time-budget exhaustion by partitioning Windows x86 CoreCLR console-runner Helix archives into six independent work items. Each process retains the existing
CollectionPerAssemblybehavior. No in-process parallelism change, test/fixture alteration, new skip, or timeout increase.The ordinary local runner remains full-suite. Mono, other architectures/platforms, NativeAOT/ReadyToRun and other single-file runners retain their existing unpartitioned paths. The investigative watchdog/module initializer, forced/snapshot-only dump logic, verbose-only setting and diagnostic README are removed.
Validation in progress: the five-minute elapsed goal has NOT yet been verified in CI. Temporary PR-only CI routing remains below, so this revision is not yet production-ready for merge.
Evidence and balancing
Treat #135001 independently; reusing #131907 is only a convenient branch/PR vehicle. #131389's old StateRepository diagnosis is historical comparison, not the presumed cause of these timeouts.
The snapshot-only exact-leg run 1621730 reached the original 900-second timeout with 632 completed rows and approximately 883.13 seconds of completed test time. Its final unmatched row is not thereby proven hanging. A matched same-architecture/configuration/queue Windows 14393 passing run, build 1618186, completed 691 rows in 640.397 test seconds and 665.216 whole-work-item seconds. Matched rows were broadly slower in the failing run; this supports the budget mitigation, not a machine/runtime root-cause claim or a claim that all historical matches are identical.
Whole classes are balanced using all 691 passing rows, observed slow method floors, conservative 2× passing timing, and 45 seconds of work-item setup/runtime overhead per shard. Duplicate display names are preserved; unobserved slow rows and non-exactly escaped names are not discarded. Five shards' conservative average exceeds 300 seconds; six leave measured-model headroom without dozens of tiny work items.
.1.2.3.4.5.6These are forecasts, not achieved CI timings. The acceptance criterion is every Process shard's observed whole-work-item elapsed ≤300s, including setup, with complete exactly-once coverage. Any over-budget shard must be rebalanced.
Implementation and coverage
ProcessTestShards.targetscustomizes only this project's archive generation using the repository's existingGenerateRunScriptandZipDirectorytasks. Default Helix packaging already maps each ZIP to a separate named work item; no new CI runner or shared infrastructure refactor is introduced.Each archive contains the unchanged full test assembly, runtimeconfig/deps, RemoteExecutor and supporting files, with class selection in its generated runner. Work items are
System.Diagnostics.Process.Tests.1through.6, each with independent extracted payload, result/upload directory, temp scope and child-process lifecycle. The original unfiltered ZIP is removed after all six archives exist, and unsupported-mode generation removes stale shard ZIPs. The full localRunTests.cmdis not replaced.The first five shards use include-class lists (OR semantics). The sixth excludes exactly those sixteen classes: it covers the remaining classes and future additions automatically. Theory rows and inherited cases stay with their executing class. All work items retain the original 900-second timeout; there is no artificial five-minute kill.
Scoped validation
installer.tasksscript-generation task: zero warnings/errors. No broad runtime baseline build.sendtohelixhelp.projgeneration produced all six expectedHelixWorkItems, the standard runtime-path command, andTimeout=00:15:00.TestWindowStyleyields one skipped theory instead of four rows, while the protected-process theory expands to two instead of one skip. Traits/platform conditions were not changed to force a count.Temporary CI routing and remaining gate
The existing build and Helix-submission opt-ins for this PR's merge ref, PullRequest reason, Windows/x86 job variables remain unchanged, ensuring
build_windows_x86_Debug_Libraries_CheckedCoreCLRruns onWindows.10.Amd64.Open(windows.10.amd64.open.rt) despite a libraries-only diff. The exact diagnosticruntime.ymlpath exclusion remains limited to this PR and does not hide genuine CoreCLR changes or alter other PRs' classification.This routing is temporary validation wiring and must be removed or replaced with justified permanent routing before merge. Do not call the mitigation complete until all intended shard work items have full elapsed ≤300s and their per-run test-row union proves no omissions or duplicates. A successful pipeline alone, predicted balance, or historical fixed count is insufficient.
Note
This pull request description and changes were generated with GitHub Copilot.