diff --git a/SPEC.md b/SPEC.md index dc5612c6c5..dd3e12f27c 100644 --- a/SPEC.md +++ b/SPEC.md @@ -108,11 +108,11 @@ END **Naming note:** `max_retries` means total attempts (including the initial request), not the number of retries after the first attempt. With `max_retries = 3`, the transport makes at most 3 attempts total (1 initial + 2 retries). This name is inherited from the shipping Ruby SDK; the behavior-model.json uses `retry.max` with identical semantics. -**Per-operation retry ceiling.** Each operation carries a per-op `retry.max` in behavior-model.json (203 ops at `3`, 44 at `2`). **TypeScript and Swift** drive their retry loops directly from this per-op value, which is unambiguous there because neither exposes a numeric client-wide cap — only an on/off (`enableRetry`). Generated Go, Python, Kotlin (`BasecampConfig.maxRetries`), and Ruby's governed GET path (`config.max_retries`) expose a numeric client cap *and* honor the per-op value as a **ceiling**: `effective_attempts = min(client_cap, op_max)`. The ceiling can only reduce attempts below the client cap, never raise them, so a client that lowered its cap (e.g. to `1` to disable retries) is still honored; governed paths coerce the cap to at least one attempt (`min(max(1, cap), op_max)`), so `0` yields a single attempt rather than none. Because every op's `max` is ≤ the default cap of `3`, a default or raised client makes exactly the per-op number of attempts in every capped SDK — matching TS/Swift. Observable changes from the former client-wide behavior, by client configuration: +**Per-operation retry ceiling.** Each operation carries a per-op `retry.max` in behavior-model.json (203 ops at `3`, 44 at `2`). **TypeScript and Swift** drive their retry loops directly from this per-op value, which is unambiguous there because neither exposes a numeric client-wide cap — only an on/off (`enableRetry`). Generated Go, Python, Kotlin (`BasecampConfig.maxRetries`), and Ruby's governed GET path (`config.max_retries`) expose a numeric client cap *and* honor the per-op value as a **ceiling**: `effective_attempts = min(client_cap, op_max)`. The ceiling can only reduce attempts below the client cap, never raise them, so a client that lowered its cap (e.g. to `1` to disable retries) is still honored. In Go, Python, and Ruby's governed path the cap is floored at one attempt before the ceiling applies (`min(max(1, cap), op_max)`), so a cap of `0` yields a single attempt rather than none whether or not the operation declares a retry block. Kotlin computes a different expression — `min(max(1, cap), op_max ?: cap)` — which is `0` for an ungoverned operation at a cap of `0`, but it lands on the same single attempt anyway because its loop consults the budget only after the first request has already gone out. Because every op's `max` is ≤ the default cap of `3`, a default or raised client makes exactly the per-op number of attempts in every capped SDK — matching TS/Swift. Observable changes from the former client-wide behavior, by client configuration: - **Default client (`max_retries = 3`):** only the **11 idempotent `max:2` operations** (account/gauge/preference writes plus two subscription-style POSTs: `UpdateAccountName`, `UpdateAccountLogo`, `RemoveAccountLogo`, `UpdateMyPreferences`, `DisableOutOfOffice`, `MarkAsRead`, `ToggleGauge`, `UpdateGaugeNeedle`, `DestroyGaugeNeedle`, `Subscribe`, `EnableCardColumnOnHold`) change — they now retry at most twice instead of three times. The other 195 retry-eligible ops are unaffected (`min(3, 3) = 3`). - **Client that raised its cap above 3:** **all 206 retry-eligible operations** are now clamped to their per-op `max` (195 to `3`, 11 to `2`) instead of retrying up to the raised cap. This is the intended meaning of a per-op ceiling and brings Go/Python into line with TS/Swift/Kotlin, which never retry beyond the per-op `max`. Go, Python, Kotlin, and Ruby's governed path all equally honor a caller who wants *fewer* attempts than the operation declares. -- **Client that lowered its cap to `1`:** unchanged — the cap still wins (`min(cap, op_max) = cap`). A cap of `0` is coerced to one attempt on governed paths (see the `max_retries = 0` divergence note in §2). Go, Python, and Ruby's governed GET path consume `max` **and** `retry_on` (the declared status gate); only the emitted `base_delay_ms`/`backoff` remain inert per-op metadata for them (retained for parity — see `scripts/check-retry-metadata-parity.py`). Ruby remains GET-only: mutations never retry there, so per-op metadata governs only its reads. +- **Client that lowered its cap to `1`:** unchanged — the cap still wins (`min(cap, op_max) = cap`). A cap of `0` is coerced to one attempt on every path, governed or not (see the `max_retries = 0` divergence note in §2). Go, Python, and Ruby's governed GET path consume `max` **and** `retry_on` (the declared status gate); only the emitted `base_delay_ms`/`backoff` remain inert per-op metadata for them (retained for parity — see `scripts/check-retry-metadata-parity.py`). Ruby remains GET-only: mutations never retry there, so per-op metadata governs only its reads. **Recommended default:** A connect timeout of 10 seconds is recommended but not a required config field. Only Ruby exposes this (Faraday `open_timeout = 10`); other SDKs use their HTTP library's default. @@ -133,10 +133,10 @@ All validation errors are `BasecampError(code: "usage")` (see §6 error taxonomy 1. Parse `base_url`. → `⊥ BasecampError(code: "usage")` if malformed. 2. If `base_url` is not the default (`https://3.basecampapi.com`) and not localhost (§9), enforce HTTPS. → `⊥ BasecampError(code: "usage", message: "base URL must use HTTPS")` if scheme ≠ `https`. 3. Validate `timeout > 0`. → `⊥ BasecampError(code: "usage")` otherwise. -4. Validate `max_retries ≥ 1`. → `⊥ BasecampError(code: "usage")` otherwise. (`max_retries` is total attempts including the initial request; 0 would mean no request is made.) **Divergence:** the SDKs handle `max_retries = 0` in three distinct outcomes across four implementations, none of which is the spec's `⊥`: +4. Validate `max_retries ≥ 1`. → `⊥ BasecampError(code: "usage")` otherwise. (`max_retries` is total attempts including the initial request; 0 would mean no request is made.) **Divergence:** the SDKs handle `max_retries = 0` in two distinct outcomes across four implementations, neither of which is the spec's `⊥`: - **Generated Go** (low-level `pkg/generated` client) and **Python** (sync + async): accept `0` as a compatibility exception and make a single attempt with no retry. Both reject a *negative* value as a configuration error (generated Go: `WithRetryConfig`/`doWithRetry` return a plain `error`; Python: `Config` raises `ValueError` at construction). - **Kotlin:** the builder rejects a *negative* value (`require(maxRetries >= 0)`) and accepts `0`, which the transport coerces to a single attempt (`config.maxRetries.coerceAtLeast(1)`). - - **Ruby:** `0` passes config validation. A **governed** GET (canonical operation ID present) coerces the cap to one attempt (`[config.max_retries, 1].max`) and makes a single request. An **ungoverned** GET keeps the old outcome: the retry loop's `break if attempt > max_retries` fires before the first request, so it makes **zero** requests and raises `Basecamp::ApiError("Request failed after 0 attempts")`. + - **Ruby:** `0` passes config validation and the transport coerces it to a single attempt (`[config.max_retries, 1].max`), matching Kotlin. The floor applies on every path: whether a request reaches the wire does not depend on whether the operation carries a declared retry block. A declared operation ceiling still clamps the floored cap downward. - **Hand-written Go** (`pkg/basecamp` client): rejects `0` — `NewClient` panics `"basecamp: max retries must be at least 1"` (its GET/download loops treat `MaxRetries` as the total attempt count with a minimum of 1). 5. Validate `max_pages > 0`. → `⊥ BasecampError(code: "usage")` otherwise. 6. Normalize `base_url`: strip trailing `/`. @@ -660,7 +660,7 @@ Kotlin, and Ruby. - **TypeScript** implements the three-gate algorithm with the retry loop beneath the openapi-fetch middleware chain (the client's custom `fetch`), so attempts run to each operation's declared `retry.max` and network errors retry under the same idempotency gate; caller aborts and request timeouts are terminal. **Kotlin** implements the three-gate algorithm for both HTTP status and network-error retries (POST retries only when `idempotent: true`, full exponential backoff): one eligibility gate covers both failure shapes, with the whole-request timeout (Ktor's `HttpRequestTimeoutException`) deliberately not retried and auth headers re-attached per attempt. - **Go** implements the three-gate on its generated operation path: it retries operations classified idempotent at generation time — GET/HEAD by method, plus any operation carrying `x-basecamp-idempotent` (the naturally-idempotent PUT/DELETE mutations like `UpdateProject`/`TrashProject`, and the flagged-idempotent POSTs like `CompleteTodo`) — with exponential backoff; non-idempotent operations (e.g. `CreateTodo`) are single-attempt. The separate hand-written `doRequestURL` helper remains GET-only for ordinary retries, with a mutation-specific single re-attempt after successful 401 token refresh. -- **Ruby** is stricter: only GET retries; all non-GET methods do not retry. Governed GETs (those carrying their canonical operation ID) are bounded by the per-op ceiling and status-gated on the declared `retryOn`; ungoverned GETs (`get_absolute`, OAuth discovery) keep the taxonomy-driven pre-metadata contract. Ruby is acceptably conservative. +- **Ruby** is stricter: only GET retries; all non-GET methods do not retry. Governed GETs (those carrying their canonical operation ID) are bounded by the per-op ceiling and status-gated on the declared `retryOn`; ungoverned GETs (`get_absolute`, OAuth discovery) keep the taxonomy-driven pre-metadata status contract, under the same floored caller cap. Ruby is acceptably conservative. - **Swift** implements the three-gate algorithm: the transport retries only when the method is naturally idempotent (GET/HEAD/PUT/DELETE) **or** the operation is marked `idempotent: true`, so non-idempotent POSTs like `CreateProject` are attempted exactly once while the seven idempotent POSTs (`CompleteTodo`, `CreateBookmark`, `EnableCardColumnOnHold`, `PauseQuestion`, `PrioritizeAssignment`, `Subscribe`, `SubscribeToCardColumn`) keep retrying. The gate covers both retry paths — HTTP status (`429`/`503`) and network errors — so Swift retries network errors but only for retry-eligible operations. A network error is classified by *meaning*, not by type: a `Transport` that reports connectivity failure as the SDK's own `BasecampError.network` reaches the retry branch exactly as a raw `URLError` does (#567). `Transport` is `public`, so that normalization is the natural implementation and must not be the one that disables retry. Any other `BasecampError` out of the transport (`.auth`, `.usage`, `.api`, …) stays terminal on sight, and the non-HTTP-response guard raises a distinct internal error so a deterministic programming fault is never mistaken for a transport blip. `BaseService` threads the per-operation flag from generated `Metadata` into the transport; the naturally-idempotent method set is allowlisted so PATCH/OPTIONS and future methods stay fail-closed. - The spec prescribes the three-gate algorithm. @@ -1316,7 +1316,7 @@ Attempt budget per SDK — disabling retry (each SDK's spelling of `enable_retry |-----|--------| | Go | `MaxRetries` as total attempts (hand-written client rejects < 1) | | Python | `max_retries` as total attempts, floored at one (`max_retries: 0` still sends one attempt) | -| Ruby | `max_retries` as total attempts, floored at one for downloads (`max_retries: 0` still sends one attempt; the general ungoverned GET path's zero-attempt behavior is tracked separately) | +| Ruby | `max_retries` as total attempts, floored at one on every path — downloads, governed GETs and ungoverned GETs alike (`max_retries: 0` still sends one attempt) | | Kotlin | `maxRetries` as total attempts, floored at one, gated on `enableRetry`; an accepted `maxRetries = 0` still sends exactly one attempt | | TypeScript | Fixed three-attempt policy when `enableRetry` is true; one attempt when false. No public numeric knob. | | Swift | Fixed three-attempt policy when `enableRetry` is true; one attempt when false. No public numeric knob. | @@ -3352,7 +3352,7 @@ Every operation has a `retry` block, including non-idempotent POSTs. For non-ide | TypeScript | Three-gate: POST retries only when `idempotent: true`. Retries on `retry_on` set from metadata to each operation's declared `max`, with the retry loop beneath the openapi-fetch middleware chain as the client's custom `fetch`. Network errors retry under the same idempotency gate; caller aborts and request timeouts are terminal. | | Kotlin | Three-gate for both HTTP status and network-error retries: POST retries only when `idempotent: true`, full exponential backoff. Network errors retry through the same eligibility gate; the whole-request timeout (Ktor's `HttpRequestTimeoutException`) is deliberately not retried. | | Go | Generated operation path retries operations classified idempotent at generation time — GET/HEAD by method, plus any operation carrying `x-basecamp-idempotent` (naturally-idempotent PUT/DELETE mutations like `UpdateProject`/`TrashProject`, and flagged-idempotent POSTs like `CompleteTodo`) — with exponential backoff; non-idempotent operations (e.g. `CreateTodo`) are single-attempt. The separate hand-written `doRequestURL` helper remains GET-only for ordinary retries, with a mutation-specific single re-attempt after successful 401 token refresh. | -| Ruby | Simplified: only GET retries. All non-GET methods never retry. Governed GETs gate status retries on the declared `retryOn` and bound attempts by `min(config.max_retries, operation max)`; ungoverned traffic (no operation ID: `get_absolute`, OAuth) keeps the taxonomy-driven contract. | +| Ruby | Simplified: only GET retries. All non-GET methods never retry. Governed GETs gate status retries on the declared `retryOn` and bound attempts by `min(config.max_retries, operation max)`; ungoverned traffic (no operation ID: `get_absolute`, OAuth) keeps the taxonomy-driven status contract, under the same floored caller cap. | | Python | Three-gate, sync and async: `_mutation()` retries only when `behavior-model` metadata classifies the operation retryable, so non-idempotent POSTs are single-attempt; GETs always retry. Gate 3 uses the operation's declared `retry_on` and `max`. Non-Smithy traffic (`get_absolute()`, Launchpad authorization) passes no operation id and keeps the pre-Smithy contract. | | Swift | Three-gate: retries when the method is naturally idempotent (GET/HEAD/PUT/DELETE) or the operation is marked `idempotent: true`; non-idempotent POSTs make a single attempt. Gate covers both HTTP status and network-error retries, so Swift *does* retry network errors, gated by idempotency. | diff --git a/ruby/README.md b/ruby/README.md index 5171f6697d..062a83aee0 100644 --- a/ruby/README.md +++ b/ruby/README.md @@ -441,14 +441,14 @@ result = account.download_url(url) ## Retry Behavior -Only plain GET requests retry — automatically, with exponential backoff. Mutation operations (POST, PUT, DELETE) do **not** retry to prevent data duplication, and the raw upload and download paths skip the retry loop entirely (the upload path is strictly one request; the download hop keeps only the one-shot 401 replay below). +Only plain GET requests retry — automatically, with exponential backoff. Mutation operations (POST, PUT, DELETE) do **not** retry to prevent data duplication, and the raw upload path skips the retry loop entirely — it is strictly one request. The download flow's authenticated first hop does retry, but under its own declared status set (`429`, `502`, `503`, `504` — never `500`); its second hop, the fetch from the signed URL, is a single unauthenticated request. -- **Which errors**: Retry keys off the error's `retryable?` classification, not a declared status list — 429 (rate limit), 500, 502, 503, 504, and any other 5xx all retry, as does `NetworkError` (connection failures, including DNS and connect-phase timeouts). Read timeouts are the exception: Faraday surfaces them as a status-less `ApiError` with `retryable? == false`, so a GET that times out mid-response fails on the first attempt. 400, 401, 403, 404, and 422 never retry. -- **`max_retries`**: Total request attempts for GET requests, including the initial request — the default `3` means one initial attempt plus two retries. **`max_retries: 0` sends zero requests** and raises `Basecamp::ApiError` (`"Request failed after 0 attempts"`). +- **Which errors**: A GET issued through a generated service carries its operation ID and is **governed** — status retries are gated on the statuses that operation declares, which is `[429, 503]` for every operation in the current metadata. A governed GET does **not** retry 500. Only the handful of GETs that carry no operation ID (`get_absolute`, OAuth discovery) fall back to the error taxonomy, where 429, 500, 502, 503, 504, and any other 5xx all retry. `NetworkError` (connection failures, including DNS and connect-phase timeouts) retries on both paths, since it has no status to gate on. Read timeouts are the exception: Faraday surfaces them as a status-less `ApiError` with `retryable? == false`, so a GET that times out mid-response fails on the first attempt. 400, 401, 403, 404, and 422 never retry. +- **`max_retries`**: Total request attempts for GET requests, including the initial request — the default `3` means one initial attempt plus two retries. `max_retries: 0` is floored to a single attempt rather than sending zero requests. - **Backoff**: Exponential with jitter — `base_delay * 2^(attempt - 1) + rand * max_jitter` — uncapped, bounded in practice by the attempt budget. - **Rate limits**: A 429's `Retry-After` header overrides the calculated backoff. Only 429 carries it: 5xx and network errors always use the exponential backoff. - **401 responses**: With a refresh-capable token provider, the SDK refreshes the token and replays the request **once** — for all methods, including mutations — outside the `max_retries` budget. A second 401 is surfaced. The raw upload path has no 401 replay. -- **Per-operation metadata**: The retry policy operations declare (`retry_on` statuses, per-operation `max`) is inert in Ruby — every API GET issued through the client, including the Launchpad authorization fetch, rides the same classification-based loop bounded by `config.max_retries` alone. (The download flow's redirect hop and OAuth discovery use their own single-attempt transports.) +- **Per-operation metadata**: Every GET a generated service issues passes its canonical operation ID, so essentially all SDK reads are **governed**: attempts are bounded by `min(config.max_retries, operation max)` and status retries are gated on the operation's declared `retry_on`. The GETs that carry no operation ID — `get_absolute` and the Launchpad authorization fetch it backs — are **ungoverned** and ride the classification-based loop bounded by `config.max_retries` alone. The declared `base_delay_ms` and `backoff` are inert in Ruby either way: the backoff is always the client's. (OAuth discovery uses its own single-attempt transport.) - **`retryable?`**: Unlike SDKs where the error classification is only a hint for your own code, in Ruby an error's `retryable?` (and `retry_after`) is exactly what the transport acts on for GET requests. ## Error Handling diff --git a/ruby/lib/basecamp/http.rb b/ruby/lib/basecamp/http.rb index 6f2a868c8c..aa705e43c9 100644 --- a/ruby/lib/basecamp/http.rb +++ b/ruby/lib/basecamp/http.rb @@ -169,10 +169,10 @@ def put_raw(path, body:, content_type:) # Performs the authenticated hop-1 GET for the download flow (SPEC §14). # # Retries network errors plus the declared {DOWNLOAD_RETRY_ON} statuses — - # never 500 — under the public max_retries total-attempt cap, floored at - # one for downloads (+max_retries: 0+ still sends one attempt). DownloadURL - # has no behavior-model entry, so the policy is passed directly rather than - # looked up by operation. + # never 500 — under the public max_retries total-attempt cap, which is + # floored at one attempt on every path, not just this one (+max_retries: 0+ + # still sends one request). DownloadURL has no behavior-model entry, so the + # policy is passed directly rather than looked up by operation. # @param url [String] absolute URL # @return [Response] def get_download(url) @@ -389,18 +389,12 @@ def request(method, path, params: {}, body: nil, allow_cross_origin: false, oper def request_with_retry(method, url, params: {}, allow_cross_origin: false, operation: nil, retry_on: nil, accept: "application/json") op_retry = operation && Http.operation_retry(operation) + # The cap is floored at one attempt on every path: whether a request + # reaches the wire at all must not depend on whether the operation + # carries a declared retry block (#532). A declared operation ceiling + # still clamps the floored cap downward. caller_cap = [ @config.max_retries, 1 ].max - # An explicit declared set (the download flow) shares the governed budget - # shape — the public cap floored at one. The ungoverned general path - # keeps its unfloored cap; its zero-attempt behavior is tracked - # separately (#532). - max_attempts = if op_retry - [ caller_cap, op_retry.fetch("maxAttempts") ].min - elsif retry_on - caller_cap - else - @config.max_retries - end + max_attempts = op_retry ? [ caller_cap, op_retry.fetch("maxAttempts") ].min : caller_cap attempt = 0 refreshed_once = false last_error = nil diff --git a/ruby/test/basecamp/download_test.rb b/ruby/test/basecamp/download_test.rb index 893358444c..c226d723f8 100644 --- a/ruby/test/basecamp/download_test.rb +++ b/ruby/test/basecamp/download_test.rb @@ -411,8 +411,8 @@ def test_download_url_zero_max_retries_still_sends_one_attempt .to_return(status: 503, body: "{}", headers: { "Content-Type" => "application/json" }) # The download attempt budget is floored at one: max_retries: 0 still - # sends exactly one request. (The general ungoverned GET path's - # zero-attempt behavior is tracked separately as #532.) + # sends exactly one request. The same floor now applies on every Ruby + # request path, governed or ungoverned (#532). account = create_account_client(config: fast_download_config(max_retries: 0)) assert_raises(Basecamp::ApiError) { account.download_url(HOP1_URL) } diff --git a/ruby/test/basecamp/http_test.rb b/ruby/test/basecamp/http_test.rb index fa5602464b..1c403faa06 100644 --- a/ruby/test/basecamp/http_test.rb +++ b/ruby/test/basecamp/http_test.rb @@ -514,6 +514,31 @@ def test_caller_lower_cap_wins assert_requested(:get, "https://3.basecampapi.com/test.json", times: 1) end + # #532: max_retries: 0 must still put one request on the wire. The count is + # the assertion that matters — the un-fixed path raises the same error class + # from the same method, so only "did a request happen" separates them. + def test_ungoverned_get_with_zero_max_retries_still_makes_one_request + stub_request(:get, "https://3.basecampapi.com/test.json") + .to_return(status: 200, body: '{"id": 1}') + + response = http_with_max_retries(0).get("/test.json") + + assert_equal 200, response.status + assert_requested(:get, "https://3.basecampapi.com/test.json", times: 1) + end + + # The governed branch already floored the cap at one attempt; pinning it here + # keeps the two branches from drifting apart again. + def test_governed_get_with_zero_max_retries_still_makes_one_request + stub_request(:get, "https://3.basecampapi.com/test.json") + .to_return(status: 200, body: '{"id": 1}') + + response = http_with_max_retries(0).get("/test.json", operation: "GetProject") + + assert_equal 200, response.status + assert_requested(:get, "https://3.basecampapi.com/test.json", times: 1) + end + def test_governed_get_does_not_retry_500 stub_request(:get, "https://3.basecampapi.com/test.json") .to_return(status: 500, body: "{}") diff --git a/scripts/check-retry-metadata-parity.py b/scripts/check-retry-metadata-parity.py index 6ee0e1a313..a09ace2cf9 100755 --- a/scripts/check-retry-metadata-parity.py +++ b/scripts/check-retry-metadata-parity.py @@ -33,11 +33,11 @@ Fields emitted but never read are guarded for PARITY only (criterion 1) and classified emitted-but-runtime-inert — NOT claimed as runtime parity: * TypeScript / Swift / Kotlin: consume the full tuple. - * Go / Python: consume `max` (per-op ceiling) AND + * Go / Python / Ruby: consume `max` (per-op ceiling) AND `retry_on` (the status gate); base_delay - and backoff are emitted-but-inert. - * Ruby: consumes NONE (GET-only retry); every - emitted retry field is inert. + and backoff are emitted-but-inert. Ruby + applies both on its governed GET path + only — mutations never retry there. Exit non-zero on any parity mismatch. """