server: Disable automatic retry when exiting from debug mode

* In debug mode, never retry (maxTries = 1).

* Fix "TypeError: Cannot read properties of undefined 'clientId'"
  at the end of certain debugging scenarios.

  When a browser is unable to complete a test run (i.e. some files
  don't load the the test gets stuck), the server will relay the
  console error messages, and eventually declare an idle timeout
  and stop the browser.

  In debug mode, we intercept the browser.stop() and leave it open.
  This works by swapping out the signal for a dummy signal.

  However, this meant that at the end of launchBrowserAttempt,
  where we check `signal.reason` that we have no signal.reason,
  and thus we try to return `result` instead of throwing.

  First: Remove the possiblity of this happening by not checking
  `signal.reason`. If we have no result, just throw. Returning
  undefined is not allowed under our contract and needlessly
  complicates the return handling for eventbus and reporters.

  Second: Fix the throw to throw the original signal's reason.
  The dummy signal should only affect what we send to the browser
  process in order to keep it open, we should not dummy ourselves
  as well. This means that if the test run finished normally then,
  once the developer ends their debug session we will return the
  result. Otherwise, we throw whatever the orignal reason was
  before we overrode it to keep the browser open (e.g. idle timeout).

While at it, remove debug statements from LocalBrowser.spawn that
are redundant with server.js/launchBrowser (browser_launch_exit,
and browser_launch_error). This was moved there so that browser
plugins for QTap won't need to worry about this.

Author: Krinkle

src/server.js

@@ -185,7 +185,7 @@ class ControlServer {
     }
     const url = await this.getProxyBase() + qtapUrlPath;
 
-    const maxTries = browserFn.allowRetries === false ? 1 : 3;
+    const maxTries = (browserFn.allowRetries === false || this.debugMode) ? 1 : 3;
     let i = 1;
     while (true) {
       try {
@@ -237,15 +237,14 @@ class ControlServer {
 
   async launchBrowserAttempt (browserFn, browserName, globalSignal, clientId, url, logger) {
     const controller = new AbortController();
-    let signal = controller.signal;
+    const signals = { browser: controller.signal, global: globalSignal };
     if (this.debugMode) {
       // Replace with a dummy signal that we never invoke
-      signal = (new AbortController()).signal;
+      signals.browser = (new AbortController()).signal;
       controller.signal.addEventListener('abort', () => {
         logger.warning('browser_signal_debugging', 'Keeping browser open for debugging');
       });
     }
-    const signals = { browser: signal, global: globalSignal };
 
     let clientIdleTimer;
 
@@ -374,16 +373,16 @@ class ControlServer {
       browser.stop(new util.BrowserStopSignal('Browser ended unexpectedly'));
     } catch (e) {
       // Silence any errors from browserFn that happen after we called browser.stop().
-      if (!signal.aborted) {
+      if (!controller.signal.aborted) {
         logger.warning('browser_launch_error', e);
         browser.stop(new util.BrowserStopSignal('Browser ended unexpectedly'));
         throw e;
       }
     }
 
-    if (!result && signal.reason) {
+    if (!result) {
       // Throw BrowserConnectTimeout for retry purposes.
-      throw signal.reason;
+      throw controller.signal.reason;
     }
 
     return result;

src/util.js

@@ -5,6 +5,8 @@ import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
 
+/** @import { Logger } from './qtap.js' */
+
 export const MIME_TYPES = {
   bin: 'application/octet-stream',
   css: 'text/css; charset=utf-8',
@@ -109,6 +111,7 @@ export const LocalBrowser = {
    * @param {Array<string>} args List of string arguments, passed to child_process.spawn()
    *  which will automatically quote and escape these.
    * @param {Object<string,AbortSignal>} signals
+   * @param {Logger} logger
    * @return {Promise<void>}
    */
   async spawn (paths, args, signals, logger) {
@@ -154,14 +157,12 @@ export const LocalBrowser = {
       spawned.on('exit', (code, sig) => {
         const indent = (str) => str.trim().split('\n').map(line => '    ' + line).join('\n');
         if (!code) {
-          logger.debug('browser_spawn_exit', `Process exited with code ${code}`);
           resolve();
         } else {
           const details = `Process exited with code ${code}`
             + (sig ? `\n  signal: ${sig}` : '')
             + (stderr ? `\n  stderr:\n${indent(stderr)}` : '')
             + (stdout ? `\n  stdout:\n${indent(stdout)}` : '');
-          logger.debug('browser_spawn_exit', details);
           reject(new Error(details));
         }
       });