fix: release the response body on non-2xx HTTPProvider responses

[LautaroPetaccio]

Problem

HTTPProvider.sendAsync discarded the fetch Response on a non-2xx status without reading or cancelling its body:

if (!$.ok) {
  callback(new Error('External error. response code: ' + $.status))   // body never consumed
} else {
  const json = await $.json()   // 2xx: body consumed
  ...
}

With native fetch (undici) and browser fetch, the underlying socket stays checked out until the response body is consumed or cancelled. So whenever an RPC endpoint returned a non-2xx (e.g. 429/5xx under load), the connection leaked. node-fetch's default non-keep-alive agent masked this, so it only surfaces once a consumer passes a native-fetch-backed FetchFunction.

Fix

Release the body before invoking the callback — cancel the stream when the implementation supports it (web streams), otherwise drain it via text() (e.g. node-fetch). The 2xx path is unchanged (it already drains via json()).

Tests

Added test/HTTPProvider.spec.ts covering: non-2xx with a cancellable body → cancel() called; non-2xx without cancel → drained via text(); 2xx → consumed via json(), not cancelled, result returned.

Verification

• tsc typecheck: clean
• make build: succeeds, API report unchanged (no public-API change)
• new unit suite: 6/6 passing
• lint: clean

🤖 Generated with Claude Code

[github-actions]

Test this pull request

• The package can be tested by running

  yarn upgrade "https://sdk-team-cdn.decentraland.org/"

[decentraland-bot]

Code Review — PR #582

fix: release the response body on non-2xx HTTPProvider responses

Verdict: ✅ Approve

This is a solid, well-scoped fix for a real connection/socket leak. The feature-detection approach (cancel() for web streams, text() drain for node-fetch) is correct across all target runtimes, the .catch(() => undefined) guards are appropriate for best-effort cleanup, and the callback contract is preserved. CI passes, API report is unchanged (no public surface change), and the new test suite covers the main branches well.

No P0 or P1 issues found. Minor suggestions below.

---

P2 — Missing negative-path test coverage

test/HTTPProvider.spec.ts

The tests cover the happy cleanup paths but don't verify behavior when the cleanup itself fails:

• cancel() rejecting (e.g. stream already errored)
• text() rejecting (e.g. connection reset mid-drain)
• $.body being null/undefined (some polyfills)

The production code handles all of these correctly (optional chaining + .catch()), but adding tests would lock in those guarantees against regressions.

P2 — Error body is discarded

src/providers/HTTPProvider.ts ~L79-83

The error response body (which may contain useful diagnostics — rate-limit details, server error messages) is cancelled/drained but never surfaced. An alternative approach would drain via text() and include the body in the error message:

const detail = await $.text().catch(() => '')
callback(new Error(`External error. response code: ${$.status}${detail ? ' — ' + detail : ''}`))

This would fix the leak and improve debuggability. However, this changes the error message format, so it's arguably a separate enhancement — not blocking.

P2 — Simpler single-path alternative

src/providers/HTTPProvider.ts ~L79-83

Calling $.text().catch(() => undefined) alone would work universally (drains the body on every fetch implementation). The cancel()-first path is a valid optimization (avoids buffering the error body into a string), but error responses are typically tiny, so the real-world gain is marginal. The current two-branch approach is defensible — just noting the simpler alternative exists.

P2 — Pre-existing: debug log serializes full response object

src/providers/HTTPProvider.ts L85 (not introduced by this PR)

JSON.stringify($) on the response could serialize reflected headers. Consider logging only safe fields (status, statusText, url) in a future cleanup.

Git conventions ✅

• Title follows fix: <summary> format
• Branch follows fix/<summary> pattern

Security ✅

No secrets, no injection vectors, no auth issues. The $.text() fallback theoretically allows draining an unbounded body from a malicious endpoint, but the host is set by the library consumer at construction time, making this low risk.

Consumer impact ✅

No public API change (confirmed by API report). Internal implementation only — safe for all downstream consumers (catalyst, builder, js-sdk-toolchain, worlds-content-server, etc.).

---

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio via Slack