SPEC §6: decide which statuses honour Retry-After, and how each loop composes it - #793
Conversation
There was a problem hiding this comment.
Pull request overview
Defines SPEC boundaries for peer-derived error rendering and Retry-After handling.
Changes:
- Requires closed-vocabulary rendering for observer-facing peer text.
- Applies
Retry-Afterto every otherwise-retryable status. - Documents current SDK divergences and host limits.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be3eccd5bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (6)
SPEC.md:646
- This universal rule conflicts with §14 at lines 1539 and 1558, which still says the authenticated DownloadURL hop honors
Retry-Afteronly on 429 even though it retries 502/503/504 too. Please update those normative statements to cover everyDOWNLOAD_RETRY_ONstatus, or declare a deliberate carve-out here; otherwise implementations have two incompatible contracts.
**A parsed `Retry-After` is honoured at any status a retry is already going to happen at.** There is
no status gate of its own. §7's three gates decide whether *this* response is retried, and where they
say yes the parsed value replaces the backoff term — at 429, at 503, and at every status a declared
`retryOn` set carries today or grows to carry later. Where they say no, the value may still be parsed
and surfaced on the error for the caller to read, but nothing sleeps on it.
SPEC.md:1206
- This scope conflicts with the existing statusless malformed-2xx rule at lines 577–585: that rule requires embedding the malformed peer value in an observer-facing
BasecampErrormessage and merely truncating it. Such a body is outside the API error-body scope defined here, so the SPEC still carries both rendering models. Please convert that error to a closed-vocabulary rendering or explicitly define why it remains in the truncation scope.
**Scope, and what this cap is not.** It governs §6's Error Body Parsing Algorithm `message` and the field-keyed composition built on it — modelled fields of the API's own error body, surfaced because reading them *is* the caller's contract. There it is a resource bound sitting under `MAX_ERROR_BODY_BYTES`, and it answers *how much* text reaches the caller. It never answers *whether* text the peer chose reaches the caller at all. Where that second question is the one being asked, the next section governs and this cap does not stand in for it.
That is the whole boundary between the two sections, and it is not "which peer": this cap applies where a contract requires the text to reach the caller, and the next section applies where nothing does — a decoder's rendering of bad bytes, a transport library's rendering of a URL, a close reason no caller asked to see.
SPEC.md:654
- RFC 9110 §10.2.3 also gives explicit semantics for 3xx responses: the value is the minimum wait before issuing the redirected request. Qualify 503 as the canonical case among statuses retried by this SDK rather than calling it the only explicit case.
RFC 9110 §10.2.3 defines `Retry-After` as a general response header field restricted to no status
set, and gives **503** the one case with explicit semantics — how long the service expects to be
unavailable. RFC 6585 §4 says a **429** *MAY* carry it. So 429 is the permitted use and 503 is the
canonical one, which makes a 429-only rule narrowest exactly where the RFCs are most specific.
Deriving the answer from retry eligibility rather than from a status list is also the only position
that needs no amendment when an operation's `retryOn` grows: a status worth retrying is a status
whose `Retry-After` was worth reading.
SPEC.md:713
- TypeScript and Swift do not parse the header on every status in their retry paths: both invoke
parseRetryAfteronly inside the 429 condition (retry.ts:188–190,services/base.ts:294–296, andHTTPClient.swift:549). State only the behavior established across all four—that honoring is gated on 429.
- **Ruby**, **Kotlin**, **Swift** and **TypeScript** parse the header on any status but gate the
*sleep* on 429 (`ruby/lib/basecamp/http.rb`, `kotlin/.../http/BasecampHttpClient.kt`,
`swift/.../HTTP/HTTPClient.swift`, `typescript/src/retry.ts` and `typescript/src/services/base.ts`).
A 503 carrying `Retry-After: 120` backs off ~1s instead. 503 is in every operation's declared
`retryOn`, so this is a live difference, not a theoretical one.
SPEC.md:704
- The following Go paragraph explicitly says Go is a third, mixed shape, not an SDK that gates on 429 alone. Describe the five SDKs as diverging from the new contract instead; the current wording contradicts the inventory immediately below it.
`[CONFLICT: the spec prescribes any retryable status; five SDKs gate on 429 alone. Converging is a
behaviour change across five SDKs, tracked in #775 — the divergence below is the current state, not
the contract.]`
SPEC.md:669
- Use “an” before the vowel sound in “honoured.”
in its place, **a honoured `Retry-After` delay MUST be awaited through the platform's cancellation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6081fe05eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A 64-bit build parses `Retry-After: 9223372036854775807` cleanly, and `time.Duration(n) * time.Second` then wraps to -1s. `time.After` on a non-positive duration fires at once, so the loop spends its whole attempt budget back to back against a server that just asked it to wait — the newly honoured header turned into a tight retry loop. Reported independently by two reviewers on #796. Both doors onto Error.RetryAfter now normalize: parseRetryAfter, which feeds checkResponse, downloadURL and RequestResult, and ErrRateLimit, which is exported and takes a bare int, so it can also carry a negative value the field's own doc calls invalid. Over-range saturates rather than falling back to "absent". Falling back would compute the millisecond backoff curve and hammer the peer, which is the same tight loop by another route; saturating waits as long as the host can express, and the wait is a select on ctx.Done() so it stays abandonable. Same split the device-flow parser draws: a digit string too long to be an int is malformed and falls back, a value that parses but exceeds what we can honour is clamped. This is a representability bound, not the policy cap #793 declined — at ~292 years it rejects nothing a server could sensibly ask for. SPEC §7 already carved out exactly this for Swift's UInt64 trap; Go joins it. The generated client's own loop (client.gen.go, from go/templates/ client.tmpl) has the identical unclamped conversion and is untouched here.
Suppressed Copilot comments (6) — triagedFour duplicated threads that were filed separately and are answered there: The two that were only raised here:
Worth noting the banner is the thing most likely to be read alone, which is why a contradiction there costs more than the same error in the body.
Gates re-run after all of it: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
SPEC.md:1299
- As written, this is a repository-wide contract, but the PR's remaining-work section and #788 cover only the event-feed connector. Existing primary transports already violate both parts: Go's
ErrNetworkrenderscause.Error()and unwraps it (go/pkg/basecamp/errors.go:197-205), Kotlin interpolates and retains the transport cause (BasecampHttpClient.kt:165-169), and Python interpolates and chains the httpx error (_http.py:335-339). This therefore creates untracked cross-SDK convergence work. Either scope the rule to §23 or mark and track the core/OAuth transport paths that must also change.
**Where no contract requires the text to reach the caller, peer-derived text in an observer-facing error is rendered from a closed vocabulary keyed on the error's type. It is never composed from peer input and then bounded by length.**
SPEC.md:706
- This Python async description is incorrect. Both
_http.pyand_async_http.pycall the samefloat(server_retry_after)conversion in_calculate_delaybefore reachingtime.sleep/asyncio.sleep; an integer beyond the float range raisesOverflowErroron both paths. Please record that both Python clients fail before sleeping rather than claiming the async client waits forever.
This issue also appears on line 1299 of the same file.
`[CONFLICT: Ruby and Python have no such width, and that is a defect rather than a third position.
Both parse arbitrary-precision integers and hand them straight to the sleep, where Ruby raises
RangeError out of the retry loop and Python's sync client raises OverflowError — so a response that
was merely retryable becomes an unrelated exception the caller never asked to handle. Python's async
client neither raises nor sleeps usefully; it waits effectively forever. Both owe the first tier
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab72882855
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A 64-bit build parses `Retry-After: 9223372036854775807` cleanly, and `time.Duration(n) * time.Second` then wraps to -1s. `time.After` on a non-positive duration fires at once, so the loop spends its whole attempt budget back to back against a server that just asked it to wait — the newly honoured header turned into a tight retry loop. Reported independently by two reviewers on #796. Both doors onto Error.RetryAfter now normalize: parseRetryAfter, which feeds checkResponse, downloadURL and RequestResult, and ErrRateLimit, which is exported and takes a bare int, so it can also carry a negative value the field's own doc calls invalid. Over-range saturates rather than falling back to "absent". Falling back would compute the millisecond backoff curve and hammer the peer, which is the same tight loop by another route; saturating waits as long as the host can express, and the wait is a select on ctx.Done() so it stays abandonable. Same split the device-flow parser draws: a digit string too long to be an int is malformed and falls back, a value that parses but exceeds what we can honour is clamped. This is a representability bound, not the policy cap #793 declined — at ~292 years it rejects nothing a server could sensibly ask for. SPEC §7 already carved out exactly this for Swift's UInt64 trap; Go joins it. The generated client's own loop (client.gen.go, from go/templates/ client.tmpl) has the identical unclamped conversion and is untouched here.
#793 rewrites that same sentence to point at §6 rather than restate the rule, and a real content conflict exists between the two branches. Its version is the one to keep, so this drops the hunk entirely rather than handing the merge a choice. What the sentence was carrying — Go's ceiling, why 2147483647 is the portable one, and the generated loop's unclamped copy (#798) — moves into §6 beside the parsing rules it belongs with, so nothing is lost when the two land in either order.
§6 step 1 said an over-range delta-seconds "saturates", which #793's rewrite of §7 note 4 contradicts: it permits both a host-limit bound and refusing a value the parser's own numeric type cannot hold, forbidding only a policy cap. Two SDKs do each. A bug fix on Go's raw path is not where a six-SDK rule gets decided, so §6 now states the classification that is not in dispute — over-range is not malformed, RFC 9110 sets no upper bound — records which SDKs do which, and leaves the convergence to #799. Go still saturates: it is one of the two permitted readings, and it removes a cliff between values one digit apart.
|
Covering the two suppressed Copilot comments from the latest review, both of which land. SPEC.md:1299 — repository-wide rule, connector-only tracking. Correct, and the three call sites it names are all real: Worth noting this is the same defect as Codex's cause-chain thread from the other side: Go's SPEC.md:706 — Python async also raises. Partly right, and the correction improved the paragraph. Both addressed in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edff845fec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
SPEC.md:1373
- This repository-wide convergence inventory omits live peer-bearing paths. Ruby's signed-download path interpolates the transport message and retains it as
cause(ruby/lib/basecamp/client.rb:649), while both Ruby primary paths expose raw causes throughBasecamp::Error#cause; Python's analogous signed-download sites atdownload.py:128,140are also absent from its row. #788's tracking comment repeats the same omission, so these violations would remain untracked despite the paragraph's stated purpose.
**The scope is the repository, and the primary transports are inside it.** #788 is written against the event-feed connector because that is where the four rounds happened, but nothing in the sentence that binds is connector-specific, and three primary-transport constructors are in scope today and non-conformant:
SPEC.md:718
- The async Python path is not fully conformant on representability:
_async_http.py:379-380performs the samefloat(server_retry_after)conversion as sync, andfloat(10**400)raisesOverflowErrorbeforeasyncio.sleepis reached. Values abovetime.sleep's ceiling but within the float range remain cancellable, but larger arbitrary-precision integers still need saturation at the float host limit.
representable as a double by construction. Python's **async** client is not a defect on this axis:
`asyncio.sleep` schedules a float delay without raising, so above `time.sleep`'s ceiling it simply
waits a long time — through `await`, so the task stays cancellable and the escape this section
requires is intact. That is precisely what "no policy cap" means, so it is conformant. Tracked in
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
SPEC.md:760
- Narrow this MUST to loops where
Retry-Aftercan be honoured. As written, “each delay loop” also includes the explicitly out-of-scoperepair-poll, pending poll, and deadline timers listed above, none of which has or needs a composition row; the document therefore violates its own new completeness rule and would classify unrelated future timers as under-specified.
So the obligation runs the other way. **Each delay loop states its own composition in its own section,
and a delay loop added to this document later MUST state its own rather than inherit one from here.**
There is deliberately no default to fall back on: a loop that says nothing is under-specified, not
governed by §7's answer. The rows that exist today:
SPEC.md:669
- The claim that no loop declares 401 retryable conflicts with §23: an unauthorized mint (401/403) below the threshold goes to
Backoff(line 2833), and §23 explicitly says those connection-level authorization failures “retry” (lines 2887–2890). Under the new definition, that makes the fresh-mint cycle a retry, but the §23 seam classifiesunauthorizedwithoutretry_afterand the inventory below includes only transient/throttled outcomes. Either define this authorization-recovery cycle as outside the boundary (as §4 is) or include it and preserve/honourRetry-After; otherwise the status rule is internally inconsistent.
This issue also appears on line 757 of the same file.
Both halves carry weight, and the second is not decoration. §4's 401 refresh-and-retry re-issues after
the origin declined to serve — but 401 is on §7's explicit never-retry list, so no loop declares it
retryable, and §4 is outside. §7's `retry_on`, §14's hop-1 `{429, 502, 503, 504}`, §23's
transient/throttled error kinds and §16's `429`-plus-`too_many_requests` pair are all declared sets, so
all four are inside.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7b554e609
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the mapping algorithm §16's device poll accepts delta-seconds only while §6's parser accepts the HTTP-date form too, so one 429 carried two prescribed waits. Declared as an exception in the composition table rather than re-pointed at the §6 parser: that loop measures every wait on an injectable monotonic clock against a deadline fixed at issuance, and a date can only be resolved against wall-clock now(), which it deliberately never reads. The fallback is the server's own cadence, not a local backoff, and the wait rule clamps to the code lifetime regardless, so the cost is bounded by the loop's shape. §16's block already pinned the shape; the five SDKs implementing it were read and all agree. §23's row 4 read as if Retry-After floored the backoff after an unauthorized mint, while §6 said no loop declares 401 retryable. The seam's `unauthorized` kind carries no retry_after at all - only throttled(retry_after) does - so the header cannot reach that branch. Row 4 now attaches the floor to transient/throttled only, §6 walks the authorization-recovery cycle back as out on the same clause as §4, and the state-machine bullet says which sense of "retry" it means. The preservation sentence promised the unclamped value through the public error, but the mapping algorithm populates retry_after in its 429 arm only. Narrowed to integrity rather than presence; growing the field at 502/503/504 is #775's status convergence, and is the same change as the status gate for the SDKs whose loop reads the delay off the error. Also: the per-loop composition MUST now binds only loops the definition puts inside, so repair-poll and the deadline timers are not made under-specified by it; Kotlin's width observation distinguishes its delta-seconds rejection from its date-form saturation (Pagination.kt:184 vs :196-210, the parser- output carve-out); and the two places that said Go's saturation shipped are marked [PENDING #796], since that PR merges after this one and the tree does not yet contain it.
"Cancellation must win over the wait" was the comment on a select that cannot promise it: when both cases are ready Go picks pseudo-randomly. The loop fires OnRetry and then waits, so a hook that cancels there, with a delay that has already elapsed, saw the timer win about half the time and sent one more request on a dead context — failing fast, but as the transport's wrapping of context.Canceled rather than ctx.Err(), and sometimes round again to "request failed after 3 attempts". A ctx.Err() check before the select closes it, and the same order goes into fetchAPIDownload, which has the identical select behind the identical OnRetry. The interleaving cannot be forced — that is what pseudo-random means — so the contract is pinned the one way it can be: with the context cancelled before the wait and a zero backoff so the timer is already ready, the loop must return ctx.Err() itself having handed the transport exactly one request, on every one of 64 runs. Against the bare select each run is a coin flip; five invocations failed five times, by run 3 at the latest, on both loops. Against the guard it cannot fail. This is also what takes the load-sensitivity out of the probe tests, whose 1ms backoff cases could lose the same flip under preemption. Three comment corrections from the same review rounds, none changing behaviour: the loop and the wait said Retry-After carries "no ceiling", which has not been true since the representability clamp landed — it carries no policy ceiling; parseRetryAfter's doc said it clamps to what a Duration can hold, where the constant is the smaller portable bound; and MIGRATING, parseRetryAfter and two test comments said "SPEC §6 draws that line" about the int64-malformed split, which §6's algorithm on its own does not — it says only to parse a positive integer. The split is Go's, it is the first tier of the rule #793 states in §6 "Retry-After Honouring", and the cross-SDK decision is #799's. Worded to be true on either side of #793 landing. The far-future HTTP-date test asserted only `<= ceiling`, which any positive value satisfies; it now asserts equality, which time.Until's saturation at the Duration maximum makes exact.
Seven suppressed Copilot comments, two rounds — three absorbed by
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d2c37f68b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The width observation said Go rejects above a 64-bit integer. At the revision it cites both Go parsers are strconv.Atoi, whose range is native int - 32 bits on the 32-bit targets this repository keeps viable. Say "native int", and say which parser #796 moves to int64 and which stays Atoi until #798. "MAY bound it against a host limit" read as optional while the second representability tier, stated once below, is mandatory. Now: an implementation MAY have a host limit, and where a parsed value meets one it MUST saturate there - never trap or wrap, never fall back. The tier rule is still stated once; this sentence points at it. §16's scope paragraph said the 429 + too_many_requests branch is the only retry in the loop, while §6's table marks the connection-timeout branch in, which the pseudocode repeats with backoff. Qualified to completed-response branches; the timeout path is in scope by the first clause and has no header to honour.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (3)
SPEC.md:695
- §23 cannot implement this rule deterministically from its current seam contract.
TicketMinterandPollSourcedefinethrottled(retry_after)but plaintransientcarries no delay, while their adapter comments only say retryable 429/503 outcomes map to “transient/throttled” without defining which. An implementation can therefore naturally map 503 totransientand discard its header, violating the new any-retryable-status rule. Specify that any exhausted retryable response with a parsedRetry-Aftermaps tothrottled(retry_after)(regardless of status), and otherwise maps totransient.
| §23 reconnect `backoff` | mint/connect outcomes classified transient or throttled | **in** |
| §23 `poll-retry` | poll outcomes classified transient or throttled | **in** |
SPEC.md:891
- “Two” understates the existing exposure. All four no-handle paths already honor
Retry-Afteron their current 429 path: TypeScript multipart uses baresetTimeout, TypeScript DownloadURL callsexecuteWithRetrywithout a signal, Ruby usessleep, and Python sync usestime.sleep. Thus all four violate the newly stated cancellation requirement independently of widening the honored status set.
`[CONFLICT: the cost of this position is not uniform — four of the SDK sleep paths give the caller no
handle at all today, and two of those already carry the exposure independently of this decision.
Per-path inventory and remedies in #775.]`
SPEC.md:901
- This PR says it closes #775, but this conflict leaves five SDKs needing status convergence and says the only per-SDK inventory lives in that issue; the same section also delegates cancellation, date-form, and scheduler-bound work there. The latest issue inventory explicitly says to leave #775 open until convergence. Either stop closing #775 or open and reference concrete follow-up issues before merge, otherwise these acknowledged conflicts lose their active tracker.
`[CONFLICT: five of the six SDKs gate honouring on a narrower status set than this section
prescribes, in three different shapes, and two of this section's other clauses are also divergent —
one policy cap and one added jitter term. Converging is a behaviour change across five SDKs. The
per-SDK inventory is deliberately NOT restated here: it states current behaviour, the convergence
work below changes the very rows it would state, and no gate can catch it going stale. It lives in
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c84ecf84a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ast round understated
|
Three suppressed Copilot comments on
Also in this push, from the two Codex threads (replied in-thread): the portable-ceiling rule for the second representability tier, and a pointer for rounding to #796's step-2 paragraph.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ounding paragraph beside #793's section
* Let Go's raw GET retry loop see the Retry-After it already parses The loop read its server-specified delay off a *retryableError that nothing in the tree ever constructed, so the branch was unreachable and every 429 fell through to the local backoff curve. The 429 arm parsed the header and spent the number on a hint string, because Error had no field to hold it. Error gains RetryAfter (seconds), populated at both 429 construction sites, and the loop sleeps it in place of the backoff when it is positive. That makes the *Error arm the only arm, so retryableError goes as dead code rather than being brought to life alongside it. Which statuses carry a value is unchanged: only the two 429 sites set the field, matching downloadURL and the generated loop. That question is #775's. Closes #795. * Clamp Retry-After to what a time.Duration can hold A 64-bit build parses `Retry-After: 9223372036854775807` cleanly, and `time.Duration(n) * time.Second` then wraps to -1s. `time.After` on a non-positive duration fires at once, so the loop spends its whole attempt budget back to back against a server that just asked it to wait — the newly honoured header turned into a tight retry loop. Reported independently by two reviewers on #796. Both doors onto Error.RetryAfter now normalize: parseRetryAfter, which feeds checkResponse, downloadURL and RequestResult, and ErrRateLimit, which is exported and takes a bare int, so it can also carry a negative value the field's own doc calls invalid. Over-range saturates rather than falling back to "absent". Falling back would compute the millisecond backoff curve and hammer the peer, which is the same tight loop by another route; saturating waits as long as the host can express, and the wait is a select on ctx.Done() so it stays abandonable. Same split the device-flow parser draws: a digit string too long to be an int is malformed and falls back, a value that parses but exceeds what we can honour is clamped. This is a representability bound, not the policy cap #793 declined — at ~292 years it rejects nothing a server could sensibly ask for. SPEC §7 already carved out exactly this for Swift's UInt64 trap; Go joins it. The generated client's own loop (client.gen.go, from go/templates/ client.tmpl) has the identical unclamped conversion and is untouched here. * Keep the over-range cases compiling where int is 32 bits `int(maxRetryAfterSeconds)` is a CONSTANT conversion, so a 32-bit build rejects it where it is written rather than where it runs: pkg/basecamp/client_retry_after_test.go:265:15: constant 9223372036 overflows int pkg/basecamp/client_retry_after_test.go:320:76: constant 9223372036 overflows int The runtime skip meant to spare those cases could never fire, because the test binary did not build. Converting a variable instead defers the check to run time, where the skip has already decided. GOARCH=386 and GOARCH=arm now vet clean. * Record the unkeyed-literal break Error.RetryAfter causes apidiff calls the field additive and is right about the API surface, but an unkeyed composite literal of a public struct is source-breaking and the tool does not model that. MIGRATING already carries the identical note for FieldErrors (#541); this follows it, with the behaviour change the raw GET loop now has. * Take the float narrowing out of the HTTP-date branch `int(time.Until(t).Seconds())` narrows a float64, and Go leaves an out-of-range float-to-int conversion implementation-defined. Where int is 32 bits, a date beyond ~68 years out — and time.Until saturates at ~292 for the year-9999 dates a server may legally send — produced a value that read as non-positive, so the header was discarded and the loop fell back to its millisecond backoff. That is the outcome the clamp exists to prevent, reached by the one path the clamp ran too late to guard. Whole seconds now come from integer division in the Duration domain, which cannot leave int64, and clampRetryAfterSeconds takes an int64 and bounds it by int's range as well as the Duration's. Cannot be executed on a 64-bit host, so the new test pins the resulting contract rather than claiming a red proof; GOARCH=386 and arm vet clean. The probe's 5s hang guard could not fire either: Get was called inline, so an uninterruptible wait never reached the elapsed check and the package timeout reported it instead — most visibly now that an over-range delay saturates at 292 years rather than wrapping negative. Get moves to a goroutine behind a select. Proved by making the retry wait a plain time.Sleep: the guard now fails in 5.00s naming the delay (2562047h47m16s) instead of hanging. SPEC said "Go saturates this conversion" where only the hand-written client does; the generated loop keeps its own unclamped copy. Says so now, pointing at #798. * Parse the delta-seconds in int64, and round the date form up Two more reads of the same branch, both from review. Atoi's range is int's, so on a 32-bit build `Retry-After: 2147483648` was ErrRange, hence malformed, hence the millisecond backoff — while the same header on a 64-bit build was honoured. ParseInt into int64 makes the parse architecture-independent and leaves the ceiling where it belongs, in the clamp, which already bounds the result by what this host can hold. A digit string too long for an int64 is still malformed everywhere. The date form truncated its remainder toward zero, so a date under a second out became 0 — read by every caller as "no delay" — and one 5s out waited 4. TypeScript, Kotlin and Swift all round up, and Kotlin's source states the rule as a convention; SPEC never wrote it down. Now it does, in §6 step 2, and Go follows it. Python and Ruby still truncate: #799. SPEC and MIGRATING said the saturation bound is ~292 years, which is only true where int is 64 bits — the field is an int, so a 32-bit build saturates at ~68. Both now name both limits. * Spell the clamp's bound where CodeQL can read it `int(min(seconds, maxRetryAfterSeconds, int64(math.MaxInt)))` is bounded, but go/incorrect-integer-conversion reads the guard rather than the arithmetic and could not see through the variadic min: it reported the int64-to-int conversion as unbounded, at high severity, and failed the CodeQL check. Two explicit comparisons instead. Same values, same result on both word sizes, and the bound is now legible to the query and to a reader. Removing either one still fails its own test. * Saturate at one portable ceiling, and at both ends of the parse The clamp's answer depended on GOARCH — ~292 years where int is 64 bits, ~68 where it is 32 — which two reviewers correctly read as a wart in the docs rather than a fact worth documenting. It is now 2147483647 seconds everywhere: at or below both host limits on every target, the number Kotlin already saturates this same header at, and the shared ceiling SPEC §16 names. The arch-conditional test machinery goes with it — no skip, no runtime-computed expectation, no constant-conversion hazard. That also gives CodeQL what it wanted. go/incorrect-integer-conversion reads the guard, not the arithmetic, and neither the variadic min nor the reassign-then-compare form satisfied it; a single comparison against a bound below every target's int range does, and the narrowing is exact by inspection. A positive ParseInt range error now saturates too. ParseInt reports ErrRange with the value already at math.MaxInt64, so 9223372036854775808 — one past the largest int64, and still 1*DIGIT, which is all RFC 9110 asks of delay-seconds — lands on the ceiling instead of falling off a boundary that exists only because of Go's word size. Honouring …807 and hammering the server for …808 is a cliff no reader could predict. The rounding test asserted an equality it could not hold: the wire form carries whole seconds, so the room before the answer drops by one is 1 - frac(now), arbitrarily small. It now re-measures the bound after the parse and asserts one-sidedly, which scheduling delay can only weaken toward vacuity. MIGRATING claimed downloads and the limiter were unchanged. They share parseRetryAfter, so both rounding and saturation reached them; says so now. * Leave §7's host-limits sentence to #793, and carry the rule in §6 #793 rewrites that same sentence to point at §6 rather than restate the rule, and a real content conflict exists between the two branches. Its version is the one to keep, so this drops the hunk entirely rather than handing the merge a choice. What the sentence was carrying — Go's ceiling, why 2147483647 is the portable one, and the generated loop's unclamped copy (#798) — moves into §6 beside the parsing rules it belongs with, so nothing is lost when the two land in either order. * Record the over-range split rather than legislating it §6 step 1 said an over-range delta-seconds "saturates", which #793's rewrite of §7 note 4 contradicts: it permits both a host-limit bound and refusing a value the parser's own numeric type cannot hold, forbidding only a policy cap. Two SDKs do each. A bug fix on Go's raw path is not where a six-SDK rule gets decided, so §6 now states the classification that is not in dispute — over-range is not malformed, RFC 9110 sets no upper bound — records which SDKs do which, and leaves the convergence to #799. Go still saturates: it is one of the two permitted readings, and it removes a cliff between values one digit apart. * Require 1*DIGIT before saturating, and stop tabling six SDKs in SPEC strconv accepts a leading sign, so saturating range errors turned `+9223372036854775808` into a 68-year wait synthesized out of input RFC 9110's delay-seconds grammar does not admit — and `+5` had always been honoured as 5. A digits-only guard first, matching SPEC §16's device parser and the reading conformance's "partly numeric rejected (1*DIGIT)" case already asserts. Removing the guard fails the three new cases, one of them showing the 596523h wait. The over-range table in §6 was wrong in three places, all mine: Swift rejects at parse (its 86,400s clamp is on the sleep, a different thing), Kotlin saturates only in its HTTP-date branch while its integer branch rejects, and Python and Ruby do not "never reach the boundary" — they reach it downstream and raise, `float(10**400)` → OverflowError and `sleep(10**400)` → RangeError, both verified. Six SDKs split four ways with two failing after parsing is not a sentence; #799 carries the table. * Defer to SPEC's two-tier rule instead of restating it — and obey it #793 decides the question my paragraph described as open. Its §6 tiers are: unrepresentable in the parser's own numeric type is MALFORMED and falls out at step 3; representable but beyond what the host can schedule SATURATES, never falls back. Merging both texts would leave SPEC carrying two live models of one question, which is the defect #793 exists to remove — so the paragraph goes rather than being reworded, and step 1 returns to its original line. The code was on the wrong side of tier 1. Accepting a positive ParseInt range error made 9223372036854775808 saturate, where the tier says a value the parser's own int64 cannot hold is malformed. #793 also states outright that Go rejects above its 64-bit integer, so that acceptance would have been false the moment the two branches met. Reverted; the two over-range digit strings move from the saturating test to the malformed table, where removing the rejection now fails both with a 596523h wait. Tier 2 is unchanged and is what this PR set out to fix: 2147483647s in clampRetryAfterSeconds. The digits-only guard stays — a sign is not 1*DIGIT — and `+5` is now the row that kills it, since the over-range signed forms are refused by the parse either way. * Delete the justification the revert left standing parseRetryAfter carried both readings at once: fifteen lines arguing that a positive range error saturates "like every other over-range value", followed by the tier that says it is malformed. The code does the second. The first is the reading two reviewers rejected, sitting in the file as though it were current — and a comment like that gets the next change made in its direction. Three more witnesses of reverted or disproved claims went with it: - the note that TypeScript and Kotlin "fall back at their own parse limits rather than saturating", which implied Go differs from them at tier 1 when it now matches them; - maxRetryAfterSeconds citing Kotlin as saturating this header at the same number, which review disproved — Kotlin saturates only in its HTTP-date branch and its integer branch rejects. It was removed from SPEC and left here; - TestClient_RetryAfterSaturatesAtDurationCeiling and two rows named "beyond duration range", where the Duration bound is no longer what binds; the ceiling is the schedulable one. Renamed to say so, and the probe's "~292 years" figure corrected to ~68. Comments and names only. The tier-1 rejection still fails both malformed cases when removed. * Check the context before the retry wait's select, where nothing competes "Cancellation must win over the wait" was the comment on a select that cannot promise it: when both cases are ready Go picks pseudo-randomly. The loop fires OnRetry and then waits, so a hook that cancels there, with a delay that has already elapsed, saw the timer win about half the time and sent one more request on a dead context — failing fast, but as the transport's wrapping of context.Canceled rather than ctx.Err(), and sometimes round again to "request failed after 3 attempts". A ctx.Err() check before the select closes it, and the same order goes into fetchAPIDownload, which has the identical select behind the identical OnRetry. The interleaving cannot be forced — that is what pseudo-random means — so the contract is pinned the one way it can be: with the context cancelled before the wait and a zero backoff so the timer is already ready, the loop must return ctx.Err() itself having handed the transport exactly one request, on every one of 64 runs. Against the bare select each run is a coin flip; five invocations failed five times, by run 3 at the latest, on both loops. Against the guard it cannot fail. This is also what takes the load-sensitivity out of the probe tests, whose 1ms backoff cases could lose the same flip under preemption. Three comment corrections from the same review rounds, none changing behaviour: the loop and the wait said Retry-After carries "no ceiling", which has not been true since the representability clamp landed — it carries no policy ceiling; parseRetryAfter's doc said it clamps to what a Duration can hold, where the constant is the smaller portable bound; and MIGRATING, parseRetryAfter and two test comments said "SPEC §6 draws that line" about the int64-malformed split, which §6's algorithm on its own does not — it says only to parse a positive integer. The split is Go's, it is the first tier of the rule #793 states in §6 "Retry-After Honouring", and the cross-SDK decision is #799's. Worded to be true on either side of #793 landing. The far-future HTTP-date test asserted only `<= ceiling`, which any positive value satisfies; it now asserts equality, which time.Until's saturation at the Duration maximum makes exact. * Show the errors.As extraction MIGRATING implied a type assertion would do * Clear the three [PENDING #796] markers SPEC carried for this PR, now that it is what lands
* origin/main: Police the endpoints a discovered issuer names, not only the issuer (#810) Refuse redirects on the signed download hop in every SDK (#809) Quiet known-noise CodeQL alerts without losing coverage (#807) Judge the advertised OAuth issuer's address, not just its spelling (#804) Let Go's raw GET retry loop see the Retry-After it already parses (#796) Deflake three tests that raced a wall clock, and gate the class that produced two of them (#794) SPEC §6: decide which statuses honour Retry-After, and how each loop composes it (#793) Pin the conformance runners' fixture reads, and give CI a leg that can see them break (#791) Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it (#790) # Conflicts: # go/go.mod
…nt-feed-go-connector * origin/event-feed-foundations: Make the fake transport honour the oversize sentinel it mirrors Police the endpoints a discovered issuer names, not only the issuer (#810) Refuse redirects on the signed download hop in every SDK (#809) Quiet known-noise CodeQL alerts without losing coverage (#807) Judge the advertised OAuth issuer's address, not just its spelling (#804) Let Go's raw GET retry loop see the Retry-After it already parses (#796) Deflake three tests that raced a wall clock, and gate the class that produced two of them (#794) SPEC §6: decide which statuses honour Retry-After, and how each loop composes it (#793) Pin the conformance runners' fixture reads, and give CI a leg that can see them break (#791) Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it (#790)
…ent-feed-conformance-driver * origin/event-feed-go-connector: Event feed: verdicts that lost coin flips, and a driver blind to eras Fix the filters-clone comment that stated the opposite of the code Event feed: a policy reason never echoes what the server wrote Event feed: wakes are not verdicts, and three more places the order was the bug Make the fake transport honour the oversize sentinel it mirrors Police the endpoints a discovered issuer names, not only the issuer (#810) Refuse redirects on the signed download hop in every SDK (#809) Event feed: the oversize verdict survived the seam but not the sanitizer Event feed: close the rendering-cap class, type the oversize rejection Event feed: cancellation could not reach the one exit, and the origin was checked after the mangle Event feed: own the ledger's bytes, cap the dial rendering, test the store fake Quiet known-noise CodeQL alerts without losing coverage (#807) Judge the advertised OAuth issuer's address, not just its spelling (#804) Let Go's raw GET retry loop see the Retry-After it already parses (#796) Deflake three tests that raced a wall clock, and gate the class that produced two of them (#794) SPEC §6: decide which statuses honour Retry-After, and how each loop composes it (#793) Pin the conformance runners' fixture reads, and give CI a leg that can see them break (#791) Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it (#790)
Merging origin/event-feed-go-connector brought SPEC §6's Retry-After decision (#793) and its follow-on text, and the discovery sweep caught the first consequence on its first run: SPEC.md:640 now says "Python and Ruby still truncate, tracked in #799" — a canonical claim with no registry entry. That is the gate working across a merge, not a defect. Reconciled against the merged tree: * Seven new entries for the promises the merge introduced: #799 (SPEC.md, MIGRATING.md, go/pkg/basecamp/client.go, client_retry_after_test.go — the rounding/over-range halves of the divergence), #798 (SPEC.md, MIGRATING.md — the generated parser stays Atoi until its template change lands), and one #775 the audit surfaced in client.go:704 (status-set widening is #775's). * Every pre-merge entry stays valid: each registered file still carries live sentences for its issue (SPEC.md's #775 sentences moved but did not go; MIGRATING.md:252 likewise). Audit on the merged tree, per the round-3 protocol extended to the two new issues: git grep -nI -e '#578' -e '#589' -e '#775' -e '#798' -e '#799' -- \ ':!spec/api-gaps' ':!spec/tracking-issues.yml' \ ':!scripts/test-check-known-defect-issues-open' 33 mention lines, residual ZERO unregistered [file, issue] pairs. Gates on the merged tree: both eventfeed Go packages green, plain and -race (the era-scoped occupancy scan and the AdvanceIfQuiet advance merged without conflict — disjoint hunks in the driver); tracking self-test 23/23; the live checker sweeps clean and verifies all 22 entries (#578, #589, #775, #798, #799 all open); doc-constants-check green under LC_ALL=C.
Decides what #775 asked for: which statuses honour
Retry-After, and how each delay loop composes the value with its own delay.Scope note. This PR previously also carried SPEC §9's peer-derived-text rule. That half is now #802 (draft). §6 is settled — a decided status rule, a composition table walked back across 11 branches, a precise definition of "retry", the representability tiers, and zero open findings. §9 moved its central boundary three times in one review cycle and the fourth candidate's premise failed a spot check, so it is a section still being designed rather than one being reviewed. Splitting lets the settled half ship.
The status rule
A parsed
Retry-Afteris honoured at any status a retry is already going to happen at. No status gate of its own — honouring is derived from retry eligibility, so it needs no amendment when a retry set grows. This is what §7's algorithm has always spelled: step 3h carries no status test and is reachable only past step 3f's declared-set check. The five SDKs' 429-only gates are narrower than the algorithm they implement."Retry" is now defined
Leaving that word to intuition cost three review rounds, each finding another repeating loop the rule appeared to reach and the code deliberately did not.
§16's
authorization_pendingandslow_downare 4xx protocol answers to a completion poll, so they are outside by definition, not by exception — the next loop is in or out by the criterion rather than by someone noticing. Both clauses carry weight: §4's 401 replay re-issues after a refusal and stays out only because no loop declares 401 retryable.Walked all 11 delay-bearing branches back through the definition — §7, §14, §16's four, §23's five, §4 — and every one lands on the side its code already implements, with no carve-outs.
Composition is per-loop
Separated from honouring, with no default, so a delay loop added later is under-specified until it states its own rather than silently inheriting one that may not fit.
max(interval, retryAfter)backoffpoll-retry§23 needs two rows because its timers genuinely differ — a 1s header against a 50s reconnect draw waits 50s, while the same header on a
poll-retrywaits 1s. That is the sharpest evidence a single rule would not have fitted.Representability
Two tiers: unrepresentable in the parser's own numeric type is malformed; representable but beyond what the host can schedule saturates. Per-SDK widths recorded, Go's ceiling corrected to the portable 2,147,483,647s, and the date-form inventory rewritten per parser after probing found the previous split backwards (Ruby accepts all three RFC 7231 forms, Python two; only TypeScript and Swift are IMF-fixdate-only, and generated Go accepts no date form at all).
No behaviour changes
Every row and tier records what the code already does. Divergences are marked
[CONFLICT]and tracked in #775, #798 and #799.Tracks #775, which stays open until the status convergence it inventories lands — this PR delegates the per-SDK inventory and three clauses to it.