fix: Set explicit upstream dial timeout and retry failed dials - #20
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughVersion 0.12.0 adds configurable upstream dial timeouts and retry attempts. A shared client applies these settings in proxy and record modes. Proxy requests retry dial-timeout failures. Documentation describes the new behavior. ChangesUpstream dialing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProxyRequest
participant NewClient
participant fasthttpTCPDialer
participant Upstream
ProxyRequest->>NewClient: Create client with configured dial timeout
ProxyRequest->>fasthttpTCPDialer: Send upstream request
fasthttpTCPDialer->>Upstream: Establish TCP connection
Upstream-->>fasthttpTCPDialer: Return dial result
fasthttpTCPDialer-->>ProxyRequest: Return response or ErrDialTimeout
ProxyRequest->>fasthttpTCPDialer: Retry timeout failures up to configured attempts
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/common/common_test.go`:
- Around line 24-26: Make the environment-dependent subtests deterministic by
always calling t.Setenv with tt.env, including when it is empty. Update both
PROXY_DIAL_TIMEOUT at internal/common/common_test.go lines 24-26 and
PROXY_DIAL_ATTEMPTS at lines 47-49; remove the conditional guards while
preserving each test’s existing assertions.
In `@internal/proxy/proxy.go`:
- Around line 23-28: Update NewClient in internal/proxy/proxy.go (lines 23-28)
to create a client-owned fasthttp.TCPDialer with DisableDNSResolution enabled
and invoke its DialTimeout method from the callback, preserving
fasthttp.ErrDialTimeout. In internal/common/common.go (lines 45-50), preserve
the ProxyDialAttempts contract while ensuring retries use the fresh-resolution
dial path; make no unrelated changes.
In `@README.md`:
- Line 74: Document PROXY_DIAL_ATTEMPTS as a maximum total TCP dial-attempt
limit, clarifying that the initial dial is included and a value of 3 means one
initial attempt plus two retries. Update README.md lines 74-74 and 84-84 to use
total-attempt wording, and update CHANGELOG.md line 10-10 to state that the
value controls total dial attempts rather than re-dials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e93cb89f-5f6d-4d11-b01e-fdf9e87cfe98
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdVERSIONinternal/common/common.gointernal/common/common_test.gointernal/proxy/proxy.gointernal/pureproxy/pureproxy.gointernal/record/record.go
Tubt
added a commit
that referenced
this pull request
Aug 3, 2026
Addresses review feedback on #20. Use a client-owned fasthttp.TCPDialer instead of the package-level fasthttp.DialTimeout, which delegates to a process-global dialer. Owning it scopes the DNS cache and the concurrency limiter to this client rather than sharing them with anything else in the process that dials via fasthttp. Concurrency is set to 1000 to match fasthttp's own default dialer so that owning it changes nothing else. Correct an inaccurate claim in the docs: attempts were described as re-resolving the address. They do not. fasthttp caches resolved addresses for DNSCacheDuration (1 minute by default). What actually happens is that getTCPAddrs advances a round-robin index over the cached set once per dial, and dial() abandons the walk as soon as one address returns ErrDialTimeout rather than trying the remainder — so consecutive attempts do start from consecutive addresses, but over the cached set, not a freshly resolved one. Fixed in the ProxyDialAttempts doc comment and in README. Also make the default-case subtests independent of the caller's environment. They previously skipped t.Setenv when the fixture value was empty, so an inherited PROXY_DIAL_TIMEOUT or PROXY_DIAL_ATTEMPTS decided the result: with PROXY_DIAL_TIMEOUT=5s the unset case failed with "ProxyDialTimeout() = 5s, want 30s". Setting the variable unconditionally (empty included, which is what the getters treat as unset) isolates them.
Tubt
added a commit
that referenced
this pull request
Aug 3, 2026
…ests Addresses review feedback on #20. The loop counts the initial dial, so PROXY_DIAL_ATTEMPTS=3 performs three dials — one plus two retries — not four. README, CHANGELOG and the ProxyDialAttempts doc comment all described it as a re-dial count, which reads as one more than it is. Reworded as a total-attempt budget. Add internal/proxy/proxy_test.go covering the contract with a counting dialer: - the budget is total, verified at 3 (default), 1 (retrying disabled) and 4 (explicit) - the count holds for GET as well as POST. fasthttp's HostClient retries idempotent methods internally up to DefaultMaxIdemponentCallAttempts, and this pins that it does not stack on top of our loop for a dial failure and multiply the dials - a non-dial-timeout error is not retried and is not misclassified as a dial timeout, which is what keeps the retry safe for non-idempotent methods This closes the gap called out in the PR description, where the retry loop itself was untested.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/proxy/proxy_test.go (1)
15-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise GET failures after connection establishment.
Use a
net.Connwhose write or read fails afterDialsucceeds. A dial error returnsretry=falsebefore connection acquisition, but write and read errors returnretry=true, so GET can retry internally up toDefaultMaxIdemponentCallAttempts. Assert one dial for this non-dial failure so it cannot bypassPROXY_DIAL_ATTEMPTS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/proxy_test.go` around lines 15 - 18, Extend the proxy test around the existing varied-method retry setup to use a net.Conn that succeeds during Dial but fails on write or read, then issue a GET and assert the dial count remains one. Cover the post-connection failure path so retry=true does not trigger fasthttp's internal GET retries beyond PROXY_DIAL_ATTEMPTS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/proxy/proxy_test.go`:
- Around line 15-18: Extend the proxy test around the existing varied-method
retry setup to use a net.Conn that succeeds during Dial but fails on write or
read, then issue a GET and assert the dial count remains one. Cover the
post-connection failure path so retry=true does not trigger fasthttp's internal
GET retries beyond PROXY_DIAL_ATTEMPTS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c347aa1-4480-4069-8ca8-813f08a81f64
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdinternal/common/common.gointernal/common/common_test.gointernal/proxy/proxy.gointernal/proxy/proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/proxy/proxy.go
- internal/common/common.go
- internal/common/common_test.go
- README.md
- CHANGELOG.md
Record and proxy modes built their upstream client as &fasthttp.Client{}
with no Dial set, so fasthttp fell back to its DefaultDialTimeout of 3s
and never retried. That is shorter than a rolling load-balancer or
ingress update: while the balancer's targets drain, SYNs go unanswered,
the dial times out, and the caller gets a 502 even though the upstream is
healthy. The 502 body is fasthttp's ErrDialTimeout wrapped by
ErrDialWithUpstream, surfaced through goodmock's proxy-error handler.
Add two settings, both applying to record and proxy modes:
- PROXY_DIAL_TIMEOUT sets the TCP connect timeout, default 30s.
- PROXY_DIAL_ATTEMPTS caps the total number of dial attempts, initial
dial included, default 3. Setting it to 1 disables retrying.
Retrying is scoped to fasthttp.ErrDialTimeout and nothing else. That
error means the TCP connection was never established, so the upstream
cannot have seen or applied the request, which makes the retry safe for
non-idempotent methods too. Once bytes are on the wire nothing is
retried, because there is no way to know whether the upstream acted on
them. errors.Is reaches the sentinel through ErrDialWithUpstream, which
implements Unwrap.
The dialer is owned by the client rather than using the package-level
fasthttp.DialTimeout, which delegates to a process-global TCPDialer.
Owning it scopes the DNS cache and the concurrency limiter to this client
instead of sharing them with anything else in the process that dials via
fasthttp. Concurrency is set to 1000 to match fasthttp's own default so
that taking ownership changes nothing else.
Read and write timeouts stay unset. Upstream responses here can
legitimately take tens of seconds, and capping them would convert
slow-but-successful calls into proxy errors.
Note on multiple upstream addresses: attempts tend to land on different
ones because getTCPAddrs advances a round-robin index once per dial and
dial() abandons the walk as soon as an address times out rather than
trying the rest. This rotates over fasthttp's cached set; names are only
re-resolved once DNSCacheDuration (1 minute) has elapsed.
Tests cover the configuration getters and the retry contract with a
counting dialer: the attempt budget is total, verified at 3, 1 and 4; the
count holds for GET as well as POST, which guards against fasthttp's own
idempotent-retry loop stacking on top and multiplying the dials; and a
non-dial-timeout error is neither retried nor misclassified as a dial
timeout. The default-case getter tests set their variables
unconditionally so an inherited environment cannot decide the result.
Tubt
force-pushed
the
fix/upstream-dial-timeout-and-retry
branch
from
August 3, 2026 09:02
64adc29 to
cf5ef87
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 86: Update the README retry behavior description to state that retries
occur only when the dial error matches fasthttp.ErrDialTimeout; clarify that
other connection-establishment failures, including connection refused, are not
retried, while preserving the existing safety explanation for non-idempotent
requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1e25a6b-248b-46a7-9f55-dac86014e060
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdVERSIONinternal/common/common.gointernal/common/common_test.gointernal/proxy/proxy.gointernal/proxy/proxy_test.gointernal/pureproxy/pureproxy.gointernal/record/record.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/record/record.go
- internal/pureproxy/pureproxy.go
- VERSION
- internal/proxy/proxy.go
- internal/common/common_test.go
- internal/proxy/proxy_test.go
- internal/common/common.go
- CHANGELOG.md
martinnaj
reviewed
Aug 5, 2026
martinnaj
reviewed
Aug 5, 2026
martinnaj
reviewed
Aug 5, 2026
martinnaj
left a comment
martinnaj
left a comment
Member
There was a problem hiding this comment.
Solid, well-scoped change overall — gating the retry strictly on ErrDialTimeout is the right call, and the test pinning GET vs POST dial counts (so fasthttp's internal idempotent retries can't stack on top of the loop) is a genuinely good catch. A few notes, the first one is the only one I'd insist on before merge:
- Env config is read on every request instead of once — see the inline comment on
ProxyRequest. This also means an invalidPROXY_DIAL_ATTEMPTSkills the process on the first proxied request instead of at startup. - The PR description's "Not covered by tests: the retry loop itself" is now inaccurate —
proxy_test.gocovers the loop directly via the injectedDial(attempt budgets, the disable case, the non-dial-error early exit). What's genuinely uncovered is only the real-network accept-then-blackhole behaviour. Worth updating so reviewers know where to look hard. - CHANGELOG rewrites history — blank-line removal and Added/Changed/Fixed reordering in already-released entries (0.2.0, 0.4.0, 0.5.0) is churn unrelated to this change and pollutes blame on a file where blame is the point. I'd revert everything except the new 0.12.0 entry and its link.
- Nit:
NewClientliving ininternal/proxywhile being consumed byrecordandpureproxyis slightly odd layering — defensible sinceProxyRequestlives there too, not blocking. - Nit: no backoff between attempts is actually correct for the LB-drain scenario (immediate retry rotates to the next address), but a sentence in the code comment saying it's deliberate would stop the next reader from "fixing" it.
| // the request, which makes a retry safe for every method including POST. | ||
| // Any other failure is returned as-is: once bytes are on the wire we have no | ||
| // way to know whether the upstream acted on them. | ||
| attempts := common.ProxyDialAttempts() |
Member
There was a problem hiding this comment.
common.ProxyDialAttempts() re-reads and re-parses the env var on every proxied request. Two problems:
- Validation happens at first request, not startup.
ProxyDialTimeout()is called inNewClient(), so a bad value kills the process at boot — good. But an invalidPROXY_DIAL_ATTEMPTSpasses startup, the pod goes Ready, and thenlog.Fatalftakes the whole proxy down when the first request arrives. - Inconsistent snapshot semantics — the timeout is fixed at client construction while attempts are re-read per request.
Suggest resolving both once at construction (capture attempts in NewClient's scope and pass it into ProxyRequest, or validate eagerly at startup). I suspect the per-request read was chosen to make t.Setenv testing easy — but the tests can just as well construct the client after setting the env.
Contributor
Author
There was a problem hiding this comment.
Good catch on both — fixed in d46a3ed, taking the first of your two suggestions.
NewClient now resolves both settings and returns a proxy.Client that carries the budget with the client:
type Client struct {
HTTP *fasthttp.Client
Attempts int
}
func NewClient() *Client {
timeout := common.ProxyDialTimeout()
attempts := common.ProxyDialAttempts()
...
}ProxyRequest takes *proxy.Client and reads client.Attempts, so an invalid PROXY_DIAL_ATTEMPTS is now fatal at boot alongside PROXY_DIAL_TIMEOUT, and the two settings are on one snapshot instead of one being frozen in the dialer closure while the other is re-read.
You were right about the motivation, too — the per-request read was there for t.Setenv. The retry-loop tests now set the budget on the Client directly, which is a better test anyway (they were asserting the retry loop, not the env parsing). TestNewClientAttempts covers the env wiring they no longer touch, and common_test.go still covers the parsing and validation.
RecordServer / ProxyServer each keep a single *proxy.Client field, so there is no second field to drift.
Tubt
added a commit
that referenced
this pull request
Aug 10, 2026
Address review feedback on #20. common.ProxyDialAttempts() was re-read on every proxied request. It calls log.Fatalf on an invalid value, so a bad PROXY_DIAL_ATTEMPTS passed startup, the pod went Ready, and the proxy then died on its first request. It also left the two dial settings on different snapshots: the timeout was captured in NewClient's dialer closure while the attempts were re-read per request. Both are now resolved once in NewClient, which returns a proxy.Client bundling the fasthttp client with its attempt budget. The retry-loop tests set the budget on the Client directly instead of through t.Setenv; TestNewClientAttempts covers the environment wiring they no longer exercise. Docs: state the retry is scoped to a dial timeout (connection refused is not retried), and spell out the compound attempts x timeout worst case with its two consequences for callers. Revert the unrelated CHANGELOG reformatting - blank lines after old version headings and section reordering within 0.5.0 and 0.2.0.
Tubt
enabled auto-merge
August 11, 2026 04:29
Address review feedback on #20. common.ProxyDialAttempts() was re-read on every proxied request. It calls log.Fatalf on an invalid value, so a bad PROXY_DIAL_ATTEMPTS passed startup, the pod went Ready, and the proxy then died on its first request. It also left the two dial settings on different snapshots: the timeout was captured in NewClient's dialer closure while the attempts were re-read per request. Both are now resolved once in NewClient, which returns a proxy.Client bundling the fasthttp client with its attempt budget. The retry-loop tests set the budget on the Client directly instead of through t.Setenv; TestNewClientAttempts covers the environment wiring they no longer exercise. Docs: state the retry is scoped to a dial timeout (connection refused is not retried), and spell out the compound attempts x timeout worst case with its two consequences for callers. Revert the unrelated CHANGELOG reformatting - blank lines after old version headings and section reordering within 0.5.0 and 0.2.0.
Tubt
force-pushed
the
fix/upstream-dial-timeout-and-retry
branch
from
August 12, 2026 02:22
d46a3ed to
d8bd1ad
Compare
martinnaj
approved these changes
Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Record and proxy modes built their upstream HTTP client as
&fasthttp.Client{}with noDialset, so fasthttp fell back to its ownDefaultDialTimeout— 3 seconds — and never retried a failed dial.Three seconds is shorter than a rolling load-balancer or ingress update. While the balancer's targets are draining, SYNs go unanswered, the dial times out, and the caller receives a 502 even though the upstream is perfectly healthy.
The chain, all in existing code:
internal/pureproxy/pureproxy.go:28,internal/record/record.go:54fasthttp@v1.69.0/tcpdialer.go:411—DefaultDialTimeout = 3 * time.Secondtcpdialer.go:376—ErrDialTimeout = errors.New("dialing to the given TCP address timed out")ErrDialWithUpstream.Error()→"error when dialing %s: %s"internal/pureproxy/pureproxy.go:62,internal/record/record.go:94Observed impact
In one GoodData
gdc-uinightly e2e run (30769815868, 2026-08-02) this produced 44 × 502/503 inside a single 2-minute window — durations min 3002 ms, max 3704 ms, avg 3119 ms, i.e. the 3s dial timeout firing repeatedly:{"error": "proxy error: error when dialing 3.229.73.213:443: dialing to the given TCP address timed out"}The three addresses appearing in those errors were the three availability-zone nodes of the same upstream load balancer, which was mid-rollout. Because GoodMock synthesises the 502 itself, the request never reaches the upstream — so there is no server-side trace, and the failures are effectively undiagnosable from backend logs.
Change
PROXY_DIAL_TIMEOUT— TCP connect timeout, Go duration, default30s(record + proxy modes)PROXY_DIAL_ATTEMPTS— how many times a request is re-dialed when the connection cannot be established, default3,1disablesfasthttp.TCPDialerwithConcurrency: 1000, matching fasthttp's own default) rather than the process-global one behindfasthttp.DialTimeout, so the DNS cache and concurrency limiter are scoped to this clientWhy retrying is safe here
Retrying is scoped to
errors.Is(err, fasthttp.ErrDialTimeout)and nothing else.That error means the TCP connection was never established, so the upstream cannot have seen — let alone applied — the request. That makes a retry safe for non-idempotent methods including
POST. Once any bytes are on the wire, nothing is retried, because there is no way to know whether the upstream acted on them.errors.Isreaches throughErrDialWithUpstream, which implementsUnwrap().What happens across multiple upstream addresses
When the upstream resolves to several addresses, consecutive attempts tend to start from different ones.
getTCPAddrsadvances a round-robin index (idx := atomic.AddUint32(&e.addrsIdx, 1)) once per dial, anddial()abandons the walk as soon as one address returnsErrDialTimeoutinstead of trying the remainder:This rotates over fasthttp's cached address set — names are only re-resolved after
DNSCacheDuration(1 minute). An earlier revision of this description claimed each attempt re-resolves the address; that was wrong and has been corrected here and in the code comments.Deliberately unchanged
Read and write timeouts stay unset. Upstream responses in this proxy's use case can legitimately take tens of seconds, and capping them would convert slow-but-successful calls into proxy errors.
Compatibility
The dial-timeout default changes from an implicit 3s to an explicit 30s. Anything relying on failing fast after 3s can restore the old behaviour with
PROXY_DIAL_TIMEOUT=3s PROXY_DIAL_ATTEMPTS=1. Recorded/replayed mapping formats are untouched. Minor version bump,0.11.0→0.12.0.Verification
The default-case tests were also checked against a hostile environment —
PROXY_DIAL_TIMEOUT=5s PROXY_DIAL_ATTEMPTS=5 go test ./internal/common/passes.Not covered by tests: the retry loop itself, which would need a listener that accepts then blackholes SYNs to exercise honestly. The env-var parsing and defaults are tested; the invalid-value paths call
log.Fatalfand would need a subprocess harness the repo does not currently have.Summary by CodeRabbit
New Features
PROXY_DIAL_TIMEOUT, defaulting to 30 seconds.PROXY_DIAL_ATTEMPTS, defaulting to three attempts.Bug Fixes
Documentation