Skip to content

fix(http): retry a connection reset mid-body as a transport failure - #608

Merged
zeshi-du merged 3 commits into
TestSprite:mainfrom
Davidson3556:fix/body-reset-transport-retry
Nov 9, 2026
Merged

zeshi-du merged 3 commits into
TestSprite:mainfrom
Davidson3556:fix/body-reset-transport-retry

Conversation

@Davidson3556

@Davidson3556 Davidson3556 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Sometimes a 200 arrives but the connection drops while the body is still being read. Node's fetch reports that from the body reader as TypeError: terminated. http.ts read and parsed the body inside one catch, so this landed in malformedResponseError():

  • It was reported as INTERNAL (exit 1) with advice to check TESTSPRITE_API_URL or a captive portal.
  • It was never retried, even for a GET that a transport failure one step earlier (before the headers) would have retried.

Following the scope in the triage comment on #431:

  • Read and parse are separate steps.
    • A body-read failure is a transport failure. It goes through the same handling as a failed fetch: retried for GETs and Idempotency-Key writes within the existing transport budget, otherwise TransportError, which is UNAVAILABLE (exit 10) with the message connection closed while reading the response body: terminated.
    • A body read in full that still isn't JSON keeps the existing malformedResponseError (INTERNAL, exit 1), with no retry.
  • That transport handling (debug trace, retry decision, progress line, TransportError) moved into one private helper, transportRetryDelayOrThrow. The fetch failure and the body-read failure share it, so the two can't drift apart. The fetch path behaves exactly as before.
  • Unchanged:
    • PAYLOAD_TOO_LARGE and other typed errors from the bounded read
    • request timeouts and caller aborts mid-body (rethrowIfAbort)
    • interrupts
    • the 204 + allowNoContent bypass
    • the non-OK error path
    • keyless writes and retry: false, which are still never replayed

No new dependency.

Related issue

Closes #431

Season 4 CLI Improvement Bonus

  • I'm entering this PR for the Season 4 CLI Improvement Bonus

Discord username: Davidson

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that changes existing behavior)
  • Documentation only
  • Build / CI / chore

Checklist

  • PR targets the main branch.
  • Commits follow Conventional Commits
    (feat(...), fix(...), docs(...), …).
  • npm run lint and npm run format:check pass.
  • npm run typecheck passes.
  • npm test passes and coverage stays at or above the 80% gate.
  • New behavior is covered by unit tests (mock-based; no network or
    credentials required).
  • No secrets, API keys, internal endpoints, or personal data are included.
  • User-facing changes are reflected in README.md / DOCUMENTATION.md where
    relevant. (No doc change: this restores the documented transport-retry behaviour for one more failure point.)

Notes for reviewers

Before / after with the built CLI and a real socket. A local server answers the first request with 200 headers plus {"items":[, then destroys the socket. The second request gets a healthy body.

# before (main @ d61e6df)
$ testsprite --output json project list --endpoint-url http://127.0.0.1:8794
{
  "error": {
    "code": "INTERNAL",
    "message": "The server returned a non-JSON response (content-type: application/json) for an HTTP 200. This usually means the endpoint is not the TestSprite API — a proxy, captive portal, or login page can return HTML with a success status.",
    ...
$ echo $?
1

# after
$ testsprite --output json project list --endpoint-url http://127.0.0.1:8794
{
  "items": [ { "id": "proj_1", "name": "Demo" } ],
  "nextToken": null
}
$ echo $?
0

Tests (src/lib/http.test.ts, new describe: "connection reset while reading a 200 body"). The reset is simulated with a ReadableStream that delivers part of the body and then errors with TypeError('terminated') carrying an UND_ERR_SOCKET cause, which is the shape Node's fetch produces. 6 of the 7 fail on main and pass with the fix:

  • A GET reset is retried, then returns the healthy body (2 fetches).
  • A write with an Idempotency-Key is retried.
  • A keyless POST gets one attempt, then TransportError / UNAVAILABLE / exit 10, with the transport message and the requestId.
  • retry: false gets one attempt.
  • A reset on every attempt exhausts the transport budget (4 attempts) and ends UNAVAILABLE.
  • The debug trace records error/retry with errorCode: TRANSPORT.

The guard that passes before and after: a body read in full that's truncated JSON ({"ok":) is still INTERNAL, exit 1, single attempt. The existing non-JSON-200 tests from #166 also still pass.

Results: 4805 passed / 2 skipped (unit), 110 passed / 1 skipped (e2e). Typecheck, lint and format all pass.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of connection resets while reading successful HTTP responses.
    • Eligible GET requests and idempotent writes now retry transient transport failures.
    • Keyless writes and requests with retries disabled are not retried.
    • Malformed, fully read JSON responses now report a clear internal error.
    • Requests stop retrying after the four-attempt transport limit is reached.
    • Debug reporting now records transport failures and retry classifications consistently.

Review status: CodeRabbit reported no actionable comments. The branch merges cleanly with current main.

A 200 whose connection dropped while the body was being read (undici's
`TypeError: terminated`) hit the same catch as JSON.parse, so it was
reported as a malformed response (INTERNAL, exit 1) telling the user to
check the endpoint or a captive portal, and the request was never retried.

Read and parse are now separate steps. A body-read failure goes through the
same transport handling as a failed fetch (shared transportRetryDelayOrThrow):
retried for GET and Idempotency-Key writes within the transport budget,
otherwise UNAVAILABLE (exit 10) naming the dropped connection. A body read
in full that is not JSON still raises the malformed-response error with no
retry; PAYLOAD_TOO_LARGE, timeouts, aborts and interrupts are unchanged.

Closes TestSprite#431
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration

Commits

Reviewing files that changed from the base of the PR and between a3294b7 and 28c4e72.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The HTTP client now handles response-body read failures as transport failures. It applies the existing retry policy to eligible requests and preserves malformed JSON as a separate error. Tests cover retry behavior, retry exhaustion, and debug events.

Changes

HTTP transport retry flow

Layer / File(s) Summary
Transport failure handling and regression tests
src/lib/http.ts, src/lib/http.test.ts
A shared helper handles transport diagnostics, retry decisions, retry events, and TransportError creation. The client uses it for fetch failures and response-body read failures. Completed invalid JSON remains a typed malformed-response error. Tests cover GET and idempotent-write retries, non-retry cases, exhaustion after four attempts, malformed JSON, and debug events.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: Minimal · previous review d6017

The response-body retry change is ready to merge after normal checks; no outstanding issue is established.

Pre-merge checks: Passed 5

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: retrying connection resets that occur while reading an HTTP response body as transport failures.
Linked Issues check Passed The PR satisfies the coding requirements in #431. src/lib/http.ts separates readBoundedText() from JSON.parse(). Body-read failures use transportRetryDelayOrThrow, which applies the existing t…
Out of Scope Changes check Passed The changes stay within #431. The production diff only refactors transport-failure handling and separates body reading from parsing. The test diff adds focused regression coverage for response-body co…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Full details: Linked Issues check

Explanation

The PR satisfies the coding requirements in #431. src/lib/http.ts separates readBoundedText() from JSON.parse(). Body-read failures use transportRetryDelayOrThrow, which applies the existing transport budget, debug events, retry transitions, and TransportError. The code preserves non-retry behavior for keyless writes and retry: false. Full-body malformed JSON still uses malformedResponseError without retry. The new tests in src/lib/http.test.ts cover GET recovery, idempotent-write recovery, non-retriable writes, disabled retries, budget exhaustion, malformed JSON, and debug classification. The diff adds no dependency or flag.

Full details: Out of Scope Changes check

Explanation

The changes stay within #431. The production diff only refactors transport-failure handling and separates body reading from parsing. The test diff adds focused regression coverage for response-body connection resets and preserved malformed-response behavior. No unrelated feature, dependency, flag, or endpoint change is present.

Finishing Touches

Fix failing CI checks

  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

ℹ️ Recent review info

⚙️ Run configuration

  • Configuration used: Repository: TestSprite/testsprite-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 54b47999-bc67-4b81-8b8a-0c7b54cef33b
📥 Commits

Reviewing files that changed from the base of the PR and between d61e6df and d601747.

📒 Files selected for processing: 2

  • src/lib/http.test.ts
  • src/lib/http.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

@github-actions

Copy link
Copy Markdown

🏆 Season 4 CLI Improvement Bonus: entry recorded. Thanks, @Davidson3556!

To be counted, this PR needs the following by Oct 29, 11:59 PM PT:

  • ✅ A linked issue assigned to you.
  • ✅ Out of draft.
  • ✅ A Discord username in the description.

Rewards go to merged PRs judged a genuine improvement, and up to three bonus-eligible PRs count per person. Untick the box to withdraw the entry. This comment refreshes when you edit the PR. Full rules.

@github-actions github-actions Bot added the s4-cli-bonus Entered for the Season 4 CLI Improvement Bonus label Oct 22, 2026
@zeshi-du
zeshi-du merged commit d5257dd into TestSprite:main Nov 9, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s4-cli-bonus Entered for the Season 4 CLI Improvement Bonus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Hackathon] bug: response-body connection resets skip safe transport retries

2 participants