fix(observability): make TRA build-stop survive corporate-network blips (SDK-7229) - #134
fix(observability): make TRA build-stop survive corporate-network blips (SDK-7229)#134Bhargavi-BS wants to merge 2 commits into
Conversation
…ps (SDK-7229) The build-stop PUT is the only signal that closes a TRA build. SDK-7061 added a 3-attempt retry, but the whole window was ~1.5s (500ms + 1000ms) — shorter than a typical corporate DNS/proxy blip — and the request carried no timeout at all, so a connection that never settled could stall onComplete indefinitely. On top of that, every failure was logged as a bare `TypeError: fetch failed`. Node's fetch keeps the actionable detail (ENOTFOUND, ECONNRESET, proxy refusal) on `error.cause`, which plain interpolation drops — so a failed build stop was indistinguishable from any other network fault in a customer log. - Widen the retry to STOP_BUILD_MAX_ATTEMPTS (4) with exponential backoff (1s/2s/4s), capped by a STOP_BUILD_TOTAL_BUDGET_MS (30s) wall-clock deadline. The deadline bounds the added shutdown cost regardless of attempt count. - Bound each attempt with an AbortController (STOP_BUILD_ATTEMPT_TIMEOUT_MS, 10s, clamped to the remaining budget), and report an aborted attempt as a timeout rather than a generic AbortError. - Add describeErrorWithCause() and use it on every build-stop failure log so the underlying transport reason is recorded. Observed against the customer's exact failure mode (fetch rejecting with an ENOTFOUND cause): 1 attempt before SDK-7061, 3 attempts / 1.5s on 9.33.1, and 4 attempts / 7.0s here — with the DNS cause now present in the log line.
|
RUN_TESTS |
|
[SDK Wdio Test] TRA build state: failed | Stability 98% — verdict: success. Passed: 83, Failed: 2, Aggregate: 85. TRA: https://observability.browserstack.com/builds/fgzvszvtubfi6ajhgkix0vn3dyrdfoz9ev248di1 |
| // so a transient transport failure is worth retrying past a short network blip — but the | ||
| // whole thing runs during shutdown, so the total cost is capped by a wall-clock deadline | ||
| // rather than by attempt count alone. | ||
| export const STOP_BUILD_MAX_ATTEMPTS = 4 |
There was a problem hiding this comment.
are these existing values? , if not on what basis were these values considered?
There was a problem hiding this comment.
Mostly derived, not random numbers:
- 4 attempts — bumped up from the existing 3 (SDK-7061), and switched the backoff to exponential (1s/2s/4s).
- 30s total budget — this is the real cap. The retry runs during shutdown (
onComplete), so I bound the whole loop on wall-clock time: long enough to ride out a short network/DNS blip, short enough to not drag out CI teardown. The attempt count matters less because of this. - 10s per attempt — same idea as the existing
nodeRequesttimeout (120sAbortController), just tighter since build-stop is best-effort. Also lines up with the ~10.5s stall we saw in the customer's logs.
rounak610
left a comment
rounak610
left a comment
There was a problem hiding this comment.
Review status — SDK-7229 build-stop resilience
Ran an expert review over the diff (retry/budget/backoff/abort, describeErrorWithCause, tests, changeset).
Core fix is sound. Hand-traced the budget/backoff/abort arithmetic against all four stopBuildUpstream scenarios and it matches the stated behaviour — 4 attempts · 1s/2s/4s exponential · 30s wall-clock budget · per-attempt AbortController clamped to the remaining budget. Abort is detected via controller.signal.aborted (state-based, not the fragile error.name check) and timeoutId is cleared in finally on every exit path. describeErrorWithCause is depth-capped, so it terminates even on a cyclic cause. The changeset patch bump is correct — none of the new exports are reachable through the package's public exports map.
Worth fixing (should-fix — can land here or as a fast follow-up)
The usage-stats telemetry still drops the cause chain. stopBuildUpstream enriches the two local [STOP_BUILD] debug lines with describeErrorWithCause, but the failure value handed to telemetry does not:
src/util.ts:843 stopBuildUsage.failed(lastError) → FeatureUsage.failed() (src/testOps/featureUsage.ts:36) → getErrorString() (src/util.ts:1824), which returns only err.message. So FeatureUsage.toJSON().error — the payload BrowserStack actually ingests — still records the bare "fetch failed", losing the exact ENOTFOUND/proxy detail that SDK-7229 exists to diagnose at scale. That telemetry channel is the one that benefits most from the enrichment, and it's the one the diff didn't touch.
One-liner: stopBuildUsage.failed(describeErrorWithCause(lastError)) — getErrorString passes strings through unchanged, so this preserves the full chain in telemetry with no other change.
Considered, non-blocking (no change needed to merge)
A handful of low-probability / nit items were reviewed and judged follow-up-grade: the abort timer stays armed through the (tiny-ack) response.text() read; CLI_STOP_TIMEOUT (5s) vs the new 30s budget disparity across the two shutdown stop paths; no minimum floor on the clamped per-attempt timeout (a budget-starved final attempt can be aborted almost immediately); and a couple of loose test assertions (range vs exact count, silent depth-cap truncation). None block merge.
Verdict:
Automated review pass.
4 participants
What is this about?
The build-stop
PUTis the only signal that closes a Test Reporting build. SDK-7061 already added a 3-attempt retry, but two gaps remained — both surfaced by SDK-7229 (Danske Bank), where builds sat as "running" for hours until the ~60-min server-side inactivity timeout marked them timed out.1. The retry window was far shorter than a real network blip. Backoff was
500ms * attemptover 3 attempts — a ~1.5s total window. The customer runs on a restricted corporate network (*.danskenet.net) where a DNS/proxy hiccup routinely outlasts that.2. The request had no timeout at all. Unlike
nodeRequest(120sAbortController), the stopPUTpassed nosignal, so a connection that never settled could stallonCompleteindefinitely. The customer's log shows a 10.5s stall betweenSending stop launch eventand the failure.3. The failure reason was thrown away. Node's fetch reports every transport failure as an opaque
TypeError: fetch failed; the actionable detail lives onerror.cause, which plain interpolation drops. Their logs only ever said:We only learned DNS was involved because a sibling EDS upload happened to print its cause (
getaddrinfo ENOTFOUND eds.browserstack.com). This is what made the ticket bounce between teams for a week.Changes
STOP_BUILD_MAX_ATTEMPTS(4) with exponential backoff (1s/2s/4s), capped by aSTOP_BUILD_TOTAL_BUDGET_MS(30s) wall-clock deadline — the deadline bounds the added shutdown cost regardless of attempt count.AbortController(STOP_BUILD_ATTEMPT_TIMEOUT_MS10s, clamped to remaining budget); an aborted attempt is reported as a timeout rather than a genericAbortError.describeErrorWithCause()and use it on every build-stop failure log.Verification
Probed the shipped npm artifact of each version with
globalThis.fetchstubbed to throw the customer's exact error (TypeError: fetch failedwith anENOTFOUNDcause):latest)Log line produced by this branch on that same failure:
Related Jira task/s
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
fetch failed.Release notes (internal): (required — engineer-facing; what actually changed / why)
stopBuildUpstreamretry widened from 3 attempts / ~1.5s linear backoff to 4 attempts / 1s-2s-4s exponential, bounded by a 30s wall-clock deadline (STOP_BUILD_TOTAL_BUDGET_MS) so shutdown cost stays capped.AbortControllersignal (STOP_BUILD_ATTEMPT_TIMEOUT_MS10s, clamped to remaining budget). Previously the stopPUThad no timeout whatsoever and could hangonComplete.describeErrorWithCause()walkserror.cause(depth-capped at 3) and is used on all[STOP_BUILD]failure logs — native fetch collapses every transport fault toTypeError: fetch failed, which made SDK-7229 undiagnosable from customer logs..changeset/pr-<number>.mdfrom the Release section above.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.