Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 28, 2025

The crash dump extension only collected the testhost process dump by PID, missing dumps from child processes that crashed alongside it. Additionally, it only collected dumps when the main process crashed, but child processes can crash even when the main process exits successfully.

Changes

Modified CrashDumpProcessLifetimeHandler.OnTestHostProcessExitedAsync:

  • Removed the HasExitedGracefully check to always collect dumps, regardless of how the main process exited
  • Changed from conditional collection (expected dump OR all dumps) to always collecting all .dmp files in the directory
  • Added directory validation before file enumeration
  • Removed error messages about missing expected dump files (no longer relevant)

Before:

if (cancellation.IsCancellationRequested
    || testHostProcessInformation.HasExitedGracefully
    || (AppDomain.CurrentDomain.GetData("ProcessKilledByHangDump") is string processKilledByHangDump && processKilledByHangDump == "true"))
{
    return;
}

// ... error display ...

string expectedDumpFile = _netCoreCrashDumpGeneratorConfiguration.DumpFileNamePattern.Replace("%p", testHostProcessInformation.PID.ToString(CultureInfo.InvariantCulture));
if (File.Exists(expectedDumpFile))
{
    await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(expectedDumpFile), ...));
}
else
{
    // Fallback: collect all dumps
    foreach (string dumpFile in Directory.GetFiles(Path.GetDirectoryName(expectedDumpFile)!, "*.dmp"))
    {
        await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(dumpFile), ...));
    }
}

After:

if (cancellation.IsCancellationRequested
    || (AppDomain.CurrentDomain.GetData("ProcessKilledByHangDump") is string processKilledByHangDump && processKilledByHangDump == "true"))
{
    return;
}

// Validate directory exists
string? dumpDirectory = Path.GetDirectoryName(expectedDumpFile);
if (RoslynString.IsNullOrEmpty(dumpDirectory) || !Directory.Exists(dumpDirectory))
{
    return;
}

// Collect all dump files in the directory to capture crashes from child processes
// We check for any dumps in the directory, even if the main process exited gracefully,
// because child processes might have crashed
foreach (string dumpFile in Directory.GetFiles(dumpDirectory, "*.dmp"))
{
    await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(dumpFile), ...));
}

This ensures all crash dumps are collected from the unique per-run directory, regardless of whether the main process crashed or exited gracefully. With .NET Core's built-in crash dump mechanism, child processes can crash independently and generate dumps even when the parent process succeeds.

Testing

Added acceptance test CrashDump_WithChildProcess_CollectsMultipleDumps that validates multiple dump collection:

  • Spawns a child process that crashes using Environment.FailFast
  • Main testhost process also crashes
  • Verifies at least 2 dump files (parent + child) are collected
  • Uses new CrashDumpWithChildFixture test asset that follows the pattern of existing process tree tests

Fixes #4186

Original prompt

This section details on the original issue you should resolve

<issue_title>Collect all dumps in the directory inside the crash dump</issue_title>
<issue_description>From @MarcoRossignoli:

Current extension is only moving the testhost process crash using the pid, but if for some reason children processes will crash together with it we won't report all dumps.</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Collect all dumps in the directory for crash reporting Collect all crash dumps in directory to capture child process crashes Oct 28, 2025
Copilot AI requested a review from Evangelink October 28, 2025 14:53
Copilot finished work on behalf of Evangelink October 28, 2025 14:53
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

@copilot add some acceptance test that would lead to multiple dumps

Copy link
Contributor Author

Copilot AI commented Oct 28, 2025

@copilot add some acceptance test that would lead to multiple dumps

Added acceptance test CrashDump_WithChildProcess_CollectsMultipleDumps that spawns a child process which crashes, then the parent crashes. The test verifies at least 2 dump files are collected. Commit: aa1052f

Copilot AI requested a review from Evangelink October 28, 2025 15:22
Copilot finished work on behalf of Evangelink October 28, 2025 15:22
if (string.Equals(dumpFile, expectedDumpFile, StringComparison.OrdinalIgnoreCase))
{
await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(dumpFile), CrashDumpResources.CrashDumpDisplayName, CrashDumpResources.CrashDumpArtifactDescription)).ConfigureAwait(false);
foundExpectedDump = true;
Copy link
Member

Choose a reason for hiding this comment

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

This still looks wrong, what if the main process does not crash?

The reason why vstest crash dumper checks for crash (it does that in stupid way of comparing test counts, but whatever), is that we always collect dump on process exit when procdump crash dumper is used. This allows us to capture out of memory and stack overflow exceptions in .net framework processes. In .net core process this is handled in a better way, the process will be able to capture dump on OOM or on stack overflow, and you don't have to collect dump always.

So all that is needed here is making unique folder per run, and reporting all dumps from there.

If this later expands to use procdump, it would need to detect premature exit of the testhost process (at least), no matter what the exit code is, to be able to distunguish a crash of the process or just successful exit.

Copy link
Member

Choose a reason for hiding this comment

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

@copilot please update handling based on this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the HasExitedGracefully check and simplified the logic to always collect all dumps from the directory. The implementation now:

  • Always scans for .dmp files in the directory, even if main process exits gracefully
  • Removes error messages about missing expected dump (no longer relevant)
  • Relies on unique folder per run to ensure we're only collecting dumps from this test run

Commit: 40aac17

return 1;
}
// Start a child process that will also crash (only when running as testhost controller)
Copy link
Member

Choose a reason for hiding this comment

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

This comment is wrong? The process that is not a testhost controller will have this parameter, so we are not running as testhost controller, we are running under testhost controller.

Might also be a good idea to crash this process in a different way than failfast. E.g. by allocating on the stack a bit of memory that is larger than the stack - immediate stack overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the comment from "running as testhost controller" to "running under testhost controller". Commit: 40aac17

Copilot AI changed the title Collect all crash dumps in directory to capture child process crashes Collect all crash dumps in directory regardless of main process exit status Oct 29, 2025
Copilot finished work on behalf of Evangelink October 29, 2025 19:32
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.

Collect all dumps in the directory inside the crash dump

3 participants