Fix six runtime defects: free-tier crash, NaN config, duplicate browser refs, erased errors, failed-job-as-data, stale-token attribution by karaposu · Pull Request #163 · brightdata/brightdata-mcp

karaposu · 2026-08-12T13:09:39Z

Six independent, self-contained bug fixes surfaced by an audit of the runtime paths. Each is a
separate commit with a detailed message; each was verified before and after the change. No feature
work, no refactors, no dependency changes. Test suite goes from 26 → 32 (6 new tests added where the
code is reachable without the full server bootstrap).

Every fix shares one shape: a path that was reporting something the server could not substantiate,
or destroying the information a caller needed. Ordered below by severity.

1. Free-tier limit message crashed with a `TypeError` instead of rendering

tool_fn's quota-exceeded branch built its message from adjacent template literals with a missing
+. Adjacent template literals are a tagged-template call at runtime — the first literal becomes
the tag function for the second — so evaluating the expression threw

TypeError: "3. Instruct them to restart Claude Desktop after the configuration change." is not a function

before the Error was ever constructed. Every free-tier user who hit the 5,000-request monthly
limit received that TypeError instead of the upgrade guidance. It was syntactically valid, so it
survived node --check, boot, and the whole test suite.

Fix: the message is now a module-level const built from single-quoted pieces joined explicitly.
It is constructed eagerly at startup, so any future construction mistake fails the boot (and the
server-health test) rather than the one moment a user needs it; single-quoted strings also make a
missing + a parse error rather than a runtime call. Repairs seven glued seams the old
concatenation produced (monthlylimit, stopthe, atbrightdata.com, …). Message content
otherwise unchanged.

Verified by executing the expression extracted verbatim from the file (reproduced the TypeError),
then re-checking the new const renders and contains the expected text.

2. A malformed numeric env var silently disabled six tools and threw a blank error

parseInt returns NaN on bad input, and every comparison with NaN is false.
BASE_MAX_RETRIES=three (or -1, or "2" with literal quotes) made base_request's loop condition
0 <= NaN — so the loop ran zero times, no request was attempted, and the function fell through
to throw last_err with last_err still undefined. Six call sites, including search_engine and
scrape_as_markdown (two of the five default tools), failed with a thrown undefined that carries
no message, stack, or status at any layer. POLLING_TIMEOUT=abc had the same effect on the dataset
poll loops (all 50 web_data_* tools would immediately report a timeout).

Fix: all three numeric env vars (BASE_MAX_RETRIES, POLLING_TIMEOUT, BASE_TIMEOUT) parse
through one validator that warns to stderr — naming the variable, the rejected value, and the
fallback — then continues. base_request also gained a last_err ?? new Error(...) backstop so it
can never rethrow a bare undefined.

Verified with the extracted parse lines + real base_request and a counting axios stub: 0 attempts
and a thrown undefined before, a real Error after. Three spawn-based tests added.

3. Duplicate DOM refs made the browser click the wrong element and report success

The DOM-scan ref counter is declared inside the page.evaluate callback, so it restarts at zero
every snapshot — but the data-fastmcp-ref attributes persist on the page, and the minting guard
skipped anything already tagged. On a second snapshot of a page that gained elements, old elements
kept dom-1..N while new elements were assigned dom-1..N again. Resolution uses .first(), which
silently picks whichever duplicate comes earlier in the document — a wrong-element click reported as
success, with no error.

This was masked by a second bug: the snapshot tool declared filtered=false while capture_snapshot
declares filtered=true; the tool's value won, so the default path skipped compaction, skipped the
DOM-fallback scan, and never populated _dom_refs. The two had to be fixed together — enabling the
filter without fixing the counter would have turned a dormant bug live.

Fix: minting always renumbers (a ref means "position in the most recent snapshot"); _dom_refs
resets per capture; the DOM branch of ref_locator verifies the element still exists and throws a
clear "stale — capture a new snapshot" instead of an uninformative interaction timeout; and the
tool default is now filtered=true, matching the method (filtered=false remains available for the
raw dump).

Verified end-to-end by driving the real Browser_session against a headless Chrome: after mutating
the page and re-snapshotting, the model-facing list contained both [dom-1] "NewOne" and
[dom-1] "Alpha" and resolution hit the wrong one; after the fix every ref is unique and resolves
to the element the list names. Three real-browser regression tests added (skip cleanly without
Chrome).

4. Error causes were erased — and one path leaked the API token

Two sites dropped the failure cause before it reached the caller:

scrape_batch stringified the raw Promise.allSettled result. Plain Errors serialize to {}
(message/stack are non-enumerable), so failures came back as {"status":"rejected","reason":{}} —
no URL, no reason. Worse, for the common axios error, AxiosError.toJSON dumps the whole request
config: reproduced against a local 403, the caller received a 2,476-char entry containing the
Authorization bearer token in plaintext and a full stack trace, while omitting the upstream
body. Fix keeps the envelope but replaces rejected entries with {status:'rejected', url, reason}
(a plain message string); fulfilled entries pass through byte-identical, so nothing that consumed
the success shape changes, and the token leak is closed.
tool_fn error translation guarded with if (message?.length), which is falsy for objects.
Bright Data returns JSON error bodies, so the actual explanation (e.g. "url must be a valid Amazon
product URL") — already logged to stderr — was discarded and replaced with axios's generic
Request failed with status code N. Fix stringifies object bodies before the guard and caps the
message at 500 chars so a large error page can't flood the caller's context; string and empty
bodies behave exactly as before.

Both verified by executing the changed code over four/five body shapes, comparing old (from git) vs
new side by side.

5. Failed dataset collections were handed back as if they were records

The dataset poll loop decided a job was finished by testing only for the pending statuses
(running/building/starting); anything else fell through to the success path. A snapshot with
status failed was serialized and returned as data — and since the trigger sets
include_errors: true, the payload returned "as records" was the collection's error rows.

Fix inverts on shape rather than guessing undocumented failure strings: a ready snapshot is a
JSON array with no status field, so any response still carrying a string status after the pending
check is terminal and is thrown with its content attached (capped), tagged so the retry catch
rethrows immediately instead of burning the polling budget into a misleading timeout. (The sibling
discover loop has a milder variant — returns [] on failure — left for a separate decision.)

Verified across five scenarios against the extracted loop, including a ready-after-pending case to
confirm the normal path is intact.

6. Rotated API token reported a false, actionable cause ("Token expired")

The server read API_TOKEN once at startup; after a user rotated their key, the running process kept
presenting the old one and every call failed with HTTP 401: Token expired — Bright Data's own text,
relayed verbatim. The cause was false (the token wasn't expired) and actionable, so users
regenerated a valid key repeatedly. The remedy — update the config and restart the client — was never
suggested. A 401 also surfaced two other ways: swallowed at startup, and retried 600× in the poll
loops before surfacing as a Timeout.

Fix: record whether the startup auth call succeeded, and render a message the server can actually
substantiate — "this credential's validity changed while the server was running", never
"you rotated your key" (a deleted zone or revoked permission produces the same evidence). The
message names both remediation scenarios it can't distinguish and gives the user the check the server
can't run. Delivered from a leaf module (auth_error.js, importable without a bootstrap cycle) at all
four places a 401 surfaces; the poll loops now treat 401 as terminal, ordered after the existing
usage-limit check so a quota response can't be misreported as an auth failure.

Notes for review

Two behavior-visible changes: the snapshot tool now defaults to filtered=true (compact list +
DOM fallback instead of the raw ARIA dump), and failed dataset jobs now throw instead of returning.
Everything else is invisible or strictly additive.
Testing: suite 32/32. Fixes 2, 3, and 6 carry new/existing automated tests. Fixes 4 and 5 live
inline in server.js, which cannot be imported without executing the full startup (including a
network call), so they were verified by executing the extracted code rather than a committed unit
test — the same testability constraint that would be resolved by splitting server.js, out of
scope here.

A rotated API key made every call fail with `HTTP 401: Token expired`. That string is Bright Data's, relayed verbatim by tool_fn without any statement of what this server independently knows. It named a cause that was false and actionable, so users acted on it -- regenerating a key that was already valid -- for weeks. The real remedy (correct mcp.json, restart the client) was never suggested. The underlying defect is that the server held no memory of its own credential state, so it had nothing true to say about why a token was rejected. Four parts: 1. Record whether the startup auth call succeeded. ensure_required_zones already makes that authenticated request; this only remembers the result. The record licenses exactly one claim -- that validity CHANGED while the server ran -- never "you rotated your key", since deleting a zone or revoking a permission produces the same evidence. 2. Render the message from that record via a pure function in a new leaf module. Leaf because server.js imports browser_tools.js, so a shared helper cannot live in server.js. Pure because this repo has no HTTP mocking, making it the only shape in which "never reports 'Token expired'" is testable at all. Built with [...].join(), the house pattern -- the one existing message of this kind (client_10100) is broken by adjacent template literals and throws a TypeError instead of running. 3. Deliver it at every end-state a 401 reaches: startup (log only -- exit vs degraded start is a separate, undecided question), the browser credential path (14 tools that bypass tool_fn), and tool_fn itself (~60 tools). Ordered after the usage-limit check so a quota response cannot be misreported as an auth failure. 4. Stop classifying a rejected credential as retryable in the two poll loops. A 401 was retried once a second for ten minutes and then reported as `Timeout after 600 seconds` -- a second false cause for the same problem, affecting 51 tools. Broadening 400 to 4xx aligns them with base_request, which already treats all client errors as terminal. Behavior change beyond the token case: those tools now fail fast on any 4xx. Where evidence cannot decide between two remedies -- mcp.json holding a replaced token vs the process holding a rotated one -- the message names both and supplies the check the user can run but the server cannot. Adds auth_error.js to package.json files[]; it is an allowlist, and without it npm publish would ship a server.js importing a missing module. Tests: 10 new cases pinning the acceptance criterion (message instructs a restart, carries the hint marker, and never contains "expired"), runnable in CI with no network. Full suite 26/26.

…dering The quota-exceeded branch in tool_fn built its message from adjacent template literals with a missing + between steps 3 and 4. Adjacent template literals are a tagged-template CALL — the first literal becomes the tag function for the second — so evaluating the expression threw TypeError: "3. Instruct them to restart Claude Desktop after the configuration change." is not a function before the Error was ever constructed. Every free-tier user who reached the 5,000-request monthly limit got that TypeError instead of the upgrade guidance. Verified by executing the expression extracted verbatim from this file; syntactically valid, so it survived node --check, boot, and the full test suite. The message is now a module-level const built from single-quoted pieces joined explicitly: - constructed eagerly at startup, so any future construction mistake fails the boot (and test/server-health.test.js) rather than the one moment the message is needed; - single-quoted strings make a missing + a PARSE error instead of a runtime tagged-template call; - repairs seven glued seams the old concatenation produced ("monthlylimit", "stopthe", "steps:1.", "atbrightdata.com", 'selecting"Unlocker', '",and creating', "newzone"). Message content is otherwise unchanged.

…olling parseInt returns NaN on malformed input, and every comparison with NaN is false. That poisoned the two loops these variables bound: - BASE_MAX_RETRIES=three (or -1, or "2" with literal quotes) made base_request's for-loop condition 0 <= NaN, so the loop ran ZERO times: no HTTP request was ever attempted, and the function fell through to `throw last_err` with last_err still undefined. Six call sites -- including search_engine and scrape_as_markdown, two of the five default tools -- failed with a thrown `undefined` that carries no message, no stack, and no status at any layer. Verified by executing the parse lines and base_request extracted verbatim from this file with a counting axios stub: attempts made = 0, thrown value === undefined. - POLLING_TIMEOUT=abc made the dataset poll loops' `attempts < max_attempts` condition 0 < NaN, so all 50 web_data_* tools plus discover skipped waiting entirely and immediately threw "Timeout after NaN seconds". - BASE_TIMEOUT=abc is the mild case: axios guards with `if (config.timeout)` and NaN is falsy, so it silently behaved as "no timeout" -- same as the default, just unannounced. Included for consistency and visibility, not severity. All three now parse through parse_int_env, which warns to stderr naming the variable, the rejected value, and the fallback used, then continues. Warn-and-continue (rather than RATE_LIMIT-style throw) keeps malformed optional tuning from taking the server down, while making the misconfiguration visible for the first time. base_request additionally gets a backstop -- `throw last_err ?? new Error(...)` -- so it can never rethrow a bare undefined even if a future edit reintroduces a zero-attempt path. Tests: three spawn-based cases boot the real entry point and assert the warnings appear (and don't appear for well-formed values). Suite 29/29.

…y default Two defects that had to land together. The collision: the DOM-scan ref counter is declared inside the page.evaluate callback, so it restarts at zero on every capture -- but the data-fastmcp-ref attributes live on the page's elements and persist, and the minting guard skipped anything already tagged. On a second capture of a page that gained elements, old elements kept dom-1..N while new elements were assigned dom-1..N again. Reproduced against the real Browser_session in a headless system Chrome: after prepending two buttons and re-snapshotting, the model-facing list contained BOTH "[dom-1] button NewOne" and "[dom-1] button Alpha", and ref_locator("dom-1") resolved via .first() to whichever came earlier in the document -- a wrong-element click reported as success. Minting now always overwrites, so a ref means exactly "position in the most recent snapshot" and every capture renumbers the whole page. The mask: the snapshot tool declared filtered=false while capture_snapshot declares filtered=true; the tool's explicit value always won, so the default path skipped compaction, skipped the DOM fallback scan (the code that catches elements invisible to the accessibility tree), and never populated _dom_refs -- which is also why the collision stayed dormant. The tool now defaults to filtered=true, matching the method; filtered=false remains available for the raw dump. Flipping the default without fixing the counter would have converted a dormant bug into a live one, hence one commit. Hardening in the same change: - capture_snapshot resets _dom_refs up front, so refs from an older capture can no longer dispatch to the DOM branch after a raw snapshot. - ref_locator's DOM branch verifies the element still exists and throws "stale -- Try capturing new snapshot" instead of letting a post-navigation ref die as an uninformative interaction timeout. Tests: three cases drive the real Browser_session against a locally launched Chrome (channel fallback, skip when unavailable): uniqueness + list/resolution agreement on a mutated page, the stale-after-navigation error, and raw-snapshot ref invalidation. Suite 32/32.

Two sites where the error path destroyed or replaced the cause before it reached the model -- and, verification showed, one where it leaked far more than the cause. scrape_batch: the raw Promise.allSettled envelope was stringified directly. For plain Errors (the remark stage) that serialized rejections to {"status":"rejected","reason":{}} -- message and stack are non-enumerable, and the url was bound inside the .then, so failures carried nothing. For axios errors -- the common case -- it was worse than documented: AxiosError defines toJSON, which dumps the entire request config into the result. Reproduced against a local 403 server with the real base_request: the model received a 2,476-char rejection entry containing the Authorization header's bearer token in plaintext and a full stack trace with filesystem paths, while omitting the upstream response body. Rejected entries now carry {status:'rejected', url, reason: <message string>}; fulfilled entries pass through byte-identical, so consumers of the success envelope are unaffected, and nothing could have depended on either failure shape (one was empty, the other noise plus a secret). tool_fn: the translation guard `if (message?.length)` only passes strings and arrays. Bright Data's API frequently returns JSON error objects, which have no .length, so the actual explanation -- already logged to stderr three lines earlier -- was discarded and axios's generic "Request failed with status code N" was rethrown instead. Object bodies are now stringified before the guard, and the message is capped at 500 chars with an ellipsis so a large error page cannot flood the model's context. String bodies and empty bodies behave exactly as before. Verified by running the old lines (from git) and the new lines (extracted from this file) side by side over four body shapes. No permanent unit test: both sites live inline in server.js, which cannot be imported without executing the full bootstrap including a network call. Verification was by executing the extracted code; proper tests are blocked on splitting server.js (a known, separate item).

The dataset poll loop (all 50 web_data_* tools) decided a job was done by testing only for the PENDING statuses running/building/starting. Anything else fell straight through to the success path and was serialized to the model as data -- so a snapshot with status "failed" was handed back as though it were the requested records. Worse, the trigger sets include_errors: true, so the payload the model received as data was the collection's error rows. Reproduced by executing the extracted loop with a stubbed axios returning {status:'failed', ...}: the loop returned that object as data. Rather than enumerate failure strings the API does not document, the fix inverts the test on the shape the data actually takes: a READY snapshot is the records payload, a JSON array with no status field. Any response that still carries a string status after the pending check is therefore a terminal non-success state (failed, canceled, or a status not yet seen), and is thrown with its content attached (capped at 500 chars) rather than returned. The error is tagged .terminal so the retry catch rethrows it immediately instead of burning the polling budget and reporting a misleading "Timeout after N seconds". Verified across five scenarios against the extracted loop: failed and canceled both throw immediately with the cause; a ready array returns unchanged; a job that is pending on early polls and ready later still returns its array (normal path intact); a perpetually-pending job still times out at the budget. Scope note: the sibling discover loop has a milder, distinct variant -- on failure it returns [] rather than error-as-data -- and is left for a separate decision. No permanent unit test: this loop lives inside the dataset-tool generation loop in server.js, which cannot be imported without running the full bootstrap. Verified by executing the extracted code; a real test is blocked on splitting server.js.

karaposu added 6 commits August 4, 2026 02:29

Navigation Menu

Uh oh!

Fix six runtime defects: free-tier crash, NaN config, duplicate browser refs, erased errors, failed-job-as-data, stale-token attribution - #163

Fix six runtime defects: free-tier crash, NaN config, duplicate browser refs, erased errors, failed-job-as-data, stale-token attribution#163
karaposu wants to merge 6 commits into
brightdata:mainfrom
karaposu:dev

karaposu commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

karaposu commented Aug 12, 2026

1. Free-tier limit message crashed with a TypeError instead of rendering

2. A malformed numeric env var silently disabled six tools and threw a blank error

3. Duplicate DOM refs made the browser click the wrong element and report success

4. Error causes were erased — and one path leaked the API token

5. Failed dataset collections were handed back as if they were records

6. Rotated API token reported a false, actionable cause ("Token expired")

Notes for review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

1. Free-tier limit message crashed with a `TypeError` instead of rendering