fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(client): surface HTTP status, bound requests, and type API errors - #29

Open
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening
Open

fix(client): surface HTTP status, bound requests, and type API errors#29
karaposu wants to merge 1 commit into
brightdata:mainfrom
karaposu:fix/http-boundary-hardening

Conversation

@karaposu

@karaposukaraposu commented Aug 27, 2026

Copy link
Copy Markdown

Three defects in src/utils/client.ts — the one function every API call in the CLI goes through. They're bundled because they're the same root cause (the boundary hides what it knows) and touch the same 30 lines.

1. The client discards the HTTP status, so polling guesses

On success the client returned only the parsed body. 202 is res.ok, so a caller could not distinguish "job accepted, still building" from "here is your data" — both arrived as a body with no status attached.

Snapshot polling therefore infers readiness from the body's shape (extract_status + RUNNING_STATUSES). That works while the body is a parsed object, i.e. --format json. It silently fails for --format csv|ndjson|jsonl, where the client returns text:

constextract_status=(result: unknown)=>{if(!result||typeofresult!='object')// a string body exits herereturnundefined;// → "not running" → treated as data

So a not-ready snapshot requested as JSONL gets printed as if it were the dataset, and the CLI exits 0. Silent wrong data is the worst outcome for exactly the ETL workflows those formats exist to serve.

Fix: an opt-in get_with_status() returning {status, headers, body}. request() / get() / post() still return the body, so no existing call site changes — only code that must reason about the protocol opts in. Readiness now checks, in order of authority: HTTP 202 → parsed-object status → text body that parses to a status. The predicate returns the real status string, so progress output still shows startingbuildingrunning instead of collapsing to one token.

Headers are exposed too — that's the plumbing a future Retry-After-aware backoff needs.

2. No request timeout — the CLI could hang forever

fetch() has no default timeout and was called without a signal. A black-holed connection (VPN drop, hung LB) produced no error at all, so the retry loop never engaged: spinner spinning, no output, Ctrl-C the only exit.

Fix:AbortSignal.timeout per attempt (fresh signal each time — hoisting it would abort retries instantly), default 120s, overridable via Request_opts.timeout_ms.

Two deliberate choices worth reviewing:

  • 120s, not 30s. The abort must bound hung connections without cutting off slow-but-alive work — protected-site scrapes and large snapshot bodies legitimately run long. Happy to lower it if you have real numbers on the long tail.
  • A timed-out attempt retries once, not 3×. Reusing the generic retry budget would mean 4 × 120s ≈ 8 minutes of silent stall — multiplying the very wait this fix exists to bound. The retry is also narrated rather than silent.

3. Retry was decided by error message prose

if(einstanceofError&&e.message.startsWith('Error:'))throwe;// treat as final API error, don't retry

Retry semantics were coupled to message wording: rewording a template silently changes behavior, and a network failure whose message happens to start with Error: was misclassified as final and never retried.

Fix: a Client_api_error class (carrying status and hint) discriminated with instanceof. Message bytes are unchanged — scraper-studio matches on error prose (REALTIME_LIMIT_MARKER = 'realtime job limit', clean_error_message), so that contract is preserved and now has a test pinning it.

Testing

  • 22 new tests, 395 total. The request() loop had no test coverage at all before this — these are the first.
  • The readiness predicate is table-tested across every combination of {200, 202} × {object body, text body}, because that matrix is precisely where the silent-corruption case lives.
  • vi.mocks utils/config so URL assertions don't depend on the developer's local config.json (load_config() runs on every request and can override api_url).
  • Verified by running the built CLI.

Scope

Deliberately not included: retry policy/backoff changes (that's #20's area), migrating other call sites (status.ts is correctly excluded — for /datasets/v3/progress the status metadata is the payload), and retiring scraper.ts's parallel fetch_raw helper. That helper exists only because the shared client couldn't expose status codes; get_with_status removes its reason to exist, but that's a follow-up, not this diff.

Independent of #28 (Node 20 fix) — different files, no conflicts, either can merge first.

Note: this PR reports no CI checks because main has no CI workflow yet (#28 adds one — type-check + tests on a Node 20.17.0 / 24 matrix). Verified locally instead: tsc --noEmit clean, 391 tests passing, and the built CLI smoke-run.

Three defects in src/utils/client.ts, the function every API call goes
through:
1. Protocol erasure. On success the client returned only the parsed body
and discarded the status code, so a caller could not tell HTTP 202
(job accepted, still building) from 200 (this is your data) — 202 is
res.ok, so both looked identical. Snapshot polling therefore inferred
readiness from the body's *shape*. That misses the not-ready case
whenever the body is text, which is exactly what --format csv/ndjson/
jsonl produce: the status stub is printed as if it were the dataset
and the CLI exits 0. Silent wrong data is the worst failure mode for
the ETL use case these formats exist for.
2. No request timeout. fetch() has no default timeout and was called
without a signal, so a black-holed connection (VPN drop, hung load
balancer) hung forever — no error ever arrived for the retry loop to
react to. The only way out was Ctrl-C.
3. Retry decided by message prose. The catch block told API errors from
network errors with message.startsWith('Error:'), so rewording an
error template silently changed retry behavior, and a network failure
worded that way was misclassified as final and never retried.
Changes:
- Add Response_envelope {status, headers, body} and an opt-in
get_with_status(). request()/get()/post() keep returning the body, so
all existing call sites are untouched; only pollers that must reason
about the protocol opt in. Headers are exposed too, which is what a
future Retry-After-aware backoff would need.
- Add Client_api_error (carrying status and hint) and discriminate with
instanceof. Message bytes are unchanged — scraper-studio matches on
error prose (e.g. 'realtime job limit'), so that contract is preserved
and now covered by a test.
- Abort each attempt with AbortSignal.timeout (default 120s, override via
Request_opts.timeout_ms). The default is deliberately generous so it
cannot cut off slow-but-alive work such as protected-site scrapes or
large snapshot downloads. A timed-out attempt retries at most once
rather than reusing the 3-retry budget, because timeout x attempts
multiplies the stall the fix exists to bound, and the retry is narrated
instead of being silent.
- Migrate the pipelines snapshot poller to decide readiness from the
protocol first, keeping body-shape checks as fallback and adding the
text-body case that the object-only check missed. The predicate returns
the real status string, so progress output still distinguishes
starting/building/running.
Tests: 22 new (395 total). The client request loop had no coverage at all
before this; the readiness predicate is table-tested across every
combination of {200, 202} x {object body, text body} because that matrix
is where the silent-corruption case lives.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@karaposu