Retry ceilings everywhere: Kotlin honors the caller cap, Ruby enforces the per-op block - #515
Merged
Conversation
github-actions
Bot
added
documentation
Member
Author
|
Copilot errored again (third PR today — service-side). Merging on green CI: full |
![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: 07dbded756
ℹ️ 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".
…floor BasecampHttpClient resolved attempts as opRetry?.maxRetries ?: config.maxRetries — and since every operation carries a retry block, the client's maxRetries was dead for retry sizing. A caller who set maxRetries = 1 inside a request-scoped deadline still got the operation's three attempts (#485 bug 1). Now effective attempts = min(caller cap coerced to >= 1, operation max): lowered caps are honored, raised caps still clamp to the declared ceiling — matching Go and Python's min(cap, op_max). BasecampClientBuilder gains the maxRetries knob; without it there was no public way to express the caller cap the ceiling is supposed to honor. Red proof: callerCapWinsOverOperationMax and zeroCapCoercesToOneAttempt FAIL against the old resolution line; operationCeilingBoundsRaisedCap pins min(5, op 2) = 2.
Ruby's retry loop was bounded by config.max_retries alone and keyed off the error taxonomy's retryable flag, so an operation declaring max: 2 was attempted up to the config count, and 500 — retryable in errors.rb but absent from every operation's declared retryOn [429, 503] — was retried on GET. The last SDK ignoring the per-operation retry block (#485 bug 2). The canonical operation ID now threads from every generated GET/paginate call site through AccountClient and Http#get/#paginate* into request_with_retry — explicit keywords at each layer (the Python pattern, fiber-safe), and the same governed get() serves pagination follow-up requests, so page 2+ cannot silently fall back to ungoverned retry. Generated services emit operation: "OperationId" on the GET family only; mutations never reach Ruby's retry loop. In the transport, a governed GET is bounded by min(max(config cap, 1), op maxAttempts), and a status-bearing error retries exactly when the declared retryOn says so — the taxonomy neither widens the set (no more 500 retries) nor vetoes it. Status-less network errors and all ungoverned traffic (get_absolute, OAuth discovery) keep the taxonomy contract. Metadata loads once, memoized at class level. Red proof (enforcement neutered, plumbing intact): ceiling, 500-gate, and paginate-page-2 tests fail on request counts; caller-lower-cap and ungoverned-500 pin the preserved behavior.
check-retry-metadata-parity.py: Ruby's runtime-consumption row flips from consumes-none (with forbidden tokens) to max + retry_on with required tokens — the gate now FAILS if the governed path is removed. Kotlin's row requires the min()/coerceAtLeast tokens of ceiling semantics. SPEC.md drops both #485 divergence notes: the Gate 3 consumption table shows Kotlin and Ruby honoring caller caps and the declared retryOn, and Appendix F's Ruby row describes governed-GET semantics. TypeScript and Swift remain the two SDKs with no numeric cap to honor.
500 is retryable in Ruby's error taxonomy but absent from every operation's declared retryOn [429, 503]. All six SDKs must let the declared set win: one request, error surfaced. Only un-fixed Ruby fails this case — GetProject dispatch already exists in all five runners, so the fixture needs no new branches.
…ro-cap coercion Codex review caught the Kotlin mirror of the Ruby page-2 hazard: all three BaseService pagination loops called requestWithRetry without operationName, so follow-up pages fell back to the caller cap as both operands — a raised cap of 5 made 5 attempts on page 2 of ListProjects (declared max 3) while page 1 clamped correctly. info.operation now threads into every follow-up request. Red proof: the new paginationFollowUpPagesKeepTheOperationCeiling test fails (5 attempts) without the threading. SPEC.md §2 now states the governed-path coercion honestly: caps are total-attempt counts coerced to at least one, so max_retries = 0 yields a single attempt on governed paths (min(max(1, cap), op_max)) — and the §2 validation divergence note gains Kotlin (builder rejects negatives, transport coerces 0) and splits Ruby's governed (one request) vs ungoverned (zero requests) outcomes.
This was referenced Jul 31, 2026
Merged
Merged
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#485: the per-operation
retry.maxis a ceiling on the caller's configured attempts, never a replacement — but two SDKs disagreed, in opposite directions. Kotlin resolvedopRetry?.maxRetries ?: config.maxRetries, so the operation value overrode an explicit caller instruction (amaxRetries = 1caller inside a request deadline still got 3 attempts). Ruby never consulted the operation block at all: attempts bounded byconfig.max_retriesalone, and its error-taxonomy gate retried GETs on 500 — a status no operation declares retryable.Stacked on #514 (both touch SPEC.md §2/§7 and the retry counts).
What
BasecampHttpClient.kt):effective attempts = min(caller cap coerced ≥ 1, op max), matching Go/Python.BasecampClientBuildergains themaxRetriesknob — there was previously no public way to express the cap the ceiling honors. Red proof:callerCapWinsOverOperationMaxandzeroCapCoercesToOneAttemptFAIL against the old resolution line;operationCeilingBoundsRaisedCappins min(5, op 2) = 2.AccountClient→Http#get/#paginate*→request_with_retry(explicit kwargs — the Python pattern, fiber-safe), and pagination follow-up pages reuse the governedget(), so page 2+ can't silently degrade. Governed GETs:min(max(cap, 1), op max)attempts, status retries gated on the declaredretryOn— errors.rb's 500/502/504 taxonomy neither widens nor vetoes it (Retry: honor the declared retry_on and max_attempts in Go and Python #486 semantics). Ungoverned traffic (get_absolute, OAuth discovery) keeps the pre-metadata contract. Metadata memoized at class level. Red proof (enforcement neutered, plumbing intact): ceiling, 500-gate, and paginate-page-2 tests fail on request counts; caller-lower-cap and ungoverned-500 pin preserved behavior.none+forbidden-tokens tomax + retry_onwith required tokens — the gate now fails if the governed path is removed. Kotlin's row requires theminOf/coerceAtLeastceiling tokens.retry.jsoncase "GET 500 is not retried" (GetProject, one 500, requestCount 1) — passes in all five runners with the fix; only un-fixed Ruby fails it.Breaking
maxRetriesabove an operation's declared max now clamp to the ceiling; callers who lowered it are finally honored.Verification
Full
makegreen (exit 0).GET 500 is not retriedPASSES in Go/Kotlin/Ruby/Python explicitly + TS totals. Parity gate: Kotlinmin(caller cap, opRetry.maxRetries), Rubymax + retry_ongoverned-GET rows.Closes #485.
Summary by cubic
Unify retry ceilings across SDKs: Kotlin now treats the caller’s
maxRetriesas a cap and keeps pagination follow-ups governed, and Ruby GETs honor each operation’sretry.maxandretryOn. This prevents extra attempts and stops retrying 500s when not declared; pagination stays governed.Bug Fixes
min(max(1, caller cap), operation max)for attempts;BasecampClientBuilderaddsmaxRetries; pagination follow-ups passoperationNameto keep the ceiling; tests cover lowered/raised caps, pagination governance, and zero-cap coercion.AccountClient→Http#get/#paginate*→request_with_retry; governed GETs retrymin(max(config cap, 1), op maxAttempts)and only on declaredretryOn; follow-up pages remain governed; ungoverned GETs and network errors keep taxonomy behavior; per-op metadata memoized.max + retry_on.Migration
maxRetries = 0makes one attempt.Written for commit 4b8d202. Summary will update on new commits.