From 02bb25e1453742f933d59643a2af001fb5b70816 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinkle@fastmail.com>
Date: Tue, 31 Dec 2024 01:24:16 +0000
Subject: [PATCH] build: switch to macos "large" to fix missing Firefox

The Arm64 images have been catered to Google and its Chrome browser
only, omitting Firefox.

Ref https://github.com/actions/runner-images/issues/9974
---
 .github/workflows/CI.yaml |  4 ++--
 src/browsers.js           | 15 ++++++++++++++-
 src/qtap.js               |  6 ++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/.github/workflows/CI.yaml b/.github/workflows/CI.yaml
index c2a39e1..e39ac9d 100644
--- a/.github/workflows/CI.yaml
+++ b/.github/workflows/CI.yaml
@@ -27,9 +27,9 @@ jobs:
             node: 20.x
 
           # Includes Firefox, Google Chrome, Safari
-          # https://github.com/actions/runner-images/blob/macos-15/20241217.493/images/macos/macos-15-Readme.md
+          # https://github.com/actions/runner-images/blob/macos-14/20241216.504/images/macos/macos-14-Readme.md
           - name: "macOS: Node 20"
-            os: macos-15
+            os: macos-14-large
             node: 20.x
 
     name: ${{ matrix.name }}
diff --git a/src/browsers.js b/src/browsers.js
index 2f35923..e8075c6 100644
--- a/src/browsers.js
+++ b/src/browsers.js
@@ -236,7 +236,20 @@ class FirefoxBrowser {
     try {
       await LocalBrowser.spawn(this.getCandidates(), args, signal, logger);
     } finally {
-      fs.rmSync(profileDir, { recursive: true, force: true });
+      // On Windows, when spawn() returns after firefox.exe has stopped, it sometimes can't delete
+      // some temporary files yet as they're somehow still in use (EBUSY). Perhaps a race condition,
+      // or an lagged background process?
+      // > BUSY: resource busy or locked,
+      // > unlink 'C:\Users\RUNNER~1\AppData\Local\Temp\qtap_EZ4IoO\bounce-tracking-protection.sqlite'
+      // Enable `maxRetries` to cover the common case, and beyond that try-catch
+      // as it is not critical for test completion.
+      // TODO: Can we abstract this so that it happens automatically for any directory
+      // obtained via LocalBrowser.makeTempDir?
+      try {
+        fs.rmSync(profileDir, { recursive: true, force: true, maxRetries: 2 });
+      } catch (e) {
+        logger.warning('firefox_cleanup_fail', e);
+      }
     }
   }
 }
diff --git a/src/qtap.js b/src/qtap.js
index 9b1a550..de027df 100644
--- a/src/qtap.js
+++ b/src/qtap.js
@@ -87,6 +87,12 @@ async function run (browserNames, files, options) {
     // exits naturally by itself.
     // TODO: Consider just calling process.exit after this await.
     // Is that faster and safe? What if any important clean up would we miss?
+    // 1. Removing of temp directories is generally done in browser "launch" functions
+    //    after the child process has properly ended (and has to, as otherwise the
+    //    files are likely still locked and/or may end up re-created). If we were to
+    //    exit earlier, we may leave temp directories behind. This is fine when running
+    //    in an ephemeral environment (e.g. CI), but not great for local dev.
+    //
     await Promise.allSettled(browserLaunches);
     // Await again, so that any error gets thrown accordingly,
     // we don't do this directly because we first want to wait for all tests