Merge pull request #4717 from dmichon-msft/improve-error-reporting

[rush] Improve error reporting

common/changes/@microsoft/rush/fix-completed-count_2024-05-15-21-45.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@microsoft/rush",
+      "comment": "Fix count of completed operations when silent operations are blocked. Add explicit message for child processes terminated by signals. Ensure that errors show up in summarized view.",
+      "type": "none"
+    }
+  ],
+  "packageName": "@microsoft/rush"
+}

common/changes/@microsoft/rush/fix-completed-count_2024-05-15-22-44.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@microsoft/rush",
+      "comment": "Ensure that errors thrown in afterExecuteOperation show up in the summary at the end of the build.",
+      "type": "none"
+    }
+  ],
+  "packageName": "@microsoft/rush"
+}

common/changes/@rushstack/terminal/fix-completed-count_2024-05-15-22-44.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/terminal",
+      "comment": "Allow use of 'preventAutoclose' flag in StdioSummarizer.",
+      "type": "minor"
+    }
+  ],
+  "packageName": "@rushstack/terminal"
+}

common/reviews/api/terminal.api.md

@@ -158,7 +158,7 @@ export interface IStdioLineTransformOptions extends ITerminalTransformOptions {
 }
 
 // @beta
-export interface IStdioSummarizerOptions {
+export interface IStdioSummarizerOptions extends ITerminalWritableOptions {
     leadingLines?: number;
     trailingLines?: number;
 }

libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts

@@ -6,7 +6,8 @@ import {
   StdioWritable,
   TextRewriterTransform,
   Colorize,
-  ConsoleTerminalProvider
+  ConsoleTerminalProvider,
+  TerminalChunkKind
 } from '@rushstack/terminal';
 import { StreamCollator, type CollatedTerminal, type CollatedWriter } from '@rushstack/stream-collator';
 import { NewlineKind, Async, InternalError, AlreadyReportedError } from '@rushstack/node-core-library';
@@ -236,13 +237,9 @@ export class OperationExecutionManager {
       try {
         await this._afterExecuteOperation?.(record);
       } catch (e) {
-        // Failed operations get reported here
-        const message: string | undefined = record.error?.message;
-        if (message) {
-          this._terminal.writeStderrLine('Unhandled exception: ');
-          this._terminal.writeStderrLine(message);
-        }
-        throw e;
+        this._reportOperationErrorIfAny(record);
+        record.error = e;
+        record.status = OperationStatus.Failure;
       }
       this._onOperationComplete(record);
     };
@@ -298,6 +295,27 @@ export class OperationExecutionManager {
     };
   }
 
+  private _reportOperationErrorIfAny(record: OperationExecutionRecord): void {
+    // Failed operations get reported, even if silent.
+    // Generally speaking, silent operations shouldn't be able to fail, so this is a safety measure.
+    let message: string | undefined = undefined;
+    if (record.error) {
+      if (!(record.error instanceof AlreadyReportedError)) {
+        message = record.error.message;
+      }
+    }
+
+    if (message) {
+      // This creates the writer, so don't do this until needed
+      record.collatedWriter.terminal.writeStderrLine(message);
+      // Ensure that the error message, if present, shows up in the summary
+      record.stdioSummarizer.writeChunk({
+        text: `${message}\n`,
+        kind: TerminalChunkKind.Stderr
+      });
+    }
+  }
+
   /**
    * Handles the result of the operation and propagates any relevant effects.
    */
@@ -313,18 +331,10 @@ export class OperationExecutionManager {
       case OperationStatus.Failure: {
         // Failed operations get reported, even if silent.
         // Generally speaking, silent operations shouldn't be able to fail, so this is a safety measure.
-        let message: string | undefined = undefined;
-        if (record.error) {
-          if (!(record.error instanceof AlreadyReportedError)) {
-            message = record.error.message;
-          }
-        }
+        this._reportOperationErrorIfAny(record);
 
         // This creates the writer, so don't do this globally
         const { terminal } = record.collatedWriter;
-        if (message) {
-          terminal.writeStderrLine(message);
-        }
         terminal.writeStderrLine(Colorize.red(`"${name}" failed to build.`));
         const blockedQueue: Set<OperationExecutionRecord> = new Set(record.consumers);
 
@@ -339,7 +349,11 @@ export class OperationExecutionManager {
             blockedRecord.status = OperationStatus.Blocked;
 
             this._executionQueue.complete(blockedRecord);
-            this._completedOperations++;
+            if (!blockedRecord.runner.silent) {
+              // Only increment the count if the operation is not silent to avoid confusing the user.
+              // The displayed total is the count of non-silent operations.
+              this._completedOperations++;
+            }
 
             for (const dependent of blockedRecord.consumers) {
               blockedQueue.add(dependent);

libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts

@@ -93,7 +93,10 @@ export class OperationExecutionRecord implements IOperationRunnerContext {
   public readonly consumers: Set<OperationExecutionRecord> = new Set();
 
   public readonly stopwatch: Stopwatch = new Stopwatch();
-  public readonly stdioSummarizer: StdioSummarizer = new StdioSummarizer();
+  public readonly stdioSummarizer: StdioSummarizer = new StdioSummarizer({
+    // Allow writing to this object after transforms have been closed. We clean it up manually in a finally block.
+    preventAutoclose: true
+  });
 
   public readonly runner: IOperationRunner;
   public readonly associatedPhase: IPhase | undefined;

libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts

@@ -99,10 +99,13 @@ export class ShellOperationRunner implements IOperationRunner {
 
         const status: OperationStatus = await new Promise(
           (resolve: (status: OperationStatus) => void, reject: (error: OperationError) => void) => {
-            subProcess.on('close', (code: number) => {
+            subProcess.on('close', (code: number, signal?: string) => {
               try {
-                if (code !== 0) {
-                  // Do NOT reject here immediately, give a chance for other logic to suppress the error
+                // Do NOT reject here immediately, give a chance for other logic to suppress the error
+                if (signal) {
+                  context.error = new OperationError('error', `Terminated by signal: ${signal}`);
+                  resolve(OperationStatus.Failure);
+                } else if (code !== 0) {
                   context.error = new OperationError('error', `Returned error code: ${code}`);
                   resolve(OperationStatus.Failure);
                 } else if (hasWarningOrError) {
@@ -111,6 +114,7 @@ export class ShellOperationRunner implements IOperationRunner {
                   resolve(OperationStatus.Success);
                 }
               } catch (error) {
+                context.error = error as OperationError;
                 reject(error as OperationError);
               }
             });

libraries/terminal/src/StdioSummarizer.ts

@@ -2,13 +2,13 @@
 // See LICENSE in the project root for license information.
 
 import { type ITerminalChunk, TerminalChunkKind } from './ITerminalChunk';
-import { TerminalWritable } from './TerminalWritable';
+import { type ITerminalWritableOptions, TerminalWritable } from './TerminalWritable';
 
 /**
  * Constructor options for {@link StdioSummarizer}.
  * @beta
  */
-export interface IStdioSummarizerOptions {
+export interface IStdioSummarizerOptions extends ITerminalWritableOptions {
   /**
    * Specifies the maximum number of leading lines to include in the summary.
    * @defaultValue `10`
@@ -62,7 +62,7 @@ export class StdioSummarizer extends TerminalWritable {
   private _abridgedStderr: boolean;
 
   public constructor(options?: IStdioSummarizerOptions) {
-    super();
+    super(options);
 
     if (!options) {
       options = {};