LOC-7420: tolerate a busy binary instead of crashing the consumer

On Windows, BrowserStackLocal.exe in ~/.browserstack is routinely
unopenable for a moment -- an AV scan of a freshly written executable, a
tunnel still releasing its handle, two test workers starting at once --
and the open fails with EBUSY/EPERM. POSIX allows opening and unlinking a
file in use, so this only manifests on Windows. Several defects turned
that transient condition into a hard crash before any session started.

download.js and LocalBinary.js registered their write-stream 'error'
handler inside the async https.get callback. createWriteStream fails at
the open() syscall and emits on the next tick, well before the TLS round
trip completes, so the error arrived with no listener and node's
`throw er` killed the download child. The handlers now attach
immediately after createWriteStream.

Handling that error is not sufficient on its own: the request is still in
flight, and without tearing it down the child stays alive downloading
into a stream nobody reads, so the parent's spawnSync blocks for a whole
download before it can retry. The throw was also doing the job of
stopping the download; download.js now destroys the request explicitly.

retryBinaryDownload did its work inside an async callback, so on the sync
path it returned undefined to a caller that had already given up -- which
surfaced as "Couldn't find binary file" while the retries carried on,
orphaned, in the background. This happened even when the unlink
succeeded, so it is not a consequence of the EPERM. The sync path is now
synchronous end to end and returns its result.

Retrying instantly against a live lock just burns the retry budget in
milliseconds, so a busy binary is now probed (openSync 'r+') and waited
on, bounded, rather than deleted.

spawnSync reports spawn-level failures through obj.error, leaving stdout
null; reading .length threw a TypeError that replaced the real cause in
the reported message, after which the binary was deleted anyway.
execFile raises the same class of failure synchronously rather than
through its callback -- EBADARCH from a binary built for another
architecture, for instance -- and inside the getBinaryPath callback an
uncaught throw there killed the consumer's process outright, the same
failure mode as the download path. Both are now handled, and the
unlinkSync calls in the error paths no longer throw out of start().

Tests cover the retry return value, the retry ceiling, the busy probe and
the unhandled-'error' regression. They force the open to fail rather than
reproducing a lock, since the defect is any createWriteStream failure
rather than EBUSY specifically -- so they need no Windows runner, network
or credentials.

.eslintrc.json moves from es6 to es2017 for Atomics/SharedArrayBuffer,
used for the bounded wait on the sync path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: pranay-v29

.eslintrc.json

@@ -20,7 +20,7 @@
     "no-constant-condition": 0
   },
   "env": {
-    "es6": true,
+    "es2017": true,
     "node": true
   },
   "extends": "eslint:recommended",

lib/Local.js

@@ -58,6 +58,11 @@ function Local(){
     }
     try{
       const obj = childProcess.spawnSync(that.binaryPath, that.getBinaryArgs());
+      /* stdout is null on a spawn-level failure; reading .length below threw a
+         TypeError that replaced the real cause in the reported message. */
+      if(obj.error) {
+        throw obj.error;
+      }
       this.tunnel = {pid: obj.pid};
       var data = {};
       if(obj.stdout.length > 0)
@@ -79,7 +84,10 @@ function Local(){
       if(that.retriesLeft > 0) {
         console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
         that.retriesLeft -= 1;
-        fs.unlinkSync(that.binaryPath);
+        /* An unremovable binary is not a reason to abort the retry: on Windows
+           this is EPERM on a locked file, and it threw straight out of
+           startSync, killing the consumer's run. */
+        try { fs.unlinkSync(that.binaryPath); } catch(err) { /* ignored */ }
         delete(that.binaryPath);
         that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
         that.binaryDownloadState.fallbackEnabled = true;
@@ -107,46 +115,59 @@ function Local(){
       }
 
       that.opcode = 'start';
-      that.tunnel = childProcess.execFile(that.binaryPath, that.getBinaryArgs(), function(error, stdout, stderr){
-        if(error) {
-          const binaryDownloadErrorMessage = `Error while trying to execute binary: ${util.format(error)}`;
-          console.error(binaryDownloadErrorMessage);
-          if(that.retriesLeft > 0) {
-            console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
-            that.retriesLeft -= 1;
-            fs.unlinkSync(that.binaryPath);
-            delete(that.binaryPath);
-            that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
-            that.binaryDownloadState.fallbackEnabled = true;
-            that.start(options, callback);
-            return;
-          } else {
-            callback(new LocalError(error.toString()));
-            return;
-          }
-        }
 
-        var data = {};
-        var output = stdout || stderr;
-        if(!output) {
-          callback(new LocalError('No output received'));
-          return;
-        }
-        try {
-          data = JSON.parse(output);
-        } catch(parseError) {
-          callback(new LocalError('Invalid output received: ' + parseError.message, output));
+      var onBinaryError = function(error) {
+        const binaryDownloadErrorMessage = `Error while trying to execute binary: ${util.format(error)}`;
+        console.error(binaryDownloadErrorMessage);
+        if(that.retriesLeft > 0) {
+          console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
+          that.retriesLeft -= 1;
+          try { fs.unlinkSync(that.binaryPath); } catch(err) { /* ignored */ }
+          delete(that.binaryPath);
+          that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
+          that.binaryDownloadState.fallbackEnabled = true;
+          that.start(options, callback);
           return;
         }
+        callback(new LocalError(error.toString()));
+      };
+
+      /* execFile raises spawn-level failures synchronously rather than through
+         its callback -- EBADARCH from a binary built for another architecture,
+         for instance. This runs inside the getBinaryPath callback, so an
+         uncaught throw here kills the consumer's process outright: the same
+         failure mode as the download path, and equally not something a caller
+         can wrap. */
+      try {
+        that.tunnel = childProcess.execFile(that.binaryPath, that.getBinaryArgs(), function(error, stdout, stderr){
+          if(error) {
+            return onBinaryError(error);
+          }
 
-        if(data['state'] != 'connected'){
-          callback(new LocalError(that.getErrorMessage(data)));
-        } else {
-          that.pid = data['pid'];
-          that.isProcessRunning = true;
-          callback();
-        }
-      });
+          var data = {};
+          var output = stdout || stderr;
+          if(!output) {
+            callback(new LocalError('No output received'));
+            return;
+          }
+          try {
+            data = JSON.parse(output);
+          } catch(parseError) {
+            callback(new LocalError('Invalid output received: ' + parseError.message, output));
+            return;
+          }
+
+          if(data['state'] != 'connected'){
+            callback(new LocalError(that.getErrorMessage(data)));
+          } else {
+            that.pid = data['pid'];
+            that.isProcessRunning = true;
+            callback();
+          }
+        });
+      } catch(spawnError) {
+        onBinaryError(spawnError);
+      }
     }, options['bs-host']);
   };
 

lib/LocalBinary.js

@@ -71,6 +71,11 @@ function LocalBinary(){
       env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key;
     }
     const obj = childProcess.spawnSync(cmd, opts, { env: env });
+    /* On a spawn-level failure stdout is null, so reading .length here threw a
+       TypeError that masked the real cause. */
+    if(obj.error) {
+      throw(util.format(obj.error));
+    }
     if(obj.stdout.length > 0) {
       this.sourceURL = obj.stdout.toString().replace(/\n+$/, '');
       this.downloadState.sourceURL = this.sourceURL;
@@ -148,23 +153,63 @@ function LocalBinary(){
     this.downloadErrorMessage = errorMessagePrefix + ' : ' + errorMessage;
   };
 
+  /* A locked binary is a transient, ordinary condition on Windows (AV scan, a
+     tunnel still releasing its handle, a concurrent run), not a corrupt file.
+     Mirrors the probe the CLI binary already uses. */
+  this.BUSY_ERROR_CODES = ['EBUSY', 'EPERM', 'ETXTBSY', 'EACCES'];
+  this.BUSY_MAX_WAITS = 3;
+  this.BUSY_WAIT_MS = 1000;
+
+  this.isBinaryBusy = function(binaryPath) {
+    try {
+      fs.closeSync(fs.openSync(binaryPath, 'r+'));
+      return false;
+    } catch(err) {
+      return this.BUSY_ERROR_CODES.indexOf(err.code) !== -1;
+    }
+  };
+
+  /* Blocking by design: the sync download path has no event loop to come back
+     to, so the wait has to happen inline. */
+  this.waitWhileBinaryBusySync = function(binaryPath) {
+    for(var i = 0; i < this.BUSY_MAX_WAITS; i++) {
+      if(!fs.existsSync(binaryPath) || !this.isBinaryBusy(binaryPath)) return;
+      console.log('Binary is in use, waiting before retrying.');
+      Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, this.BUSY_WAIT_MS);
+    }
+  };
+
   this.retryBinaryDownload = function(conf, destParentDir, callback, retries, binaryPath) {
     var that = this;
-    if(retries > 0) {
-      console.log('Retrying Download. Retries left', retries);
-      /* Single unlink instead of stat-then-unlinkSync: the gap between the two
-         let a concurrent writer swap the file, and a failing unlinkSync threw
-         out of the stat callback where it could not be caught. A missing file
-         is the expected case here, so any error is ignored. */
+    if(retries <= 0) {
+      console.error('Number of retries to download exceeded.');
+      return;
+    }
+    console.log('Retrying Download. Retries left', retries);
+
+    /* The sync path must stay synchronous end to end: its return value is what
+       downloadSync -> binaryPath() -> Local.getBinaryPath hands back. Doing the
+       unlink in a callback returned undefined to the caller before the retry had
+       done anything, which surfaced as "Couldn't find binary file" while the
+       retries carried on, orphaned, in the background. */
+    if(!callback) {
+      that.waitWhileBinaryBusySync(binaryPath);
+      /* A missing file is the expected case, and a still-locked one is better
+         re-downloaded over than crashed on. */
+      try { fs.unlinkSync(binaryPath); } catch(err) { /* ignored */ }
+      return that.downloadSync(conf, destParentDir, retries - 1);
+    }
+
+    var attemptAsync = function(waitsLeft) {
+      if(waitsLeft > 0 && fs.existsSync(binaryPath) && that.isBinaryBusy(binaryPath)) {
+        console.log('Binary is in use, waiting before retrying.');
+        return setTimeout(function() { attemptAsync(waitsLeft - 1); }, that.BUSY_WAIT_MS);
+      }
       fs.unlink(binaryPath, function() {
-        if(!callback) {
-          return that.downloadSync(conf, destParentDir, retries - 1);
-        }
         that.download(conf, destParentDir, callback, retries - 1);
       });
-    } else {
-      console.error('Number of retries to download exceeded.');
-    }
+    };
+    attemptAsync(that.BUSY_MAX_WAITS);
   };
 
   this.downloadSync = function(conf, destParentDir, retries) {
@@ -198,6 +243,10 @@ function LocalBinary(){
       const userAgent = [packageName, version].join('/');
       const env = Object.assign({ 'USER_AGENT': userAgent }, process.env);
       const obj = childProcess.spawnSync(cmd, opts, { env: env });
+      if(obj.error) {
+        that.binaryDownloadError('Download failed with error', util.format(obj.error));
+        return that.retryBinaryDownload(conf, destParentDir, null, retries, binaryPath);
+      }
       let output;
       if(obj.stdout.length > 0) {
         if(fs.existsSync(binaryPath)){
@@ -234,6 +283,23 @@ function LocalBinary(){
       var binaryPath = path.join(destParentDir, destBinaryName);
       var fileStream = fs.createWriteStream(binaryPath);
 
+      /* Now that the stream handler is registered before https.get, a failed
+         open and the in-flight request can both report an error for the same
+         attempt. Collapse them: one attempt triggers at most one retry. */
+      var retried = false;
+      var retryOnce = function(prefix, err) {
+        that.binaryDownloadError(prefix, util.format(err));
+        if(retried) return;
+        retried = true;
+        that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
+      };
+
+      /* Same reasoning as lib/download.js: the open() failure lands before the
+         https.get callback can register a listener. */
+      fileStream.on('error', function (err) {
+        retryOnce('Got Error while downloading binary file', err);
+      });
+
       var options = url.parse(this.httpPath);
       if(conf.proxyHost && conf.proxyPort) {
         options.agent = new HttpsProxyAgent({
@@ -267,21 +333,15 @@ function LocalBinary(){
         }
 
         response.on('error', function(err) {
-          that.binaryDownloadError('Got Error in binary download response', util.format(err));
-          that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
-        });
-        fileStream.on('error', function (err) {
-          that.binaryDownloadError('Got Error while downloading binary file', util.format(err));
-          that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
+          retryOnce('Got Error in binary download response', err);
         });
         fileStream.on('close', function () {
           fs.chmod(binaryPath, '0755', function() {
             callback(binaryPath);
           });
         });
       }).on('error', function(err) {
-        that.binaryDownloadError('Got Error in binary downloading request', util.format(err));
-        that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
+        retryOnce('Got Error in binary downloading request', err);
       });
     });
   };

lib/download.js

@@ -9,6 +9,24 @@ const binaryPath = process.argv[2], httpPath = process.argv[3], proxyHost = proc
 
 var fileStream = fs.createWriteStream(binaryPath);
 
+/* Attached synchronously, before the async https.get below. createWriteStream
+   fails at the open() syscall and emits 'error' on the next tick -- on Windows
+   an EBUSY/EPERM when the .exe is held by AV, a shutting-down tunnel or a
+   concurrent run. Registering the handler inside the https.get callback left a
+   window with no listener, which Node turns into an unhandled 'error' and a
+   hard `throw er` that killed this child. */
+var request;
+
+fileStream.on('error', function (err) {
+  console.error('Got Error while downloading binary file', err);
+  process.exitCode = 1;
+  /* The request is still in flight. Without tearing it down this child stays
+     alive downloading into a stream nobody is reading, and the parent's
+     spawnSync blocks for a whole download before it can retry — where the
+     unhandled 'error' at least failed immediately. */
+  if(request) request.destroy();
+});
+
 var options = url.parse(httpPath);
 /* isUndefined, not plain truthiness: the parent passes literal `undefined`
    placeholders for the proxy slots when only a CA is configured, and those
@@ -37,7 +55,7 @@ options.headers = Object.assign({}, options.headers, {
   'user-agent': process.env.USER_AGENT,
 });
 
-https.get(options, function (response) {
+request = https.get(options, function (response) {
   const contentEncoding = response.headers['content-encoding'];
   if (typeof contentEncoding === 'string' && contentEncoding.match(/gzip/i)) {
     if (process.env.BROWSERSTACK_LOCAL_DEBUG_GZIP) {
@@ -52,12 +70,11 @@ https.get(options, function (response) {
   response.on('error', function(err) {
     console.error('Got Error in binary download response', err);
   });
-  fileStream.on('error', function (err) {
-    console.error('Got Error while downloading binary file', err);
-  });
   fileStream.on('close', function () {
     console.log('Done');
   });
 }).on('error', function(err) {
+  /* Already reported and already exiting; this is our own destroy() landing. */
+  if(process.exitCode === 1) return;
   console.error('Got Error in binary downloading request', err);
 });