LOC-7325: third review round — download-flow hardening + payload semantics

- start() now parses binary output BEFORE the retry gate: a structured
  payload means the binary executed, so deterministic failures fail fast
  with the payload's message instead of 9 delete/re-download cycles; an
  unusable payload message keeps the raw payload as error.extra.
- A connected payload with pid is treated as success even when the
  foreground process exits non-zero (matches startSync), instead of
  reporting an error while isRunning() is true.
- Invalid (unparseable) output from a downloaded binary evicts it —
  without retrying — so the next start self-heals with a fresh download;
  user-supplied binarypath binaries are never evicted.
- LocalBinary.download(): gunzip stream gets an 'error' handler routed
  into the settled retry path (corrupt gzip was an uncatchable crash or
  a silent hang); non-2xx responses no longer write the error body to
  the binary and report success; source-url failures retry without
  deleting a pre-existing binary (also removes the this.windows-unset
  wrong-path trap); retryBinaryDownload's unlinkSync is guarded; retry
  exhaustion delivers callback(null) and Local.start maps a falsy path
  to a terminal LocalError carrying the recorded download error, instead
  of ENOENT-driven retry cascades (~100 attempts worst case).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: vivianludrick

lib/Local.js

@@ -18,6 +18,7 @@ function Local(){
   this.windows = os.platform().match(/mswin|msys|mingw|cygwin|bccwin|wince|emc|win32/i);
   this.pid = undefined;
   this.isProcessRunning = false;
+  this.userProvidedBinaryPath = false;
   this.retriesLeft = 9;
   this.key = process.env.BROWSERSTACK_ACCESS_KEY;
   this.logfile = this.sanitizePath(path.join(process.cwd(), 'local.log'));
@@ -52,8 +53,14 @@ function Local(){
         throw obj.error;
       this.tunnel = {pid: obj.pid};
       var result = that.parseBinaryOutput(obj.stdout, obj.stderr);
-      if(result.error)
+      if(result.error) {
+        if(result.invalidOutput) {
+          // A cached binary that runs but prints garbage may be corrupt on
+          // disk; evict it so the next start re-downloads a fresh copy.
+          that.evictDownloadedBinary();
+        }
         return result.error;
+      }
       var data = result.data;
       if(data['state'] != 'connected'){
         return new LocalError(that.getErrorMessage(data));
@@ -83,6 +90,12 @@ function Local(){
       return callback();
 
     this.getBinaryPath(function(binaryPath){
+      if(!binaryPath){
+        // Terminal download failure signalled by LocalBinary (falsy path);
+        // retrying here would only re-run the whole download cascade.
+        var downloadErrorMessage = (that.binary && that.binary.downloadErrorMessage) || 'Unable to download BrowserStack Local binary';
+        return callback(new LocalError(downloadErrorMessage));
+      }
       that.binaryPath = binaryPath;
       try {
         fs.writeFileSync(that.logfile, '');
@@ -102,36 +115,53 @@ function Local(){
           callback(err);
         };
         try {
+          var result = that.parseBinaryOutput(stdout, stderr);
           if(error) {
             const binaryDownloadErrorMessage = `Error while trying to execute binary: ${util.format(error)}`;
             console.error(binaryDownloadErrorMessage);
+            if(result.data) {
+              // The binary executed and reported a structured result, so the
+              // failure is not a corrupt download — retrying (delete +
+              // re-download) cannot help; fail fast with the richer diagnostic
+              // instead of burning the retry budget first.
+              if(result.data['state'] == 'connected' && result.data['pid']) {
+                // The daemon came up even though the foreground process exited
+                // non-zero; treat it as success so isRunning()/stop() agree
+                // with reality (startSync likewise ignores the exit status
+                // when the payload says connected).
+                that.pid = result.data['pid'];
+                that.isProcessRunning = true;
+                safeCallback();
+                return;
+              }
+              var payloadMessage = that.extractErrorMessage(result.data);
+              if(payloadMessage) {
+                safeCallback(new LocalError(payloadMessage));
+                return;
+              }
+              // Payload parsed but carries no usable message: surface the
+              // exec error and keep the raw payload as extra.
+              var rawPayload = (stdout && stdout.length > 0) ? stdout : stderr;
+              safeCallback(new LocalError(error.toString(), that.truncateForExtra(rawPayload)));
+              return;
+            }
             if(that.retriesLeft > 0) {
               that.prepareBinaryRetry(binaryDownloadErrorMessage);
               that.start(options, callback);
               return;
             }
-            // The binary can exit non-zero while still printing a JSON
-            // diagnostic on stdout; keep whichever diagnostic is richer.
-            var failure = that.parseBinaryOutput(stdout, stderr);
-            if(failure.data && failure.data['state'] == 'connected' && failure.data['pid']) {
-              // The daemon came up even though the foreground process
-              // errored; record the pid so a later stop() can still reach it.
-              that.pid = failure.data['pid'];
-              that.isProcessRunning = true;
-            }
-            var payloadMessage = (failure.data && failure.data['state'] != 'connected') ? that.extractErrorMessage(failure.data) : null;
-            if(payloadMessage) {
-              safeCallback(new LocalError(payloadMessage));
-            } else {
-              // Keep any raw (non-JSON) output as extra — it usually holds
-              // the crash text.
-              safeCallback(new LocalError(error.toString(), failure.error && failure.error.extra));
-            }
+            // Keep any raw (non-JSON) output as extra — it usually holds
+            // the crash text.
+            safeCallback(new LocalError(error.toString(), result.error && result.error.extra));
             return;
           }
 
-          var result = that.parseBinaryOutput(stdout, stderr);
           if(result.error) {
+            if(result.invalidOutput) {
+              // A cached binary that runs but prints garbage may be corrupt
+              // on disk; evict it so the next start re-downloads a fresh copy.
+              that.evictDownloadedBinary();
+            }
             safeCallback(result.error);
             return;
           }
@@ -208,13 +238,27 @@ function Local(){
     try {
       data = JSON.parse(output);
     } catch(parseError) {
-      return { error: new LocalError('Invalid output received: ' + parseError.message, this.truncateForExtra(output)) };
+      return { error: new LocalError('Invalid output received: ' + parseError.message, this.truncateForExtra(output)), invalidOutput: true };
     }
     if(!data || typeof data !== 'object')
-      return { error: new LocalError('Invalid output received: expected a JSON object', this.truncateForExtra(output)) };
+      return { error: new LocalError('Invalid output received: expected a JSON object', this.truncateForExtra(output)), invalidOutput: true };
     return { data: data };
   };
 
+  // A binary that executes but prints unparseable output may be corrupt on
+  // disk; evicting it lets the next start() download a fresh copy instead of
+  // failing identically forever. Binaries supplied by the user via the
+  // `binarypath` option are never evicted.
+  this.evictDownloadedBinary = function(){
+    if(this.userProvidedBinaryPath || !this.binaryPath) return;
+    try {
+      fs.unlinkSync(this.binaryPath);
+    } catch(unlinkError) {
+      console.error('Could not delete binary: ', unlinkError.message);
+    }
+    delete(this.binaryPath);
+  };
+
   this.isRunning = function(){
     return this.pid && running(this.pid) && this.isProcessRunning;
   };
@@ -324,8 +368,10 @@ function Local(){
         break;
 
       case 'binarypath':
-        if(value)
+        if(value){
           this.binaryPath = value;
+          this.userProvidedBinaryPath = true;
+        }
         break;
 
       default:

lib/LocalBinary.js

@@ -137,7 +137,13 @@ function LocalBinary(){
       console.log('Retrying Download. Retries left', retries);
       fs.stat(binaryPath, function(err) {
         if(err == null) {
-          fs.unlinkSync(binaryPath);
+          try {
+            fs.unlinkSync(binaryPath);
+          } catch(unlinkError) {
+            // A held handle (AV scan) or permissions can make the delete
+            // fail; the retry will overwrite the file anyway.
+            console.error('Could not delete binary: ', unlinkError.message);
+          }
         }
         if(!callback) {
           return that.downloadSync(conf, destParentDir, retries - 1);
@@ -146,11 +152,10 @@ function LocalBinary(){
       });
     } else {
       console.error('Number of retries to download exceeded.');
-      // Still hand the (missing or truncated) path back instead of never
-      // calling the callback: the caller's execFile then fails with a real
-      // error the user sees, rather than hanging forever.
+      // Deliver terminal failure instead of never calling back; Local.start
+      // treats a falsy path as a non-retryable download failure.
       if(callback) {
-        callback(binaryPath);
+        callback(null);
       }
     }
   };
@@ -209,11 +214,18 @@ function LocalBinary(){
   this.download = function(conf, destParentDir, callback, retries){
     this.getDownloadPath(conf, retries, (err, downloadUrl) => {
       if(err) {
-        // Route through the retry path (which eventually surfaces a failure
-        // to the caller) instead of returning without ever calling back.
+        // Nothing has touched the disk on this path, so retry the source-url
+        // fetch directly — going through retryBinaryDownload here would
+        // delete a pre-existing binary that this attempt never wrote to.
         this.binaryDownloadError('Unable to fetch the source url to download the binary with error', util.format(err));
-        var destName = (this.windows) ? 'BrowserStackLocal.exe' : 'BrowserStackLocal';
-        return this.retryBinaryDownload(conf, destParentDir, callback, retries, path.join(destParentDir, destName));
+        if(retries > 0) {
+          console.log('Retrying Download. Retries left', retries);
+          return this.download(conf, destParentDir, callback, retries - 1);
+        }
+        console.error('Number of retries to download exceeded.');
+        // Deliver terminal failure instead of never calling back; Local.start
+        // treats a falsy path as a non-retryable download failure.
+        return callback(null);
       }
 
       this.httpPath = downloadUrl;
@@ -256,13 +268,33 @@ function LocalBinary(){
       });
 
       https.get(options, function (response) {
+        if (response.statusCode >= 400) {
+          // Without this check, an error body (404/403 HTML) was written to
+          // binaryPath and reported as a successful download.
+          if(settle()) return;
+          response.resume();
+          fileStream.destroy();
+          that.binaryDownloadError('Got bad response while downloading binary, status code', String(response.statusCode));
+          return that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
+        }
+
         const contentEncoding = response.headers['content-encoding'];
         if (typeof contentEncoding === 'string' && contentEncoding.match(/gzip/i)) {
           if (process.env.BROWSERSTACK_LOCAL_DEBUG_GZIP) {
             console.info('Using gzip in ' + options.headers['user-agent']);
           }
 
-          response.pipe(zlib.createGunzip()).pipe(fileStream);
+          var gunzip = zlib.createGunzip();
+          gunzip.on('error', function (err) {
+            // pipe() does not forward stream errors: without this listener a
+            // corrupt gzip body was an uncaughtException (or a silent hang,
+            // since fileStream then never emits 'error'/'close').
+            if(settle()) return;
+            fileStream.destroy();
+            that.binaryDownloadError('Got Error while unzipping binary', util.format(err));
+            that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
+          });
+          response.pipe(gunzip).pipe(fileStream);
         } else {
           response.pipe(fileStream);
         }

test/local_start_output_handling.js

@@ -168,29 +168,50 @@ describe('Local.start output handling', function () {
     });
   });
 
-  it('records the daemon pid when a connected payload precedes a non-zero exit', function (done) {
+  it('treats a connected payload as success even when the process exits non-zero', function (done) {
     this.timeout(10000);
     run(stub('connected-then-fail.sh', 'echo \'{"state":"connected","pid":12345}\'; exit 1'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
-      expect(calls[0]).to.be.an('object');
-      // The daemon is up even though the foreground process errored;
-      // stop() must still be able to reach it.
+      // The daemon is up despite the foreground exit status; reporting an
+      // error here left isRunning() true while start() claimed failure.
+      expect(calls[0]).to.equal(undefined);
       expect(bsLocal.pid).to.equal(12345);
+      expect(bsLocal.isProcessRunning).to.equal(true);
+    });
+  });
+
+  it('keeps the unparsed payload as extra when a non-zero exit has an unusable message', function (done) {
+    this.timeout(10000);
+    run(stub('nonzero-bad-message.sh', 'echo \'{"state":"disconnected","message":42}\'; exit 1'), done, function (calls, uncaught) {
+      expect(uncaught).to.eql([]);
+      expect(calls.length).to.equal(1);
+      expect(calls[0].message).to.match(/Command failed/);
+      expect(calls[0].extra).to.match(/"message":42/);
     });
   });
 
-  it('startSync returns an error without deleting the binary on non-JSON output', function () {
+  it('startSync keeps a user-supplied binary on non-JSON output', function () {
     var stubPath = stub('sync-garbage.sh', 'echo "segmentation fault"; exit 0');
-    bsLocal.binaryPath = stubPath;
-    var err = bsLocal.startSync({ key: 'dummy-key', localIdentifier: 'loc-7325' });
+    var err = bsLocal.startSync({ key: 'dummy-key', localIdentifier: 'loc-7325', binarypath: stubPath });
     expect(err).to.be.an('object');
     expect(err.message).to.match(/^Invalid output received: /);
     // The old code misclassified parse failures as binary-execution failures
-    // and deleted the binary before re-downloading it.
+    // and deleted the binary (even a user-supplied one) before re-downloading.
     expect(fs.existsSync(stubPath)).to.equal(true);
   });
 
+  it('startSync evicts a downloaded binary that prints non-JSON output, without retrying', function () {
+    var stubPath = stub('sync-garbage-evict.sh', 'echo "segmentation fault"; exit 0');
+    bsLocal.binaryPath = stubPath; // simulates a previously downloaded binary
+    var err = bsLocal.startSync({ key: 'dummy-key', localIdentifier: 'loc-7325' });
+    expect(err).to.be.an('object');
+    expect(err.message).to.match(/^Invalid output received: /);
+    // Corrupt-but-runnable downloads self-heal on the NEXT start via a fresh
+    // download, instead of the old delete-and-retry-9-times loop.
+    expect(fs.existsSync(stubPath)).to.equal(false);
+  });
+
   it('startSync returns an error when the binary emits literal null', function () {
     var stubPath = stub('sync-null.sh', 'echo "null"; exit 0');
     bsLocal.binaryPath = stubPath;