Harden e2e setup/teardown cleanup and scope the BCV nav helper to the top toolbar

.gitignore

@@ -40,6 +40,6 @@ temp-build
 # Playwright output
 e2e-tests/.cdp-*
 e2e-tests/.dev-*
-e2e-tests/.e2e-settings-backup
+e2e-tests/.e2e-settings-backup*
 e2e-tests/playwright-report
 e2e-tests/test-results

e2e-tests/fixtures/app.fixture.ts

@@ -5,13 +5,15 @@ import {
   ElectronApplication,
   Page,
   TestInfo,
+  WorkerInfo,
 } from '@playwright/test';
 import {
   E2E_SETTINGS_OVERRIDES,
   launchElectronWithExtension,
   preConfigureSettings,
   PROCESS_READY_TIMEOUT,
   teardownElectronApp,
+  describePageError,
 } from './helpers';
 
 export { expect } from '@playwright/test';
@@ -36,9 +38,12 @@ export const test = base.extend<TestAppFixtures, WorkerAppFixtures>({
   // avoiding the process startup/teardown cost per test.
   electronApp: [
     // eslint-disable-next-line no-empty-pattern
-    async ({}, use) => {
+    async ({}, use, workerInfo: WorkerInfo) => {
       console.log('[startup] Configuring settings for worker-scoped app launch...');
-      const restoreSettings = preConfigureSettings(E2E_SETTINGS_OVERRIDES);
+      const restoreSettings = preConfigureSettings(
+        E2E_SETTINGS_OVERRIDES,
+        workerInfo.config.workers,
+      );
       try {
         const ctx = await launchElectronWithExtension();
         try {
@@ -65,7 +70,7 @@ export const test = base.extend<TestAppFixtures, WorkerAppFixtures>({
      *
      * @param err The error thrown in the page context.
      */
-    const onPageError = (err: Error) => console.error(`Page error: ${err.message}`);
+    const onPageError = (err: Error) => console.error(`Page error: ${describePageError(err)}`);
 
     /**
      * Log console error messages from the page to the process console.

e2e-tests/global-setup.ts

@@ -5,6 +5,7 @@ import http from 'http';
 import net from 'net';
 import path from 'path';
 import fs from 'fs';
+import { killProcessTree } from './process-utils';
 
 export const WEBSOCKET_PORT = 8876;
 export const RENDERER_PORT = 1212;
@@ -13,6 +14,10 @@ export const RENDERER_PORT = 1212;
  * File the renderer dev server's PID is written to, for {@link globalTeardown} to kill it — and for
  * a CDP setup failure path to kill it too, since Playwright skips `globalTeardown` when
  * `globalSetup` throws.
+ *
+ * Names the dev server the CURRENT run spawned, never one it merely reused: a kill from here
+ * signals the recorded PID's whole process tree, so a foreign or already-exited PID could take down
+ * something unrelated.
  */
 export const DEV_SERVER_PID_FILE = path.join(__dirname, '.dev-server.pid');
 
@@ -149,9 +154,15 @@ export function waitForPort(port: number, timeout: number): Promise<void> {
  * (recording its PID for teardown). Shared by both the smoke {@link globalSetup} (whose fixture then
  * launches Electron) and the CDP setup (which launches Electron itself with remote debugging).
  *
+ * Self-cleaning on failure: if a dev server started here never becomes ready, it is killed before
+ * the error propagates (see {@link killSpawnedDevServer}), so neither caller leaks it. Callers
+ * therefore need no dev-server cleanup of their own for a bootstrap failure.
+ *
  * @returns Resolves when the renderer dev server is ready.
  * @throws {Error} If port 8876 is already in use (a running Platform.Bible would conflict).
  * @throws {Error} If the extension dist is missing.
+ * @throws {Error} If a dev server started here does not open port 1212 within 60s, or (outside CI)
+ *   fails the HTTP compilation probe within 120s.
  */
 export async function bootstrapRendererDevServer(): Promise<void> {
   const extensionRoot = path.resolve(__dirname, '..');
@@ -209,6 +220,9 @@ export async function bootstrapRendererDevServer(): Promise<void> {
   // Start the webpack dev server for the renderer if not already running
   if (await isPortInUse(RENDERER_PORT)) {
     console.log(`Renderer dev server already running on port ${RENDERER_PORT}.`);
+    // A marker on disk can only be a prior run's leftover here, and killing by a stale PID risks a
+    // process the OS has since recycled it onto. The reused server is left running instead.
+    removeDevServerPidMarker();
   } else {
     console.log('Starting paranext-core renderer dev server...');
     const devServer = spawn('npm', ['run', 'start:renderer'], {
@@ -221,33 +235,100 @@ export async function bootstrapRendererDevServer(): Promise<void> {
 
     devServer.unref();
 
-    if (devServer.pid) {
-      fs.writeFileSync(DEV_SERVER_PID_FILE, String(devServer.pid));
-    }
-
-    console.log(`Waiting for renderer dev server on port ${RENDERER_PORT}...`);
-    await waitForPort(RENDERER_PORT, 60_000);
-    console.log(
-      `Port ${RENDERER_PORT} is accepting connections. Waiting for webpack compilation...`,
-    );
-    // webpack-dev-middleware holds requests open until initial compilation finishes. Probe a
-    // renderer URL to opportunistically wait for compilation, but do not hard-fail CI on this
-    // probe because CI runners can be noisy and late-compiling; the fixture has a longer CI-ready
-    // timeout and will keep waiting for the renderer window to recover.
+    // From here on this run owns a spawned, detached dev server. Playwright skips globalTeardown
+    // when globalSetup throws, so a failure below must kill it on the spot or it survives the
+    // failed run, holds port 1212, and silently serves a stale bundle to every later run. The
+    // PID-marker write is inside this try for the same reason: a synchronous writeFileSync failure
+    // would otherwise leak the running server with no marker for any later teardown to find it by.
+    // Scoped to this branch only: the already-running branch above didn't start the server, so this
+    // run must leave it alone.
     try {
-      await waitForHttpOk(`http://127.0.0.1:${RENDERER_PORT}/`, 120_000);
-    } catch (error) {
-      if (!process.env.CI) throw error;
-      const message =
-        error instanceof Error ? error.message : 'Unknown renderer readiness probe failure';
-      console.warn(
-        `Renderer HTTP readiness probe timed out in CI: ${message}. Continuing with port-only readiness.`,
+      /**
+       * Rejects if the dev server could not be spawned. `spawn` reports that by emitting `'error'`
+       * asynchronously, and an unhandled `'error'` on an EventEmitter throws as an
+       * uncaughtException that no catch here could reach; racing it against the readiness wait
+       * routes it through this block's cleanup instead, and fails immediately rather than after the
+       * full 60s port wait.
+       */
+      const spawnFailed = new Promise<never>((_resolve, reject) => {
+        devServer.once('error', (error) => {
+          reject(new Error(`Failed to spawn the renderer dev server: ${error}`));
+        });
+      });
+      // Nothing awaits this once the waits below succeed, so mark it handled.
+      spawnFailed.catch(() => {});
+
+      if (devServer.pid) {
+        fs.writeFileSync(DEV_SERVER_PID_FILE, String(devServer.pid));
+      }
+
+      console.log(`Waiting for renderer dev server on port ${RENDERER_PORT}...`);
+      await Promise.race([waitForPort(RENDERER_PORT, 60_000), spawnFailed]);
+      console.log(
+        `Port ${RENDERER_PORT} is accepting connections. Waiting for webpack compilation...`,
       );
+      // webpack-dev-middleware holds requests open until initial compilation finishes. Probe a
+      // renderer URL to opportunistically wait for compilation, but do not hard-fail CI on this
+      // probe because CI runners can be noisy and late-compiling; the fixture has a longer CI-ready
+      // timeout and will keep waiting for the renderer window to recover.
+      try {
+        await waitForHttpOk(`http://127.0.0.1:${RENDERER_PORT}/`, 120_000);
+      } catch (error) {
+        if (!process.env.CI) throw error;
+        const message =
+          error instanceof Error ? error.message : 'Unknown renderer readiness probe failure';
+        console.warn(
+          `Renderer HTTP readiness probe timed out in CI: ${message}. Continuing with port-only readiness.`,
+        );
+      }
+    } catch (error) {
+      killSpawnedDevServer(devServer.pid);
+      throw error;
     }
     console.log('Renderer dev server is ready.');
   }
 }
 
+/**
+ * Kill a dev server this run spawned but never got ready, and clear its PID marker. Used only on
+ * {@link bootstrapRendererDevServer}'s failure path, where the caller's own cleanup cannot reach:
+ * the CDP setup's `cleanUpFailedLaunch` runs only for failures after the bootstrap returns, and
+ * Playwright skips `globalTeardown` entirely when `globalSetup` throws.
+ *
+ * Deliberately does not reuse `killProcessFromPidFile` from global-teardown.ts: importing it here
+ * would make setup and teardown mutually dependent (teardown already imports
+ * {@link DEV_SERVER_PID_FILE} from this module). The PID is in hand anyway, so the read-back that
+ * helper exists to do is unnecessary.
+ *
+ * Best-effort: every step swallows its own error, since this runs while an exception is already
+ * propagating and must not replace the real failure with a cleanup one.
+ *
+ * @param pid PID of the spawned dev server; `undefined` if the spawn never reported one, in which
+ *   case there is nothing to kill and no marker was written.
+ * @returns Nothing.
+ */
+function killSpawnedDevServer(pid: number | undefined): void {
+  if (!pid) return;
+  console.log(`Renderer dev server failed to become ready — stopping it (PID: ${pid})...`);
+  killProcessTree(pid, 'SIGTERM');
+  removeDevServerPidMarker();
+}
+
+/**
+ * Delete the dev server's PID marker so no later cleanup kills by the PID it holds. Best-effort: a
+ * filesystem error is warned about and swallowed, since one caller runs while an exception is
+ * already propagating and the other must not fail a healthy bootstrap over a marker.
+ */
+function removeDevServerPidMarker(): void {
+  try {
+    fs.rmSync(DEV_SERVER_PID_FILE, { force: true });
+  } catch (error) {
+    console.warn(
+      `Could not remove renderer dev server PID marker ${DEV_SERVER_PID_FILE}: ${error}`,
+    );
+  }
+}
+
 /**
  * Playwright global setup for the smoke config. Runs once before any test worker starts. Bootstraps
  * the renderer dev server via {@link bootstrapRendererDevServer}; the smoke fixture

e2e-tests/global-teardown.ts

@@ -14,6 +14,24 @@ export interface KillFromPidFileResult {
   pid?: number;
 }
 
+/**
+ * Remove a PID marker file, swallowing any filesystem error. Separate from the kill it accompanies
+ * so a marker-removal failure cannot be mistaken for a kill failure (see the call sites in
+ * {@link killProcessFromPidFile}). A leftover marker is harmless — the next run re-reads it and
+ * finds a dead PID — so warning and continuing beats aborting teardown.
+ *
+ * @param pidFile Absolute path to the PID marker file to remove.
+ * @param label Human-readable name of the process the marker belongs to, used in the warning log.
+ * @returns Nothing.
+ */
+function removePidFile(pidFile: string, label: string): void {
+  try {
+    fs.unlinkSync(pidFile);
+  } catch (e) {
+    console.warn(`Could not remove ${label} PID marker ${pidFile}: ${e}`);
+  }
+}
+
 /**
  * Kill a process recorded in a PID file (whole tree), then remove the PID file. Shared by this
  * teardown's dev-server kill and {@link globalTeardownCdp}'s launched-app kill, which follow the
@@ -23,7 +41,9 @@ export interface KillFromPidFileResult {
  * A missing PID file is a no-op; a file whose contents don't parse as an integer is warned about
  * and skipped rather than used to kill an arbitrary PID. The PID file is always removed when
  * present, so no run leaves a stale marker behind. A filesystem error (concurrent deletion, a
- * locked file) is warned about and swallowed so teardown continues rather than aborting.
+ * locked file) is warned about and swallowed so teardown continues rather than aborting — with the
+ * marker removal guarded separately (see {@link removePidFile}) so it cannot downgrade a successful
+ * kill to `killed: false`.
  *
  * @param pidFile Absolute path to the file holding the target process's PID.
  * @param signal Kill signal to send (`'SIGKILL'` when the target may ignore SIGTERM).
@@ -44,12 +64,16 @@ export function killProcessFromPidFile(
     const pid = parseInt(fs.readFileSync(pidFile, 'utf-8').trim(), 10);
     if (Number.isNaN(pid)) {
       console.warn(`Invalid PID in ${pidFile}, skipping ${label} kill`);
-      fs.unlinkSync(pidFile);
+      removePidFile(pidFile, label);
       return { killed: false };
     }
     console.log(`Stopping ${label} (PID: ${pid})...`);
     const killed = killProcessTree(pid, signal);
-    fs.unlinkSync(pidFile);
+    // Remove the marker independently of the kill result: an fs error here says nothing about
+    // whether the kill succeeded, and callers act on `killed`/`pid` (globalTeardownCdp waits for the
+    // PID to exit before removing the app's user-data dir). Folding this into the outer catch would
+    // report a successful kill as `killed: false` and skip that wait, hitting a live SingletonLock.
+    removePidFile(pidFile, label);
     return { killed, pid };
   } catch (e) {
     console.warn(`Failed to kill ${label} from ${pidFile}: ${e}`);

e2e-tests/process-utils.ts

@@ -49,6 +49,17 @@ export function killProcessTree(pid: number, signal: NodeJS.Signals = 'SIGTERM')
   }
 }
 
+/**
+ * How long either tier waits for a SIGKILLed app to actually exit before touching the files it held
+ * (its user-data dir, and paranext-core's shared dev-appdata settings the run seeded). One budget
+ * for both so the two paths that guard the same settings file cannot drift apart.
+ *
+ * SIGKILL gives the app no chance to flush settings on the way out, so what protects the seeded
+ * file is restoring it only after the kill — never the length of this wait, which merely keeps a
+ * still-open user-data dir from failing removal.
+ */
+export const POST_SIGKILL_EXIT_WAIT_MS = 3_000;
+
 /**
  * Bound the wait for a killed process to actually exit without blindly sleeping the full timeout
  * when it dies sooner. Use before touching files the process may still hold open (e.g. its