Skip to content

fix(cli): fall back to BROWSERSTACK_BINARY_URL when update_cli fails (SDK-6948) #224

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

Open
AakashHotchandani wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(cli): fall back to BROWSERSTACK_BINARY_URL when update_cli fails (SDK-6948) #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27288d6
fix(cli): fall back to BROWSERSTACK_BINARY_URL when update_cli fails …
AakashHotchandani Sep 24, 2026
f3694d8
fix(cli): treat a non-2xx update_cli reply as a failure for the BINAR…
AakashHotchandani Oct 7, 2026
4eb68d2
fix(cli): address review on update_cli fallback (SDK-6948)
AakashHotchandani Oct 8, 2026
5c67d07
fix(cli): keep the cached binary on an up-to-date reply or failed fal…
AakashHotchandani Oct 8, 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
7 changes: 7 additions & 0 deletions .changeset/sdk-6948-binary-url-fallback.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@wdio/browserstack-service": patch
---

fix(cli): fall back to `BROWSERSTACK_BINARY_URL` when `update_cli` fails (SDK-6948)

`update_cli` runs during bootstrap against production `api.browserstack.com`, so on an internal staging run (`BROWSERSTACK_STAGING_ENV`) it answers `401`. The SDK then found no CLI binary and fell back to the non-CLI flow, or silently reused a stale cached binary. When `BROWSERSTACK_BINARY_URL` is set and `update_cli` fails (non-2xx or network error), the binary is now downloaded from that URL. If that download fails, a cached binary is used when there is one. A 2xx reply with neither `url` nor `updated_cli_version` keeps the cached binary, and only downloads from the URL when none is cached. `CLIUtils.requestToUpdateCLI` now rejects on a non-2xx reply (with `response.statusCode`) instead of resolving the error body. With `BROWSERSTACK_BINARY_URL` unset, behaviour is unchanged: a non-2xx reply keeps the existing binary, and network errors propagate.

@AakashHotchandani AakashHotchandani Oct 8, 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.

💡 Suggestion: [DOCS] "Behaviour is unchanged" is slightly too broad without the URL

With BROWSERSTACK_BINARY_URL unset, a non-2xx reply with a non-JSON body (for example an HTML 502/503 from the load balancer) used to surface as a SyntaxError. setupCliPath then returned null, and the run used the non-CLI flow. Now requestToUpdateCLI attaches statusCode (cliUtils.ts:476-485), so checkAndUpdateCli returns the cached binary (:288-291) and the CLI flow continues. This affects prod customers, and it is an improvement, but the changeset says the behaviour did not change.

Suggested Fix: add a short clause, for example: "…a non-2xx reply (including a non-JSON error page) keeps the existing binary, and network errors propagate."

137 changes: 96 additions & 41 deletions packages/browserstack-service/src/cli/cliUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,56 +227,101 @@ export class CLIUtils {

PerformanceTester.start(PerformanceEvents.SDK_CLI_CHECK_UPDATE)
logger.info(`Current CLI Path Found: ${existingCliPath}`)
const queryParams: Record<string, string> = {
sdk_version: CLIUtils.getSdkVersion(),
os: platform(),
os_arch: arch(),
cli_version: '0',
sdk_language: this.getSdkLanguage(),
}
if (!isNullOrEmpty(existingCliPath)) {
// If binary is busy (being executed by another process), skip version check
// and API call entirely — use existing binary as-is
if (this.isBinaryBusy(existingCliPath)) {
logger.warn(`Existing binary is currently in use, skipping update: ${existingCliPath}`)
PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE)
return existingCliPath
// Single close for the span on every path, including a rejected download.
try {
const queryParams: Record<string, string> = {
sdk_version: CLIUtils.getSdkVersion(),
os: platform(),
os_arch: arch(),
cli_version: '0',
sdk_language: this.getSdkLanguage(),
}
const version = await this.runShellCommand(
`${existingCliPath} version`,
)
if (version.toLowerCase().includes('text file busy')) {
logger.warn(`Binary busy during version check, skipping update: ${existingCliPath}`)
PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE)
return existingCliPath
if (!isNullOrEmpty(existingCliPath)) {
// If binary is busy (being executed by another process), skip version check
// and API call entirely — use existing binary as-is
if (this.isBinaryBusy(existingCliPath)) {
logger.warn(`Existing binary is currently in use, skipping update: ${existingCliPath}`)
return existingCliPath
}
const version = await this.runShellCommand(
`${existingCliPath} version`,
)
if (version.toLowerCase().includes('text file busy')) {
logger.warn(`Binary busy during version check, skipping update: ${existingCliPath}`)
return existingCliPath
}
queryParams.cli_version = version
}
queryParams.cli_version = version
}
const response = await this.requestToUpdateCLI(queryParams, config)
if (nestedKeyValue(response, ['updated_cli_version'])) {
logger.debug(
`Need to update binary, current binary version: ${queryParams.cli_version}`,
)

const browserStackBinaryUrl =
process.env.BROWSERSTACK_BINARY_URL || null
if (!isNullOrEmpty(browserStackBinaryUrl)) {
logger.debug(
`Using BROWSERSTACK_BINARY_URL: ${browserStackBinaryUrl}`,

const fallbackToBinaryUrl = async (reason: unknown) => {
logger.warn(
`update_cli request failed (${reason}); falling back to BROWSERSTACK_BINARY_URL`,
)
response.url = browserStackBinaryUrl
try {
return await this.downloadLatestBinary(browserStackBinaryUrl as string, cliDir)
} catch (downloadErr) {
if (isNullOrEmpty(existingCliPath)) {
throw downloadErr
}
logger.warn(
`BROWSERSTACK_BINARY_URL download failed (${util.format(downloadErr)}); using cached binary ${existingCliPath}`,
)
return existingCliPath
}
}

const finalBinaryPath = await this.downloadLatestBinary(
nestedKeyValue(response, ['url']),
cliDir,
nestedKeyValue(response, ['updated_cli_version']),
)
let response
try {
response = await this.requestToUpdateCLI(queryParams, config)
} catch (err) {
// update_cli runs during bootstrap — before the binary is spawned and before GRR
// localizes the API hosts — so it always targets the production api host. On an internal
// staging run (BROWSERSTACK_STAGING_ENV) the staging creds are rejected there (401). If an
// explicit binary URL was supplied, use it so the run is not blocked on this call. Opt-in
// only: with no BROWSERSTACK_BINARY_URL behaviour is exactly as before.
const statusCode = (err as { response?: { statusCode?: number } })?.response?.statusCode
if (!isNullOrEmpty(browserStackBinaryUrl)) {
return await fallbackToBinaryUrl(statusCode ?? (err as Error)?.message)
}
if (statusCode !== undefined) {
// A non-2xx reply used to be returned as a body without `updated_cli_version`,
// which kept the existing binary — preserve that.
return existingCliPath
}
throw err
}

// A 2xx reply with neither `updated_cli_version` nor `url` means there is nothing to update:

@AakashHotchandani AakashHotchandani Oct 8, 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.

💡 Suggestion: [DOCS] The comment's reading of a 2xx reply with neither field contradicts the PR's own evidence

The comment says a 2xx reply with neither updated_cli_version nor url "means there is nothing to update". The PR body says the opposite. It reports that prod update_cli returns both updated_cli_version and url for cli_version 0, 1.58.5 and 99.0.0, so an up-to-date client still gets both fields. This branch is therefore a guard for an unexpected reply shape (for example {success:true} or an error-shaped 2xx), not the up-to-date reply. The behaviour itself is fine and matches python#1282: keep the cache, and download only when nothing is cached.

Suggested Fix: reword the comment, for example: // A 2xx reply with neither field is not a normal update_cli reply: keep a cached binary, and only fetch from BROWSERSTACK_BINARY_URL when there is none.

// keep a cached binary, and only fetch from the explicit binary URL when there is none.
if (!isNullOrEmpty(browserStackBinaryUrl) && isNullOrEmpty(existingCliPath) &&
!nestedKeyValue(response, ['updated_cli_version']) && !nestedKeyValue(response, ['url'])) {
return await fallbackToBinaryUrl(JSON.stringify(response))
}

if (nestedKeyValue(response, ['updated_cli_version'])) {
logger.debug(
`Need to update binary, current binary version: ${queryParams.cli_version}`,
)

if (!isNullOrEmpty(browserStackBinaryUrl)) {
logger.debug(
`Using BROWSERSTACK_BINARY_URL: ${browserStackBinaryUrl}`,
)
response.url = browserStackBinaryUrl
}

return await this.downloadLatestBinary(
nestedKeyValue(response, ['url']),
cliDir,
nestedKeyValue(response, ['updated_cli_version']),
)
}
return existingCliPath
} finally {
PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE)
return finalBinaryPath
}
PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE)
return existingCliPath
}

static getCliDir() {
Expand Down Expand Up @@ -428,6 +473,16 @@ export class CLIUtils {
`${APIUtils.BROWSERSTACK_AUTOMATE_API_URL}/${UPDATED_CLI_ENDPOINT}?${params.toString()}`,
requestInit,
)
if (!response.ok) {

@AakashHotchandani AakashHotchandani Oct 8, 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.

💡 Suggestion — [CORRECTNESS] response.json() runs before the !response.ok check; non-JSON error bodies lose the status code

Problem

requestToUpdateCLI parses the body (cliUtils.ts:472) before checking response.ok (:474). An HTML or empty error body (an LB 502/503/504) makes json() throw a SyntaxError with no response.statusCode. With BROWSERSTACK_BINARY_URL unset, that error is re-thrown, setupCliPath returns null, and the run drops to the non-CLI flow. A JSON-bodied 5xx instead keeps the existing binary. The same HTTP status therefore takes two different paths depending on body format. This is pre-existing for the no-URL case, but the new statusCode branch is meant to cover it.

Check ok first and parse defensively:

const body = await response.json().catch(() => null)
if (!response.ok) {
    throw Object.assign(new Error(`update_cli request failed with status ${response.status}`),
        { response: { statusCode: response.status, body } })
}

Also consider reusing the existing ResponseError from src/fetchWrapper.ts instead of a new ad-hoc { response: { statusCode } } shape. Note that ResponseError.response is a Response, so it exposes .status, not .statusCode.

@AakashHotchandani AakashHotchandani Oct 8, 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.

Partly fixed in 4eb68d2: requestToUpdateCLI now checks response.ok first and parses the error body defensively (response.json().catch(() => null)), so an HTML or empty 502/503/504 rejects with response.statusCode instead of a bare SyntaxError. The new test is keeps the status code when a non-2xx reply has a non-JSON body. I kept the { response: { statusCode, body } } shape rather than reusing ResponseError. ResponseError.response is a Response (it exposes .status), and its message consumes the body via res.text(). The fallback catch in checkAndUpdateCli and the first-commit tests read response.statusCode, so switching would change both for no behavioural gain.

// Check the status before parsing: an LB error page (HTML/empty 5xx) must still carry
// its status code rather than surface as a SyntaxError.
const body = await response.json().catch(() => null)
logger.debug(`response ${response.status} ${JSON.stringify(body)}`)
throw Object.assign(
new Error(`update_cli request failed with status ${response.status}`),
{ response: { statusCode: response.status, body } },
)
}
const jsonResponse = await response.json()
logger.debug(`response ${JSON.stringify(jsonResponse)}`)
return jsonResponse
Expand Down
Loading
Loading