fix(error-reporting): stop reporting network fetch failures as crashes (CLI-16W)#1152
Merged
Conversation
When the CLI cannot reach the Sentry API (offline, DNS failure, connection refused/timeout, proxy), Node's fetch rejects with a raw "TypeError: fetch failed". handleFetchError only special-cased abort, TLS, and timeout errors, so generic network failures propagated as an uncaught TypeError — surfaced to the user as a cryptic crash and reported to Sentry as a bug (CLI-16W and related DNS/connect-timeout issues). - Add isNetworkError() in errors.ts (ApiError status 0, or a raw "fetch failed" TypeError) as the single source of truth. - handleFetchError now wraps an exhausted network failure in a clear ApiError(status 0) with an actionable message, mirroring throwApiError. - isUserError treats network errors as user-environment (no upgrade nudge). - classifySilenced silences network errors (reason network_error): a "user is offline" report is not actionable — same rationale as dropping EPIPE/EBADF OS noise in beforeSend.
Contributor
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Contributor
Codecov Results 📊✅ Patch coverage is 100.00%. Project has 5132 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.46% 81.47% +0.01%
==========================================
Files 397 397 —
Lines 27694 27699 +5
Branches 17983 17989 +6
==========================================
+ Hits 22561 22567 +6
- Misses 5133 5132 -1
- Partials 1861 1862 +1Generated by Codecov Action |
Seer (HIGH) flagged that isNetworkError matched ApiError(status 0), which is shared by genuine connectivity failures AND TLS certificate errors — the latter being actionable user-config issues (e.g. a missing CA cert) that must stay visible. isTlsCertError can't reliably re-detect TLS from the wrapped ApiError(0), so distinguishing by status is unsafe. Narrow isNetworkError to ONLY the raw `TypeError: "fetch failed"` (undici's unambiguous network-failure signature), which is exactly what escapes uncaught in CLI-16W. ApiError(status 0) — including TLS cert errors — is no longer treated as a network error and stays captured/actionable. Revert the handleFetchError conversion (it routed network failures through ApiError(0), reintroducing the TLS overload); isUserError keeps its explicit status-0 check so TLS/network ApiErrors still skip the upgrade nudge. Added a regression test proving TLS ApiError(0) is not silenced.
BYK
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When the CLI cannot reach the Sentry API — offline, DNS failure, connection
refused/timeout — Node's
fetch(undici) rejects with a rawTypeError: "fetch failed". This escapes uncaught and is reported to Sentry asa CLI bug. This is CLI-16W
(
TypeError: fetch failed, culpritGetAddrInfoReqWrap.onlookupall (node:dns))— 17 users, ongoing on 0.38.0.
A "user is offline" report is not actionable for the CLI team — the same class
of noise the codebase already drops for EPIPE/EBADF in
beforeSend.Fix
isNetworkError()(new, inerrors.ts) — matches the rawTypeError: "fetch failed", undici's unambiguous signature for everynetwork-level failure (the real errno is in
cause).classifySilencedsilences network errors (reason: network_error);volume is preserved via the
cli.error.silencedmetric.isUserErrortreats a raw networkTypeErroras user-environment (no"Upgrading may resolve this" nudge).
Why not
ApiError(status 0)(addressing Seer review)status 0is shared by genuine connectivity failures and TLS certificateerrors. TLS errors are actionable user-config issues (e.g. a missing CA cert)
and must stay visible, and
isTlsCertErrorcan't reliably re-detect TLS fromthe already-wrapped
ApiError(0). SoisNetworkErrordeliberately matchesonly the raw
TypeError—ApiError(status 0), including TLS, stayscaptured/actionable.
isUserErrorkeeps its explicitstatus === 0check sothose still skip the upgrade nudge.
Test plan
isNetworkError: rawfetch failedTypeError → true;ApiError(0)(incl.TLS), HTTP statuses, unrelated TypeErrors, non-errors → false.
classifySilenced:fetch failedTypeError →network_error; TLSApiError(0)→ not silenced (regression test); unrelated TypeError →captured.
isUserError: rawfetch failedTypeError → true.reportCliError: network error emits the metric and is not captured.vitest run test/lib/{errors,error-reporting,telemetry,sentry-client}.test.ts→ 316 passed;
biome checkclean.Fixes CLI-16W