Skip to content

LOC-7420: tolerate a busy binary instead of crashing the consumer #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AdityaHirapara merged 3 commits into from
Sep 24, 2026
+307 −31
Merged

LOC-7420: tolerate a busy binary instead of crashing the consumer #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eecad0c
LOC-7420: tolerate a busy binary instead of crashing the consumer
pranay-v29 Sep 21, 2026
0a426a3
Complete the async callback contract on every download path
pranay-v29 Sep 22, 2026
2ee720a
Replace an unusable binary rather than re-spawning it
pranay-v29 Sep 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions lib/Local.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ function Local(){
}
try{
const obj = childProcess.spawnSync(that.binaryPath, that.getBinaryArgs());
/* stdout is null on a spawn failure; reading .length masked the real cause
and the binary was deleted on a TypeError rather than the actual error. */
if(obj.error) {
throw obj.error;
}
this.tunnel = {pid: obj.pid};
var data = {};
if(obj.stdout.length > 0)
Expand All @@ -79,7 +84,12 @@ function Local(){
if(that.retriesLeft > 0) {
console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
that.retriesLeft -= 1;
fs.unlinkSync(that.binaryPath);
if(that.binary) that.binary.waitWhileBinaryBusySync(that.binaryPath);
try { fs.unlinkSync(that.binaryPath); } catch(err) { /* ignored */ }

@07souravkunda 07souravkunda Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this unlink fails (the locked-file case this PR is about), delete(that.binaryPath) + startSync → binaryPath() passes the checkPath(X_OK) at LocalBinary.js:355 — on Windows X_OK behaves like F_OK — so the same corrupt file is re-spawned for all 9 retries with no wait.

Evidence: reproduced with a corrupt binary whose unlink fails: it was spawned 10× in 73 ms and never replaced. The new busy wait lives only in retryBinaryDownload, which this path never reaches.

Suggestion: call this.binary.waitWhileBinaryBusySync(that.binaryPath) before the unlink here (and the setTimeout variant in start() at :128), and don't reuse the file if the unlink still fails. Otherwise defect 4's recovery only works when the unlink succeeds.

@pranay-v29 pranay-v29 Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — confirmed, and this was a real hole in the fix. The recovery only worked when the unlink succeeded, which is exactly the case this branch is not about.

Fixed in 2ee720a. Wait for the lock first, and stop rather than retry when the file survives the unlink:

that.retriesLeft -= 1;
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);
}

The setTimeout variant is in start() as you suggested, since that path cannot block.

Added the existsSync check beyond your suggestion: waiting alone still leaves the loop reachable if the lock never clears, and re-spawning a file we could not replace can only fail the same way.

Covered by does not retry when the binary cannot be replaced, which asserts retriesLeft goes 9 → 8 rather than 9 → 0. It measures the bug one step earlier than your repro — counting spawns faithfully would need control over getAvailableDirs() and the real ~/.browserstack.

/* 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;
Expand All @@ -99,6 +109,11 @@ function Local(){
return callback();

this.getBinaryPath(function(binaryPath){
/* Matches startSync's check below: the download can exhaust its retries
and hand back nothing, and execFile(undefined) throws uncatchably. */
if(!binaryPath) {
return callback(new LocalError('Couldn\'t find binary file'));
}
that.binaryPath = binaryPath;
try {
fs.writeFileSync(that.logfile, '');
Expand All @@ -114,11 +129,21 @@ function Local(){
if(that.retriesLeft > 0) {
console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
that.retriesLeft -= 1;
fs.unlinkSync(that.binaryPath);
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()));
Expand Down
106 changes: 85 additions & 21 deletions lib/LocalBinary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* global Atomics, SharedArrayBuffer -- ES2017, used for the blocking wait in
waitWhileBinaryBusySync; declared here rather than widening the lint env. */
var https = require('https'),
fs = require('fs'),
path = require('path'),
Expand Down Expand Up @@ -71,6 +73,10 @@ function LocalBinary(){
env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key;
}
const obj = childProcess.spawnSync(cmd, opts, { env: env });
/* stdout is null on a spawn failure; reading .length masked the real cause. */
if(obj.error) {
throw(util.format(obj.error));
}
if(obj.stdout.length > 0) {
this.sourceURL = obj.stdout.toString().replace(/\n+$/, '');
this.downloadState.sourceURL = this.sourceURL;
Expand Down Expand Up @@ -148,23 +154,60 @@ function LocalBinary(){
this.downloadErrorMessage = errorMessagePrefix + ' : ' + errorMessage;
};

/* A locked binary is transient on Windows (AV scan, a tunnel still releasing
its handle), not a corrupt one. Mirrors the CLI binary's existing probe. */
this.BUSY_ERROR_CODES = ['EBUSY', 'EPERM', 'ETXTBSY', 'EACCES'];
this.BUSY_MAX_WAITS = 3;
this.BUSY_WAIT_MS = 1000;

this.isBinaryBusy = function(binaryPath) {
try {
fs.closeSync(fs.openSync(binaryPath, 'r+'));
return false;
} catch(err) {
return this.BUSY_ERROR_CODES.indexOf(err.code) !== -1;
}
};

/* Blocking by design: the sync path has no event loop to come back to. */
this.waitWhileBinaryBusySync = function(binaryPath) {
for(var i = 0; i < this.BUSY_MAX_WAITS; i++) {
if(!fs.existsSync(binaryPath) || !this.isBinaryBusy(binaryPath)) return;
console.log('Binary is in use, waiting before retrying.');
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, this.BUSY_WAIT_MS);
}
};

this.retryBinaryDownload = function(conf, destParentDir, callback, retries, binaryPath) {
var that = this;
if(retries > 0) {
console.log('Retrying Download. Retries left', retries);
/* Single unlink instead of stat-then-unlinkSync: the gap between the two
let a concurrent writer swap the file, and a failing unlinkSync threw
out of the stat callback where it could not be caught. A missing file
is the expected case here, so any error is ignored. */
if(retries <= 0) {
console.error('Number of retries to download exceeded.');
/* The async contract has to be completed or Local.start() waits forever.
An empty path is the signal; the caller reports it. */
if(callback) callback();

@07souravkunda 07souravkunda Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completes the contract when retries are exhausted, but download() at :271 still does return console.error(...) without calling callback when getDownloadPath fails — invalid key (reqBody.error), a network error, or the fallback re-fetch at retries==4 partway through the retry chain. Local.start() then never calls back.

Evidence: stub repro with getDownloadPath returning an error: the download callback never fires. Pre-existing on master, but it's the same contract this commit is closing.

Suggestion:

if(err) {
  console.error('Unable to fetch the source url to download the binary with error: ', err);
  return callback();
}

The new !binaryPath check in Local.start already reports it.

@pranay-v29 pranay-v29 Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 2ee720a, as suggested:

if(err) {
  console.error("Unable to fetch the source url to download the binary with error: ", err);
  return callback();
}

You are right that it is the same contract 0a426a3 was closing, and leaving one caller of it hanging would have been inconsistent. The !binaryPath check in Local.start reports it, so no new error plumbing was needed.

Covered by completes the callback when the download url cannot be fetched.

return;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
console.log('Retrying Download. Retries left', retries);

/* Must stay synchronous: this return value is what downloadSync ->
binaryPath() -> Local.getBinaryPath hands back. Retrying inside a callback
returned undefined before the retry had done anything. */
if(!callback) {
that.waitWhileBinaryBusySync(binaryPath);
try { fs.unlinkSync(binaryPath); } catch(err) { /* missing or locked */ }
return that.downloadSync(conf, destParentDir, retries - 1);
}

var attemptAsync = function(waitsLeft) {
if(waitsLeft > 0 && fs.existsSync(binaryPath) && that.isBinaryBusy(binaryPath)) {
console.log('Binary is in use, waiting before retrying.');
return setTimeout(function() { attemptAsync(waitsLeft - 1); }, that.BUSY_WAIT_MS);
}
fs.unlink(binaryPath, function() {
if(!callback) {
return that.downloadSync(conf, destParentDir, retries - 1);
}
that.download(conf, destParentDir, callback, retries - 1);
});
} else {
console.error('Number of retries to download exceeded.');
}
};
attemptAsync(that.BUSY_MAX_WAITS);
};

this.downloadSync = function(conf, destParentDir, retries) {
Expand Down Expand Up @@ -198,6 +241,14 @@ 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);
}
let output;
if(obj.stdout.length > 0) {
if(fs.existsSync(binaryPath)){
Expand All @@ -221,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;
Expand All @@ -234,6 +286,21 @@ function LocalBinary(){
var binaryPath = path.join(destParentDir, destBinaryName);
var fileStream = fs.createWriteStream(binaryPath);

/* A failed open and the in-flight request can both report on the same
attempt; one attempt must trigger at most one retry. */
var retried = false;
var retryOnce = function(prefix, err) {
that.binaryDownloadError(prefix, util.format(err));
if(retried) return;
retried = true;
that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
};

/* Same as lib/download.js: the open() failure lands first. */
fileStream.on('error', function (err) {
retryOnce('Got Error while downloading binary file', err);
});

var options = url.parse(this.httpPath);
if(conf.proxyHost && conf.proxyPort) {
options.agent = new HttpsProxyAgent({
Expand Down Expand Up @@ -267,21 +334,18 @@ function LocalBinary(){
}

response.on('error', function(err) {
that.binaryDownloadError('Got Error in binary download response', util.format(err));
that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
});
fileStream.on('error', function (err) {
that.binaryDownloadError('Got Error while downloading binary file', util.format(err));
that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
retryOnce('Got Error in binary download response', err);
});
fileStream.on('close', function () {
/* node emits 'close' after 'error' too, so without this a failed
attempt reports success alongside the retry it just started. */
if(retried) return;
fs.chmod(binaryPath, '0755', function() {
callback(binaryPath);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
}).on('error', function(err) {
that.binaryDownloadError('Got Error in binary downloading request', util.format(err));
that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
retryOnce('Got Error in binary downloading request', err);
});
});
};
Expand Down
19 changes: 15 additions & 4 deletions lib/download.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@ const binaryPath = process.argv[2], httpPath = process.argv[3], proxyHost = proc

var fileStream = fs.createWriteStream(binaryPath);

/* Must be attached before the async https.get: createWriteStream emits 'error'
on the next tick, and with no listener node turns that into a hard throw. */
var request;

fileStream.on('error', function (err) {
console.error('Got Error while downloading binary file', err);
process.exitCode = 1;

@07souravkunda 07souravkunda Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing reads this exit code yet: downloadSync checks obj.stdout.length > 0 first and never looks at obj.status. On a write error partway through a download, 'close' (attached inside the https.get callback) still fires after 'error' and prints Done, so the parent accepts the partial file — the same close-after-error issue fixed in LocalBinary.js. (Not reproduced at runtime; based on Node's documented close-after-error behaviour.)

Suggestion: have downloadSync treat obj.status !== 0 as a failed attempt before inspecting stdout, or guard the close log here with process.exitCode !== 1.

@pranay-v29 pranay-v29 Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct on both counts, and this is arguably the most consequential of the three — it is a plausible mechanism for the corrupt binaries the customer keeps clearing by hand. Fixed in 2ee720a using both of your suggestions rather than either alone:

// download.js
fileStream.on("close", function () {
  if(process.exitCode === 1) return; // errored; not a completed download
  console.log("Done");
});
// LocalBinary.js downloadSync
if(obj.status !== 0) {
  that.binaryDownloadError("Download failed with status", String(obj.status));
  return that.retryBinaryDownload(conf, destParentDir, null, retries, binaryPath);
}

The status check is the more robust half, since it catches any non-zero exit including a signal kill, where status is null. Guarding the log as well keeps the output honest.

Partially covered — the existing download.js test now asserts Done is absent from stdout, but that is the open-failure case. The mid-download write error, where Done would actually leak, is not reproduced; as you noted, that rests on the documented close-after-error behaviour.

/* Otherwise the child keeps downloading into a dead stream and the parent's
spawnSync blocks for a whole download before it can retry. */
if(request) request.destroy();
});

var options = url.parse(httpPath);
/* isUndefined, not plain truthiness: the parent passes literal `undefined`
placeholders for the proxy slots when only a CA is configured, and those
Expand Down Expand Up @@ -37,7 +49,7 @@ options.headers = Object.assign({}, options.headers, {
'user-agent': process.env.USER_AGENT,
});

https.get(options, function (response) {
request = https.get(options, function (response) {
const contentEncoding = response.headers['content-encoding'];
if (typeof contentEncoding === 'string' && contentEncoding.match(/gzip/i)) {
if (process.env.BROWSERSTACK_LOCAL_DEBUG_GZIP) {
Expand All @@ -52,12 +64,11 @@ https.get(options, function (response) {
response.on('error', function(err) {
console.error('Got Error in binary download response', err);
});
fileStream.on('error', function (err) {
console.error('Got Error while downloading binary file', err);
});
fileStream.on('close', function () {
if(process.exitCode === 1) return; // errored; not a completed download
console.log('Done');
});
}).on('error', function(err) {
if(process.exitCode === 1) return; // our own destroy() landing
console.error('Got Error in binary downloading request', err);
});
Loading
Loading