SPEC §6: decide which statuses honour Retry-After, and how each loop composes it - #793
SPEC §6: decide which statuses honour Retry-After, and how each loop composes it#793jeremy wants to merge 3 commits into
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 (3)
SPEC.md:743
- This width inventory is inaccurate for Kotlin's HTTP-date branch.
Pagination.kt:206-210computes the date delta as aLongand then saturates it withcoerceAtMost(Int.MAX_VALUE), so a future date beyondIntseconds is not rejected as malformed. Please qualify the rejection claim as applying to delta-seconds and record Kotlin's date conversion as the parser-side saturation carve-out described immediately above.
The width itself is deliberately not fixed here, because it is a property of the host: TypeScript
rejects above `Number.MAX_SAFE_INTEGER`, Kotlin above `Int.MAX_VALUE`, Go and Swift above their
64-bit integers. All four reject cleanly, and their thresholds differ by nine orders of magnitude
without any of them misbehaving — a `Retry-After` that names a wait longer than the host can count is
not a delay any caller is worse off for missing.
SPEC.md:791
- These counts include waits that cannot receive
Retry-After: generated Go's network-error sleep (client.gen.go:5769) and the token-refill sleep (rate_limit.go:161) only use local delays, while Kotlin'sBasecampHttpClient.kt:183andDownload.kt:323are likewise network-error backoffs. TheRetry-After-fed waits are four Go paths after #796 and two Kotlin status-response paths. Correct or remove the counts so this purported audit does not imply those unrelated sleeps consume the header.
| Go — all six sleep sites | `select` on `ctx.Done()` vs `time.After(delay)` | yes — `context` |
| Kotlin — all four sites | `kotlinx.coroutines.delay` inside `suspend` functions | yes — job cancellation |
SPEC.md:1402
- The “freshly constructed ... value of the same identity” requirement conflicts with the Go mechanism prescribed at line 1408.
errors.Ispreservescontext.Canceled/DeadlineExceededby reaching the canonical sentinel; a newly constructed same-text error does not have that identity, while the worked example correctly chains the bare sentinel. Amend rule 3 to permit either a canonical peer-free sentinel or a freshly reconstructed type/code projection, and adjust line 1406's claim that both sentinels are values of an unexported empty type (context.Canceledis created byerrors.New).
3. **Do not leave the peer-bearing cause in the chain — project it out first.** Wrapping the original so callers can unwrap it hands the unbounded original straight back and undoes the rendering. But severing the chain outright breaks classification that legitimately reads *through* it, so the order is load-bearing: **before the peer-bearing wrapper is discarded, a cause the constructor recognizes is projected onto a freshly constructed, peer-free value of the same identity, and that is what gets chained.** Recognition is by type or sentinel (rule 1); the projection is a re-construction, never the received instance; and an unrecognized cause is chained as nothing at all (rule 2). What survives is an identity the SDK chose the entire content of.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6842083ed9
ℹ️ 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 1 comment.
Suppressed comments (2)
SPEC.md:743
- This inventory is inaccurate in two ways: Go's current
strconv.Atoiwidth is the platformint(and #796 changes it toint64), not unconditionally 64-bit, and Kotlin's HTTP-date branch does not reject aboveInt.MAX_VALUE—it computes inLongand saturates toInt.MAX_VALUE(Pagination.kt:206-210). As written, this contradicts both the accepted parser-output carve-out above and the laterGOARCHexplanation.
The width itself is deliberately not fixed here, because it is a property of the host: TypeScript
rejects above `Number.MAX_SAFE_INTEGER`, Kotlin above `Int.MAX_VALUE`, Go and Swift above their
64-bit integers. All four reject cleanly, and their thresholds differ by nine orders of magnitude
without any of them misbehaving — a `Retry-After` that names a wait longer than the host can count is
not a delay any caller is worse off for missing.
SPEC.md:804
- This “caller escape” is not available through the standard generated service API. Service option types such as
ListProjectOptions/PaginationOptionsexpose noAbortSignal; middleware instead creates an internal timeout signal from client-widerequestTimeoutMs. Raw openapi-fetch calls can supply a caller-owned signal, but the JSON service path cannot, so it fails the strict caller-held-handle requirement at lines 789-795/829-831. Either make a caller-supplied total-time budget an explicit alternative in the normative rule (which would cover this internal timeout signal), or mark generated services as divergent and add a signal-bearing API to the owed work.
| TypeScript — JSON client path | `sleep(delay, signal)`, which rejects with the signal's abort reason | yes — `AbortSignal` |
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:736
- This
MAYcontradicts the mandatory rule below: lines 791–796 require a representable-but-unschedulable value to saturate, never fall back. Leaving the host bound optional permits exactly the timer overflow/trap that the later rule forbids. Make saturation mandatory when a parsed value exceeds a host limit.
it makes the client wait longer than it was told for no benefit. An implementation MAY bound it
against a **host limit** — a timer that cannot schedule the value, a conversion that would trap or
wrap — and a bound of that kind belongs at the sleep, so a caller reading the error's `retry_after`
SPEC.md:1449
- The scope names only
message, but §6 step 3 also requires peer-providederror_descriptionashint, and default error renderings append that hint (for example, GoError.Error()and SwifterrorDescription). As written,hintis observer-facing peer text yet is absent from the enumerated contract exception, so the closed-vocabulary rule conflicts with §6. Explicitly placehinton one side of this boundary—normally include it withmessageand require the same truncation—or state that it must no longer be rendered.
**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 — and it governs the other two renderings this document contractually requires: §6's statusless `api_error` for a malformed 2xx body, which embeds the offending wire value because that value is the whole diagnostic, and §23's origin-only projection of a refused redirect or rejected continuation URL. 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37099863cd
ℹ️ 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 (5)
SPEC.md:3784
- The parser retains a URL's query; the origin projection is the operation that removes it. Keeping that distinction explicit is important for this security invariant.
why an origin is safe to require: the parse has already dropped the query the ticket
rides in, so there is nothing left to search for. A URL that does not parse has no origin
SPEC.md:1487
- URL parsing does not drop the query; selecting the parsed URL's origin does. Attribute the safety property to the projection so implementers do not mistake a successfully parsed full URL for a safe rendering.
This issue also appears on line 3783 of the same file.
**Projection is permitted; redaction is not.** Extracting a structurally defined component of a *successfully parsed* value and discarding the rest is an allowlist, and stays lawful: §23 carries a refused redirect `Location` and a rejected continuation URL as their **origin only**, which cannot hold a query-string credential because the parse already dropped the query. Searching arbitrary text for a credential and removing what matched is a blocklist, and is the thing this section forbids.
SPEC.md:1491
- This summary conflicts with rule 1, which permits dynamic peer-selected HTTP status and close codes. Their decimal renderings are derived from peer bytes and are not members of a fixed phrase set. Describe the shape as fixed phrases plus the structurally text-free scalar diagnostics allowed by rule 1; otherwise an implementation cannot satisfy both statements.
**The vocabulary's wording is per-SDK; its shape is not.** Nothing here fixes the English. What every SDK owes is that an observer-facing rendering of this class is drawn from a fixed set chosen by error type and carries no peer bytes. §23's conformance surface asserts the typed terminal `reason` and the invalid-frame indication as a flag (`conformance/event-feed/schema.json`), never a rendered message, so the six SDKs owe the classification rather than a string.
SPEC.md:1507
- Ruby has the same post-construction egress, so this cannot be Python-only.
Kernel#raiseautomatically stores the current$!as the new exception's cause unlesscause: nilis supplied; the repository already relies on that behavior atruby/lib/basecamp/oauth/exchange.rb:206-217, whose comment notes that the implicit chain leaks throughfull_message. Consequently, sanitizingNetworkError's constructor while continuing toraise errorinside the Faraday rescue blocks (ruby/lib/basecamp/http.rb:558-563,608-613) still leaves the raw peer-bearing exception in Ruby's runtime cause chain. Extend the raising-boundary requirement and convergence work to Ruby as well.
**Python owes a second boundary, because in Python the constructor cannot discharge this alone.** What follows is an obligation on **Python only** — it falls out of a CPython runtime behaviour with no equivalent in the other five, which inherit nothing from it and owe nothing extra on its account.
SPEC.md:1476
context.Canceledis created byerrors.New("context canceled"); onlycontext.DeadlineExceededuses an unexported empty struct type. The safety argument still holds because both are fixed package-level sentinels, but the type claim should be removed.
This is not hypothetical tidiness — two live classifiers walk that chain today, and a literal reading of the old wording broke both. Go's `shouldTripCircuit` (`go/pkg/basecamp/resilience.go:75`) tests **two** sentinels in one condition — `errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)` — and reaches either only through `(*Error).Unwrap` returning `Cause`, which `ErrNetwork` populates; sever the chain, or project only the first, and a **cancelled *or timed-out* request falls through to the `Code == CodeNetwork` arm and trips the circuit breaker**. Both are safe to project and both must survive: they are package-level sentinel values of an unexported empty type, rendering the standard library's own fixed `"context canceled"` and `"context deadline exceeded"`, structurally incapable of carrying a byte the peer chose. Swift's `isCancellation` (`swift/Sources/Basecamp/HTTP/HTTPClient.swift`) walks `BasecampError.network(_, cause:)` looking for `CancellationError` or a `URLError` with code `.cancelled`, and its result is what makes cancellation terminal; sever the chain and a **cancelled request is retried** instead.
…composes it Closes what #775 asked for. Re-cut from main to carry only the Retry-After material; the peer-derived-text work that shared this branch is split to its own PR because its boundary is still being designed. The status rule. A parsed Retry-After is honoured at any status a retry is already going to happen at - no status gate of its own, derived from retry eligibility rather than from a list, so it needs no amendment when a retry set grows. This is what §7's algorithm has always spelled: step 3h has no status test and is reachable only past step 3f's declared-set check. The word "retry" is then defined, because leaving it to intuition cost three review rounds - each finding another repeating loop the rule appeared to reach and the code deliberately did not. A retry is the re-issue of a request whose previous attempt produced no answer: the transport failed, or the origin declined to serve it with a status the loop DECLARES retryable. Re-issuing because the answer was "not yet" is a poll. §16's authorization_pending and slow_down are therefore outside by definition rather than by exception. Both clauses carry weight - §4's 401 replay re-issues after a refusal and stays out only because no loop declares 401 retryable. Composition is separated from honouring and made per-loop with no default, so a delay loop added later is under-specified until it states its own. Five rows, each verified in the section that owns it: §7 and §14 replace, §16 takes max(interval, retryAfter), §23's backoff timer floors and its poll-retry timer waits exactly. §23 needs two rows because its timers genuinely differ, which is the evidence one rule would not have fitted. Representability is split into two tiers - unrepresentable in the parser's own type is malformed, representable but unschedulable saturates - with the per-SDK widths and the Go ceiling recorded, and the date-form inventory rewritten per parser after probing found the previous split backwards. No behaviour changes: every row and tier records what the code already does, with the divergences marked CONFLICT and tracked in #775, #798 and #799.
jeremy
changed the title
|
Split: this PR is now §6 only. The peer-derived-text half moved to #802 (draft). Why. §6 is settled — decided status rule, composition table walked back across 11 branches, a precise definition of "retry", the representability tiers, zero open findings. §9 moved its central boundary three times in one review cycle (call-site list → constructor → egress), and the fourth candidate's premise failed a spot check before it was written. Shipping them together would hold settled work behind a section still being designed. The 23 resolved threads above are not orphaned, and none were reopened or closed. Roughly two thirds were §6 and their outcomes are all in this branch — the status rule, the per-loop composition split, the retry definition, the tier reassignment, the generated-Go date-form row, the #796 ceiling correction. The §9 threads — the cancellation cause chain, the Mechanics, for the record. Re-cut from #796 note: the split did not clean that merge. The one remaining conflict is entirely §6-vs-§6 — their new step-2 rounding paragraph sits above the base paragraph this branch replaces — so it stays with this PR and remains mechanical adjacency. |
I asserted verification I had not done, twice, in the table whose whole purpose
was to be checkable.
The §23 rows claimed "code today". No event-feed connector ships in any of the
six SDKs - Appendix A says it lands in later PRs - so those four rows are
contract-only and were checked against §23's written text, not against code.
The column now says which of the two each row is, and notes that the §23 rows
are the weaker evidence and should be re-checked when the connector exists
rather than assumed. Seven code rows were genuinely read; four were not, and
the table said otherwise.
The download conflict claimed every SDK honours Retry-After on 429 alone. False
for both Python clients: get_download passes DOWNLOAD_RETRY_ON =
{429,502,503,504} into the shared loop, error_from_response attaches the parsed
header to the ApiError at every status, and _calculate_delay honours any
positive retry_after with no status test - so a Python hop-1 503 already waits
what the origin named. Python is conformant on this axis and owes nothing;
four of six diverge, not six.
Third is an internal inconsistency rather than a fact: the escape remedy
offered "a bound against a caller-supplied total-time budget" as an option,
which cannot satisfy the MUST two paragraphs above it. That MUST asks for a
handle the caller can act on AFTER the call begins, and a numeric deadline
fixed beforehand cannot be. It is a policy cap by another name - the exact
trade this section declined - so it is now named as non-satisfying rather than
listed as an alternative.
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:2240
- This says the 429/
too_many_requestsbranch is the only retry in the device loop, but the same new section classifies the connection-timeout branch as a retry (line 688), and the pseudocode retries it at line 2229. Limit this claim to branches that received an HTTP response so the scope does not contradict its own table.
other whitespace (NBSP, Unicode spaces) is malformed and falls
back — never trimmed into validity. Cancellation stays live through the (possibly longer)
SPEC.md:821
asyncio.sleepis not schedulable up to the double range. CPython’s current selector path converts timeouts to milliseconds and the native poll/epoll binding rejects values aboveINT_MAX; Windows’ proactor similarly raisesValueError("timeout too big")at its finite millisecond limit (Lib/selectors.py,Modules/selectmodule.c,Lib/asyncio/windows_events.py). Thus a large but float-representableRetry-Aftercan fail immediately—on common platforms around tens of days, not 1.8e308 seconds—so the stated async ceiling and the claimed conformant band are incorrect. The second-tier requirement must use the actual event-loop scheduler limit (or chunk the wait), not_calculate_delay’s float conversion.
nothing to fire on. The failure is one layer down, at the scheduler: Ruby's `sleep` raises
`RangeError` ("bignum too big to convert into 'long'") and Python's `time.sleep` raises
`OverflowError` ("timestamp too large to convert to C _PyTime_t") — so a response that was merely
retryable becomes an unrelated exception the caller never asked to handle. Reading that as the first
tier would oblige an implementer to invent a parser limit this section otherwise forbids, and to
SPEC.md:640
- The PR promises to decide the peer-derived error boundary as well, but the resulting SPEC still has no closed-vocabulary rule: §9 ends after the existing truncation contract, and §23 still says to truncate frame-derived error text (
SPEC.md:1437-1458,SPEC.md:3679-3685). This leaves #788 at exactly the unresolved boundary the PR description says it settles. Please restore the normative §9 rule and update §23’s security invariant to point to it.
### Retry-After Honouring `[CONFLICT]`
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5645dfef
ℹ️ 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 (5)
SPEC.md:1790
- This conflict inventory again omits Kotlin:
Download.kt:340-345gates the server-directed delay onstatus == 429, so Kotlin also owes convergence for 502/503/504. The count should be five of six and the list should include the Kotlin counterpart; otherwise the follow-up can incorrectly appear complete while one SDK still diverges.
That last clause changed with §6's "Retry-After Honouring", and the reason it changed is the reason this set is declared here at all: honouring is derived from retry eligibility, so a loop that declares its own eligibility set inherits the honouring rule over that set rather than over §7's. A 502, 503 or 504 on hop 1 carrying `Retry-After` therefore waits what the origin named, exactly as a 429 does. `[CONFLICT: four of the six download loops honour it on 429 alone today — go/pkg/basecamp/download.go, typescript/src/download.ts, and the Ruby and Swift counterparts — so this is owed convergence, tracked with the rest in #775. **Python is already conformant on this axis and owes nothing here:** `get_download` passes its own `DOWNLOAD_RETRY_ON = {429, 502, 503, 504}` into the shared retry loop (`_http.py`, `_async_http.py`), `error_from_response` attaches the parsed header to the resulting `ApiError` at every status (`err.retry_after = err.retry_after or retry_after`), and `_calculate_delay` honours any positive `error.retry_after` with no status test — so a Python hop-1 503 already waits the value the origin named. The existing downloads.json case covering the 429 path stays valid; the other three statuses need cases of their own.]` The honoured value is subject to §6's other two clauses on this path as well: nothing is added to it, and it must be awaited through a cancellation handle the caller holds — which TypeScript's download loop, listed in §6's table, does not yet give them.
SPEC.md:691
- This count omits Kotlin.
kotlin/.../Download.kt:340-345also parsesRetry-Afterbut uses it only whenstatus == 429; 502/503/504 fall back to local backoff. Python is therefore the sole conformant download loop, and the other five SDKs are 429-only.
This issue also appears on line 1790 of the same file.
| §14 `DownloadURL` hop 1 | declared `{429, 502, 503, 504}`, or a network error | **in** | *code:* honours it — Python at all four statuses, the other four SDKs at 429 only (tracked conflict, not a boundary question) |
SPEC.md:669
- This exhaustive boundary misses §23's below-threshold unauthorized mint branch. The existing state table at SPEC.md:2940 sends a 401/403 mint to
Backoffand explicitly says itsRetry-Afterfloors the next delay, while the new definition excludes 401 when no loop declares it retryable. Either classify that connector branch as an in-scope retry and carry its delay, or remove the existing honouring requirement; as written the two contracts disagree.
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.
SPEC.md:811
- Kotlin does not uniformly reject values above
Int.MAX_VALUE. Its delta-seconds branch rejects them viatoIntOrNull, buthttpDateDelaySecondsexplicitly saturates a far-future parsed date atInt.MAX_VALUE(Pagination.kt:196-210). The inventory should preserve that branch-specific behavior because it is the parser-output saturation carve-out described immediately above.
The width itself is deliberately not fixed here, because it is a property of the host: TypeScript
rejects above `Number.MAX_SAFE_INTEGER`, Kotlin above `Int.MAX_VALUE`, Go and Swift above their
64-bit integers. All four reject cleanly, and their thresholds differ by nine orders of magnitude
without any of them misbehaving — a `Retry-After` that names a wait longer than the host can count is
not a delay any caller is worse off for missing.
SPEC.md:883
- Widening also adds 503 to the non-cancellable TypeScript
DownloadURLpath listed immediately above.download.ts:205calls the same retry loop without a signal, and its retry set includes 503, so after convergence a 503 header can create the same un-abandonable wait as the upload path. Include it here so the operational impact and required cancellation work are complete.
The four rows without an escape are the ones that owe work, and this position makes them owe it
sooner rather than creating the exposure: Python's **sync** client already honours `Retry-After` at
any status, unclamped, in a bare `time.sleep`, so an origin sending `Retry-After: 3600` buys an hour
the caller cannot abandon **today**. Widening the status set adds 503 to that reach in Ruby and on
TypeScript's upload path. The remedy is open in its *shape* but not in what it must achieve: whatever
Fixes Kotlin's missing row in the download divergence first: Download.kt
declares DOWNLOAD_RETRY_ON = {429,502,503,504} at line 24 but its delay branch
tests status == 429, so it falls back to local backoff on the other three like
the rest. Five of six diverge, not four.
That is the fourth round in which one of these tables needed a factual
correction - §23's rows labelled "Code today" with no connector shipping
anywhere, the download loop claimed 429-only when Python already conforms, the
date-form split backwards, now Kotlin. Every one was caught by a reviewer or by
me, never by the table. These state per-SDK current behaviour, which by this
repo's own doc-constants convention is a class-A current-value claim living in
prose, and the convergence work they describe changes the very rows they state.
They are stale by design and nothing in CI can see it.
So they move to #775 where they can be edited as work lands, verified as of
this branch. SPEC keeps the normative rule and a pointer. Removed: the
nine-row cancellation-escape table, the six-bullet status-gate divergence, the
added-jitter and bounds bullets, the seven-row date-form parser table, the
Ruby/Python representability detail, and the per-SDK download call sites.
Kept as contract: the composition table, which is normative per loop rather
than observed; the retry walk-back verdicts, which are what the criterion
decides; and the two-tier rule with Go as its one worked example. The
walk-back table loses its "Source" column - the verdicts are contract, whether
today's code agrees is observation, and that column is the one that needed
correcting twice.
Kept as an explicit as-of observation, because the rule is unreadable without
it: the four host widths behind "the width is deliberately not fixed here".
Marked with what it was verified against so the next reader knows it is not a
live claim.
Net effect is 107 fewer lines of SPEC stating things that were going to drift.
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".
|
|
||
| ### Retry-After Honouring `[CONFLICT]` | ||
|
|
||
| **A parsed `Retry-After` is honoured at any status a retry is already going to happen at.** There is |
There was a problem hiding this comment.
When the device-token endpoint returns 429 plus too_many_requests with a valid HTTP-date Retry-After, the parsing algorithm at lines 630–634 produces a delay and this universal rule requires the retry to honor it. However, §16 lines 2116–2124 accepts only a positive integral delta and otherwise falls back to interval, so the same response has two incompatible prescribed waits. Apply the §6 parser to this branch or explicitly declare its delta-only parsing as an exception.
Useful? React with 👍 / 👎.
| tier) at a ceiling chosen to be portable rather than maximal — `math.MaxInt32` seconds, the same | ||
| 2,147,483,647 §16 already names as a shared cross-SDK ceiling. The portability matters because two | ||
| host limits sit above a Go `Retry-After` and a ceiling derived from only the larger would change with | ||
| `GOARCH`. Shipped in #796. |
There was a problem hiding this comment.
In the reviewed tree, go/pkg/basecamp/client.go:1075-1090 still parses this header with strconv.Atoi, and go/pkg/basecamp/download.go:199 still multiplies the result directly into a time.Duration; there is no int64 parser or math.MaxInt32 saturation. For example, a 64-bit authenticated download receiving Retry-After: 10000000000 accepts the value and then overflows the duration conversion, while a 32-bit build rejects it into local backoff, so marking the portable behavior as shipped hides a live cross-architecture divergence. Land the #796 dependency before this claim or keep the hand-written Go path marked as a conflict.
Useful? React with 👍 / 👎.
| wrap — and a bound of that kind belongs at the sleep, so a caller reading the error's `retry_after` | ||
| still sees what the server said. TypeScript's `Math.min(seconds × 1000, MAX_TIMEOUT_MS)`, applied in | ||
| both of its retry loops as the delay is computed, is the worked example: the parser's result stays | ||
| the public `retryAfter`, and only what reaches the timer is clamped. |
There was a problem hiding this comment.
When a retryable 503 carries a valid Retry-After, this paragraph says the unclamped parsed value remains available through the public error, but the normative HTTP Status Mapping Algorithm at lines 565–568 constructs every 502/503/504 BasecampError without retry_after; only the 429 arm populates it. Consequently an exhausted 503 loses the server value before the caller can inspect it (and a retry hook error constructed by §7 step 3i loses it as well), despite the sleep having used that value on earlier attempts. Populate retry_after for these mapped statuses or explicitly narrow the preservation guarantee.
Useful? React with 👍 / 👎.
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.Closes #775.