From 904f38b0c6f2e76397a5de59d009eb44b1a8977f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 28 Jul 2026 00:45:07 -0700 Subject: [PATCH] Retry: honor the declared retry_on status set in Go and Python MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on the per-operation ceiling work, which fixed retry.max. This fixes the other half of Gate 3: the status set. behavior-model.json declares retry_on: [429, 503] for all 226 operations. Go used a global {429, 500, 502, 503, 504} allowlist regardless of what an operation declared, and Python had no status gate at all — its loop keyed off the retryable flag errors.py sets for 500/502/503/504. Both retried three statuses the spec never sanctioned. Go emits the declared set as operationRetryOn, a sibling of operationRetryMax and likewise kept off the exported OperationMetadata struct so wiring it up cannot break external unkeyed composite literals. isRetryableStatus now takes the operationId and consults it. Python gates on the same declared set. errors.py is untouched: its retryable flag is the caller-facing classification and must not widen the transport's gate. A network error carries no status and stays retryable under the section 7 network-error rule. Ungoverned traffic is protected structurally rather than by convention. get_absolute() — the hand-written Launchpad authorization request — passes no operation id, so both the status gate and the attempt ceiling no-op on it and it keeps its pre-Smithy contract. Four regressions, sync and async, fail if generated policy ever reaches OAuth traffic; they pass identically before and after this change, which is the point. check-retry-metadata-parity gains a Go operationRetryOn parity check and reclassifies Go and Python from "max only" to "max + retry_on". Its forbidden-token list previously asserted Go does NOT consume RetryOn, which this makes false. SPEC: removed "Implementations may expand this set to include other 5xx statuses" — that sentence sanctioned the bug. Added the Gate 3 consumption table: five SDKs now gate on the declared retryOn, Ruby does not; only Go and Python apply maxAttempts as a ceiling rather than a replacement. Tracked in #485. Breaking: callers relying on a 500 being retried get one attempt. --- SPEC.md | 56 ++-- .../generated_retry_statusset_test.go | 146 +++++++++ go/pkg/generated/client.gen.go | 278 +++++++++++++++++- go/templates/client.tmpl | 69 ++++- python/src/basecamp/_async_http.py | 37 ++- python/src/basecamp/_http.py | 37 ++- python/tests/test_http.py | 175 ++++++++++- scripts/check-retry-metadata-parity.py | 58 +++- 8 files changed, 799 insertions(+), 57 deletions(-) create mode 100644 go/pkg/basecamp/generated_retry_statusset_test.go diff --git a/SPEC.md b/SPEC.md index 48e218585..af8ea1625 100644 --- a/SPEC.md +++ b/SPEC.md @@ -112,7 +112,7 @@ END - **Default client (`max_retries = 3`):** only the **9 idempotent `max:2` operations** (account/gauge/preference writes: `UpdateAccountName`, `UpdateAccountLogo`, `RemoveAccountLogo`, `UpdateMyPreferences`, `DisableOutOfOffice`, `MarkAsRead`, `ToggleGauge`, `UpdateGaugeNeedle`, `DestroyGaugeNeedle`) change — they now retry at most twice instead of three times. The other 178 retry-eligible ops are unaffected (`min(3, 3) = 3`). - **Client that raised its cap above 3:** **all 187 retry-eligible operations** are now clamped to their per-op `max` (178 to `3`, 9 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 and Python are, however, the only two that also honor a caller who wants *fewer* attempts than the operation declares — see the Kotlin note above.) -- **Client that lowered its cap to `0`/`1`:** unchanged — the cap still wins (`min(cap, op_max) = cap`). Go/Python consume only `max`; the emitted `base_delay_ms`/`backoff`/`retry_on` remain inert per-op metadata for them (retained for parity — see `scripts/check-retry-metadata-parity.py`). Ruby retries GET only and bounds its loop by `config.max_retries` alone, so the per-op `max` is not enforced there at all — the opposite gap from Kotlin's. All per-op retry fields are inert in Ruby. +- **Client that lowered its cap to `0`/`1`:** unchanged — the cap still wins (`min(cap, op_max) = cap`). Go and Python 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 retries GET only and bounds its loop by `config.max_retries` alone, so the per-op `max` is not enforced there at all — the opposite gap from Kotlin's. All per-op retry fields are inert in Ruby. **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. @@ -403,20 +403,39 @@ If `behavior-model.json` marks an operation with `idempotent: true`, the POST be The error must be retryable. Two categories qualify: -- **HTTP status retry:** Response status is in the transport's retryable set. The `behavior-model.json` specifies `retry_on: [429, 503]` for all operations. Implementations may expand this set to include other 5xx statuses (500, 502, 504). +- **HTTP status retry:** Response status is in the operation's **declared** retryable set. `behavior-model.json` specifies `retry_on: [429, 503]` for all operations. The declared set is **exhaustive**: a status outside it — including 500, 502, and 504 — is not retried and is surfaced to the caller on the first attempt. An implementation may still *classify* those statuses as retryable in its error taxonomy (§6); that is a caller-facing hint and must not widen the transport's gate. - **Network error retry:** Connection failures, timeouts, and DNS errors (no HTTP response received) are retryable. These correspond to `BasecampError(code: "network", retryable: true)` in §6. **Divergence:** **Go, Python, and Swift** retry network errors for retry-eligible operations — including idempotent mutations — with Swift and Go gating on operation idempotency, so a non-idempotent POST is attempted once. **Ruby** retries network errors too, but only on GET: its transport routes every non-GET to a single-attempt path, so no mutation ever sees a network retry. **TypeScript** retries only after receiving an HTTP response; **Kotlin** surfaces network errors immediately without retry. The spec prescribes network error retry as the target behavior. **Non-retryable statuses (never retry regardless of method):** 401, 403, 404, 400, 422. -**Gate 3 status-set fidelity `[CONFLICT]`.** Gates 1 and 2 read `behavior-model.json`. Gate 3's -parameters are honored unevenly. The per-operation attempt **ceiling** is consumed by every SDK -except **Ruby**, which bounds its GET-only loop by `config.max_retries` alone and never reads the -operation's `max` (see *Per-operation retry ceiling* in §2). The declared **status set** is honored -by fewer still. **Go and Ruby retry statuses the spec never declared retryable**: Go's -`isRetryableStatus` covers `{429, 500, 502, 503, 504}` regardless of the operation's declared -`retry_on`, and Ruby's loop keys off `retryable?`, which `errors.rb` sets for 500/502/503/504. -**Python** has no status gate at all — its loop keys off the `retryable` flag `errors.py` sets for -the same statuses. TypeScript, Kotlin, and Swift do gate on the declared set. +**Gate 3 consumption `[CONFLICT]`.** Gates 1 and 2 read `behavior-model.json`. Gate 3's parameters +are consumed unevenly: + +| | Gates status retry on the declared `retryOn` | Honors a caller asking for *fewer* attempts than the operation declares | +|---|---|---| +| Go | yes | yes — `min(caller_cap, operation_max)` | +| Python | yes | yes — `min(caller_cap, operation_max)` | +| TypeScript | yes | n/a — exposes no numeric cap (only `enableRetry`), so the operation value is the only input. Additionally caps at one retry (waiver 2B.1) | +| Swift | yes | n/a — exposes no numeric cap (only `enableRetry`) | +| **Kotlin** | yes | **no** — `BasecampConfig.maxRetries` *is* numeric, but `opRetry?.maxRetries ?: config.maxRetries` lets the operation value override it, so a caller who lowers the cap is ignored | +| **Ruby** | **no** — the loop keys off `retryable?`, which `errors.rb` sets for 500/502/503/504, so a 500 is retried on GET | n/a for the ceiling — bounded by `config.max_retries` alone, so the operation's `max` is never enforced at all | + +Only **Kotlin** turns the ceiling into a floor: it is the one SDK with both a numeric caller cap and +a resolution order that discards it. TypeScript and Swift have no cap to discard, and Ruby's gap is +the mirror image — it honors the caller and ignores the operation. Tracked in #485. + +**Ungoverned traffic.** A transport may carry requests that are not Smithy operations — today the +Launchpad authorization request. Those carry no behavior-model metadata, and the declared policy +must **not** be applied to them: they keep whatever contract the SDK gives non-generated traffic. +Python enforces this structurally — `get_absolute()` passes no operation id, so both the status gate +and the attempt ceiling no-op — and pins it with sync and async regressions that fail if generated +policy ever reaches OAuth traffic. + +**Enforcement.** `scripts/check-retry-metadata-parity.py` asserts every SDK's emitted per-operation +retry metadata equals `behavior-model.json`, and records which fields each SDK actually consumes at +runtime. It deliberately does not assert *effective behavior* parity, since TypeScript's one-retry +cap and Ruby's GET-only transport are intentional. Effective Gate 3 behavior is pinned by tests in +Go and Python. ### Cross-SDK Divergence `[CONFLICT]` @@ -1610,18 +1629,15 @@ 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. Chains at most 1 retry via `fetch(retryRequest)` which bypasses middleware (waiver 2B.1). | | Kotlin | Three-gate for HTTP status retries: POST retries only when `idempotent: true`, full exponential backoff. Does not retry network errors (transport exceptions returned immediately). | | 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. Ruby retries on any error with `retryable? == true`. | -| 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. Retries on any error with `retryable == True`. | +| Ruby | Simplified: only GET retries. All non-GET methods never retry. Ruby retries on any error with `retryable? == true`, so it does not gate on the declared `retryOn` — a 500 is retried on GET. | +| 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 (unlike Kotlin/TS), gated by idempotency. | The table above describes **Gate 1 and Gate 2** — *whether* an operation retries. Gate 3's parameters -are tracked separately, and neither is uniform. The per-operation attempt ceiling is honored by Go, -Python, TypeScript, Swift and Kotlin — but **Ruby never consults it**: `request_with_retry` loops -until `@config.max_retries` (`ruby/lib/basecamp/http.rb`) without reading operation metadata, so a -Ruby caller who raises the cap above 3 exceeds the operation's declared `max`. (Kotlin honors the -ceiling but lets it override a *lower* caller cap — see #485.) The declared `retry_on` set is not -uniform either: Go, Python, and Ruby all retry a superset of it. See the Gate 3 status-set note at -§7 above. +are tracked separately: five SDKs gate status retry on the declared `retryOn` (Ruby does not), and +only Go and Python honor a caller asking for *fewer* attempts than an operation declares — Kotlin +lets the operation value override its numeric cap, TypeScript and Swift expose no such cap, and Ruby +never consults the operation's `max`. See the Gate 3 consumption table in §7 above. ### Integer Precision (§10) diff --git a/go/pkg/basecamp/generated_retry_statusset_test.go b/go/pkg/basecamp/generated_retry_statusset_test.go new file mode 100644 index 000000000..97684952b --- /dev/null +++ b/go/pkg/basecamp/generated_retry_statusset_test.go @@ -0,0 +1,146 @@ +package basecamp + +import ( + "context" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/generated" +) + +// These tests pin Gate 3's STATUS SET, the half the per-operation ceiling work +// did not cover. behavior-model.json declares retry_on: [429, 503] for all 226 +// operations; the generated client previously used a global +// {429, 500, 502, 503, 504} allowlist, so it retried three statuses the spec +// never declared retryable. The declared set is now emitted as +// operationRetryOn and consulted by operationId. +// +// ParseHTTPError still classifies 500/502/503/504 as retryable errors for the +// caller's benefit. That is a caller-facing hint, and these tests exist partly +// to pin that it does NOT widen the transport's gate. +// +// They stay outside pkg/generated per the repo rule that generated code carries +// no hand-written tests. + +func statusRetryConfig(maxRetries int) generated.RetryConfig { + return generated.RetryConfig{ + MaxRetries: maxRetries, + BaseDelay: time.Millisecond, + MaxDelay: 2 * time.Millisecond, + Multiplier: 2.0, + } +} + +func countingStatusHandler(status int, counter *int32) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(counter, 1) + w.WriteHeader(status) + } +} + +func TestGeneratedRetry_HonorsDeclaredStatusSet(t *testing.T) { + cases := []struct { + name string + status int + wantAttempts int32 + }{ + {"429 is declared retryable", http.StatusTooManyRequests, 3}, + {"503 is declared retryable", http.StatusServiceUnavailable, 3}, + {"500 is not declared retryable", http.StatusInternalServerError, 1}, + {"502 is not declared retryable", http.StatusBadGateway, 1}, + {"504 is not declared retryable", http.StatusGatewayTimeout, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var attempts int32 + server := httptest.NewServer(countingStatusHandler(tc.status, &attempts)) + defer server.Close() + + client, err := generated.NewClient(server.URL, generated.WithRetryConfig(statusRetryConfig(3))) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + + // CompleteTodo is an idempotent POST with maxAttempts 3, so the only + // thing varying across these cases is the status. + resp, err := client.CompleteTodo(context.Background(), "99999", 100) + if err != nil { + t.Fatalf("CompleteTodo returned error: %v", err) + } + if resp.StatusCode != tc.status { + t.Errorf("got status %d, want %d", resp.StatusCode, tc.status) + } + _ = resp.Body.Close() + + if got := atomic.LoadInt32(&attempts); got != tc.wantAttempts { + t.Errorf("made %d attempts for status %d, want %d", got, tc.status, tc.wantAttempts) + } + }) + } +} + +// The read path must be gated too: a GET is retry-eligible by method, so +// without the status gate a 500 would retry to exhaustion. +func TestGeneratedRetry_ReadPathHonorsDeclaredStatusSet(t *testing.T) { + var attempts int32 + server := httptest.NewServer(countingStatusHandler(http.StatusInternalServerError, &attempts)) + defer server.Close() + + client, err := generated.NewClient(server.URL, generated.WithRetryConfig(statusRetryConfig(3))) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + + resp, err := client.GetAccount(context.Background(), "99999") + if err != nil { + t.Fatalf("GetAccount returned error: %v", err) + } + _ = resp.Body.Close() + + if got := atomic.LoadInt32(&attempts); got != 1 { + t.Errorf("made %d attempts on a 500, want 1 (500 is not in the declared retry_on set)", got) + } +} + +// The generated table must carry the declared set verbatim. This is the raw +// metadata half; the behavioral half is above. +func TestGeneratedRetryOn_MatchesDeclaredSet(t *testing.T) { + for _, operation := range []string{"CompleteTodo", "GetAccount", "UpdateGaugeNeedle"} { + retryOn, ok := generated.GetOperationRetryOn(operation) + if !ok { + t.Errorf("%s missing from operationRetryOn", operation) + continue + } + if len(retryOn) != 2 || retryOn[0] != 429 || retryOn[1] != 503 { + t.Errorf("%s retryOn = %v, want [429 503]", operation, retryOn) + } + } +} + +// An operation's declared set is authoritative in both directions. A +// present-but-EMPTY retryOn means "never retry on any status"; only an absent +// entry falls back to the default. isRetryableStatus is unexported, so this +// exercises the exported accessor plus the documented fallback rule. +func TestGeneratedRetryOn_EmptySetIsNotAbsent(t *testing.T) { + // Every generated operation carries a set today, so the fallback is only + // reachable for an unknown id. Pin that it IS the default, so the two cases + // stay distinguishable if an operation ever declares retryOn: []. + if _, ok := generated.GetOperationRetryOn("NoSuchOperation"); ok { + t.Fatal("an unknown operation must not be present in operationRetryOn") + } + + // And pin that no shipped operation declares an empty set, which is what + // makes the fallback unreachable from a generated call site today. + for _, operation := range []string{"CompleteTodo", "GetAccount", "UpdateGaugeNeedle"} { + retryOn, ok := generated.GetOperationRetryOn(operation) + if !ok { + t.Fatalf("%s missing from operationRetryOn", operation) + } + if len(retryOn) == 0 { + t.Errorf("%s declares an empty retryOn; it would never retry on any status", operation) + } + } +} diff --git a/go/pkg/generated/client.gen.go b/go/pkg/generated/client.gen.go index cef7b41cc..c3ff4d468 100644 --- a/go/pkg/generated/client.gen.go +++ b/go/pkg/generated/client.gen.go @@ -4015,18 +4015,32 @@ func WithLogger(logger *slog.Logger) ClientOption { } } -// isRetryableStatus returns true if the HTTP status code indicates a retryable error. -func isRetryableStatus(statusCode int) bool { - switch statusCode { - case http.StatusTooManyRequests, // 429 - http.StatusInternalServerError, // 500 - http.StatusBadGateway, // 502 - http.StatusServiceUnavailable, // 503 - http.StatusGatewayTimeout: // 504 - return true - default: - return false +// defaultRetryOn is behavior-model.json's declared default, applied only when an +// operation carries no x-basecamp-retry.retryOn — which cannot happen for a +// generated call site. +var defaultRetryOn = []int{http.StatusTooManyRequests, http.StatusServiceUnavailable} + +// isRetryableStatus reports whether the status is in the operation's DECLARED +// retry_on set. The set is per-operation and exhaustive: a status the spec never +// declared retryable — including 500, 502, and 504 — is not retried, and is +// surfaced to the caller on the first attempt. ParseHTTPError still classifies +// those statuses as retryable errors for the caller's benefit; that is a +// caller-facing hint and deliberately does not widen this gate. +func isRetryableStatus(statusCode int, operationId string) bool { + // Only an ABSENT entry falls back. A present-but-empty set is an operation + // declaring "never retry on any status", which is not the same as declaring + // nothing — collapsing the two would silently re-enable 429/503 retries on an + // operation that opted out. + retryOn, ok := operationRetryOn[operationId] + if !ok { + retryOn = defaultRetryOn + } + for _, status := range retryOn { + if status == statusCode { + return true + } } + return false } // captureReplayBody inspects the finalized first-attempt request and returns a @@ -4285,7 +4299,7 @@ func (c *Client) doWithRetry(ctx context.Context, buildRequest func() (*http.Req } // Success or non-retryable status - if !isRetryableStatus(resp.StatusCode) { + if !isRetryableStatus(resp.StatusCode, operationId) { return resp, nil } @@ -20290,6 +20304,246 @@ var operationRetryMax = map[string]int{ "UpdateWebhook": 3, } +// operationRetryOn maps operation IDs to their declared retryable status set +// from the x-basecamp-retry extension. Like operationRetryMax it is a separate +// map rather than a field on OperationMetadata, so wiring it up does not change +// that exported struct's shape. +var operationRetryOn = map[string][]int{ + "GetAccount": {429, 503}, + "RemoveAccountLogo": {429, 503}, + "UpdateAccountLogo": {429, 503}, + "UpdateAccountName": {429, 503}, + "CreateAttachment": {429, 503}, + "GetEverythingBoosts": {429, 503}, + "DeleteBoost": {429, 503}, + "GetBoost": {429, 503}, + "SetCardColumnColor": {429, 503}, + "DisableCardColumnOnHold": {429, 503}, + "EnableCardColumnOnHold": {429, 503}, + "DeleteWormhole": {429, 503}, + "UpdateWormhole": {429, 503}, + "CreateWormhole": {429, 503}, + "ListMessageTypes": {429, 503}, + "CreateMessageType": {429, 503}, + "DeleteMessageType": {429, 503}, + "GetMessageType": {429, 503}, + "UpdateMessageType": {429, 503}, + "CreateTool": {429, 503}, + "ListWebhooks": {429, 503}, + "CreateWebhook": {429, 503}, + "GetCard": {429, 503}, + "UpdateCard": {429, 503}, + "MoveCard": {429, 503}, + "RepositionCardStep": {429, 503}, + "CreateCardStep": {429, 503}, + "GetCardColumn": {429, 503}, + "UpdateCardColumn": {429, 503}, + "ListCards": {429, 503}, + "CreateCard": {429, 503}, + "UnsubscribeFromCardColumn": {429, 503}, + "SubscribeToCardColumn": {429, 503}, + "GetCardStep": {429, 503}, + "UpdateCardStep": {429, 503}, + "SetCardStepCompletion": {429, 503}, + "GetCardTable": {429, 503}, + "CreateCardColumn": {429, 503}, + "MoveCardColumn": {429, 503}, + "GetEverythingCompletedCards": {429, 503}, + "GetEverythingNoDueDateCards": {429, 503}, + "GetEverythingNotNowCards": {429, 503}, + "GetEverythingOpenCards": {429, 503}, + "GetEverythingOverdueCards": {429, 503}, + "GetEverythingUnassignedCards": {429, 503}, + "ListCampfires": {429, 503}, + "GetCampfire": {429, 503}, + "ListChatbots": {429, 503}, + "CreateChatbot": {429, 503}, + "DeleteChatbot": {429, 503}, + "GetChatbot": {429, 503}, + "UpdateChatbot": {429, 503}, + "ListCampfireLines": {429, 503}, + "CreateCampfireLine": {429, 503}, + "DeleteCampfireLine": {429, 503}, + "GetCampfireLine": {429, 503}, + "UpdateCampfireLine": {429, 503}, + "ListCampfireUploads": {429, 503}, + "CreateCampfireUpload": {429, 503}, + "GetEverythingCheckins": {429, 503}, + "ListPingablePeople": {429, 503}, + "ListClientApprovals": {429, 503}, + "GetClientApproval": {429, 503}, + "ListClientCorrespondences": {429, 503}, + "GetClientCorrespondence": {429, 503}, + "ListClientReplies": {429, 503}, + "GetClientReply": {429, 503}, + "GetEverythingComments": {429, 503}, + "GetComment": {429, 503}, + "UpdateComment": {429, 503}, + "DeleteTool": {429, 503}, + "GetTool": {429, 503}, + "UpdateTool": {429, 503}, + "GetDocument": {429, 503}, + "UpdateDocument": {429, 503}, + "GetEverythingFiles": {429, 503}, + "GetEverythingForwards": {429, 503}, + "DestroyGaugeNeedle": {429, 503}, + "GetGaugeNeedle": {429, 503}, + "UpdateGaugeNeedle": {429, 503}, + "GetForward": {429, 503}, + "ListForwardReplies": {429, 503}, + "CreateForwardReply": {429, 503}, + "GetForwardReply": {429, 503}, + "GetInbox": {429, 503}, + "ListForwards": {429, 503}, + "ListLineupMarkers": {429, 503}, + "CreateLineupMarker": {429, 503}, + "DeleteLineupMarker": {429, 503}, + "UpdateLineupMarker": {429, 503}, + "GetMessageBoard": {429, 503}, + "ListMessages": {429, 503}, + "CreateMessage": {429, 503}, + "GetEverythingMessages": {429, 503}, + "GetMessage": {429, 503}, + "UpdateMessage": {429, 503}, + "GetMyAssignments": {429, 503}, + "GetMyCompletedAssignments": {429, 503}, + "GetMyDueAssignments": {429, 503}, + "GetMyPreferences": {429, 503}, + "UpdateMyPreferences": {429, 503}, + "GetMyProfile": {429, 503}, + "UpdateMyProfile": {429, 503}, + "GetQuestionReminders": {429, 503}, + "GetMyNotifications": {429, 503}, + "GetBubbleUps": {429, 503}, + "MarkAsRead": {429, 503}, + "ListPeople": {429, 503}, + "GetPerson": {429, 503}, + "DisableOutOfOffice": {429, 503}, + "GetOutOfOffice": {429, 503}, + "EnableOutOfOffice": {429, 503}, + "ListProjects": {429, 503}, + "CreateProject": {429, 503}, + "ListRecordings": {429, 503}, + "TrashProject": {429, 503}, + "GetProject": {429, 503}, + "UpdateProject": {429, 503}, + "ToggleGauge": {429, 503}, + "ListGaugeNeedles": {429, 503}, + "CreateGaugeNeedle": {429, 503}, + "ListProjectPeople": {429, 503}, + "UpdateProjectAccess": {429, 503}, + "GetProjectTimeline": {429, 503}, + "GetProjectTimesheet": {429, 503}, + "GetAnswer": {429, 503}, + "UpdateAnswer": {429, 503}, + "GetQuestionnaire": {429, 503}, + "ListQuestions": {429, 503}, + "CreateQuestion": {429, 503}, + "GetQuestion": {429, 503}, + "UpdateQuestion": {429, 503}, + "ListAnswers": {429, 503}, + "CreateAnswer": {429, 503}, + "ListQuestionAnswerers": {429, 503}, + "GetAnswersByPerson": {429, 503}, + "UpdateQuestionNotificationSettings": {429, 503}, + "ResumeQuestion": {429, 503}, + "PauseQuestion": {429, 503}, + "UnpinMessage": {429, 503}, + "PinMessage": {429, 503}, + "GetRecording": {429, 503}, + "ListRecordingBoosts": {429, 503}, + "CreateRecordingBoost": {429, 503}, + "SetClientVisibility": {429, 503}, + "ListComments": {429, 503}, + "CreateComment": {429, 503}, + "ListEvents": {429, 503}, + "ListEventBoosts": {429, 503}, + "CreateEventBoost": {429, 503}, + "UnarchiveRecording": {429, 503}, + "ArchiveRecording": {429, 503}, + "TrashRecording": {429, 503}, + "Unsubscribe": {429, 503}, + "GetSubscription": {429, 503}, + "Subscribe": {429, 503}, + "UpdateSubscription": {429, 503}, + "GetRecordingTimesheet": {429, 503}, + "CreateTimesheetEntry": {429, 503}, + "DisableTool": {429, 503}, + "EnableTool": {429, 503}, + "RepositionTool": {429, 503}, + "ListGauges": {429, 503}, + "GetProgressReport": {429, 503}, + "GetUpcomingSchedule": {429, 503}, + "GetTimesheetReport": {429, 503}, + "ListAssignablePeople": {429, 503}, + "GetAssignedTodos": {429, 503}, + "GetOverdueTodos": {429, 503}, + "GetPersonProgress": {429, 503}, + "GetScheduleEntry": {429, 503}, + "UpdateScheduleEntry": {429, 503}, + "GetScheduleEntryOccurrence": {429, 503}, + "GetSchedule": {429, 503}, + "UpdateScheduleSettings": {429, 503}, + "ListScheduleEntries": {429, 503}, + "CreateScheduleEntry": {429, 503}, + "Search": {429, 503}, + "GetSearchMetadata": {429, 503}, + "ListTemplates": {429, 503}, + "CreateTemplate": {429, 503}, + "DeleteTemplate": {429, 503}, + "GetTemplate": {429, 503}, + "UpdateTemplate": {429, 503}, + "CreateProjectFromTemplate": {429, 503}, + "GetProjectConstruction": {429, 503}, + "GetTimesheetEntry": {429, 503}, + "UpdateTimesheetEntry": {429, 503}, + "RepositionTodolistGroup": {429, 503}, + "GetTodolistOrGroup": {429, 503}, + "UpdateTodolistOrGroup": {429, 503}, + "ListTodolistGroups": {429, 503}, + "CreateTodolistGroup": {429, 503}, + "ListTodos": {429, 503}, + "CreateTodo": {429, 503}, + "GetEverythingCompletedTodos": {429, 503}, + "GetEverythingNoDueDateTodos": {429, 503}, + "GetEverythingOpenTodos": {429, 503}, + "GetEverythingOverdueTodos": {429, 503}, + "GetEverythingUnassignedTodos": {429, 503}, + "TrashTodo": {429, 503}, + "GetTodo": {429, 503}, + "ReplaceTodo": {429, 503}, + "UncompleteTodo": {429, 503}, + "CompleteTodo": {429, 503}, + "RepositionTodo": {429, 503}, + "RepositionTodolist": {429, 503}, + "GetTodoset": {429, 503}, + "GetHillChart": {429, 503}, + "UpdateHillChartSettings": {429, 503}, + "ListTodolists": {429, 503}, + "CreateTodolist": {429, 503}, + "GetUpload": {429, 503}, + "UpdateUpload": {429, 503}, + "ListUploadVersions": {429, 503}, + "GetVault": {429, 503}, + "UpdateVault": {429, 503}, + "ListDocuments": {429, 503}, + "CreateDocument": {429, 503}, + "ListUploads": {429, 503}, + "CreateUpload": {429, 503}, + "ListVaults": {429, 503}, + "CreateVault": {429, 503}, + "DeleteWebhook": {429, 503}, + "GetWebhook": {429, 503}, + "UpdateWebhook": {429, 503}, +} + +// GetOperationRetryOn returns the declared retryable status set for the given +// operation ID. ok is false when the operation carries no per-op set. +func GetOperationRetryOn(operationId string) ([]int, bool) { + r, ok := operationRetryOn[operationId] + return r, ok +} + // GetOperationRetryMax returns the per-operation retry ceiling (maximum total // attempts) for the given operation ID. ok is false when the operation carries // no per-op override, in which case the client-configured cap applies unchanged. diff --git a/go/templates/client.tmpl b/go/templates/client.tmpl index 215490aa9..b2b53f6f5 100644 --- a/go/templates/client.tmpl +++ b/go/templates/client.tmpl @@ -144,18 +144,32 @@ func WithLogger(logger *slog.Logger) ClientOption { } } -// isRetryableStatus returns true if the HTTP status code indicates a retryable error. -func isRetryableStatus(statusCode int) bool { - switch statusCode { - case http.StatusTooManyRequests, // 429 - http.StatusInternalServerError, // 500 - http.StatusBadGateway, // 502 - http.StatusServiceUnavailable, // 503 - http.StatusGatewayTimeout: // 504 - return true - default: - return false +// defaultRetryOn is behavior-model.json's declared default, applied only when an +// operation carries no x-basecamp-retry.retryOn — which cannot happen for a +// generated call site. +var defaultRetryOn = []int{http.StatusTooManyRequests, http.StatusServiceUnavailable} + +// isRetryableStatus reports whether the status is in the operation's DECLARED +// retry_on set. The set is per-operation and exhaustive: a status the spec never +// declared retryable — including 500, 502, and 504 — is not retried, and is +// surfaced to the caller on the first attempt. ParseHTTPError still classifies +// those statuses as retryable errors for the caller's benefit; that is a +// caller-facing hint and deliberately does not widen this gate. +func isRetryableStatus(statusCode int, operationId string) bool { + // Only an ABSENT entry falls back. A present-but-empty set is an operation + // declaring "never retry on any status", which is not the same as declaring + // nothing — collapsing the two would silently re-enable 429/503 retries on an + // operation that opted out. + retryOn, ok := operationRetryOn[operationId] + if !ok { + retryOn = defaultRetryOn + } + for _, status := range retryOn { + if status == statusCode { + return true + } } + return false } // captureReplayBody inspects the finalized first-attempt request and returns a @@ -414,7 +428,7 @@ func (c *{{ $clientTypeName }}) doWithRetry(ctx context.Context, buildRequest fu } // Success or non-retryable status - if !isRetryableStatus(resp.StatusCode) { + if !isRetryableStatus(resp.StatusCode, operationId) { return resp, nil } @@ -818,6 +832,37 @@ var operationRetryMax = map[string]int{ {{end}} } +// operationRetryOn maps operation IDs to their declared retryable status set +// from the x-basecamp-retry extension. Like operationRetryMax it is a separate +// map rather than a field on OperationMetadata, so wiring it up does not change +// that exported struct's shape. +var operationRetryOn = map[string][]int{ +{{range . -}} +{{$opid := .OperationId -}} +{{$retryOn := "" -}} +{{if .Spec -}} +{{if .Spec.Extensions -}} +{{$retry := index .Spec.Extensions "x-basecamp-retry" -}} +{{if $retry -}} +{{if index $retry "retryOn" -}} +{{range $i, $status := index $retry "retryOn" -}} +{{if $i}}{{$retryOn = printf "%s, %v" $retryOn $status}}{{else}}{{$retryOn = printf "%v" $status}}{{end -}} +{{end -}} +{{end -}} +{{end -}} +{{end -}} +{{end -}} + "{{$opid}}": { {{$retryOn}} }, +{{end}} +} + +// GetOperationRetryOn returns the declared retryable status set for the given +// operation ID. ok is false when the operation carries no per-op set. +func GetOperationRetryOn(operationId string) ([]int, bool) { + r, ok := operationRetryOn[operationId] + return r, ok +} + // GetOperationRetryMax returns the per-operation retry ceiling (maximum total // attempts) for the given operation ID. ok is false when the operation carries // no per-op override, in which case the client-configured cap applies unchanged. diff --git a/python/src/basecamp/_async_http.py b/python/src/basecamp/_async_http.py index 3f430e551..2797bf8a4 100644 --- a/python/src/basecamp/_async_http.py +++ b/python/src/basecamp/_async_http.py @@ -174,7 +174,7 @@ async def _request_with_retry( allow_cross_origin=allow_cross_origin, ) except (RateLimitError, NetworkError, ApiError) as e: - if not e.retryable: + if not self._is_retryable_error(e, operation): raise last_error = e if attempt >= max_attempts: @@ -311,6 +311,41 @@ def _is_retryable_operation(self, operation: str) -> bool: op_meta = self._metadata.get(operation, {}) return op_meta.get("idempotent", False) + # behavior-model.json's declared default, used when an operation carries no + # retry_on of its own. + DEFAULT_RETRY_ON = frozenset({429, 503}) + + def _is_retryable_error(self, error: BasecampError, operation: str | None) -> bool: + # A network error carries no HTTP status, so the declared status set does + # not apply; SPEC section 7's network-error rule governs, and errors.py's + # classification is the right signal there. + if error.http_status is None: + return error.retryable + # No operation id means no behavior-model metadata — today only the + # Launchpad authorization GET issued by get_absolute(). Non-Smithy + # traffic keeps its pre-Smithy retry contract; applying the generated + # gate here would impose API policy on OAuth authorization traffic. + if operation is None: + return error.retryable + # For a governed operation the DECLARED set is authoritative in BOTH + # directions. It must not be widened by errors.py (which marks + # 500/502/503/504 retryable for the caller's benefit), and it must not be + # vetoed by it either: if an operation declares a status retryable, a + # future errors.py classifying that status non-retryable must not + # silently disable the declared retry. + return error.http_status in self._operation_retry_on(operation) + + def _operation_retry_on(self, operation: str) -> frozenset[int]: + retry = (self._metadata.get(operation) or {}).get("retry") or {} + statuses = retry.get("retry_on") + # `is None`, not truthiness: an operation that declares `retry_on: []` + # means "never retry on any status", which is not the same as declaring + # nothing. Collapsing the two would silently re-enable 429/503 retries on + # an operation that opted out. + if statuses is None: + return self.DEFAULT_RETRY_ON + return frozenset(statuses) + def _apply_operation_retry_max(self, operation: str | None, max_attempts: int) -> int: # Apply the per-operation retry ceiling from metadata as an upper bound on # attempts: effective = min(client cap, operation's retry.max). The diff --git a/python/src/basecamp/_http.py b/python/src/basecamp/_http.py index cb5f1fa9e..771ed8180 100644 --- a/python/src/basecamp/_http.py +++ b/python/src/basecamp/_http.py @@ -174,7 +174,7 @@ def _request_with_retry( allow_cross_origin=allow_cross_origin, ) except (RateLimitError, NetworkError, ApiError) as e: - if not e.retryable: + if not self._is_retryable_error(e, operation): raise last_error = e if attempt >= max_attempts: @@ -311,6 +311,41 @@ def _is_retryable_operation(self, operation: str) -> bool: op_meta = self._metadata.get(operation, {}) return op_meta.get("idempotent", False) + # behavior-model.json's declared default, used when an operation carries no + # retry_on of its own. + DEFAULT_RETRY_ON = frozenset({429, 503}) + + def _is_retryable_error(self, error: BasecampError, operation: str | None) -> bool: + # A network error carries no HTTP status, so the declared status set does + # not apply; SPEC section 7's network-error rule governs, and errors.py's + # classification is the right signal there. + if error.http_status is None: + return error.retryable + # No operation id means no behavior-model metadata — today only the + # Launchpad authorization GET issued by get_absolute(). Non-Smithy + # traffic keeps its pre-Smithy retry contract; applying the generated + # gate here would impose API policy on OAuth authorization traffic. + if operation is None: + return error.retryable + # For a governed operation the DECLARED set is authoritative in BOTH + # directions. It must not be widened by errors.py (which marks + # 500/502/503/504 retryable for the caller's benefit), and it must not be + # vetoed by it either: if an operation declares a status retryable, a + # future errors.py classifying that status non-retryable must not + # silently disable the declared retry. + return error.http_status in self._operation_retry_on(operation) + + def _operation_retry_on(self, operation: str) -> frozenset[int]: + retry = (self._metadata.get(operation) or {}).get("retry") or {} + statuses = retry.get("retry_on") + # `is None`, not truthiness: an operation that declares `retry_on: []` + # means "never retry on any status", which is not the same as declaring + # nothing. Collapsing the two would silently re-enable 429/503 retries on + # an operation that opted out. + if statuses is None: + return self.DEFAULT_RETRY_ON + return frozenset(statuses) + def _apply_operation_retry_max(self, operation: str | None, max_attempts: int) -> int: # Apply the per-operation retry ceiling from metadata as an upper bound on # attempts: effective = min(client cap, operation's retry.max). The diff --git a/python/tests/test_http.py b/python/tests/test_http.py index 0d88822b0..82f52bfa0 100644 --- a/python/tests/test_http.py +++ b/python/tests/test_http.py @@ -14,6 +14,7 @@ from basecamp.errors import ( ApiError, AuthError, + BasecampError, NetworkError, NotFoundError, RateLimitError, @@ -117,7 +118,11 @@ def test_get_retries_on_503(self): assert route.call_count == 2 @respx.mock - def test_get_retries_on_500(self): + def test_get_without_operation_still_retries_500(self): + # No operation id means no behavior-model metadata, so the declared + # retry_on gate does not apply and the historical contract stands. + # Generated reads always pass an operation; see + # TestDeclaredRetryStatuses for the governed path. route = respx.get("https://3.basecampapi.com/test") route.side_effect = [ httpx.Response(500), @@ -524,3 +529,171 @@ async def test_async_mutation_respects_op_ceiling(self): await client.for_account("999").account.update_account_name(name="x") assert route.call_count == 2 await client.close() + + +# behavior-model.json declares retry_on: [429, 503] for all 226 operations. +# These fixtures mirror that so the tests do not depend on any single real op. +STATUS_GATE_METADATA = { + "GovernedOp": {"idempotent": True, "retry": {"max": 3, "retry_on": [429, 503]}}, +} + + +class TestDeclaredRetryStatuses: + """Gate 3's status set. A status outside the operation's declared retry_on — + including 500, 502 and 504 — is surfaced on the first attempt. errors.py + still reports those as retryable to callers; that classification is + deliberately not the transport's gate.""" + + @pytest.mark.parametrize( + "status,want_attempts", + [(429, 3), (503, 3), (500, 1), (502, 1), (504, 1)], + ) + @respx.mock + def test_sync_mutation_status_gate(self, status, want_attempts): + route = respx.put("https://3.basecampapi.com/thing").mock( + return_value=httpx.Response(status, headers={"Retry-After": "0"}) + ) + client = make_client_meta(STATUS_GATE_METADATA) + with pytest.raises((ApiError, RateLimitError)): + client.put("/thing", json_body={"x": 1}, operation="GovernedOp") + assert route.call_count == want_attempts + + @pytest.mark.parametrize( + "status,want_attempts", + [(429, 3), (503, 3), (500, 1), (502, 1), (504, 1)], + ) + @pytest.mark.asyncio + @respx.mock + async def test_async_mutation_status_gate(self, status, want_attempts): + route = respx.put("https://3.basecampapi.com/thing").mock( + return_value=httpx.Response(status, headers={"Retry-After": "0"}) + ) + client = make_async_client_meta(STATUS_GATE_METADATA) + with pytest.raises((ApiError, RateLimitError)): + await client.put("/thing", json_body={"x": 1}, operation="GovernedOp") + assert route.call_count == want_attempts + + @respx.mock + def test_sync_read_status_gate(self): + # Generated reads pass an operation id (threaded by _base.py), so the + # read path is governed too. + route = respx.get("https://3.basecampapi.com/thing").mock(return_value=httpx.Response(500)) + client = make_client_meta(STATUS_GATE_METADATA) + with pytest.raises(ApiError): + client.get("/thing", operation="GovernedOp") + assert route.call_count == 1 + + @pytest.mark.asyncio + @respx.mock + async def test_async_read_status_gate(self): + route = respx.get("https://3.basecampapi.com/thing").mock(return_value=httpx.Response(500)) + client = make_async_client_meta(STATUS_GATE_METADATA) + with pytest.raises(ApiError): + await client.get("/thing", operation="GovernedOp") + assert route.call_count == 1 + + +class TestAuthorizationRetryContractUnchanged: + """Launchpad authorization goes through get_absolute(), which shares the + retry loop with generated traffic but is NOT a Smithy operation — it passes + no operation id and carries no behavior-model metadata. The declared status + gate and the per-operation ceiling must not reach it. These fail if + generated policy ever leaks onto OAuth traffic.""" + + @respx.mock + def test_sync_authorization_still_retries_500(self): + route = respx.get("https://launchpad.37signals.com/authorization.json") + route.side_effect = [ + httpx.Response(500), + httpx.Response(200, json={"ok": True}), + ] + client = make_client_meta(STATUS_GATE_METADATA) + resp = client.get_absolute("https://launchpad.37signals.com/authorization.json") + assert resp.status_code == 200 + assert route.call_count == 2 + + @pytest.mark.asyncio + @respx.mock + async def test_async_authorization_still_retries_500(self): + route = respx.get("https://launchpad.37signals.com/authorization.json") + route.side_effect = [ + httpx.Response(500), + httpx.Response(200, json={"ok": True}), + ] + client = make_async_client_meta(STATUS_GATE_METADATA) + resp = await client.get_absolute("https://launchpad.37signals.com/authorization.json") + assert resp.status_code == 200 + assert route.call_count == 2 + + @respx.mock + def test_sync_authorization_uses_full_configured_attempts(self): + # No operation ceiling applies either, so all 5 configured attempts run. + route = respx.get("https://launchpad.37signals.com/authorization.json").mock(return_value=httpx.Response(503)) + client = make_client_meta(STATUS_GATE_METADATA, max_retries=5) + with pytest.raises(ApiError): + client.get_absolute("https://launchpad.37signals.com/authorization.json") + assert route.call_count == 5 + + @pytest.mark.asyncio + @respx.mock + async def test_async_authorization_uses_full_configured_attempts(self): + route = respx.get("https://launchpad.37signals.com/authorization.json").mock(return_value=httpx.Response(503)) + client = make_async_client_meta(STATUS_GATE_METADATA, max_retries=5) + with pytest.raises(ApiError): + await client.get_absolute("https://launchpad.37signals.com/authorization.json") + assert route.call_count == 5 + + +class TestDeclaredSetIsAuthoritative: + """The declared retry_on set governs a Smithy operation in BOTH directions. + + errors.py classifies statuses for the CALLER (it marks 500/502/503/504 + retryable). That classification must neither widen the transport's gate nor + veto it: if an operation declares a status retryable, the transport retries + it even if errors.py would not have.""" + + @respx.mock + def test_declared_status_retries_even_when_classified_non_retryable(self): + # 409 is not retryable per errors.py, but this operation declares it. + metadata = {"OptInOp": {"idempotent": True, "retry": {"max": 3, "retry_on": [409]}}} + route = respx.put("https://3.basecampapi.com/thing").mock(return_value=httpx.Response(409)) + client = make_client_meta(metadata) + with pytest.raises(BasecampError): + client.put("/thing", json_body={"x": 1}, operation="OptInOp") + assert route.call_count == 3, "a declared status must retry regardless of errors.py" + + @respx.mock + def test_explicitly_empty_retry_on_never_retries(self): + # `retry_on: []` means "never retry on any status" — distinct from + # declaring nothing, which falls back to the [429, 503] default. + metadata = {"OptOutOp": {"idempotent": True, "retry": {"max": 3, "retry_on": []}}} + route = respx.put("https://3.basecampapi.com/thing").mock( + return_value=httpx.Response(503, headers={"Retry-After": "0"}) + ) + client = make_client_meta(metadata) + with pytest.raises(ApiError): + client.put("/thing", json_body={"x": 1}, operation="OptOutOp") + assert route.call_count == 1, "an explicitly empty retry_on must not fall back to the default" + + @respx.mock + def test_absent_retry_on_falls_back_to_the_default(self): + metadata = {"NoBlockOp": {"idempotent": True, "retry": {"max": 3}}} + route = respx.put("https://3.basecampapi.com/thing").mock( + return_value=httpx.Response(503, headers={"Retry-After": "0"}) + ) + client = make_client_meta(metadata) + with pytest.raises(ApiError): + client.put("/thing", json_body={"x": 1}, operation="NoBlockOp") + assert route.call_count == 3, "no declared set means the [429, 503] default applies" + + @pytest.mark.asyncio + @respx.mock + async def test_async_explicitly_empty_retry_on_never_retries(self): + metadata = {"OptOutOp": {"idempotent": True, "retry": {"max": 3, "retry_on": []}}} + route = respx.put("https://3.basecampapi.com/thing").mock( + return_value=httpx.Response(503, headers={"Retry-After": "0"}) + ) + client = make_async_client_meta(metadata) + with pytest.raises(ApiError): + await client.put("/thing", json_body={"x": 1}, operation="OptOutOp") + assert route.call_count == 1 diff --git a/scripts/check-retry-metadata-parity.py b/scripts/check-retry-metadata-parity.py index 11f7bb7c7..f244b1c94 100755 --- a/scripts/check-retry-metadata-parity.py +++ b/scripts/check-retry-metadata-parity.py @@ -21,8 +21,8 @@ - typescript/src/generated/metadata.ts (camelCase) - kotlin/.../generated/Metadata.kt (positional) - swift/Sources/Basecamp/Generated/Metadata.swift (labelled) - * max only: - - go/pkg/generated/client.gen.go operationRetryMax map + * max + retry_on: + - go/pkg/generated/client.gen.go operationRetryMax + operationRetryOn maps (2) Runtime consumption (TOKEN-SMOKE classification, NOT behavioral proof). Which emitted fields are read at runtime, per SDK, checked by the presence @@ -33,8 +33,9 @@ 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 only `max` (per-op ceiling); the - other fields are emitted-but-inert. + * Go / Python: 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. @@ -163,6 +164,21 @@ def from_go_max() -> dict[str, int]: return {m.group("op"): int(m.group("max")) for m in pat.finditer(block.group(1))} +def from_go_retry_on() -> dict[str, tuple]: + # Go emits the declared retryable status set as `operationRetryOn`, a + # sibling of operationRetryMax and likewise kept off the exported + # OperationMetadata struct. + text = (ROOT / "go/pkg/generated/client.gen.go").read_text() + block = re.search(r"var operationRetryOn = map\[string\]\[\]int\{(.*?)\n\}", text, re.DOTALL) + if not block: + raise ValueError("operationRetryOn map not found in client.gen.go") + pat = re.compile(r'"(?P\w+)":\s*\{(?P[0-9,\s]*)\},') + return { + m.group("op"): tuple(int(x) for x in re.findall(r"\d+", m.group("ro"))) + for m in pat.finditer(block.group(1)) + } + + # --- checks ------------------------------------------------------------------ @@ -224,18 +240,39 @@ def check_max_only(name: str, emitted: dict[str, int], model: dict[str, tuple]) ("Kotlin", "full tuple", "kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt", ["opRetry.retryOn", "opRetry?.maxRetries", "opRetry?.baseDelayMs"], [], "BasecampHttpClient.kt: status in opRetry.retryOn, opRetry.maxRetries/baseDelayMs"), - ("Go", "max only", "go/templates/client.tmpl", - ["opMax < maxAttempts", "operationRetryMax[operationId]"], ["RetryBaseDelayMs", "RetryOn"], - "doWithRetry applies min(client cap, operationRetryMax); base_delay/retry_on/backoff emitted-but-inert"), - ("Python", "max only", "python/src/basecamp/_http.py", - ['.get("retry", {}).get("max")'], [], - "_request_with_retry applies min(client cap, retry.max); other fields emitted-but-inert"), + ("Go", "max + retry_on", "go/templates/client.tmpl", + ["opMax < maxAttempts", "operationRetryMax[operationId]", + "operationRetryOn[operationId]", "isRetryableStatus(resp.StatusCode, operationId)"], + ["RetryBaseDelayMs"], + "doWithRetry applies min(client cap, operationRetryMax) and gates status retry on operationRetryOn; base_delay/backoff emitted-but-inert"), + ("Python", "max + retry_on", "python/src/basecamp/_http.py", + ['.get("retry", {}).get("max")', 'retry.get("retry_on")', "_is_retryable_error"], [], + "_request_with_retry applies min(client cap, retry.max) and gates status retry on the declared retry_on; other fields emitted-but-inert"), ("Ruby", "none", "ruby/lib/basecamp/http.rb", [], ["maxAttempts", "maxRetries"], "http.rb retries GET only; every emitted per-op retry field is inert"), ] +def check_retry_on_only(name: str, emitted: dict[str, tuple], model: dict[str, tuple]) -> int: + errors = 0 + missing = sorted(set(model) - set(emitted)) + extra = sorted(set(emitted) - set(model)) + if missing: + fail(f"{name}: {len(missing)} operation(s) missing a retryOn set, e.g. {missing[:3]}") + errors += len(missing) + if extra: + fail(f"{name}: {len(extra)} operation(s) not in behavior-model.json, e.g. {extra[:3]}") + errors += len(extra) + for op in sorted(set(model) & set(emitted)): + if emitted[op] != model[op][3]: + fail(f"{name}: {op} retryOn {emitted[op]} != model {model[op][3]}") + errors += 1 + if errors == 0: + print(f" ✓ {name}: {len(emitted)} operations match behavior-model.json (retry_on)") + return errors + + def check_runtime_consumption() -> int: errors = 0 for sdk, level, rel, required, forbidden, note in RUNTIME_CONSUMPTION: @@ -265,6 +302,7 @@ def main() -> int: errors += check_full_tuple("Kotlin Metadata.kt", from_kotlin(), model) errors += check_full_tuple("Swift Metadata.swift", from_swift(), model) errors += check_max_only("Go operationRetryMax", from_go_max(), model) + errors += check_retry_on_only("Go operationRetryOn", from_go_retry_on(), model) print("\nCriterion 2 — runtime consumption (token-smoke classification, not behavioral proof):") errors += check_runtime_consumption()