LOC-7325: stop uncatchable TypeError on empty binary output in Local.start

[vivianludrick]

Fixes an uncatchable TypeError thrown out of Local.start() when the BrowserStackLocal binary exits with no output.

JIRA Story: https://browserstack.atlassian.net/browse/LOC-7325

The bug

start() handles the binary's output inside an execFile callback. The empty-output branch called back with No output received but did not return, so control fell through to the next statement, which dereferences data['message']['message'] on data = {}:

TypeError: Cannot read properties of undefined (reading 'message')
    at .../browserstack-local/lib/Local.js:127:50

Two things make this worse than a normal error path:

• The callback fires twice — once legitimately with No output received, then again from the throwing statement.
• The caller cannot catch it. The throw happens inside a callback invoked by node's internal exithandler, so no try/catch around local.start(...) intercepts it. It surfaces as an uncaughtException, which means the blast radius is set by the host process's exception policy, not by this package. In the case that surfaced it, a host with a fatal uncaughtException handler lost its entire reporting plane because an optional tunnel failed to start.

The trigger is not exotic — any environment where the binary exits without emitting JSON reaches it: wrong or blocked binary path, killed process, permission failure, or a shimmed binary in CI.

The fix

Three paths reached the same unguarded deref; all three are now closed:

Path	Before	After
Empty stdout and stderr	callback, then fall through and throw	callback once, then return
Terminal branch of the error handler	callback, then fall through and throw	callback once, then return
Non-connected payload with no message key	throws	falls back to Failed to start BrowserStack Local

Also guarded JSON.parse: non-JSON output (a plain-text crash message, for instance) threw a SyntaxError from the same uncatchable position. It is now reported through the callback as Invalid output received: <reason>, with the raw output attached as the error's extra field.

startSync shared the unguarded deref and now uses the same helper. Its empty-output branch already returned, so it was never exposed to the fall-through.

Every changed path now invokes the callback exactly once and lets the caller handle the failure normally.

Tests

Added test/local_start_output_handling.js — drives start() with stub binaries for each output shape and asserts the callback fires exactly once and that nothing escapes as an uncaughtException. No credentials or network needed.

Verified the tests actually catch the defect by toggling the fix:

• On master: 3 of the 4 fail, with the ticket's exact TypeError: Cannot read properties of undefined (reading 'message').
• With this change: all 4 pass.

Full suite run with real credentials, excluding the LocalBinary > Download block (it hangs on master and on this branch — its harness never sets binary.key, which is a separate pre-existing problem):

• 34 passing, 1 failing, 2 pending

The single failure is should stop local, which fails identically on master — verified by running that test against origin/master's lib. It is unrelated to this change: stop() calls back when SIGTERM is sent rather than when the process has exited. npm run pretest (eslint over lib/* index.js) is clean.

Note: the fix avoids optional chaining because the repo's eslint config sets env: es6 (ES2015).

Scope

Code fix only — no version bump or publish here. 1.5.13 is the latest published version and carries the defect, so this needs a release to reach consumers.

---

Behavior note (changelog)

startSync now returns a LocalError('Invalid output received: ...') when the binary prints unparseable output, instead of throwing after deleting and re-downloading the binary. This matches its existing return-an-error contract for empty-output and non-connected payloads (the known external consumer, browserstack-node-sdk, checks the return value). Raw binary output attached to errors is exposed as error.extra, truncated to 1KB.

Round 3 additions:

• A connected payload with a pid now counts as success even if the foreground process exits non-zero (previously contradictory: error reported while isRunning() was true; startSync already behaved this way).
• A downloaded binary that runs but prints unparseable output is evicted without retrying, so the next start re-downloads a fresh copy; binaries passed via binarypath are never evicted.
• Terminal download failure (source URL unreachable / retries exhausted) now fails start() immediately with the recorded download error instead of hanging or cascading into ~100 retry attempts.
• Deterministic binary failures that exit non-zero with a JSON diagnostic fail fast with that message instead of burning the 9-cycle re-download budget first.

[vivianludrick]

Claude Code Review

Verdict: the fix works for the output shapes it targets, but it doesn't yet guarantee the PR's stated invariant ("callback fires exactly once and nothing escapes"). Three confirmed gaps in lib/Local.js should be fixed before merge; the new test suite also has hazards worth addressing.

10 findings survived adversarial verification (7 confirmed by live execution or code inspection, 3 plausible), ranked by severity:

Confirmed — code

1. lib/Local.js:132 — null output still crashes uncatchably. JSON.parse('null') succeeds, so the new try/catch never fires, and data['state'] then throws Cannot read properties of null inside the execFile callback — the exact uncatchable TypeError this PR set out to fix, and the callback never fires. Fix: if(!data || data['state'] != 'connected') or route null to the error callback.

2. lib/Local.js:54 — startSync only got half the fix. Its JSON.parse is still unguarded, so non-JSON output falls into the outer catch, which treats it as a binary-execution failure: it deletes the binary (including a user-supplied binarypath) and re-downloads it up to 9 times, then throws a raw SyntaxError instead of the new "Invalid output received" error. Suggest extracting one shared parse helper used by both start and startSync.

3. lib/Local.js:107 — bare fs.unlinkSync in the retry branch. If the binary path is already gone (prior retry, concurrent instance, AV quarantine), unlinkSync throws out of the execFile callback — same uncatchable-throw class, and the caller's callback never fires (promise wrappers hang forever). A once-guarded callback + whole-body try/catch would close this class structurally instead of per-branch.

Confirmed — tests (test/local_start_output_handling.js)

4. Line 35 — retry path is left armed. retriesLeft stays 9, so any execFile failure on the stub (e.g. noexec tmpdir in CI) deletes the stub and downloads + runs the real BrowserStackLocal binary from the network with key dummy-key, up to 9 times. One line fixes it: bsLocal.retriesLeft = 0 in beforeEach.

5. Line 41 — fixed setTimeout(1000) settle. Races the execFile callback on loaded CI boxes (flaky expected 0 to equal 1) and adds 4+ seconds of dead sleep; a late throw after the window hits mocha instead of the suite's uncaught-capture. Settle on the first callback, or hook the tunnel's close event.

6. Line 30 — removeAllListeners('uncaughtException'). Strips mocha's global handler with restoration living only inside the 1s timer — not exception-safe, and it swallows unrelated async errors process-wide during each window. process.on('uncaughtExceptionMonitor') observes without detaching.

7. Line 48 — tmpdir leak. mkdtempSync(bs-local-7325-*) has no after() cleanup, so every run leaks a directory into the system tmpdir. One fs.rmSync(stubDir, { recursive: true }) in after() fixes it.

Plausible (depends on binary payload shapes / caller patterns)

8. lib/Local.js:151 — getErrorMessage can return a non-string ({"message":42} → error.message === 42; nested objects → [object Object]), which crashes consumers doing error.message.match(...). Add a typeof message === 'string' check.

9. lib/Local.js:115 — the added return discards the binary's JSON diagnostic when it exits non-zero with a valid failure payload on stdout; callers now get only the generic "Error while trying to execute binary" after 9 delete/re-download cycles. Consider parsing stdout in the exhausted branch and preferring the payload's message.

10. lib/Local.js:128 — the parse-failure path attaches the full raw output (up to execFile's 1MB maxBuffer) as error.extra; serializers/log shippers will dump it wholesale. Truncate at attach time (~1KB is plenty).

---

One candidate finding was dropped as refuted (getErrorMessage as instance method / minimal typings — that matches this repo's existing conventions). Lint (npm run pretest) passes clean. The test-toggling methodology in the new suite is a good idea — the items above are about making it deterministic and safe in CI.

🤖 Generated with Claude Code

[vivianludrick]

Code review — scoped to LOC-7325

Verdict: MEETS TICKET. No findings.

Reviewed origin/master...c2bb790 strictly against the ticket's requirements.

Per-requirement check

Ticket requirement	Status	Evidence
Path 1 — empty stdout and stderr (the reported defect)	Closed	lib/Local.js:119-123; the return is present. stdout || stderr preserves the original if/else if precedence exactly.
Path 2 — the error branch (orig :100-113)	Closed	lib/Local.js:113-116. On master this else had no return, so it fell into the data block, hit 'No output received' (double-callback) and then the unguarded deref.
Path 3 — state != 'connected' payload with no message key	Closed	lib/Local.js:131 → helper at lib/Local.js:143-149.
"Audit the whole callback for further double-callback paths"	Clean	All six exits enumerated; exactly one callback invocation on each, no surviving fall-through.

Payload shapes traced through the deref

data.message	master	this PR
{message: "Invalid key"}	"Invalid key"	"Invalid key" (unchanged)
absent / undefined	throws	"Failed to start BrowserStack Local"
null	throws	fallback
{}	LocalError(undefined)	fallback
"some string"	LocalError(undefined)	"some string"

The ticket suggested data['message']?.['message']. The helper is a superset of that — optional chaining alone would still produce LocalError(undefined) for {}. (It also avoids optional chaining because this repo's eslint config sets env: es6, where ?. fails to parse.)

Do the tests prove the fix?

Yes. Against origin/master's lib, 3 of the 4 fail — and each fails with the real defect rather than an assertion mismatch:

• empty-output → Uncaught TypeError: Cannot read properties of undefined (reading 'message') (the ticket's exact repro)
• no-message-key → same TypeError
• garbage-output → Uncaught SyntaxError: Unexpected token 's', "segmentation fault"
• with-message → passes on both, correctly — it is the control case for the path that was never broken

The JSON.parse guard is an addition beyond the ticket's literal wording, but it closes a real never-callback path in the same block: on master, non-JSON output threw a SyntaxError from the same uncatchable position and the callback never fired at all. Same defect class as the reported one, which is what "audit the whole callback" points at.

Correctness

• npm run pretest (eslint over lib/* index.js) passes.
• getErrorMessage is assigned in the constructor body, so it exists before any start()/startSync() callback can run. Both call sites use the captured that.
• The parallel startSync change (lib/Local.js:60) hardens the identical deref and cannot regress — that branch already returned on empty output.
• Only message-text change on a previously-working path: {message: {message: ""}} now yields the fallback instead of "".

Full suite with real credentials, excluding the LocalBinary > Download block (pre-existing harness debt — it never sets binary.key): 34 passing, 1 failing. The single failure, should stop local, fails identically on origin/master and is unrelated to this change.

Out of scope — FYI only, not to be actioned here

This PR was deliberately reduced to the ticket. These were noted and left alone:

• JSON.parse('null') returns null, and data['state'] would then throw — not one of the three named paths, and not a shape the binary emits.
• fs.unlinkSync at lib/Local.js:107 can throw out of the execFile callback with no callback fired (pre-existing, in the retry/eviction machinery).
• test/local.js requires a real access key and a live binary download, so npm test stalls locally.

An earlier iteration of this branch also fixed a set of adjacent defects in lib/LocalBinary.js (download flow, binary lifecycle, retry policy). Those were removed to keep this PR scoped to LOC-7325 and are preserved on loc-7325-extra-findings-archive for separate tickets.