Replace an unusable binary rather than re-spawning it

Addresses the three findings from review.

The recovery added for a corrupt binary only worked when the unlink
succeeded. When it failed -- the locked-file case this branch is about
-- binaryPath() handed the same file straight back, because checkPath
uses X_OK and Windows treats that as F_OK, so the binary was re-spawned
for every remaining retry with no wait. Wait for the lock before
replacing it, and stop rather than retry when the file survives.

download() returned on a source-url error without calling back, so an
invalid key or network failure left Local.start() waiting. Pre-existing,
but the same contract the previous commit closed.

download.js printed Done from the close handler, which node emits after
error too, so downloadSync accepted a partially written binary as a
completed download. Guard the log, and treat a non-zero exit status as a
failed attempt before inspecting stdout.

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

Author: pranay-v29

lib/Local.js

@@ -84,8 +84,12 @@ function Local(){
       if(that.retriesLeft > 0) {
         console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
         that.retriesLeft -= 1;
-        /* EPERM on a locked file threw straight out of startSync. */
+        if(that.binary) that.binary.waitWhileBinaryBusySync(that.binaryPath);
         try { fs.unlinkSync(that.binaryPath); } catch(err) { /* ignored */ }
+        /* Still there: binaryPath() would hand back the same unusable file. */
+        if(fs.existsSync(that.binaryPath)) {
+          return new LocalError(binaryDownloadErrorMessage);
+        }
         delete(that.binaryPath);
         that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
         that.binaryDownloadState.fallbackEnabled = true;
@@ -125,11 +129,21 @@ function Local(){
           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);
+            var replace = function(waitsLeft) {
+              if(waitsLeft > 0 && fs.existsSync(that.binaryPath) &&
+                 that.binary && that.binary.isBinaryBusy(that.binaryPath)) {
+                return setTimeout(function() { replace(waitsLeft - 1); }, 1000);
+              }
+              try { fs.unlinkSync(that.binaryPath); } catch(err) { /* ignored */ }
+              if(fs.existsSync(that.binaryPath)) {
+                return callback(new LocalError(binaryDownloadErrorMessage));
+              }
+              delete(that.binaryPath);
+              that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
+              that.binaryDownloadState.fallbackEnabled = true;
+              that.start(options, callback);
+            };
+            replace(3);
             return;
           } else {
             callback(new LocalError(error.toString()));

lib/LocalBinary.js

@@ -241,6 +241,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.status !== 0) {
+        that.binaryDownloadError('Download failed with status', String(obj.status));
+        return that.retryBinaryDownload(conf, destParentDir, null, retries, binaryPath);
+      }
       if(obj.error) {
         that.binaryDownloadError('Download failed with error', util.format(obj.error));
         return that.retryBinaryDownload(conf, destParentDir, null, retries, binaryPath);
@@ -268,7 +272,8 @@ function LocalBinary(){
   this.download = function(conf, destParentDir, callback, retries){
     this.getDownloadPath(conf, retries, (err, downloadUrl) => {
       if(err) {
-        return console.error('Unable to fetch the source url to download the binary with error: ', err);
+        console.error('Unable to fetch the source url to download the binary with error: ', err);
+        return callback();
       }
 
       this.httpPath = downloadUrl;

lib/download.js

@@ -65,6 +65,7 @@ request = https.get(options, function (response) {
     console.error('Got Error in binary download response', err);
   });
   fileStream.on('close', function () {
+    if(process.exitCode === 1) return; // errored; not a completed download
     console.log('Done');
   });
 }).on('error', function(err) {

test/local_binary_busy_download.js

@@ -3,7 +3,8 @@ var expect = require('expect.js'),
     fs = require('fs'),
     os = require('os'),
     path = require('path'),
-    LocalBinary = require('../lib/LocalBinary');
+    LocalBinary = require('../lib/LocalBinary'),
+    browserstack = require('../index');
 
 // Regression tests for LOC-7420.
 //
@@ -93,6 +94,42 @@ describe('LocalBinary busy-binary download handling', function () {
     });
   });
 
+  describe('source url failure', function () {
+    it('completes the callback when the download url cannot be fetched', function (done) {
+      var binary = new LocalBinary();
+      binary.getDownloadPath = function (conf, retries, cb) { cb(new Error('invalid key')); };
+      binary.download({}, os.tmpdir(), function (binaryPath) {
+        expect(binaryPath).to.be(undefined);
+        done();
+      }, 9);
+    });
+  });
+
+  describe('unremovable binary', function () {
+    // An unusable binary whose unlink fails used to be handed straight back by
+    // binaryPath() and re-spawned for every remaining retry.
+    it('does not retry when the binary cannot be replaced', function () {
+      var dir = fs.mkdtempSync(path.join(os.tmpdir(), 'bs-local-')),
+          binaryPath = path.join(dir, 'BrowserStackLocal');
+      fs.writeFileSync(binaryPath, 'not executable', { mode: 0o644 });
+      fs.chmodSync(dir, 0o555); // so the unlink fails
+
+      try {
+        var bsLocal = new browserstack.Local();
+        bsLocal.binaryPath = binaryPath;
+        var result = bsLocal.startSync({ key: 'dummy-key' });
+
+        expect(result).to.be.a(Object);
+        expect(result.message).to.contain('Error while trying to execute binary');
+        expect(bsLocal.retriesLeft).to.equal(8); // one attempt, not nine
+      } finally {
+        fs.chmodSync(dir, 0o755);
+        fs.unlinkSync(binaryPath);
+        fs.rmdirSync(dir);
+      }
+    });
+  });
+
   describe('isBinaryBusy', function () {
     it('reports a readable file as free', function () {
       var binary = new LocalBinary(),
@@ -129,6 +166,7 @@ describe('LocalBinary busy-binary download handling', function () {
       expect(stderr).to.contain('Got Error while downloading binary file');
       // The signature of the old defect: node's unhandled-'error' bail-out.
       expect(stderr).to.not.contain('Unhandled \'error\' event');
+      expect(obj.stdout.toString()).to.not.contain('Done');
       expect(obj.status).to.equal(1);
 
       fs.rmdirSync(target);