mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 18:00:54 -10:00
Vasanth T
8acd42a3bd
* fix(providers): retry provably pre-send transport failures (#447) 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. * fix(providers): classify pre-send dials by errno so Windows retries too 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. * fix(providers): scope pre-send retry to the dial phase and fail fast 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. * fix(providers): match Windows WSA dial errnos so refused dials retry 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. * fix(providers): exclude Plan 9 from the POSIX dial-errno build tag * fix(providers): keep the pre-send errno classification off Plan 9 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. * fix(providers): stop pre-send retry from replaying a redirected POST 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. * fix(providers): decouple the status retry budget from pre-send retries Addresses jatmn's follow-up on #750. The previous change gave pre-send failures their own counter, but every retry still advanced the loop-wide attempt counter that the 429/503 branch used. So the coupling survived in the opposite direction: two safely replayed refused dials shortened the later status retry window and started its backoff at a later rung. Give the status path its own statusAttempts counter and backoff ordinal. With both budgets tracked separately the shared loop counter has no remaining reader, so the loop no longer carries one. Behaviour is unchanged when only one kind of failure occurs: the first retryable status still waits on rung 1 and still gets maxAttempts requests. Adds TestSendWithRetryStatusBudgetSurvivesPreSendRetries, the inverse of the existing mixed-sequence test: two refused dials followed by rate limiting must still leave the status path its full window. * fix(providers): keep following redirects, but never replay a redirected POST Addresses jatmn's follow-up on #750. Blocking redirects outright was too blunt. SendWithRetry is the shared completion path, so replacing every caller's CheckRedirect with ErrUseLastResponse turned a legitimate 307/308 from a redirecting OpenAI-compatible gateway into a terminal non-2xx that providers report as an upstream error, and it discarded an injected client's redirect policy. Observe the redirect instead of preventing it: a per-attempt flag is set from CheckRedirect, which then delegates to the caller's policy when one is set and otherwise mirrors net/http's default ten-hop cap. The pre-send retry is gated on that flag, so an attempt that entered a redirect is never replayed while redirecting endpoints keep working. The safety property is unchanged, since a dial failure on a redirect hop follows a POST the origin already received. Adds three regressions: a redirect is followed through to the final response, a dial failure after a redirect is not replayed, and a caller-supplied CheckRedirect still runs and still decides. * test(providerio): pin the redirect asymmetry for retryable statuses A reviewer read the redirect guard as incomplete, because it stops a redirected POST being replayed after a transport error but not after a 429/503/529. The asymmetry is intentional, and nothing recorded that, so the next reader would reasonably reach the same conclusion. The two cases differ in what is known about the request. A transport failure after a redirect leaves it unknown whether the redirect target received and began processing the POST, so replaying could duplicate billable work. A retryable status means every hop answered: the 307/308 is the origin declining and delegating, and the 429 is the target declining as well, so nothing accepted the request and a replay duplicates nothing. The exposure is also the same as a retryable status with no redirect involved, which this package has always retried, so guarding here would not close a gap. It would make a rate-limited completion behind a redirecting gateway fail outright rather than back off, which is the regression class already raised against disabling redirects wholesale. Adds the case as a test asserting the intended behaviour, plus a comment at the branch explaining why it is not guarded. * fix(providers): retry connect timeouts and proxy CONNECT failures Three classification gaps against #447, all raised in review. A connect-phase timeout was never retried. It is provably pre-send for the same structural reason a refused dial is, since the connection was never established and no request bytes left this host, but it arrives carrying "i/o timeout" and the blanket wording exclusion rejected it before the connect gate could admit it. Intermittent connect timeouts are exactly the transient failure the issue is about, so the most common case was the one not covered. It is now admitted ahead of the wording exclusions, gated on the connect phase so a post-send read timeout, which is also Timeout(), still cannot replay a POST. A failed CONNECT through an HTTP proxy was never retried either. net tags it Op "proxyconnect" rather than "dial", so it missed the gate entirely. Same guarantee applies: the tunnel to the model host was never built. The gate now accepts both, which matters most in the environment where transient connect failures are most common. ETIMEDOUT and WSAETIMEDOUT join the errno lists so a connect timeout still classifies if it reaches us with the errno but without the timeout flag. The pre-send budget also now resets after a hop that reaches the server. It is meant to bound consecutive connect failures, not the lifetime of the call, and without the reset an early run of blips permanently spent it, so a later blip surfaced as a transport error while the status budget still had room. That is the coupling the two counters were split apart to remove, in the one ordering the earlier tests missed. Total work stays bounded, since every reset costs a status attempt and those are capped. Also corrects the SendWithAuthRetry comment, which still claimed the inner path retried only 429/503/529. Tests cover dial and proxyconnect timeouts as retryable and read/write timeouts as not, a refused proxy CONNECT, and the interleaved pre-send/status/pre-send sequence. Each fix fails its test when reverted.