Skip to content

retry: retryTransport leaks response body on 429 retry and returns a closed body on final failure #31

Description

@timimsms

Context

The retry transport in internal/api/retry.go mishandles response bodies in two ways and only partially parses Retry-After, causing connection leaks and a potential read-after-close for callers.

Evidence

All in internal/api/retry.go (retryTransport.RoundTrip):

  1. Body leak on 429 retry — when a 429 has a parsable Retry-After, the code sleeps and continues before reaching the resp.Body.Close() below, so the previous response body is never closed:
if resp != nil && resp.StatusCode == 429 {
	if retryAfter := resp.Header.Get("Retry-After"); retryAfter != "" {
		if seconds, parseErr := time.ParseDuration(retryAfter + "s"); parseErr == nil {
			time.Sleep(seconds)
			continue // <- skips the Body.Close() below
		}
	}
}
// Close response body if exists
if resp != nil && resp.Body != nil {
	_ = resp.Body.Close()
}
  1. Closed body returned on final failure — after the last attempt, the loop has already closed resp.Body, yet return resp, err hands that response to the caller, who will read from a closed body.

  2. Retry-After HTTP-date form unhandled — only the delta-seconds form is parsed; RFC 9110 also allows an HTTP-date, which currently falls through to generic backoff.

Suggested fix

  • Close the previous body before every retry path (including the 429 continue).
  • On the final attempt, return the response without closing its body (or drain/close and return a synthesized error).
  • Parse Retry-After as either delta-seconds or HTTP-date (http.ParseTime).
  • Add unit tests with a stub RoundTripper covering 429-with-Retry-After, exhausted retries, and body lifecycle.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions