LOC-7325: harden adjacent callback paths from second review round

- start() exhausted-retry branch: record the daemon pid from a connected
  payload so stop() can reach an orphan; prefer the payload's message
  only when it actually carries one, otherwise surface the exec error
  and keep raw crash output as error.extra.
- Extract prepareBinaryRetry so start/startSync share one retry block;
  split extractErrorMessage out of getErrorMessage.
- stop(): add missing return — a treeKill error fired the callback twice.
- LocalBinary.download(): settle-once guard across response/stream/request
  handlers (an errored stream still emits 'close', double-firing the
  callback); exhausted retries and source-url failures now still deliver
  the callback instead of hanging the caller forever.
- index.d.ts: declare error.extra, startSync, and error-typed callbacks.
- Tests: observe throws via uncaughtExceptionMonitor instead of detaching
  mocha's handler (regressions now fail with the real error, not a bare
  timeout); route assertion failures to mocha's done; new cases for
  non-zero exit with crash text and connected-payload pid recording.

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

Author: vivianludrick

index.d.ts

@@ -19,9 +19,15 @@ declare module "browserstack-local" {
     [key: string]: string | boolean;
   }
 
+  interface LocalError extends Error {
+    /** Raw binary output (truncated to 1KB) attached when the output could not be parsed. */
+    extra?: string;
+  }
+
   class Local {
-    start(options: Partial<Options>, callback: (error?: Error) => void): void;
+    start(options: Partial<Options>, callback: (error?: LocalError) => void): void;
+    startSync(options: Partial<Options>): LocalError | undefined;
     isRunning(): boolean;
-    stop(callback: () => void): void;
+    stop(callback: (error?: LocalError) => void): void;
   }
 }

lib/Local.js

@@ -66,18 +66,7 @@ function Local(){
       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(unlinkError) {
-          // The binary may already be gone (a prior retry or a concurrent
-          // instance); the retry only needs the path cleared for re-download.
-          console.error('Could not delete binary: ', unlinkError.message);
-        }
-        delete(that.binaryPath);
-        process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
-        process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
+        that.prepareBinaryRetry(binaryDownloadErrorMessage);
         return that.startSync(options);
       } else {
         throw new LocalError(error.toString());
@@ -117,29 +106,26 @@ function Local(){
             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(unlinkError) {
-                // The binary may already be gone (a prior retry or a concurrent
-                // instance); the retry only needs the path cleared for re-download.
-                console.error('Could not delete binary: ', unlinkError.message);
-              }
-              delete(that.binaryPath);
-              process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
-              process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
+              that.prepareBinaryRetry(binaryDownloadErrorMessage);
               that.start(options, callback);
               return;
             }
             // The binary can exit non-zero while still printing a JSON
-            // diagnostic on stdout; prefer its message over the generic
-            // execution error.
+            // diagnostic on stdout; keep whichever diagnostic is richer.
             var failure = that.parseBinaryOutput(stdout, stderr);
-            if(failure.data && failure.data['state'] != 'connected') {
-              safeCallback(new LocalError(that.getErrorMessage(failure.data)));
+            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 {
-              safeCallback(new LocalError(error.toString()));
+              // Keep any raw (non-JSON) output as extra — it usually holds
+              // the crash text.
+              safeCallback(new LocalError(error.toString(), failure.error && failure.error.extra));
             }
             return;
           }
@@ -170,13 +156,35 @@ function Local(){
   // the execFile callback that throw is an uncaughtException the caller cannot
   // catch; a non-string message crashes consumers doing error.message.match().
   // See LOC-7325.
-  this.getErrorMessage = function(data){
+  this.extractErrorMessage = function(data){
     var message = data && data['message'];
     if(message && typeof message === 'object')
       message = message['message'];
     if(typeof message === 'string' && message.length > 0)
       return message;
-    return 'Failed to start BrowserStack Local';
+    return null;
+  };
+
+  this.getErrorMessage = function(data){
+    return this.extractErrorMessage(data) || 'Failed to start BrowserStack Local';
+  };
+
+  // Shared retry bookkeeping for start and startSync: drop the (possibly
+  // corrupt) binary so the next attempt re-downloads it, and record the
+  // failure for the fallback download source.
+  this.prepareBinaryRetry = function(binaryDownloadErrorMessage){
+    console.log('Retrying Binary Download. Retries Left', this.retriesLeft);
+    this.retriesLeft -= 1;
+    try {
+      fs.unlinkSync(this.binaryPath);
+    } catch(unlinkError) {
+      // The binary may already be gone (a prior retry or a concurrent
+      // instance); the retry only needs the path cleared for re-download.
+      console.error('Could not delete binary: ', unlinkError.message);
+    }
+    delete(this.binaryPath);
+    process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
+    process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
   };
 
   // Raw binary output can be up to execFile's 1MB maxBuffer; truncate before
@@ -214,7 +222,9 @@ function Local(){
   this.stop = function (callback) {
     if(!this.pid) return callback();
     this.killAllProcesses(function(error){
-      if(error) callback(new LocalError(error.toString()));
+      // Without the return, a treeKill error fired the callback twice:
+      // once with the error, then once with undefined.
+      if(error) return callback(new LocalError(error.toString()));
       callback();
     });
   };

lib/LocalBinary.js

@@ -146,6 +146,12 @@ 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.
+      if(callback) {
+        callback(binaryPath);
+      }
     }
   };
 
@@ -203,7 +209,11 @@ 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);
+        // Route through the retry path (which eventually surfaces a failure
+        // to the caller) instead of returning without ever calling back.
+        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));
       }
 
       this.httpPath = downloadUrl;
@@ -216,6 +226,15 @@ function LocalBinary(){
       var binaryPath = path.join(destParentDir, destBinaryName);
       var fileStream = fs.createWriteStream(binaryPath);
 
+      // Exactly one of the handlers below may resolve this download attempt
+      // (retry or callback); a retried attempt gets a fresh settled flag.
+      var settled = false;
+      var settle = function() {
+        if(settled) return true;
+        settled = true;
+        return false;
+      };
+
       var options = url.parse(this.httpPath);
       if(conf.proxyHost && conf.proxyPort) {
         options.agent = new HttpsProxyAgent({
@@ -249,19 +268,25 @@ function LocalBinary(){
         }
 
         response.on('error', function(err) {
+          if(settle()) return;
           that.binaryDownloadError('Got Error in binary download response', util.format(err));
           that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
         });
         fileStream.on('error', function (err) {
+          if(settle()) return;
           that.binaryDownloadError('Got Error while downloading binary file', util.format(err));
           that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
         });
         fileStream.on('close', function () {
+          // An errored stream still emits 'close' (autoDestroy); without the
+          // guard this fired the callback in addition to the retry above.
+          if(settle()) return;
           fs.chmod(binaryPath, '0755', function() {
             callback(binaryPath);
           });
         });
       }).on('error', function(err) {
+        if(settle()) return;
         that.binaryDownloadError('Got Error in binary downloading request', util.format(err));
         that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
       });

test/local_start_output_handling.js

@@ -13,31 +13,43 @@ var expect = require('expect.js'),
 // `start()` with stub binaries that reproduce each output shape and assert the
 // callback fires exactly once with an error, and that nothing throws.
 //
+// Throws are observed via process.on('uncaughtExceptionMonitor'), which sees
+// every uncaught exception WITHOUT detaching mocha's own handler: a regression
+// fails the test with the real thrown error instead of a bare timeout, and no
+// unrelated error is ever swallowed.
+//
 // Stubs are shell scripts, so these are skipped on Windows.
 describe('Local.start output handling', function () {
-  var stubDir, bsLocal, uncaught, mochaListeners;
+  var stubDir, bsLocal, uncaught, monitorListener;
 
   function stub(name, body) {
     var stubPath = path.join(stubDir, name);
     fs.writeFileSync(stubPath, '#!/bin/sh\n' + body + '\n', { mode: 0o755 });
     return stubPath;
   }
 
-  // Drives start() with the given stub and collects every callback invocation.
-  // The callback, any second (fall-through) invocation and any throw all come
-  // out of the same synchronous execFile exithandler, so settling two ticks
-  // after the first callback observes all of them deterministically — no
-  // fixed sleep. The guard timer only fires if the callback never does (the
-  // exact regression this suite exists to catch), so that failure mode shows
-  // up as an assertion on calls.length instead of a mocha timeout.
-  function run(stubPath, done) {
+  // Drives start() with the given stub, then hands every callback invocation
+  // plus the uncaught array to `assert`. The callback, any second
+  // (fall-through) invocation and any throw all come out of the same
+  // synchronous execFile exithandler, so settling two ticks after the first
+  // callback observes all of them deterministically — no fixed sleep. The
+  // guard timer only fires if the callback never does, so that failure mode
+  // shows up as an assertion on calls.length instead of a mocha timeout.
+  // Assertion failures are routed to mocha's done, never left to throw
+  // asynchronously.
+  function run(stubPath, done, assert) {
     var calls = [], finished = false;
 
     function finish() {
       if (finished) return;
       finished = true;
       clearTimeout(guard);
-      done(calls, uncaught);
+      try {
+        assert(calls, uncaught);
+        done();
+      } catch (assertionError) {
+        done(assertionError);
+      }
     }
 
     var guard = setTimeout(finish, 5000);
@@ -67,18 +79,13 @@ describe('Local.start output handling', function () {
     // executes the real binary from the network.
     bsLocal.retriesLeft = 0;
 
-    // Capture uncaughtExceptions for the duration of each test. Mocha's own
-    // handler is snapshotted here and restored in afterEach, so restoration
-    // survives a throwing test body.
     uncaught = [];
-    mochaListeners = process.listeners('uncaughtException');
-    process.removeAllListeners('uncaughtException');
-    process.on('uncaughtException', function (err) { uncaught.push(err); });
+    monitorListener = function (err) { uncaught.push(err); };
+    process.on('uncaughtExceptionMonitor', monitorListener);
   });
 
   afterEach(function () {
-    process.removeAllListeners('uncaughtException');
-    mochaListeners.forEach(function (listener) { process.on('uncaughtException', listener); });
+    process.removeListener('uncaughtExceptionMonitor', monitorListener);
   });
 
   if (os.platform().match(/win32/i)) {
@@ -88,73 +95,88 @@ describe('Local.start output handling', function () {
 
   it('reports an error exactly once when the binary exits with no output', function (done) {
     this.timeout(10000);
-    run(stub('empty-output.sh', 'exit 0'), function (calls, uncaught) {
+    run(stub('empty-output.sh', 'exit 0'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0]).to.be.an('object');
       expect(calls[0].message).to.equal('No output received');
-      done();
     });
   });
 
   it('reports an error exactly once when the binary emits non-JSON output', function (done) {
     this.timeout(10000);
-    run(stub('garbage-output.sh', 'echo "segmentation fault"; exit 0'), function (calls, uncaught) {
+    run(stub('garbage-output.sh', 'echo "segmentation fault"; exit 0'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0].message).to.match(/^Invalid output received: /);
       expect(calls[0].extra).to.match(/segmentation fault/);
-      done();
     });
   });
 
   it('reports an error exactly once when the binary emits literal null', function (done) {
     this.timeout(10000);
-    run(stub('null-output.sh', 'echo "null"; exit 0'), function (calls, uncaught) {
+    run(stub('null-output.sh', 'echo "null"; exit 0'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0].message).to.match(/^Invalid output received: /);
-      done();
     });
   });
 
   it('reports a fallback message when a non-connected payload has no message key', function (done) {
     this.timeout(10000);
-    run(stub('no-message-key.sh', 'echo \'{"state":"disconnected"}\'; exit 0'), function (calls, uncaught) {
+    run(stub('no-message-key.sh', 'echo \'{"state":"disconnected"}\'; exit 0'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0].message).to.equal('Failed to start BrowserStack Local');
-      done();
     });
   });
 
   it('reports a fallback message when the payload message is not a string', function (done) {
     this.timeout(10000);
-    run(stub('non-string-message.sh', 'echo \'{"state":"disconnected","message":42}\'; exit 0'), function (calls, uncaught) {
+    run(stub('non-string-message.sh', 'echo \'{"state":"disconnected","message":42}\'; exit 0'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0].message).to.equal('Failed to start BrowserStack Local');
-      done();
     });
   });
 
   it('surfaces the binary message when a non-connected payload carries one', function (done) {
     this.timeout(10000);
-    run(stub('with-message.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 0'), function (calls, uncaught) {
+    run(stub('with-message.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 0'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0].message).to.equal('Invalid key');
-      done();
     });
   });
 
   it('surfaces the JSON diagnostic when the binary exits non-zero with a payload', function (done) {
     this.timeout(10000);
-    run(stub('nonzero-with-payload.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 1'), function (calls, uncaught) {
+    run(stub('nonzero-with-payload.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 1'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0].message).to.equal('Invalid key');
-      done();
+    });
+  });
+
+  it('keeps crash output as extra when the binary exits non-zero with non-JSON output', function (done) {
+    this.timeout(10000);
+    run(stub('nonzero-garbage.sh', 'echo "segmentation fault"; exit 139'), 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(/segmentation fault/);
+    });
+  });
+
+  it('records the daemon pid when a connected payload precedes a non-zero exit', 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.
+      expect(bsLocal.pid).to.equal(12345);
     });
   });
 
@@ -180,13 +202,12 @@ describe('Local.start output handling', function () {
   it('truncates oversized non-JSON output attached to the error', function (done) {
     this.timeout(10000);
     // ~64KB of garbage; extra should be capped at 1KB plus a truncation note.
-    run(stub('huge-output.sh', 'head -c 65536 /dev/zero | tr "\\0" "x"; exit 0'), function (calls, uncaught) {
+    run(stub('huge-output.sh', 'head -c 65536 /dev/zero | tr "\\0" "x"; exit 0'), done, function (calls, uncaught) {
       expect(uncaught).to.eql([]);
       expect(calls.length).to.equal(1);
       expect(calls[0].message).to.match(/^Invalid output received: /);
       expect(calls[0].extra.length).to.be.below(1100);
       expect(calls[0].extra).to.match(/\[truncated \d+ bytes\]$/);
-      done();
     });
   });
 });