Collect all crash dumps in dump directory (#4186)

src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs

@@ -22,6 +22,24 @@ internal sealed class CrashDumpProcessLifetimeHandler : ITestHostProcessLifetime
     private readonly IOutputDevice _outputDisplay;
     private readonly CrashDumpConfiguration _netCoreCrashDumpGeneratorConfiguration;
 
+    // The dump enumeration relies on this set to identify dumps that pre-date the current test
+    // run so it can skip them when publishing artifacts. File paths are case-insensitive on
+    // Windows but case-sensitive on Linux/macOS, so use an OS-appropriate comparer to avoid
+    // treating freshly produced dumps as "pre-existing" merely because of casing differences.
+    //
+    // KNOWN LIMITATION: when multiple testhost processes share the same dump directory (e.g. a
+    // user running two `dotnet test` invocations into the same --results-directory), each
+    // handler instance snapshots only the files present at *its own* start time. If process B
+    // writes a dump after handler A's snapshot, handler A may publish B's dump as if it were
+    // its own. The previous "PID-only match" code had the same issue. A more robust fix would
+    // require tracking per-file creation times against the snapshot time, which we deliberately
+    // avoid here to keep the handler free of an `IClock` dependency.
+    private static readonly StringComparer PathComparer = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
+        ? StringComparer.OrdinalIgnoreCase
+        : StringComparer.Ordinal;
+
+    private readonly HashSet<string> _preExistingDumpFiles;
+
     public CrashDumpProcessLifetimeHandler(
         ICommandLineOptions commandLineOptions,
         IMessageBus messageBus,
@@ -32,6 +50,9 @@ public CrashDumpProcessLifetimeHandler(
         _messageBus = messageBus;
         _outputDisplay = outputDisplay;
         _netCoreCrashDumpGeneratorConfiguration = netCoreCrashDumpGeneratorConfiguration;
+#pragma warning disable IDE0028 // Collection initialization can be simplified - target-typed `new` cannot pass the comparer in the same syntactic form expected.
+        _preExistingDumpFiles = new(PathComparer);
+#pragma warning restore IDE0028
     }
 
     /// <inheritdoc />
@@ -56,7 +77,42 @@ public Task<bool> IsEnabledAsync()
 
     public Task BeforeTestHostProcessStartAsync(CancellationToken _) => Task.CompletedTask;
 
-    public Task OnTestHostProcessStartedAsync(ITestHostProcessInformation testHostProcessInformation, CancellationToken cancellationToken) => Task.CompletedTask;
+    public Task OnTestHostProcessStartedAsync(ITestHostProcessInformation testHostProcessInformation, CancellationToken cancellationToken)
+    {
+        // Snapshot any pre-existing files in the dump directory so we can later restrict dump publication
+        // to files that appeared during this run. Without this, when the results/dump directory is reused
+        // across runs, stale dumps from a previous crash whose names also match the configured pattern
+        // would be surfaced as artifacts of the current failure.
+        //
+        // We *union* into the existing set rather than reassign it, so multiple invocations of this
+        // callback (e.g. host restart) cannot drop entries that we have already classified as
+        // pre-existing.
+        ApplicationStateGuard.Ensure(_netCoreCrashDumpGeneratorConfiguration.DumpFileNamePattern is not null);
+        string dumpFileNamePattern = _netCoreCrashDumpGeneratorConfiguration.DumpFileNamePattern;
+        string dumpDirectory = GetDumpDirectory(dumpFileNamePattern);
+        if (Directory.Exists(dumpDirectory))
+        {
+            // Narrow the snapshot to files that share the configured dump extension when one is
+            // present, matching the search pattern we use on exit. This avoids paying for an
+            // enumeration of every entry in the dump directory (which may also contain TRX files,
+            // logs, attachments, ...), especially when `dumpDirectory` resolves to "." or to a
+            // large shared --results-directory.
+            string dumpSearchPattern = GetDumpSearchPattern(dumpFileNamePattern);
+            foreach (string file in Directory.EnumerateFiles(dumpDirectory, dumpSearchPattern))
+            {
+                cancellationToken.ThrowIfCancellationRequested();
+                _preExistingDumpFiles.Add(file);
+            }
+        }
+
+        return Task.CompletedTask;
+    }
+
+    private static string GetDumpSearchPattern(string dumpFileNamePattern)
+    {
+        string dumpExtension = Path.GetExtension(Path.GetFileName(dumpFileNamePattern));
+        return dumpExtension.Length == 0 ? "*" : $"*{dumpExtension}";
+    }
 
     public async Task OnTestHostProcessExitedAsync(ITestHostProcessInformation testHostProcessInformation, CancellationToken cancellationToken)
     {
@@ -71,20 +127,93 @@ public async Task OnTestHostProcessExitedAsync(ITestHostProcessInformation testH
         bool generateDump = _commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashDumpOptionName);
         bool generateCrashReport = _commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOptionName);
 
-        // TODO: Crash dump supports more placeholders that we don't handle here.
+        // The crash dump file name pattern can contain placeholders such as %p (PID), %e (process exe name),
+        // %h (hostname), %t (timestamp), etc. that are expanded by the .NET runtime when it writes the dump.
         // See "Dump name formatting" in:
         // https://github.com/dotnet/runtime/blob/82742628310076fff22d7e7ee216a74384352056/docs/design/coreclr/botr/xplat-minidump-generation.md
-        string expectedDumpFile = _netCoreCrashDumpGeneratorConfiguration.DumpFileNamePattern.Replace("%p", testHostProcessInformation.PID.ToString(CultureInfo.InvariantCulture));
+        // We convert the file name part of the pattern into a regular expression (escaping literal characters
+        // and turning %X placeholders into '.*') so we can collect not just the testhost dump but also dumps
+        // produced by any of its child processes that may have crashed alongside it. Using a regex (instead
+        // of passing the pattern as a glob to Directory.EnumerateFiles) ensures that any literal glob
+        // metacharacter (e.g. '*' or '?') in the configured file name is matched literally and not as a
+        // wildcard, which would otherwise cause unrelated files to be picked up on file systems that allow
+        // these characters in file names (e.g. Linux/macOS).
+        string dumpFileNamePattern = _netCoreCrashDumpGeneratorConfiguration.DumpFileNamePattern;
+        string expectedDumpFile = dumpFileNamePattern.Replace("%p", testHostProcessInformation.PID.ToString(CultureInfo.InvariantCulture));
         string expectedCrashReportFile = $"{expectedDumpFile}{CrashReportFileExtension}";
+        string dumpDirectory = GetDumpDirectory(dumpFileNamePattern);
+        string dumpFileNameOnly = Path.GetFileName(dumpFileNamePattern);
+        Regex dumpFileNameRegex = BuildDumpFileNameRegex(dumpFileNameOnly);
+
+        // Stricter regex that bakes in the testhost PID for any '%p' placeholder before expanding
+        // the remaining placeholders as wildcards. We use this to recognize the testhost's own
+        // dump (versus a child process dump) regardless of whether the configured name relies on
+        // additional placeholders such as '%e', '%h' or '%t' - relying on `File.Exists` with the
+        // literal-`%p`-substituted path would only work when '%p' is the only placeholder.
+        //
+        // Note: when the configured pattern omits '%p', this regex collapses to `dumpFileNameRegex`
+        // (the `Replace("%p", ...)` call is a no-op) and we cannot distinguish testhost from child
+        // dumps by name — any matching dump is treated as the testhost's. The runtime in that case
+        // can only produce one dump per process matching the configured shape, so the practical
+        // impact is limited to setups that pre-create files with the same shape under the dump
+        // directory.
+        Regex testhostDumpRegex = BuildDumpFileNameRegex(
+            dumpFileNameOnly.Replace("%p", testHostProcessInformation.PID.ToString(CultureInfo.InvariantCulture)));
+
+        // Narrow the file system enumeration to files that share the configured dump extension when
+        // one is present. This avoids scanning every entry of the dump directory (which may also
+        // contain TRX files, logs, attachments, ...). The placeholder-expanded regex above still
+        // applies to filter out anything that does not match the configured name pattern.
+        string dumpExtension = Path.GetExtension(dumpFileNameOnly);
+        string dumpSearchPattern = GetDumpSearchPattern(dumpFileNamePattern);
+
+        bool publishedAnyDump = false;
+        bool testhostDumpProduced = false;
+        if (generateDump && Directory.Exists(dumpDirectory))
+        {
+            foreach (string dumpFile in Directory.EnumerateFiles(dumpDirectory, dumpSearchPattern))
+            {
+                // Filter by exact extension to defend against Windows' legacy 8.3 short-name
+                // matching where a pattern like '*.dmp' can also match files whose extension
+                // merely starts with '.dmp' (for example 'foo.dmp.crashreport.json').
+                if (dumpExtension.Length != 0
+                    && !Path.GetExtension(dumpFile).Equals(dumpExtension, StringComparison.OrdinalIgnoreCase))
+                {
+                    continue;
+                }
+
+                string dumpFileNameOnDisk = Path.GetFileName(dumpFile);
+                if (dumpFileNameRegex.IsMatch(dumpFileNameOnDisk)
+                    && !_preExistingDumpFiles.Contains(dumpFile))
+                {
+                    await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(dumpFile), CrashDumpResources.CrashDumpArtifactDisplayName, CrashDumpResources.CrashDumpArtifactDescription)).ConfigureAwait(false);
+                    publishedAnyDump = true;
+
+                    if (testhostDumpRegex.IsMatch(dumpFileNameOnDisk))
+                    {
+                        testhostDumpProduced = true;
+                    }
+                }
+            }
+        }
+
+        // The crash banner and the "expected dump not found" warning are scoped to the testhost
+        // dump specifically. We must not suppress them just because we published a dump for a
+        // crashed child process: when only the child writes a dump, the user still needs to know
+        // that the testhost's own dump never materialized.
+        // Fall back to checking `expectedDumpFile` existence on disk to cover the edge case where
+        // a file matching the literal-`%p`-substituted name was already present at start time (and
+        // therefore skipped by the regex loop because it is in `_preExistingDumpFiles`) - we still
+        // want the banner to reflect that the testhost dump is, technically, present on disk.
+        testhostDumpProduced = generateDump && (testhostDumpProduced || File.Exists(expectedDumpFile));
+        bool dumpArtifactProduced = generateDump && (testhostDumpProduced || publishedAnyDump);
+        bool crashReportArtifactProduced = generateCrashReport && File.Exists(expectedCrashReportFile);
 
         // Inspect the disk before emitting the crash banner so the message reflects
         // what was actually produced, not what was requested. The runtime may fail
         // to emit one (or both) of the artifacts, e.g. when EnableCrashReport is
         // unsupported on the current platform/version.
-        bool dumpArtifactProduced = generateDump && File.Exists(expectedDumpFile);
-        bool crashReportArtifactProduced = generateCrashReport && File.Exists(expectedCrashReportFile);
-
-        string? processCrashedMessage = (dumpArtifactProduced, crashReportArtifactProduced) switch
+        string processCrashedMessage = (dumpArtifactProduced, crashReportArtifactProduced) switch
         {
             (true, true) => string.Format(CultureInfo.InvariantCulture, CrashDumpResources.CrashDumpProcessCrashedDumpAndReportFileCreated, testHostProcessInformation.PID),
             (false, true) => string.Format(CultureInfo.InvariantCulture, CrashDumpResources.CrashDumpProcessCrashedReportFileCreated, testHostProcessInformation.PID),
@@ -93,23 +222,25 @@ public async Task OnTestHostProcessExitedAsync(ITestHostProcessInformation testH
         };
         await _outputDisplay.DisplayAsync(this, new ErrorMessageOutputDeviceData(processCrashedMessage), cancellationToken).ConfigureAwait(false);
 
-        if (generateDump)
+        if (generateDump && !testhostDumpProduced)
         {
-            if (dumpArtifactProduced)
-            {
-                await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(expectedDumpFile), CrashDumpResources.CrashDumpArtifactDisplayName, CrashDumpResources.CrashDumpArtifactDescription)).ConfigureAwait(false);
-            }
-            else
-            {
-                await _outputDisplay.DisplayAsync(this, new ErrorMessageOutputDeviceData(string.Format(CultureInfo.InvariantCulture, CrashDumpResources.CannotFindExpectedCrashDumpFile, expectedDumpFile)), cancellationToken).ConfigureAwait(false);
+            await _outputDisplay.DisplayAsync(this, new ErrorMessageOutputDeviceData(string.Format(CultureInfo.InvariantCulture, CrashDumpResources.CannotFindExpectedCrashDumpFile, expectedDumpFile)), cancellationToken).ConfigureAwait(false);
 
+            // Only fall back to a directory-wide `*.dmp` scan when neither the testhost dump nor any
+            // other matching dump was published. This avoids re-enumerating the directory when we
+            // already published at least one dump (e.g. a child process dump) above.
+            if (!publishedAnyDump && Directory.Exists(dumpDirectory))
+            {
                 // Filter by exact extension to defend against Windows' legacy 8.3 short-name
                 // matching where a pattern like '*.dmp' can also match files whose extension
                 // merely starts with '.dmp' (for example 'foo.dmp.crashreport.json').
-                foreach (string dumpFile in Directory.EnumerateFiles(Path.GetDirectoryName(expectedDumpFile)!, "*.dmp")
+                foreach (string dumpFile in Directory.EnumerateFiles(dumpDirectory, "*.dmp")
                     .Where(static f => Path.GetExtension(f).Equals(".dmp", StringComparison.OrdinalIgnoreCase)))
                 {
-                    await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(dumpFile), CrashDumpResources.CrashDumpDisplayName, CrashDumpResources.CrashDumpArtifactDescription)).ConfigureAwait(false);
+                    if (!_preExistingDumpFiles.Contains(dumpFile))
+                    {
+                        await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(dumpFile), CrashDumpResources.CrashDumpArtifactDisplayName, CrashDumpResources.CrashDumpArtifactDescription)).ConfigureAwait(false);
+                    }
                 }
             }
         }
@@ -135,4 +266,63 @@ public async Task OnTestHostProcessExitedAsync(ITestHostProcessInformation testH
             }
         }
     }
+
+    internal static string GetDumpDirectory(string dumpFileNamePattern)
+    {
+        // Path.GetDirectoryName returns "" (not null) for a bare filename on .NET Core/5+ but throws
+        // ArgumentException for an empty string on .NET Framework; treat both as the current working
+        // directory so the dump enumeration is not silently skipped.
+        if (dumpFileNamePattern is null or "")
+        {
+            return ".";
+        }
+
+        string? rawDirectory = Path.GetDirectoryName(dumpFileNamePattern);
+        return rawDirectory is null or "" ? "." : rawDirectory;
+    }
+
+    internal static Regex BuildDumpFileNameRegex(string fileName)
+        => new(BuildDumpFileNameRegexPattern(fileName), RegexOptions.CultureInvariant);
+
+    internal static string BuildDumpFileNameRegexPattern(string fileName)
+    {
+        var sb = new StringBuilder("^");
+        bool lastWasWildcard = false;
+        for (int i = 0; i < fileName.Length; i++)
+        {
+            if (fileName[i] == '%' && i + 1 < fileName.Length)
+            {
+                // The .NET runtime's createdump tool treats "%%" as an escape for a literal '%'.
+                // Preserve that behavior so a configured name like "My%%App_%p.dmp" produces a regex
+                // that requires a literal '%' (rather than collapsing both characters into a wildcard
+                // and over-matching unrelated files).
+                if (fileName[i + 1] == '%')
+                {
+                    // '%' is not a regex metacharacter so it does not need escaping.
+                    sb.Append('%');
+                    lastWasWildcard = false;
+                    i++;
+                    continue;
+                }
+
+                // Replace any other %X placeholder with ".*". Collapse consecutive wildcards to keep
+                // the regex simple and to avoid backtracking on patterns like "%p%t".
+                if (!lastWasWildcard)
+                {
+                    sb.Append(".*");
+                    lastWasWildcard = true;
+                }
+
+                i++;
+            }
+            else
+            {
+                sb.Append(Regex.Escape(fileName[i].ToString()));
+                lastWasWildcard = false;
+            }
+        }
+
+        sb.Append('$');
+        return sb.ToString();
+    }
 }