fix(go): return the response when the deadline is shorter than the retry backoff - #17521
Open
adidavid014 wants to merge 1 commit into
Open
fix(go): return the response when the deadline is shorter than the retry backoff#17521adidavid014 wants to merge 1 commit into
adidavid014 wants to merge 1 commit into
Conversation
…try backoff When a retryable response arrived and the request context's deadline would elapse before the computed backoff finished, the retrier slept anyway and the caller received `context deadline exceeded` — discarding the response that explained the failure. With `Retry-After` honored up to `maxRetryDelay` (60s), a single 429 could turn into an opaque timeout. The retrier now compares the remaining deadline against the delay and returns the response instead of sleeping, so the caller sees the actual 429 (with its headers and body). An explicit `cancel()` during the backoff still returns the context's error, which is the case `sleepWithContext` exists for. `TestRetryWaitIsInterruptedByContext` previously asserted `context.DeadlineExceeded` for a short-deadline case that this change now resolves to a 429, so it is rewritten to cover cancellation (its actual subject) and two tests are added for the deadline-vs-backoff comparison. Reported by name.com as the optional follow-up to #17498. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
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
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.
Description
Follow-up to #17498, requested by name.com as the optional item in their report.
When a retryable response arrived and the request context's deadline would elapse before the computed backoff finished, the retrier slept anyway and the caller received
context deadline exceeded— discarding the response that explained the failure. WithRetry-Afterhonored up tomaxRetryDelay(60s), a single 429 became an opaque timeout.The retrier now compares the remaining deadline against the delay and returns the response instead of sleeping:
The
defer response.Body.Close()moves below the early return — otherwise the caller gets a closed body.Behavior change worth a close look
This is user-visible, not a pure bugfix. Code doing
errors.Is(err, context.DeadlineExceeded)on a rate-limited call now gets a*core.APIErrorwithStatusCode: 429instead. That is what was asked for and it is strictly more informative, but it is a change in what callers observe.It also flips a test added in #17498.
TestRetryWaitIsInterruptedByContextusedWithTimeout(100ms)againstRetry-After: 5and assertedcontext.DeadlineExceeded— exactly the case this change now resolves to a 429. I treated the two behaviors as complementary rather than competing:cancel()during the backoff → returnctx.Err()(unchanged; still reachable when there is no deadline, which is the Ctrl-C casesleepWithContextexists for)So that test is rewritten to use
context.WithCancel, which is what it was actually about.One deliberate omission: the comparison is a bare
time.Until(deadline) < delaywith no margin for the request itself. If the remaining time only slightly exceeds the delay, we still sleep and the next attempt likely dies on the deadline. I matched the reporter's suggestion exactly rather than pick a margin size — happy to add one if reviewers prefer.Changes
generators/go-v2/base/src/asIs/internal/retrier.go_: deadline-vs-backoff comparison;Body.Close()reordered.generators/go-v2/base/src/asIs/internal/retrier_test.go_:TestRetryWaitIsInterruptedByContextrewritten to cover cancellation; addedTestRetryReturnsResponseWhenDeadlineShorterThanBackoffandTestRetryProceedsWhenDeadlineLongerThanBackoff.seed/go-sdk/**/internal/retrier{,_test}.go(366 files).Testing
Built the generator from this branch and generated name.com's real SDK from their spec and
generators.yml. Their original repro now returns429: {"message":"slow down"}in ~3ms, where 1.57.9 returnscontext deadline exceededafter 2s.go build ./...andgo vet ./...clean.exhaustive/no-custom-config,imdb/no-custom-config,idempotency-headersbuild and pass.Note on the snapshots: I patched them mechanically rather than running seed, since these are
asIsfiles whose only per-fixture variation is the module path (verified: all 183shouldRetrybodies identical, all 366 filesgofmt-clean). I validated the shortcut by diffing a patched snapshot against genuine generator output — byte-identical modulo the module path. Worth re-runningpnpm seedif you would rather not rely on that.Generated with Claude Code