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, excluding the LocalBinary > Download block that needs real credentials:

• master: 28 passing, 3 failing, 2 pending
• this branch: 32 passing, 3 failing, 2 pending

Same 3 failures before and after (should return is running properly ×2, should stop local) — all pre-existing and credential-gated, none related to this change. 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