Make uhttp transport timeouts configurable and add connection retry-with-reconnect#1003
Make uhttp transport timeouts configurable and add connection retry-with-reconnect#1003johnallers wants to merge 1 commit into
Conversation
The uhttp transport hardcoded its connection timeouts (idle-conn, response-header, and the HTTP/2 PING keepalive), tuned for chatty API connectors behind a proxy. Two problems followed: - A transient connection-level failure -- a proxy silently dropping a pooled/tunneled connection, surfacing as "http2: client connection lost" or "connection reset" -- was wrapped as a fatal codes.Unavailable and returned with no retry, which could abort an entire sync on a single blip. - Endpoints that block before sending response headers (e.g. a synchronous report-generation call) hit the fixed 60s ResponseHeaderTimeout with no way to raise it. Changes: - Expose the previously-hardcoded transport timeouts as options: WithIdleConnTimeout, WithResponseHeaderTimeout, WithHTTP2KeepAlive, and WithMaxConnectionRetries. Timeout defaults are unchanged; a nil option leaves the default in place so an explicit 0 can still mean "disabled/unlimited". - Add retryRoundTripper: transparently reconnect-and-replay on reconnectable transport errors (new IsReconnectableError helper). Only idempotent requests (safe methods or an Idempotency-Key) with a rewindable body are retried; slow-peer timeouts are excluded. Uses jittered exponential backoff bounded by the request context. Behavior change: retries default to 3 (previously none); set WithMaxConnectionRetries(0) to opt out. - Plumb the response-header timeout from CLI/config via a new http-response-header-timeout-seconds field; an explicit option takes precedence over the config value. Adds unit tests for error classification, retry/backoff behavior, idempotency and body-rewind guards, context cancellation, and option/config precedence.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Transparently reconnect-and-retry transient connection-level failures so a | ||
| // proxy silently dropping a pooled/tunneled connection doesn't surface as a | ||
| // fatal error to the caller (e.g. aborting a whole sync). | ||
| if maxRetries := intOr(t.maxConnRetries, defaultMaxConnectionRetries); maxRetries > 0 { |
There was a problem hiding this comment.
🟡 Suggestion: defaultMaxConnectionRetries = 3 changes default behavior for every downstream connector (previously zero retries). Two things worth confirming per SDK-compat criteria: (1) unlike the response-header timeout, there's no CLI/config field for max retries, so connectors can only disable it via a code-level WithMaxConnectionRetries(0) — consider a config field for parity; (2) pkg/sdk/version.go is still v0.18.1 and wasn't bumped to signal this default-behavior change. The scoping (idempotent + rewindable + reconnectable-only, context-bounded) looks sound. (confidence: medium)
General PR Review: Make uhttp transport timeouts configurable and add connection retry-with-reconnectBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. The change makes Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
|
Sol 5.6 says: Cancellation during retry backoff returns the prior network error instead of context.Canceled/DeadlineExceeded, misclassifying canceled requests as retryable Unavailable errors. Fix both cancellation paths in pkg/uhttp/transport.go. |
3 participants
Summary
The
uhttptransport hardcodes its connection timeouts — idle-conn (30s), response-header (60s), and the HTTP/2 PING keepalive (15s+15s = 30s). Those defaults were deliberately tuned for chatty API connectors behind an egress proxy, but two gaps fall out of them:http2: client connection lostorconnection reset, whichwrapTransientNetworkErrormaps tocodes.Unavailable. Nothing inuhttpretries it, so the error propagates up and the sync workflow discards its in-progressc1zand restarts. On a long, per-entity (N+1) walk through a flaky proxy, a run may rarely get an uninterrupted window to completion.ResponseHeaderTimeout— which, because the transport useshttp2.ConfigureTransports, applies over HTTP/2 too — with no knob to raise it.What changed
Configurable timeouts (
pkg/uhttp): new optionsWithIdleConnTimeout,WithResponseHeaderTimeout,WithHTTP2KeepAlive, andWithMaxConnectionRetries. The tuning fields become nil-able pointers: aniloption preserves the existing default (via the newdefault*constants), so an explicit0can still mean "disabled/unlimited" per net/http semantics. Timeout defaults are unchanged.Retry-with-reconnect (
pkg/uhttp): a newretryRoundTrippersits below the error-wrapping layer and transparently replays a request on reconnectable transport errors (newIsReconnectableErrorhelper —http2 client connection lost,ECONNRESET,EPIPE,net.ErrClosed, unexpected/EOF; timeouts/DeadlineExceededare deliberately excluded). When the HTTP/2 health check tears down a deadClientConnit is evicted from the pool, so the retriedRoundTripdials fresh — retry here also reconnects. Only idempotent requests (safe methods, or a request carrying anIdempotency-Key) with a rewindable body are retried; jittered exponential backoff is bounded by the request context.Config plumbing (
pkg/field,pkg/cli): a newhttp-response-header-timeout-secondsfield (default 60,0disables) is injected into the run context and consumed by the transport. An explicitWithResponseHeaderTimeoutoption takes precedence over the config value, mirroring the existinghttp-timeout-secondsbehavior.WithMaxConnectionRetriesdefaults to 3 — previously there was no retry. This is the intended fix, and it's scoped tightly (idempotent + rewindable + reconnectable-only, context-bounded). To opt out, passWithMaxConnectionRetries(0). Happy to flip the default to opt-in (0) if reviewers prefer.Note
The retry sits at the transport layer, below
BaseHttpClient.Do's rate limiter, so a retried attempt does not take a fresh rate-limit token. For rare connection-death retries this is negligible; called out for visibility.Testing
go test ./pkg/uhttp/... ./pkg/field/...andgolangci-lintpass. New tests cover error classification, retry-until-success, stop-at-max, non-reconnectable/non-idempotent/non-rewindable guards, body rewind on retry, context cancellation, and option-vs-config precedence.🤖 Generated with Claude Code