Skip to content

fix(generator): a request dropped by the HTTP/2 transport is not retried unless its error has a recognized shape - #129

Merged
giraffesyo merged 2 commits into
canaryfrom
fix/network-retry-exclusion
Sep 15, 2026
Merged

giraffesyo merged 2 commits into
canaryfrom
fix/network-retry-exclusion

Conversation

@giraffesyo

@giraffesyo giraffesyo commented Sep 15, 2026

Copy link
Copy Markdown
Member

What does this PR do?

isRetryableNetworkError in the generated retry.go now retries any request that got no response unless the failure is known to be permanent: a canceled context, a certificate the client does not trust, a server that does not speak TLS, an unsupported URL scheme, or a redirect loop. It no longer enumerates transient shapes (EOF sentinels, net.Error timeouts, net.OpError).

An expired deadline is deliberately not excluded. Since Go 1.23 a per-attempt http.Client.Timeout satisfies errors.Is(err, context.DeadlineExceeded), and that attempt is exactly the one worth repeating. When the caller's own context has expired, the retry loop's wait returns on ctx.Done() before sleeping.

The new e2e test drives the generated client over a hand-rolled HTTP/2 connection that resets the first stream with INTERNAL_ERROR and answers the retry, checks that a client timeout is retried for every attempt, and pins the untrusted-certificate and unsupported-scheme cases to a single attempt. The reset and timeout tests each fail against the previous template.

Why is this PR needed?

net/http serves HTTP/2 from an internal package and reports a dropped connection through unexported types: a reset stream, "client connection lost", or "cannot retry err after Request.Body was written" when its own retry gives up. None of those is a net.OpError or a timeout, so the generated client returned them to the caller on the first attempt even with retries enabled, while a plain TCP reset on HTTP/1 was retried. x/net v0.59.0 deprecated the last exported handle on these errors with no replacement, which settles that inspecting them is not the supported path.

Excluding the permanent failures is the pattern go-retryablehttp uses. It is stable because the exclusions are the client's own misconfiguration rather than transport wording. Two of them still have to match text ("unsupported protocol scheme" and "stopped after N redirects") because net/http gives those errors no exported type, and both strings have been unchanged since Go 1.0.

The idempotency guard is unchanged: the typed client still retries only idempotent methods, and the streaming path only when the caller opted in with a replayable body.

A consumer with a hand-written test asserting that an opaque RoundTripper error is not retried will need to update it to a permanent error such as context.Canceled; core's sdk/go/raw_test.go has one.

Steps to Reproduce

go test ./internal/generator/ -run TestE2E_NetworkRetry -count=1
go test ./... -count=1
golangci-lint run ./...

@giraffesyo
giraffesyo merged commit 8f1e70a into canary Sep 15, 2026
7 checks passed
@giraffesyo
giraffesyo deleted the fix/network-retry-exclusion branch September 15, 2026 20:18
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