fix(providers): retry provably pre-send transport failures (#447)#750
fix(providers): retry provably pre-send transport failures (#447)#750Vasanthdev2004 wants to merge 7 commits into
Conversation
A flaky link that failed the streaming completion POST at connect time stopped the turn and forced a manual continue, because the shared retry layer never retries transport errors: a completion POST is non-idempotent, so a replay could duplicate billable work. That rationale does not apply to a PROVABLY pre-send failure, where no request bytes ever left this host: a replay cannot duplicate anything, exactly like a 429/503 where the server told us it did not accept the request. SendWithRetry now retries, with the existing backoff, only that narrow class: DNS resolution failure, a refused or unreachable dial, and a TLS handshake timeout. Every ambiguous or post-send failure stays unretried and is checked first so a mixed message can never be misread: connection reset, broken pipe, EOF, a bare i/o timeout, context deadline. All three providers route their completion POST through this layer, so one change covers them uniformly. This deliberately narrows the issue's ask (which listed connection reset) to the provably-safe subset, and keeps the reset-stays-unretried test green. Tests cover the retry-on-pre-send, no-retry-on-ambiguous, and predicate-classification paths. NOTE FOR REVIEW: this refines a deliberately conservative, documented no-retry stance. Opening it for the owner's sign-off on the stance, not as a settled call.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Walkthrough
ChangesTransport retry safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SendWithRetry
participant HTTPClient
participant preSendBackoff
SendWithRetry->>HTTPClient: client.Do(request)
HTTPClient-->>SendWithRetry: pre-send transport error
SendWithRetry->>preSendBackoff: apply bounded pre-send delay
preSendBackoff-->>SendWithRetry: retry delay
SendWithRetry->>HTTPClient: client.Do(request) again
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 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 `@internal/providers/providerio/retry.go`:
- Around line 159-175: Update isPreSendTransportError to recognize
Windows-specific connection-refused and network-unreachable dial error wording
or errno text while preserving the existing exclusion precedence and POSIX
matches. Add a Windows-formatted error case to the tests in retry_test.go that
verifies the error is classified as pre-send and retried.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b065d57b-09dd-4fd0-99da-fb5f33795841
📒 Files selected for processing (2)
internal/providers/providerio/retry.gointernal/providers/providerio/retry_test.go
CodeRabbit flagged that the pre-send predicate matched only POSIX dial wording, so a refused or unreachable dial on Windows (whose text differs entirely: 'No connection could be made because the target machine actively refused it') would not be retried. Match the syscall errno via errors.Is instead: Go maps ECONNREFUSED / ENETUNREACH / EHOSTUNREACH / ECONNRESET to the host's real values, including the WSA codes on Windows, so classification is identical on every platform. Errno matching is also authoritative where the string markers were only heuristic, closing the theoretical substring gap. The string markers stay as a fallback for errors already flattened to text. ECONNRESET is added to the post-send exclusions by identity so a Windows reset is excluded too. Tests build the production error shape (net.OpError -> os.SyscallError -> errno) so the Windows path is exercised portably on every platform, plus an end-to-end SendWithRetry case for an errno-wrapped refused dial. Cross -compiled and vetted for linux and darwin.
|
Good catch, fixed in 4c54a8b. Windows dial wording is completely different ("No connection could be made because the target machine actively refused it"), so the POSIX string markers would have missed it. Rather than add Windows strings, the predicate now classifies by syscall errno via errors.Is: ECONNREFUSED / ENETUNREACH / EHOSTUNREACH for the pre-send inclusions and ECONNRESET for the post-send exclusion. Go maps those constants to the host's real values (the WSA codes on Windows), so the classification is identical on every platform, and it's authoritative where the strings were only heuristic. The string markers stay as a fallback for errors that arrive already flattened to text with no errno in the chain. Tests build the real production error shape (net.OpError wrapping os.SyscallError wrapping the errno), which is the portable way to exercise the Windows path: the same errno assertion holds on Linux/macOS/Windows even though .Error() reads differently on each. Added errno cases to the predicate table (refused, network-unreachable, host-unreachable, and reset-is-post-send) plus an end-to-end SendWithRetry case for an errno-wrapped refused dial. Cross-compiled and vetted for linux and darwin so the platform-specific constants are confirmed to build everywhere. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE. The safety-critical property is airtight: isPreSendTransportError evaluates the post-send/ambiguous exclusions (EOF, ECONNRESET, broken pipe, i/o timeout, DeadlineExceeded) BEFORE any inclusion, so a genuinely post-send failure can never be classified pre-send and never replays a billable POST (verified: exactly 1 call on ambiguous errors). Request body is rebuilt via bytes.NewReader per attempt; context cancellation is checked first; ECONNREFUSED is connect-only; retries are bounded; no bad interaction with the auth-retry path.
Findings (none blocking):
- [Minor] A connect/dial
i/o timeoutis NOT retried — Go surfaces it as*net.OpError{Op:"dial"}whose text containsi/o timeout, caught by the exclusion at retry.go:169. That misses part of #447's own motivating case (intermittent connectivity → connect timeout). Safe fix: before the string exclusion, admit*net.OpErrorwhenOp=="dial"/"connect" && Timeout()— unambiguously pre-send, without loosening the post-send read-timeout exclusion. - [Nit] Backoff has no jitter (issue mentioned it) — a non-issue for a single-client CLI; optional.
- [Nit]
ENETUNREACH/EHOSTUNREACHerrno admits (retry.go:176) aren't scoped toOp=="dial". Residual risk is very low and mitigated (established-connection failures surface as read reset/EOF/timeout, excluded first), but scoping to dial/connect would make the no-bytes-sent guarantee structural rather than argued.
Merge is kevin's call per the program gate.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Request changes
The safety reasoning here is genuinely careful, but the central claim doesn't hold: isPreSendTransportError matches the pre-send errnos anywhere in the error chain regardless of which socket operation raised them, so a network-unreachable error on a read/write after the request was fully sent is classified as "provably pre-send" and the non-idempotent completion POST is replayed.
Build/vet/tests are clean — see the bottom of this review.
[Major] Pre-send errno classification is not scoped to the dial, so a post-send failure replays a billable POST
internal/providers/providerio/retry.go:177-181
if errors.Is(err, syscall.ECONNREFUSED) ||
errors.Is(err, syscall.ENETUNREACH) ||
errors.Is(err, syscall.EHOSTUNREACH) {
return true
}errors.Is matches the errno wherever it appears in the chain — it says nothing about which operation produced it. These errnos are not dial-only: the kernel reports them on an established connection too, when a route goes away or an ICMP unreachable arrives.
Concrete scenario: a user is mid-generation against api.anthropic.com, the request has been fully written, the model is producing a billable completion, and Wi-Fi drops or a VPN toggles. Linux/macOS surface that on the pending read as *url.Error -> *net.OpError{Op:"read"} -> *os.SyscallError -> EHOSTUNREACH. Verified in this worktree:
&net.OpError{Op: "read", Err: os.NewSyscallError("read", syscall.EHOSTUNREACH)}→isPreSendTransportErrorreturns true- same for
Op: "write"withENETUNREACH - same via the string fallback at
retry.go:185for already-flattened text likewrite tcp: write: network is unreachable
From there SendWithRetry (retry.go:98) replays the POST up to 5 more times — all providers pass maxAttempts=0 → defaultMaxRetryAttempts=6 (anthropic/provider.go:196, openai/provider.go:196, gemini/provider.go:151, openai/codex_responses.go:354) — producing up to 6 duplicate billable completions for one user turn.
Worth flagging that the existing table case gives false assurance:
{"errno reset is post-send", wrapDialErrno("read", syscall.ECONNRESET), false}, // retry_test.go:133It passes because ECONNRESET is in the exclusion list at retry.go:165, not because the operation is inspected. No case exercises a non-dial op carrying a pre-send-classified errno.
Fix: gate on the dial phase before the errno checks — unwrap to *net.OpError and require opErr.Op == "dial" (Go sets this for connect failures) around retry.go:177-181, and apply the same gate to the "connection refused" / "network is unreachable" / "no route to host" string fallbacks at retry.go:183-186. Then add table cases for wrapDialErrno("write", syscall.ENETUNREACH) and wrapDialErrno("read", syscall.EHOSTUNREACH) expecting false.
[Major] Permanent failures now hang the agent ~60s silently, on a 429-tuned backoff
internal/providers/providerio/retry.go:98, schedule at retry.go:44-48
Deterministic, never-going-to-succeed failures go through the full retry loop on a schedule explicitly tuned for rate-limit windows:
- NXDOMAIN —
net.DNSErrorreturnstrueunconditionally atretry.go:160, beforeIsNotFoundis ever consulted - A refused dial to a local endpoint
Verified in this worktree with maxAttempts=0: the transport is invoked exactly 6 times, and backoffWait for attempts 1..5 sums to 2+4+8+16+30 = 60s (schedule pinned by TestBackoffWaitSchedule, retry_test.go:218).
Scenario: user points the openai provider at http://127.0.0.1:11434 for a local Ollama/LM-Studio daemon that isn't running, or mistypes a base URL. Every attempt fails with ECONNREFUSED / no such host. SendWithRetry emits no notifier callback, and the agent-layer reconnectNoticeFor never fires for these providers (StreamCompletion returns nil synchronously and delivers the error as a StreamEventError), so the TUI just sits for a full minute per turn before reporting something that was known on the first millisecond. Previously this surfaced instantly.
Fix: two parts.
- Don't retry
net.DNSErrorwhenIsNotFoundis true — NXDOMAIN is authoritative and deterministic. OnlyIsTimeout/IsTemporarylookups are worth replaying. (retry.go:159-162) - Give the pre-send transport path its own bound and schedule instead of reusing the rate-limit one — cap around 2-3 attempts starting from a sub-second base. The mid-stream reconnect layer already uses 500ms (
internal/agent/reconnect.go:34) and is a reasonable model.
[Minor] SendWithRetry's own doc comment now states the opposite of what it does
internal/providers/providerio/retry.go:53-54
The package note at retry.go:24-36 was correctly updated for the new behavior, but the function doc immediately below it wasn't:
Other 5xx and transport/network errors are returned immediately, never replayed (see the package note).
That contradicts retry.go:98. This is the doc that shows on hover/godoc at every call site in anthropic, openai, gemini and codex_responses — a maintainer reads it, concludes no transport error is ever replayed, and reasons about idempotency on exactly the assumption this PR invalidates. In a codebase where these comments are the primary record of the retry-safety contract, that's a live hazard rather than a cosmetic nit.
Fix: update retry.go:50-55 to mention the pre-send exception and point at isPreSendTransportError, matching the package note.
What's well done
Genuinely above average for this class of change, and worth saying:
- Ambiguous/post-send errors are checked before the inclusion set (
retry.go:163-173), so an error matching both is never treated as pre-send. That ordering is the right call and the "exclusion wins over inclusion" test case covers it directly. - EOF and ECONNRESET matched by identity via
errors.Israther than by wording (retry.go:165) — a hostname containing "eof" can't fool it. ctx.Err()checked before any retry decision (retry.go:87), so cancellation never becomes a retry.- Deliberately leaving http2 GOAWAY/stream errors,
net/http: timeout awaiting response headers, andcontext deadline exceededunmatched — all correct omissions. - Typed
net.DNSError/errno matching preferred over strings, with strings only as a fallback for already-flattened errors: the right hierarchy, and the Windows WSA* reasoning in the comment is sound. TestSendWithRetryDoesNotReplayAmbiguousTransportErrors(retry_test.go:99) is a real guard — it asserts exactly one call, so a regression that started replaying resets or broken pipes would fail it.shrinkBackoffcorrectly scoped witht.Cleanup.
The dial-scoping fix should be small; the structure it slots into is already right.
Build & tests
All clean on the PR head (go1.26.5, darwin/arm64), no failures of any kind:
gofmt -l ./internal/providers/providerio/— cleango build ./...— exit 0go vet ./internal/providers/...— exit 0, no diagnosticsgo test -count=1 ./internal/providers/providerio/...— ok 0.854s, all subtests passgo test -count=1 -race ./internal/providers/...— exit 0 across all 5 packages, no race reports
No merge-base comparison was needed since nothing failed. The sandbox-related pre-existing failures in internal/agent and internal/cli are out of scope — this PR touches neither package.
Scope note: the stated merge base fbf8598 is not actually this branch's merge base (git merge-base fbf8598 HEAD = ce4a996). Diffing fbf8598..HEAD shows a misleading 44-file / -2225-line stat because it renders main's later commits as deletions. The real PR content vs ce4a996 is 2 files, +207/-13 — retry.go and retry_test.go — which matches the supplied diff.
Merge is kevin's call per the program gate.
…on permanent errors Addresses gnanam's two [Major] review findings on this PR. [Major] Pre-send errno classification was not scoped to the dial, so a post-send read/write carrying ECONNREFUSED/ENETUNREACH/EHOSTUNREACH (a route dropping mid-generation) was misclassified "provably pre-send" and could replay a non-idempotent completion POST up to 6 times. errors.Is matches an errno anywhere in the chain regardless of which operation raised it, so isPreSendTransportError now requires an Op=="dial" *net.OpError before admitting those errnos or their POSIX wording. That makes "no request bytes were sent" structural: a post-send failure can no longer replay a POST. The dial errno match still covers Windows (Go maps the WSA* codes), so the platform coverage this PR added is preserved. [Major] Permanent failures hung the agent ~60s on the 429-tuned schedule. NXDOMAIN (net.DNSError with IsNotFound) is now treated as permanent and not retried, and the pre-send path gets its own short bound and sub-second schedule (preSendMaxAttempts=3, 500ms/1s) instead of reusing the 2/4/8/16/30s rate-limit backoff, so a mistyped host or a dead local daemon fails in about 1.5s. [Minor] Fixed the stale SendWithRetry doc that still claimed transport errors are never replayed. Tests: post-send read/write errnos and NXDOMAIN now assert exactly 1 call (no replay); dial errnos plus the string fallback assert the pre-send cap; added the pre-send backoff schedule. Race detector and all provider packages green.
|
Both [Major] findings fixed in [Major] dial-scoping. You nailed it: [Major] fast-fail. NXDOMAIN ( [Minor] doc. Fixed the One deliberate call worth flagging: the dial-scoping means the plain-string test fixtures (which carry no @coderabbitai your Windows point is covered by the errno path: a Windows dial refusal arrives as I left the earlier [Minor] (admitting a dial |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
…on Windows too
CodeRabbit caught a real Windows bug, which I verified with a real dial on
Windows: errors.Is(err, syscall.ECONNREFUSED) returns FALSE for a refused dial
there, because a real Windows dial carries the raw WSAECONNREFUSED (10061) while
Go's syscall.ECONNREFUSED is a distinct value the net package never produces. The
POSIX string fallback ("connection refused") does not match the Windows wording
("No connection could be made because the target machine actively refused it")
either. So the pre-send dial classification silently never fired on Windows, the
exact platform this PR set out to fix.
Move the dial errnos into a platform-specific list (dialPreSendErrnos): POSIX
everywhere, plus the WSA* codes on Windows. The existing wrapDialErrno fixtures
wrap the Go syscall constants, which is why they passed on both platforms and
missed this. Adds TestIsPreSendTransportErrorRealRefusedDial, which does a REAL
refused dial and asserts pre-send classification, so the platform's true error
shape is exercised on every OS.
|
Good catch @coderabbitai, and verified on Windows rather than assumed: a real refused dial there gives Fixed in @gnanam1990 heads-up: your earlier CHANGES_REQUESTED was on the pre-fix commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- current retry-related files ---'
fd -a 'retry.*\.go$|.*errno.*\.go$' internal/providers/providerio 2>/dev/null || true
printf '%s\n' '--- structure ---'
ast-grep outline internal/providers/providerio/retry.go --items all 2>/dev/null || true
ast-grep outline internal/providers/providerio/retry_test.go --items all 2>/dev/null || true
printf '%s\n' '--- relevant implementation and tests ---'
rg -n -C 12 \
'dialPreSendErrnos|isPreSendTransportError|dialOpError|TestIsPreSendTransportErrorRealRefusedDial|ECONNREFUSED|WSAECONNREFUSED|net\.Dial|DialContext' \
internal/providers/providerio
printf '%s\n' '--- build-constraint declarations ---'
rg -n -C 8 '^(//go:build|// \+build)|dialPreSendErrnos|syscall\.' internal/providers/providerio -g '*.go'
printf '%s\n' '--- latest relevant changes ---'
git diff --find-renames 3af605d^ 3af605d -- internal/providers/providerio || trueLength of output: 56989 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- module metadata ---'
sed -n '1,180p' go.mod
printf '%s\n' '--- CI and release platform references ---'
rg -n -C 4 \
'GOOS|GOARCH|windows|darwin|linux|plan9|wasip1|js/wasm|cross-compil' \
.github README.md go.mod .goreleaser.yml 2>/dev/null || true
printf '%s\n' '--- loopback ephemeral-port tests and their behavior ---'
rg -n -C 8 \
'net\.Listen\("tcp", "(127\.0\.0\.1|localhost):0"\)|Addr\(\)\.String\(\).*Close|expected a refused dial|port accepted a connection' \
-g '*.go' .Length of output: 17555
One minor follow-up: the comment above ✏️ Learnings added
|
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 `@internal/providers/providerio/retry_dialerrno.go`:
- Around line 1-14: Update the build constraint for the retry dial errno
implementation to exclude Plan 9 as well as Windows, while preserving the
existing POSIX errno definitions in dialPreSendErrnos.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66103c7b-04fd-4760-8ffa-0d92440c8ac8
📒 Files selected for processing (4)
internal/providers/providerio/retry.gointernal/providers/providerio/retry_dialerrno.gointernal/providers/providerio/retry_dialerrno_windows.gointernal/providers/providerio/retry_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/providers/providerio/retry.go
- internal/providers/providerio/retry_test.go
|
Addressed in Plan 9 build tag (Major): applied your suggested retry.go Windows wording (Minor): this is already handled by the previous commit ( |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE. Re-reviewed at 7467a3bc. My earlier CHANGES_REQUESTED is fully addressed — I re-ran my exact attack against the new code rather than reading the diff.
The concern I raised, and how it's fixed
I flagged that isPreSendTransportError matched ENETUNREACH/EHOSTUNREACH/ECONNREFUSED anywhere in the error chain regardless of which socket op raised them, so a post-send read/write failure (route drop / ICMP-unreachable mid-generation) was misclassified as "provably pre-send" and would replay the non-idempotent completion POST — double billing / double tool execution, the exact harm the PR exists to prevent.
The fix is the right shape: the errno match is now gated on dialOpError(err) != nil (retry.go:220), which requires an Op=="dial" *net.OpError (retry.go:248-254). That makes "no request bytes left this host" structural — a post-send read/write carrying the same errno can't match. The string-marker fallback (connection refused / network is unreachable / no route to host) is correctly inside the same dial gate, and TLS-handshake-timeout stays outside it (genuinely pre-send by wording, not a dial OpError).
Verified with the precise scenario:
dial ENETUNREACH -> preSend=true (safe retry preserved)
dial EHOSTUNREACH -> preSend=true
dial ECONNREFUSED -> preSend=true
read ENETUNREACH -> preSend=false (was the bug — now NOT retried)
write EHOSTUNREACH -> preSend=false (no billable POST replay)
read ECONNRESET -> preSend=false
Nice additions
- Regression coverage for exactly this:
retry_test.go:102-103—"host unreachable on read is post-send"and"net unreachable on write is post-send"— with a comment stating scoping toOp=="dial"must keep these from replaying the POST. That pins the fix so it can't silently regress. - The Windows errno split is correct and non-obvious.
retry_dialerrno_windows.goadds the raw Winsock codes (WSAECONNREFUSED= 10061, etc.) becauseerrors.Isagainst the POSIXsyscall.ECONNREFUSEDreturns false on Windows — the net package never produces the POSIX value there. Without that list a refused dial would silently never retry on Windows. Good catch, and the comment explains why. - DNS handling is sound:
NXDOMAIN(IsNotFound) is not retried (deterministic — retrying just stalls the agent through the whole backoff), while a timeout/SERVFAIL is.
Verification
gofmt and go vet ./internal/providers/providerio/ clean; the full providerio suite passes. CodeRabbit has also approved this head.
Solid iteration — the dial-scoping is the correct structural fix, not a string-match patch, and it ships with a regression test for the exact post-send case. Merge is kevin's call per the program gate.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Request changes: this regresses Plan 9 compilation. retry_dialerrno.go excludes plan9, but retry.go still references syscall.ECONNRESET and dialPreSendErrnos, for which there is no Plan 9 definition; retry_test also directly uses the unavailable syscall types/constants. passes at this PR's merge base but fails on this head. Please provide a compatible Plan 9 fallback or gate the new code and tests consistently.
|
Verification detail for my change request: running GOOS=plan9 GOARCH=amd64 go test -c ./internal/providers/providerio succeeds at this PR's merge base and fails on this head. |
retry.go referenced syscall.ECONNRESET and dialPreSendErrnos directly, but the errno definitions live in build-tagged files that exclude Plan 9 (which has no BSD-style errno constants), so the package stopped compiling for GOOS=plan9 on this branch even though it builds on the merge base. Move the two syscall touch-points behind per-platform helpers: retry.go now calls isConnResetErrno and ranges dialPreSendErrnos, both provided by retry_dialerrno.go, retry_dialerrno_windows.go, and a new retry_dialerrno_plan9.go. The Plan 9 variant supplies an empty errno list and a false isConnResetErrno, so retry.go falls back to its existing string-marker heuristics there. Behavior on linux/darwin/windows is unchanged (isConnResetErrno matches the same syscall.ECONNRESET as before). retry_test.go is tagged !plan9 because it constructs real syscall.Errno dial shapes. Verified: GOOS=plan9 build and vet now pass; windows and linux build, vet, gofmt, and the full providerio test suite stay green.
|
Good catch, and you were right: I checked, and main builds clean for GOOS=plan9 while this branch did not, so it was a real regression I introduced (my earlier take that the package never built on Plan 9 was wrong). Fixed by moving the two syscall touch-points out of retry.go behind per-platform helpers. retry.go no longer references syscall or dialPreSendErrnos directly; it calls isConnResetErrno and ranges dialPreSendErrnos, both supplied by retry_dialerrno.go / retry_dialerrno_windows.go and a new retry_dialerrno_plan9.go (empty errno list + isConnResetErrno returning false, so Plan 9 falls back to the existing string-marker heuristics). retry_test.go is tagged !plan9 since it builds real syscall.Errno dial shapes. Verified GOOS=plan9 build and vet now pass, and windows/linux build, vet, gofmt, and the full providerio suite stay green with behavior unchanged on the real targets. Re-requested your review. |
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 `@internal/providers/providerio/retry_dialerrno_plan9.go`:
- Around line 5-10: Update the package comment in retry_dialerrno_plan9.go to
accurately describe retry.go’s string-marker fallback: mention only
connection-refused, network-unreachable, and no-route-to-host dial markers, and
explicitly state that connection-reset errors remain non-retryable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04d4997d-0ef8-4483-ba44-82e7d0b12ed8
📒 Files selected for processing (5)
internal/providers/providerio/retry.gointernal/providers/providerio/retry_dialerrno.gointernal/providers/providerio/retry_dialerrno_plan9.gointernal/providers/providerio/retry_dialerrno_windows.gointernal/providers/providerio/retry_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/providers/providerio/retry_dialerrno_windows.go
- internal/providers/providerio/retry.go
- internal/providers/providerio/retry_test.go
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not classify a redirect-hop failure as pre-send
internal/providers/providerio/retry.go:123
client.Docan follow a redirect after it has already transmitted the original non-idempotent POST. If DNS, dialing, or the TLS handshake then fails while reaching the redirect target, the resulting error satisfiesisPreSendTransportErrorand this loop replays the original request. A 307/308 provider redirect followed by a transient target-host failure can therefore run and bill the first completion, then submit it again. Disable redirects for this request path, or preserve enough redirect-hop state to retry only a failure on the initial request; add a redirect-then-transport-failure regression test. -
[P2] Keep the pre-send retry budget separate from status retries
internal/providers/providerio/retry.go:123
The pre-send condition uses the loop-wideattemptcounter, which already includes 429/503 retries. After two safe status retries, the first refused-dial/DNS/TLS failure arrives as attempt 3 and is returned without any of the advertised pre-send retries; after one it gets only one retry. This contradicts the independent short pre-send budget and makes mixed rate-limit/network blips fail prematurely. Track pre-send failures (and their backoff ordinal) separately and cover a status-to-transport sequence. -
[P3] Correct the platform fallback/fixture documentation
internal/providers/providerio/retry_dialerrno_plan9.go:5
internal/providers/providerio/retry_test.go:19
The Plan 9 comment says reset classification is unavailable and lists reset as a fallback inclusion, althoughretry.gostill excludes the platform-independent"connection reset"marker. Separately, the test fixture says Windowssyscall.E*constants exercise the real WSA dial path, while the new Windows implementation correctly says real dials carry distinctwindows.WSA*errors; only the real-dial test covers that branch. Correct both descriptions so future maintenance does not weaken the cross-platform retry safety coverage.
Addresses jatmn's review on #750. [P1] client.Do could follow a 307/308 after already transmitting the original POST, and a dial failure to the redirect target then satisfied isPreSendTransportError and replayed the request, re-billing a completion the first host already received. SendWithRetry now disables redirect following (CheckRedirect returns http.ErrUseLastResponse on a cloned client), so any dial/DNS/TLS error from Do can only come from the single initial request and the pre-send classification is sound. A 3xx surfaces as the response for the caller's existing non-2xx handling. Regression test asserts the POST is sent once and the 307 is not followed. [P2] The pre-send retry used the loop-wide attempt counter shared with 429/503 retries, so a couple of status retries exhausted the pre-send budget and the first dial/DNS/TLS blip after them got no retry. Track pre-send failures and their backoff ordinal in a separate counter. Regression test covers a status-then-transport-failure sequence. [P3] Correct the Plan 9 and wrapDialErrno comments: the Plan 9 string-marker fallback keeps the pre-send inclusions and the separate reset exclusion, and the errno fixtures do not reproduce a real Windows dial (that carries distinct WSA errnos), which the real-dial test covers.
|
All three fixed in the latest push, thanks jatmn, genuinely good catches. [P1] You were right that a redirect hop breaks the pre-send guarantee. SendWithRetry now disables redirect following (CheckRedirect returns http.ErrUseLastResponse on a cloned client), so any dial/DNS/TLS error from Do can only come from the single initial request, and a 3xx surfaces as the response for the existing non-2xx handling. Added a regression test asserting the POST is sent once and the 307 is not followed. [P2] The pre-send retries now use their own counter and backoff ordinal, independent of the loop-wide attempt counter, so 429/503 retries no longer eat the pre-send budget. Added a status-then-transport-failure test that would have caught it. [P3] Corrected both comments (the Plan 9 string-marker fallback and the wrapDialErrno fixture note about the real Windows WSA path). Verified: gofmt, vet, and build clean on linux/windows/plan9, and the full providerio suite passes. Re-requested your review. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Do not let pre-send retries consume the later status-retry budget
internal/providers/providerio/retry.go:115
preSendAttemptsgives a dial/DNS/TLS failure its own limit, but each retry still advances the loop-wideattemptused by the 429/503 branch at line 158. For example, withmaxAttempts=3, two safely retried refused dials followed by a 429 return that first 429 immediately; with the default, those failures also shorten the status retry window and start its backoff at a later rung. This is the inverse of the mixed sequence covered by the new test and contradicts the stated independent budgets. Track status attempts/backoff separately and add a pre-send-then-status regression case.
4 participants
Closes #447.
The ask, and the stance it touches
A flaky link that fails the streaming completion POST at connect time (TLS handshake timeout, connection refused, DNS failure) stops the turn and forces the user to type
continue. The reporter asked for auto-retry of transport failures, with the right safety instinct: retry only failures that provably never reached the model.The shared retry layer deliberately does NOT retry transport errors, and the reasoning is sound and documented: a completion POST is non-idempotent, so if the request may have reached the server, a replay could duplicate billable work.
This PR is opening that decision for your sign-off, not treating it as settled. It changes a stance you staked out on purpose, so it should be your call.
Why a narrow carve-out is consistent with that stance
The "may have reached the server" concern does not apply to a provably pre-send failure, where no request bytes ever left this host. There a replay cannot duplicate anything, which is exactly the logic that already makes 429 and 503 safe to retry (the server told us it did not accept the request).
So
SendWithRetrynow retries, with the existing backoff and attempt budget, only that narrow class:net.DNSError, matched by type through any wrapping)connection refused,network is unreachable,no route to host)Everything ambiguous or post-send stays unretried, and those exclusions are checked first so a message carrying both an excluded and an included marker can never be misread as pre-send: connection reset, broken pipe, EOF, a bare
i/o timeout(which also covers post-send read timeouts), context deadline.This deliberately narrows the issue's ask (it listed connection reset, which stays unretried) down to the provably-safe subset. The existing reset-stays-unretried test stays green.
Scope and placement
Every provider send path routes the completion POST through
SendWithAuthRetrythenSendWithRetry, so this one change covers them uniformly, and it fires before the in-band stream error that stops the turn is ever emitted. The 429/503 path, the auth-retry path, and streaming are untouched.A note on the classification
Pre-send detection uses
errors.Asfor the DNS type,errors.Isfor EOF, and narrow string markers for the dial/TLS cases. String markers are the pragmatic choice here (they are stable and portable, unlike platform-specific syscall errnos). The one theoretical way a post-send error could carry an inclusion marker is aurl.Errorwhose URL contains the phrase, which cannot happen: every marker contains a space, and the HTTP client rejects a space in the host and percent-encodes it in the path.Testing
go build,go vet,gofmtclean; fullinternal/providers/...suite passes. New tests cover retry-on-pre-send (refused dial, network unreachable, TLS handshake timeout, DNS), no-retry-on-ambiguous (i/o timeout, broken pipe, EOF), and the predicate classification table including the exclusion-wins and host-named-eof cases.Summary by CodeRabbit