Report jitdiff errors better

MihaZupan · MihaZupan · commit 873052e89525 · 2026-07-01T22:51:58.000+02:00
diff --git a/Runner/Helpers/JitDiffUtils.cs b/Runner/Helpers/JitDiffUtils.cs
@@ -69,14 +69,32 @@ public static async Task RunJitDiffOnFrameworksAsync(JobBase job, string coreRoo
         await RunJitDiffAsync(job, coreRootFolder, checkedClrFolder, outputFolder, "--frameworks");
     }
 
-    public static async Task RunJitDiffOnAssembliesAsync(JobBase job, string coreRootFolder, string checkedClrFolder, string outputFolder, string[] assemblyPaths, string? logPrefix = null, CancellationToken cancellationToken = default)
+    public static async Task RunJitDiffOnAssembliesAsync(JobBase job, string coreRootFolder, string checkedClrFolder, string outputFolder, string[] assemblyPaths, string? logPrefix = null, List<string>? output = null, CancellationToken cancellationToken = default)
     {
         ArgumentOutOfRangeException.ThrowIfZero(assemblyPaths.Length);
 
-        await RunJitDiffAsync(job, coreRootFolder, checkedClrFolder, outputFolder, string.Join(' ', assemblyPaths.Select(path => $"--assembly \"{path}\"")), logPrefix, cancellationToken);
+        await RunJitDiffAsync(job, coreRootFolder, checkedClrFolder, outputFolder, string.Join(' ', assemblyPaths.Select(path => $"--assembly \"{path}\"")), logPrefix, output, cancellationToken);
     }
 
-    private static async Task RunJitDiffAsync(JobBase job, string coreRootFolder, string checkedClrFolder, string outputFolder, string frameworksOrAssembly, string? logPrefix = null, CancellationToken cancellationToken = default)
+    // jit-diff prints a line like "Error running <corerun> on <assembly path>" for every assembly whose
+    // dasm generation failed (see jitutils DiffTool.RunDasmTool). Extract those assembly file names so the
+    // caller can report them and drop their (missing/one-sided) dasm from the cross-branch comparison.
+    public static IEnumerable<string> ParseFailedAssemblyNames(List<string> jitDiffOutput)
+    {
+        foreach (string line in jitDiffOutput)
+        {
+            Match match = JitDiffAssemblyFailureRegex().Match(line);
+            if (match.Success)
+            {
+                yield return Path.GetFileName(match.Groups[1].Value.Trim());
+            }
+        }
+    }
+
+    [GeneratedRegex(@"Error running \S+ on (.+)$")]
+    private static partial Regex JitDiffAssemblyFailureRegex();
+
+    private static async Task RunJitDiffAsync(JobBase job, string coreRootFolder, string checkedClrFolder, string outputFolder, string frameworksOrAssembly, string? logPrefix = null, List<string>? output = null, CancellationToken cancellationToken = default)
     {
         bool useCctors = !job.TryGetFlag("nocctors");
         bool useTier0 = job.TryGetFlag("tier0");
@@ -111,6 +129,7 @@ await job.RunProcessAsync("jitutils/bin/jit-diff",
                 $"{frameworksOrAssembly} --pmi " +
                 $"--core_root {coreRootFolder} " +
                 $"--base {checkedClrFolder}",
+                output: output,
                 logPrefix: $"jit-diff {logPrefix ?? coreRootFolder}",
                 envVars: envVars,
                 cancellationToken: cancellationToken);
diff --git a/Runner/JobBase.cs b/Runner/JobBase.cs
@@ -549,6 +549,31 @@ public async Task UploadTextArtifactAsync(string fileName, string contents)
         }
     }
 
+    // Well-known artifact used to tell the host about a problem that should be surfaced to the user even
+    // though the job as a whole succeeded (see ReportUserVisibleErrorAsync). Content is plain Markdown -
+    // the host owns the presentation (alert block, comment references, ...). Summary is always shown;
+    // Details (if any) is appended in the tracking issue only; a comment is posted when PostComment is set.
+    public const string UserVisibleErrorArtifactFileName = "UserVisibleError.json";
+
+    public sealed record UserVisibleError(string Summary, string? Details, bool PostComment);
+
+    /// <summary>
+    /// Reports an error that should be surfaced to the user even when the job as a whole succeeds. Use this
+    /// for problems that don't fail the job but that the user should still be told about (e.g. part of the
+    /// work failing in a way that likely indicates a bug in the change being tested). Unlike a fatal error,
+    /// this does not terminate the job. Pass plain Markdown and leave presentation to the host:
+    /// <paramref name="summary"/> is always surfaced; <paramref name="details"/> is only added to the
+    /// tracking issue (keep it out of anything that may be posted as a comment); set
+    /// <paramref name="postComment"/> when the problem is likely actionable (e.g. caused by the change under
+    /// test) so the host also posts a comment.
+    /// </summary>
+    public async Task ReportUserVisibleErrorAsync(string summary, string? details = null, bool postComment = false)
+    {
+        await LogAsync($"Surfacing user-visible error to the host (postComment: {postComment}):\n{summary}");
+
+        await UploadTextArtifactAsync(UserVisibleErrorArtifactFileName, JsonSerializer.Serialize(new UserVisibleError(summary, details, postComment)));
+    }
+
     public async Task UploadArtifactAsync(string path, string? fileName = null)
     {
         string name = fileName ?? Path.GetFileName(path);
diff --git a/Runner/Jobs/JitDiffJob.cs b/Runner/Jobs/JitDiffJob.cs
@@ -352,6 +352,8 @@ private async Task DownloadExtraTestAssembliesAsync()
 
     private async Task<string> CollectFrameworksDiffsAsync(bool skipMain)
     {
+        List<ExtraAssemblyDiffFailure> extraAssemblyFailures = new();
+
         try
         {
             await Task.WhenAll(
@@ -364,9 +366,11 @@ await Task.WhenAll(
 
             int memoryAvailableGB = GetRemainingSystemMemoryGB();
 
-            await Task.WhenAll(
+            List<ExtraAssemblyDiffFailure>[] failuresByBranch = await Task.WhenAll(
                 DiffExtraProjectsAsync("artifacts-main", "clr-checked-main", DiffsMainDirectory, memoryAvailableGB),
                 DiffExtraProjectsAsync("artifacts-pr", "clr-checked-pr", DiffsPrDirectory, memoryAvailableGB));
+
+            extraAssemblyFailures.AddRange(failuresByBranch.SelectMany(branchFailures => branchFailures));
         }
         finally
         {
@@ -378,14 +382,24 @@ await Task.WhenAll(
         CombineAllDiffs(DiffsMainDirectory, CombinedDasmMainDirectory);
         CombineAllDiffs(DiffsPrDirectory, CombinedDasmPrDirectory);
 
+        // Drop the dasm of any assembly that failed to diff on either branch. Otherwise its one-sided
+        // (missing / partial) dasm shows up in jit-analyze as a flood of spurious +100% / -100% diffs.
+        RemoveFailedAssemblyDasm(extraAssemblyFailures, CombinedDasmMainDirectory, CombinedDasmPrDirectory);
+
         string diffAnalyzeSummary = await JitDiffUtils.RunJitAnalyzeAsync(this, CombinedDasmMainDirectory, CombinedDasmPrDirectory);
 
+        // Surface any extra-assembly diff failures to the user (rendered prominently by the host) rather
+        // than burying them inside the collapsed diff summary code block.
+        await ReportExtraAssemblyFailuresAsync(extraAssemblyFailures);
+
         PendingTasks.Enqueue(UploadTextArtifactAsync("diff-frameworks.txt", diffAnalyzeSummary));
 
         return diffAnalyzeSummary;
 
-        async Task DiffExtraProjectsAsync(string coreRootFolder, string checkedClrFolder, string outputFolder, int memoryAvailableGB)
+        async Task<List<ExtraAssemblyDiffFailure>> DiffExtraProjectsAsync(string coreRootFolder, string checkedClrFolder, string outputFolder, int memoryAvailableGB)
         {
+            var failures = new List<ExtraAssemblyDiffFailure>();
+
             string projectsRoot = ExtraProjectsDirectory;
             string branch = coreRootFolder.Contains("main", StringComparison.Ordinal) ? "main" : "pr";
 
@@ -413,7 +427,7 @@ async Task DiffExtraProjectsAsync(string coreRootFolder, string checkedClrFolder
                 .OrderByDescending(d => Directory.GetFiles(d, "*.dll").Sum(f => new FileInfo(f).Length)));
             if (projectDirs.Count == 0)
             {
-                return;
+                return failures;
             }
 
             int coreRootCopies = Math.Min(Math.Min(Environment.ProcessorCount, memoryAvailableGB * 2), projectDirs.Count);
@@ -472,13 +486,38 @@ await Parallel.ForAsync(0, coreRootCopies, async (index, _) =>
 
                     string[] assemblyPaths = testedAssemblies.Select(a => Path.GetFullPath(Path.Combine(projectDir, a))).ToArray();
 
+                    string projectName = Path.GetFileName(projectDir);
+                    List<string> jitDiffOutput = new();
+
                     try
                     {
-                        await JitDiffUtils.RunJitDiffOnAssembliesAsync(this, newCoreRootFolder, checkedClrFolder, outputFolder, assemblyPaths, logPrefix: $"{branch} {Path.GetFileName(projectDir)}");
+                        await JitDiffUtils.RunJitDiffOnAssembliesAsync(this, newCoreRootFolder, checkedClrFolder, outputFolder, assemblyPaths, logPrefix: $"{branch} {projectName}", output: jitDiffOutput);
                     }
                     catch (Exception ex)
                     {
-                        await LogAsync($"Failed to diff {Path.GetFileName(projectDir)}: {ex.Message}");
+                        await LogAsync($"Failed to diff {projectName} on {branch}: {ex.Message}");
+
+                        // Prefer the specific assemblies jit-diff named as failing; fall back to all of the
+                        // project's assemblies if it failed without naming one (e.g. a crash before dasm).
+                        string[] failedAssemblies = JitDiffUtils.ParseFailedAssemblyNames(jitDiffOutput)
+                            .Distinct(StringComparer.OrdinalIgnoreCase)
+                            .ToArray();
+
+                        if (failedAssemblies.Length == 0)
+                        {
+                            failedAssemblies = testedAssemblies.Select(Path.GetFileName).ToArray()!;
+                        }
+
+                        string[] errorLines = ExtractJitDiffErrorLines(jitDiffOutput);
+                        if (errorLines.Length == 0)
+                        {
+                            errorLines = [ex.Message];
+                        }
+
+                        lock (failures)
+                        {
+                            failures.Add(new ExtraAssemblyDiffFailure(branch, projectName, failedAssemblies, errorLines));
+                        }
                     }
                 }
 
@@ -487,6 +526,8 @@ await Parallel.ForAsync(0, coreRootCopies, async (index, _) =>
                     try { Directory.Delete(newCoreRootFolder, recursive: true); } catch { }
                 }
             });
+
+            return failures;
         }
 
         void CombineAllDiffs(string directory, string destination)
@@ -500,6 +541,142 @@ void CombineAllDiffs(string directory, string destination)
         }
     }
 
+    private sealed record ExtraAssemblyDiffFailure(string Branch, string Project, string[] Assemblies, string[] ErrorLines);
+
+    private static readonly string[] s_jitDiffErrorMarkers =
+    [
+        "Error running",
+        "errors compiling set",
+        "returned with",
+        "Dasm commands returned",
+        "Dasm task failed",
+        "Failures detected generating asm",
+        "Assert failure",
+        "Unhandled exception",
+    ];
+
+    private static string[] ExtractJitDiffErrorLines(List<string> jitDiffOutput)
+    {
+        return jitDiffOutput
+            .Where(line => s_jitDiffErrorMarkers.Any(marker => line.Contains(marker, StringComparison.OrdinalIgnoreCase)))
+            .Distinct()
+            .ToArray();
+    }
+
+    private static void RemoveFailedAssemblyDasm(List<ExtraAssemblyDiffFailure> failures, params string[] combinedDasmDirectories)
+    {
+        foreach (string dasmFile in failures
+            .SelectMany(failure => failure.Assemblies)
+            .Select(assembly => $"{Path.GetFileNameWithoutExtension(assembly)}.dasm")
+            .Distinct(StringComparer.OrdinalIgnoreCase))
+        {
+            foreach (string directory in combinedDasmDirectories)
+            {
+                try { File.Delete(Path.Combine(directory, dasmFile)); }
+                catch { }
+            }
+        }
+    }
+
+    private async Task ReportExtraAssemblyFailuresAsync(List<ExtraAssemblyDiffFailure> failures)
+    {
+        if (failures.Count == 0)
+        {
+            return;
+        }
+
+        bool failedOnMain = failures.Any(failure => failure.Branch == "main");
+        bool failedOnPr = failures.Any(failure => failure.Branch == "pr");
+
+        // Only the PR branch failing (nothing on main) points at the PR itself; failures that also happen
+        // on main are more likely environmental/pre-existing, so they're reported but not flagged on the PR.
+        bool likelyCausedByPr = failedOnPr && !failedOnMain;
+
+        var failedAssemblies = failures
+            .SelectMany(failure => failure.Assemblies.Select(assembly => (failure.Branch, Assembly: assembly)))
+            .GroupBy(entry => entry.Assembly, StringComparer.OrdinalIgnoreCase)
+            .OrderBy(group => group.Key, StringComparer.OrdinalIgnoreCase)
+            .ToArray();
+
+        // Plain Markdown - the host owns presentation (alert block, comment, ...). The failed-assembly list
+        // is capped so it stays readable; this concise summary is all that's posted as a comment.
+        const int MaxListedAssemblies = 10;
+
+        StringBuilder summary = new();
+
+        summary.AppendLine($"**{failedAssemblies.Length} extra test {(failedAssemblies.Length == 1 ? "assembly" : "assemblies")} failed to produce JIT diffs** and {(failedAssemblies.Length == 1 ? "was" : "were")} excluded from the diff analysis:");
+
+        foreach (var group in failedAssemblies.Take(MaxListedAssemblies))
+        {
+            string branches = string.Join(", ", group.Select(entry => entry.Branch).Distinct().OrderBy(branch => branch, StringComparer.Ordinal));
+            summary.AppendLine($"- `{group.Key}` ({branches})");
+        }
+
+        if (failedAssemblies.Length > MaxListedAssemblies)
+        {
+            summary.AppendLine($"- ... and {failedAssemblies.Length - MaxListedAssemblies} more");
+        }
+
+        if (likelyCausedByPr)
+        {
+            summary.AppendLine();
+            summary.AppendLine("All failures happened only on the PR branch, which likely indicates a bug introduced by the PR (e.g. a JIT assert or crash while compiling these assemblies).");
+        }
+
+        string summaryMarkdown = summary.ToString().TrimEnd();
+
+        // The full per-assembly output can be large, so it's only shown in the (collapsible) tracking issue
+        // details - never in a posted comment.
+        StringBuilder details = new();
+
+        details.AppendLine("<details>");
+        details.AppendLine("<summary>Relevant output</summary>");
+        details.AppendLine();
+        details.AppendLine("```");
+
+        const int MaxOutputLines = 60;
+        int outputLines = 0;
+        bool truncated = false;
+
+        foreach (ExtraAssemblyDiffFailure failure in failures
+            .OrderBy(failure => failure.Branch, StringComparer.Ordinal)
+            .ThenBy(failure => failure.Project, StringComparer.OrdinalIgnoreCase))
+        {
+            foreach (string line in failure.ErrorLines)
+            {
+                if (outputLines++ == MaxOutputLines)
+                {
+                    truncated = true;
+                    break;
+                }
+
+                details.AppendLine($"[{failure.Branch} {failure.Project}] {line}");
+            }
+
+            if (truncated)
+            {
+                break;
+            }
+        }
+
+        if (truncated)
+        {
+            details.AppendLine("... (truncated, see the full logs for details)");
+        }
+
+        details.AppendLine("```");
+        details.AppendLine();
+        details.AppendLine("</details>");
+
+        string detailsMarkdown = details.ToString().TrimEnd();
+
+        await LogAsync($"{summaryMarkdown}\n\n{detailsMarkdown}");
+
+        // Post a comment (summary only, no verbose output) when the PR is the likely cause; the full report
+        // always goes to the tracking issue.
+        await ReportUserVisibleErrorAsync(summaryMarkdown, details: detailsMarkdown, postComment: likelyCausedByPr);
+    }
+
     private async Task UploadJitDiffExamplesAsync(string diffAnalyzeSummary, bool regressions)
     {
         var (diffs, noisyDiffsRemoved) = await JitDiffUtils.GetDiffMarkdownAsync(
