Skip to content

fix(api): clone request body per retry + classify timeouts (CLI-1D6)#829

Merged
BYK merged 3 commits intomainfrom
fix/sentry-client-retry-body-reuse
Apr 23, 2026
Merged

fix(api): clone request body per retry + classify timeouts (CLI-1D6)#829
BYK merged 3 commits intomainfrom
fix/sentry-client-retry-body-reuse

Conversation

@BYK
Copy link
Copy Markdown
Member

@BYK BYK commented Apr 23, 2026

Summary

Fixes CLI-1D6. The authenticated fetch retry loop reused the same (input, init) pair across attempts; any Request with a body (SDK path) or a ReadableStream init.body had its body consumed on attempt 1, so every retry after a timeout/5xx/401-refresh failed with TypeError: Request body already used. That bubbled up as the misleading ApiError: Unable to reach Sentry API. Cause: Request body already used — seen in the wild on a POST /autofix/ that timed out at 30s and couldn't retry.

Changes

All in src/lib/sentry-client.ts:

  1. Body re-use fixbuildAttemptFactory yields a fresh (input, init) per attempt:

    • Request input → request.clone() per attempt (teed body stream).
    • ReadableStream body → materialized once to an ArrayBuffer (single-use shape).
    • Everything else (string / ArrayBuffer / TypedArray / Blob / FormData / URLSearchParams / none) passes through unchanged. fetch re-serializes these on every call, and re-serialization is required for FormData so the auto-negotiated Content-Type: multipart/form-data; boundary=... header is regenerated (used by sourcemap chunk upload).
  2. Internal-timeout classification — Our per-request setTimeout-triggered abort now tags the caught error with a Symbol marker plus timeoutMs. handleFetchError surfaces a TimeoutError with "Request timed out after Ns." + actionable hint on the last attempt instead of the opaque "Network error" wrapper. User aborts (external signal) still propagate the original AbortError unchanged.

  3. Per-endpoint timeout allowlistENDPOINT_TIMEOUT_OVERRIDES table. Seer /autofix/ POSTs trigger server-side root-cause analysis that can take ~2 minutes; they now get 120s. Default stays 30s. Adding new slow endpoints is a one-line change.

No caller changes needed — TimeoutError and ApiError both extend CliError, so the central error handler and existing instanceof ApiError branches in explain.ts / plan.ts keep working.

User-facing effect

Before: sentry issue explain <issue> against a Seer-slow backend produced ApiError: Unable to reach Sentry API. Cause: Request body already used after ~33s (30s timeout + 3s retry backoff that always failed).

After: the request either succeeds thanks to the 120s budget, or produces a clean TimeoutError: Request timed out after 120s. with a link to https://status.sentry.io.

Tests

New test/lib/sentry-client.test.ts — first test coverage this file has had. 8 tests:

  • Retry-after-5xx reuses a string body.
  • Retry-after-5xx reuses a Request body (SDK path, was CLI-1D6 root cause).
  • Retry-after-5xx reuses a ReadableStream body via one-time materialization.
  • Retry-after-5xx preserves the multipart Content-Type on a FormData body (regression from Cursor Bugbot finding on 78dea18).
  • Internal timeout surfaces as TimeoutError after exhausting retries (uses __injectTimeoutOverrideForTests so the test doesn't wait 30s).
  • External abort still propagates the original AbortError unchanged.
  • resolveTimeoutMs returns 120_000 for /autofix/ paths (with/without trailing slash, with query params).
  • resolveTimeoutMs returns 30_000 for non-overridden paths, including substring false-positives like /autofixation-report/.

Verification

  • bunx tsc --noEmit — clean
  • bun run lint — clean (one pre-existing warning in markdown.ts, unrelated)
  • bun test test/lib/sentry-client.test.ts — 8/8 pass
  • bun test test/lib/api/ test/commands/issue/ — broader suite green

Notes

  • AGENTS.md churn from this branch is deliberately excluded — it belongs to separate lore-commits.
  • The hot path (string body via apiRequestToRegion / rawApiRequest) is allocation-free on retry — no regressions for the typical call.
  • Cursor Bugbot caught a real regression on the first iteration: materializing FormData stripped the multipart boundary. Fixed in f659777 by narrowing the materialization branch to just ReadableStream.

The authenticated fetch retry loop reused the same `(input, init)` pair
across attempts. On the SDK path (a `Request` object with a stream body)
or any non-primitive `init.body`, the body was consumed on attempt 1 and
every retry after a timeout/5xx/401-refresh failed with
`TypeError: Request body already used`. That bubbled up as the misleading
"Unable to reach Sentry API. Cause: Request body already used" surfaced
in CLI-1D6.

Three linked fixes, all internal to `src/lib/sentry-client.ts`:

1. `buildAttemptFactory` yields a fresh `Request.clone()` per attempt
   (SDK path) or materializes streams/FormData/Blobs once to an
   ArrayBuffer (defensive path). String bodies pass through unchanged,
   so the common `apiRequestToRegion`/`rawApiRequest` hot path has zero
   allocation overhead on retry.
2. Distinguish our internal per-request timeout from user-initiated
   aborts and generic network errors. The timeout's abort now tags the
   thrown error with a `Symbol` marker; on the last retry,
   `handleFetchError` surfaces a `TimeoutError` with "Request timed out
   after Ns." plus an actionable hint instead of the opaque "Network
   error" envelope. `isUserAbort` semantics are unchanged.
3. Per-endpoint timeout allowlist (`ENDPOINT_TIMEOUT_OVERRIDES`). Seer
   `/autofix/` POSTs trigger server-side root-cause analysis that can
   exceed 30s; they now get 120s. Default stays 30s for everything
   else. Adding new slow endpoints is a one-line change.

No caller changes needed — `TimeoutError` and `ApiError` both extend
`CliError` so existing error handlers (explain.ts/plan.ts) keep
working. Adds first test coverage for sentry-client.ts (9 focused tests
covering body-reuse, timeout classification, user-abort passthrough,
and the per-endpoint allowlist).
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://cli.sentry.dev/_preview/pr-829/

Built to branch gh-pages at 2026-04-23 11:29 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

Comment thread src/lib/sentry-client.ts
Cleanup pass after self-review:

- File-level recap block, `ENDPOINT_TIMEOUT_OVERRIDES` multi-paragraph
  docstring, and narrating inline comments collapsed to single-line
  forms consistent with the rest of the file.
- Dropped the unused `reason` field from `TimeoutOverride`. It wasn't
  read at runtime, only documented the pattern — the inline comment
  next to each entry does the same job.
- Replaced the try/catch in `resolveTimeoutMs` with a call to the
  existing `extractUrlPath` helper, which already handles unparseable
  URLs consistently.
- Renamed `getRequestTimeoutMs` to `__resolveRequestTimeoutMsForTests`
  to match the sibling `__injectTimeoutOverrideForTests` naming and
  signal the test-only intent.
- Replaced `Object.defineProperty` calls in `fetchWithTimeout` with a
  typed cast + direct assignment. `enumerable: false` wasn't load-
  bearing; Symbol-keyed properties are non-enumerable by default and
  `timeoutMs` doesn't need to be hidden.
- Removed the `401 refresh path` test — its own 10-line block comment
  admitted it only exercised the no-refresh-token early-return and not
  the actual refresh path. Dead coverage.
- Removed the `survives unparseable URLs` test for the same reason
  (now redundant with `extractUrlPath`'s own coverage).
- Removed ASCII `==========` section dividers (AGENTS.md prohibits
  decorative section markers) and the file-header numbered test list.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

Codecov Results 📊

138 passed | Total: 138 | Pass Rate: 100% | Execution Time: 0ms

📊 Comparison with Base Branch

Metric Change
Total Tests
Passed Tests
Failed Tests
Skipped Tests

✨ No test changes detected

All tests are passing successfully.

✅ Patch coverage is 100.00%. Project has 1933 uncovered lines.
✅ Project coverage is 95.32%. Comparing base (base) to head (head).

Coverage diff
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
+ Coverage    95.27%    95.32%    +0.05%
==========================================
  Files          284       284         —
  Lines        41152     41297      +145
  Branches         0         0         —
==========================================
+ Hits         39206     39364      +158
- Misses        1946      1933       -13
- Partials         0         0         —

Generated by Codecov Action

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 78dea18. Configure here.

Comment thread src/lib/sentry-client.ts Outdated
Cursor Bugbot caught a real regression: buildAttemptFactory was
materializing FormData to an ArrayBuffer on the "streams / Blobs /
FormData" defensive path. fetch auto-negotiates
`Content-Type: multipart/form-data; boundary=...` only for FormData
bodies, so the materialized ArrayBuffer arrived at the server with
no Content-Type header. The sourcemap chunk upload
(src/lib/api/sourcemaps.ts) sends FormData through this path — even
the first upload attempt would fail with a server 400/415.

FormData, Blob, and URLSearchParams are all re-readable by fetch
(each call re-serializes / re-opens a new stream). Only a bare
ReadableStream is single-use. Narrow the materialization branch to
ReadableStream and let everything else pass through.

Adds a regression test that posts FormData, fails the first attempt
with 503, and asserts both attempts carry a well-formed multipart
Content-Type and the FormData contents.
@BYK BYK merged commit 2adca5a into main Apr 23, 2026
26 checks passed
@BYK BYK deleted the fix/sentry-client-retry-body-reuse branch April 23, 2026 11:49
BYK added a commit that referenced this pull request Apr 23, 2026
## Summary

Fix the flaky `sentry-client.test.ts` unit tests — #829's merge commit
then tripped on [run
24835339085](https://github.com/getsentry/cli/actions/runs/24835339085/job/72694061015)
with `expect(callCount).toBe(2) → Received: 7`. The test replaced
`globalThis.fetch` with a counting mock and asserted the exact number of
calls, but during the 1 s retry backoff window five foreign HTTP calls
leaked through — `/api/0/organizations/1/`,
`/api/0/projects/1/4510776311808000/`, etc., i.e. this CLI's own Sentry
telemetry project. They came from async work in a prior test that
outlived that test's `afterEach` fetch restore, so by the time the
promises resolved they hit whatever `globalThis.fetch` pointed at — my
counting mock.

## Fix

Introduce `scopedFetchMock(marker, handler)` that only invokes `handler`
for URLs containing the unique per-test marker (`__test_string_body__`,
`__test_request_body__`, `__test_stream_body__`,
`__test_formdata_body__`, `__test_user_abort__`, `___timeout-test___`).
Any URL without the marker is delegated to the captured `originalFetch`
— which is the preload.ts blocker in the common case, so foreign calls
fail fast in their own context without polluting this test's
`callCount`.

No production code touched.

## Verification

- `bun test test/lib/sentry-client.test.ts` — 8/8 pass.
- `bun test test/lib/sentry-client.test.ts --rerun-each 30` — 240/240
pass.
- `bun test test/lib/sentry-client.test.ts test/lib/api/
test/commands/issue/` — 221/221 pass.
- `bunx tsc --noEmit` — clean.
- `bun run lint` — clean (unrelated pre-existing `markdown.ts` warning).

Also simulated the pollution scenario locally by running a parallel test
file that fires `setTimeout` →
`globalThis.fetch('https://foreign.example/…')` calls during
`sentry-client.test.ts` — the scoped mock correctly ignores them and the
assertions hold.
Sign up for free to 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