Temporarily use procdump to capture testhost dumps on helix #55657

dibarbet · 2021-08-17T00:56:09Z

Normally, dumps on helix are captured by windows error reporting (configured by registry values on helix machines) and output as attachments to the helix work item.

However, WER is seemingly unable to capture dumps for failures listed in #55639 (more details on the issue).
Only procdump configured to record dumps on process exit seems to consistently produce dumps. This change temporarily introduces procdump to the EditorFeatures.UnitTests work items to capture dumps on exit so that we can investigate the test failures. This should be removed once we have dumps.

Only need to review the current changes, I will do a squash merge on this.

CI run with dumps -https://dev.azure.com/dnceng/public/_build/results?buildId=1316212&view=ms.vss-test-web.build-test-results-tab&runId=38724164&resultId=215635&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab

…or sure runs in CI?

This reverts commit 3e7d723.

dibarbet · 2021-08-25T19:06:37Z

src/Tools/Source/RunTests/TestRunner.cs

+            // This should be removed once we have enough dumps to investigate the above issue.
+            if (!string.IsNullOrEmpty(options.ProcDumpFilePath))
+            {
+                ConsoleUtil.WriteLine($"Copying procdump files from {options.ProcDumpFilePath} to {duplicateDir}");


this was the most expedient place to put this. The actual test run step runs build.ps1 which will download procdump (doesn't necessarily happen in prepare tests). Since this is temporary I think its fine to copy the files to the correlation payload here.

dibarbet · 2021-08-25T19:08:24Z

src/Tools/Source/RunTests/ProcessTestExecutor.cs

@@ -75,6 +75,8 @@ void MaybeAddSeparator(char separator = '|')
                builder.AppendFormat($@" --logger {sep}html;LogFileName={GetResultsFilePath(assemblyInfo, "html")}{sep}");
            }

+            builder.Append(" --blame");


I found this output useful when running local tests as it outputs more data on the test that was running when the testhost crashed
See https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-test#options

The other flags like --blame-crash claim to produce a dump, but similar to WER never for these failures.

Sounds good. Let's just keep an eye out for possible regressions in test run time/artifact upload time.

RikkiGibson · 2021-08-25T20:06:11Z

I saw a lot of what looked like procdump output in this log: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-roslyn-refs-pull-55657-merge-60762369fadd42aebe/Microsoft.CodeAnalysis.EditorFeatures.UnitTests.dll.1/1/console.5894deb5.log?sv=2019-07-07&se=2021-09-14T02%3A20%3A43Z&sr=c&sp=rl&sig=knq4d%2F3O8sRZtpuLdRG5N7YwUEF5g1b%2FhmiXbrlB4mQ%3D

Does this also happen in a successful run? Should we divert some of this output elsewhere?

dibarbet · 2021-08-25T20:20:27Z

Does this also happen in a successful run? Should we divert some of this output elsewhere?

Yes, unfortunately the only way to capture dumps for this failure is if procdump records a dump whenever the process exits (including successful runs). Hence the reason to limit to only that specific dll. I find the procdump output useful as well as it also shows exceptions the process is hitting and that it actually recorded the dump. And the output isn't generally longer than the one you linked.

RikkiGibson · 2021-08-25T22:13:40Z

src/Tools/Source/RunTests/TestRunner.cs

@@ -160,6 +178,17 @@ string makeHelixWorkItemProject(AssemblyInfo assemblyInfo)
                var rehydrateCommand = isUnix ? $"./{rehydrateFilename}" : $@"call .\{rehydrateFilename}";
                var setRollforward = $"{(isUnix ? "export" : "set")} DOTNET_ROLL_FORWARD=LatestMajor";
                var setPrereleaseRollforward = $"{(isUnix ? "export" : "set")} DOTNET_ROLL_FORWARD_TO_PRERELEASE=1";
+
+                var procDumpCommand = @"echo ""Skip""";


Consider making this message slightly more detailed, for example "Skipping procdump collection."

RikkiGibson · 2021-08-25T22:16:34Z

src/Tools/Source/RunTests/TestRunner.cs

+            // to capture dumps on test host exit.  Typically WER is good enough,
+            // but https://github.com/dotnet/roslyn/issues/55639 requires procdump dumps.
+            // This should be removed once we have enough dumps to investigate the above issue.
+            if (!string.IsNullOrEmpty(options.ProcDumpFilePath))


As long as this step is temporary, I think it's ok to do this here. If it were permanent, I would prefer for copy of this tool to happen as part of the build of the relevant test projects, and to happen automatically as part of running tests on the relevant project from as many environments as possible--not just in the Helix environment.

I only plan to do this temporarily as WER is generally preferred over procdump for stability.

RikkiGibson · 2021-08-25T22:17:21Z

src/Tools/Source/RunTests/TestRunner.cs

+                if (!isUnix && Equals("Microsoft.CodeAnalysis.EditorFeatures.UnitTests.dll", assemblyInfo.AssemblyName))
+                {
+                    ConsoleUtil.WriteLine("Enabling procdump collection for this work item.");
+                    procDumpCommand = @$"start /b ""ProcDump"" %HELIX_CORRELATION_PAYLOAD%\procdump.exe /accepteula -ma -w -t -e testhost ""C:\cores""";


What's C:\cores? I think Helix provides us with a path where we can write out artifacts that we want uploaded after the work item completes. Are we using that?

c:\cores is the directory that WER is configured to store dumps in, was just an expedient place to get the dumps automatically uploaded.

RikkiGibson · 2021-08-25T22:19:25Z

Done with review pass (iteration 19).

A general tip for making changes in this area: you might find it helpful to push a branch to the dotnet/roslyn remote and manually kick off CI for it to verify your changes. Unlike launching CI runs via PR, you can test multiple different modifications concurrently without each one canceling the ones before it.

dibarbet · 2021-08-25T22:51:34Z

Talked with Sam offline - based on his advice I will instead manually run these tests a bunch of times with procdump rather than introduce more instability into main caused via procdump itself.
@RikkiGibson I'll take your advice there and just queue them up manually from a branch. instead of triggering /azp run a bunch :)

dibarbet · 2021-08-25T22:52:18Z

Also investigating locally adding synchronization to the failfast calls

dibarbet · 2021-08-31T18:17:03Z

not needed, replaced by #55939

testing crash in error reporting

5ac02c8

dotnet-issue-labeler bot added the Area-IDE label Aug 17, 2021

dibarbet added 2 commits August 17, 2021 13:12

verify dumps present in configured location

0a58c2b

only run failing jobs

3e7d723

This comment has been minimized.

Sign in to view

dibarbet added 3 commits August 17, 2021 18:39

add failing test

88b5ccf

update console writeline

685225b

remove unnecessary changes

eb0425e

dibarbet mentioned this pull request Aug 18, 2021

Test process crash on Microsoft.CodeAnalysis.EditorFeatures.UnitTests.dll #55639

Closed

dibarbet added 3 commits August 17, 2021 20:18

maybe not constructed?

2778062

update

7aa0730

For some reason the test wont run in CI, moving to a test file that f…

66f9039

…or sure runs in CI?

This comment has been minimized.

Sign in to view

dibarbet added 3 commits August 23, 2021 20:06

Attempt to use procdump to capture dumps on exit

e90985c

Copy procdump to helix

741d50e

use new procdump path

dc4357a

dibarbet force-pushed the helix_dump_logging branch from a8fbd76 to dc4357a Compare August 24, 2021 18:30

dibarbet added 6 commits August 24, 2021 14:22

move files to correlation payload

f84b2bc

use correlation payload in command

0c24ad8

test if helix is using return code of last command

7fe4d90

cleanup

b6ea37e

Revert "only run failing jobs"

c9e5fc4

This reverts commit 3e7d723.

Cleanup

2407e0c

dibarbet force-pushed the helix_dump_logging branch from 0a871e9 to 2407e0c Compare August 25, 2021 19:00

dibarbet changed the title ~~testing crash in error reporting~~ Temporarily use procdump to capture testhost dumps on helix Aug 25, 2021

dibarbet added Area-Infrastructure and removed Area-IDE labels Aug 25, 2021

dibarbet marked this pull request as ready for review August 25, 2021 19:05

dibarbet requested a review from a team as a code owner August 25, 2021 19:05

dibarbet requested a review from RikkiGibson August 25, 2021 19:05

dibarbet commented Aug 25, 2021

View reviewed changes

JoeRobich approved these changes Aug 25, 2021

View reviewed changes

add back getdirectoryname I accidentally removed

999e708

RikkiGibson self-assigned this Aug 25, 2021

RikkiGibson reviewed Aug 25, 2021

View reviewed changes

sharwell marked this pull request as draft August 25, 2021 22:16

RikkiGibson reviewed Aug 25, 2021

View reviewed changes

dibarbet closed this Aug 31, 2021

dibarbet deleted the helix_dump_logging branch August 31, 2021 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporarily use procdump to capture testhost dumps on helix #55657

Temporarily use procdump to capture testhost dumps on helix #55657

dibarbet commented Aug 17, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dibarbet Aug 25, 2021

dibarbet Aug 25, 2021

RikkiGibson Aug 25, 2021

RikkiGibson commented Aug 25, 2021

dibarbet commented Aug 25, 2021 •

edited

Loading

RikkiGibson Aug 25, 2021

RikkiGibson Aug 25, 2021

dibarbet Aug 25, 2021

RikkiGibson Aug 25, 2021

dibarbet Aug 25, 2021

RikkiGibson commented Aug 25, 2021

dibarbet commented Aug 25, 2021

dibarbet commented Aug 25, 2021

dibarbet commented Aug 31, 2021

Temporarily use procdump to capture testhost dumps on helix #55657

Temporarily use procdump to capture testhost dumps on helix #55657

Conversation

dibarbet commented Aug 17, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Aug 25, 2021

dibarbet commented Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Aug 25, 2021

dibarbet commented Aug 25, 2021

dibarbet commented Aug 25, 2021

dibarbet commented Aug 31, 2021

dibarbet commented Aug 17, 2021 •

edited

Loading

dibarbet commented Aug 25, 2021 •

edited

Loading