Enhance crash dump collection options in RemoteExecutor#16716
Conversation
- Update package references for Microsoft.Diagnostics.Runtime and Microsoft.Diagnostics.NETCore.Client. - Introduce CrashDumpCollectionType enum to specify types of crash dumps. - Add options to enable/disable crash dump collection and configure related environment variables.
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, thank you for adding the ability to disable the collection @hoyosjs !
I am not approving the PR yet as I believe somebody more familiar with the APIs for ClrMD should verify the correct usage of APIs.
leculver
left a comment
leculver
left a comment
There was a problem hiding this comment.
I am not approving the PR yet as I believe somebody more familiar with the APIs for ClrMD should verify the correct usage of APIs.
It's funny, I looked through this and didn't mash approve because I didn't understand the other half of the change. This LGTM from a ClrMD perspective.
There shouldn't be any regressions moving from 1.1 -> 4.0, in fact some things should start working that didn't before.
|
@adamsitnik - fixed all the feedback you had. Thanks |
|
Github is being weird and I can't comment on a specific line. But you should bump ClrMD to 4.0 rtm now. |
|
@missymessa I had to update MS.Identity.Client and update a couple things in Microsoft.DotNet.ArcadeAzureIntegration/DefaultIdentityTokenCredential.cs - would you mind taking a look? |
…crash-dumps # Conflicts: # Directory.Packages.props # src/Microsoft.DotNet.ArcadeAzureIntegration/DefaultIdentityTokenCredential.cs # src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs
There was a problem hiding this comment.
Pull request overview
This PR enhances diagnostics for remote test processes by adding configurable crash dump collection options to Microsoft.DotNet.RemoteExecutor, updating dump collection behavior on timeouts, and updating related dependencies.
Changes:
- Add
CrashDumpCollectionType+CrashDumpPathoptions to controlDOTNET_Dbg*crash dump environment variables (including explicit disable behavior). - Add
EnableTimeoutDumpCollectionand refactor timeout diagnostics collection (dump + thread stacks) inRemoteInvokeHandle. - Update package references (add
Microsoft.Diagnostics.NETCore.Client) and adjust compilation ofMiniDump.csper TFM.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.DotNet.RemoteExecutor/tests/RemoteExecutorTests.cs | Adds tests validating crash dump env var behavior and a dump-on-crash scenario. |
| src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeOptions.cs | Introduces CrashDumpCollectionType, CrashDumpPath, and EnableTimeoutDumpCollection options. |
| src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs | Refactors and expands timeout diagnostics collection; switches to DiagnosticsClient on .NET Core. |
| src/Microsoft.DotNet.RemoteExecutor/src/RemoteExecutor.cs | Implements env var wiring/removal for crash dump collection options. |
| src/Microsoft.DotNet.RemoteExecutor/src/Microsoft.DotNet.RemoteExecutor.csproj | Adds Microsoft.Diagnostics.NETCore.Client for .NET Core builds and removes MiniDump.cs there. |
| src/Microsoft.DotNet.ArcadeAzureIntegration/DefaultIdentityTokenCredential.cs | Fixes managed identity selection to support system-assigned identity when no client id is provided. |
| Directory.Packages.props | Adds centrally managed version for Microsoft.Diagnostics.NETCore.Client. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (options.CrashDumpCollectionType.HasValue) | ||
| { | ||
| psi.Environment["DOTNET_DbgEnableMiniDump"] = "1"; | ||
| psi.Environment["DOTNET_DbgMiniDumpType"] = ((int)options.CrashDumpCollectionType.Value).ToString(); |
| string dumpPath = Path.Combine(uploadPath, $"{Process.Id}.{Path.GetRandomFileName()}.dmp"); | ||
| #if NETCOREAPP | ||
| // These define guards assume that harness running on .NET Framework implies test process runs on .NET Framework. | ||
| var client = new DiagnosticsClient(Process.Id); | ||
| client.WriteDump(DumpType.Full, dumpPath, logDumpGeneration: false); |
| [Fact] | ||
| public void CrashDumpCollection_DefaultLeavesEnvVarsUntouched() | ||
| { | ||
| // When neither option is set, env vars should pass through from the parent unchanged | ||
| using RemoteInvokeHandle h = RemoteExecutor.Invoke(() => | ||
| { | ||
| // Without explicit config, the child inherits whatever the parent has. | ||
| // The parent test process shouldn't have DOTNET_DbgEnableMiniDump set, | ||
| // so the child shouldn't either. | ||
| string parentValue = Environment.GetEnvironmentVariable("DOTNET_DbgEnableMiniDump"); | ||
| Assert.Null(parentValue); | ||
| }, new RemoteInvokeOptions { RollForward = "Major" }); | ||
| } |
This pull request introduces improved and more flexible crash dump collection for remote test processes, with enhanced configuration options and updated diagnostics support. The changes modernize how dumps are collected, update dependencies, and add new options for controlling dump behavior.