Swift transport: gate retries on operation idempotency (#411)#417
Merged
Conversation
The Swift HTTP transport retried any request whose status was in retry_on (429/503) and retried on network errors without checking operation idempotency, so non-idempotent POSTs were retried against the SDK contract. A 503 returned after the backend created a resource issued a second create; for CreateWormhole a double-create consumes one of the four-per-board slots. Thread a per-operation idempotent bit from generated Metadata into the transport's retry decision (SPEC §7): retry-eligible iff the method is naturally idempotent (GET/HEAD/PUT/DELETE) or the operation is marked idempotent. Non-idempotent POST -> single attempt; the three idempotent POSTs (CompleteTodo, PauseQuestion, SubscribeToCardColumn) keep retrying. - Generator emits idempotentOperations set + isIdempotent(for:) into Metadata.swift from the existing behavior-model.json idempotent flag; no service files change. - HTTPClient.performRequest gains a defaulted idempotent param and folds the method allowlist gate into maxAttempts, covering both the status-retry and network-error retry paths. Allowlisting (not excluding POST) keeps PATCH/OPTIONS and future methods fail-closed. - BaseService computes idempotency from Metadata and threads it through all four request variants; generated services and hand-written extensions inherit the gate unchanged. - SPEC.md: replace the Swift over-retry known-bug entries with the three-gate behavior and correct the stale network-error claim.
github-actions
Bot
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR fixes Swift SDK retry behavior to match SPEC §7 by gating retries (HTTP 429/503 and network errors) on operation idempotency: naturally idempotent methods retry as before, and POST retries only when behavior-model.json marks the operation idempotent: true.
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.
Changes:
- Extend the Swift generator to parse
idempotentfrombehavior-model.jsonand emitMetadata.isIdempotent(for:)backed by anidempotentOperationsset. - Add an
idempotentparameter toHTTPClient.performRequest(defaultfalse) and gate retry attempts on (method allowlist OR metadata idempotency); thread the flag fromBaseService. - Add/extend XCTest coverage for transport-level gating, service-layer wiring, and generator parsing/emission; update SPEC.md to reflect the corrected Swift behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| swift/Tests/BasecampTests/RetryTests.swift | Adds transport- and service-level regression tests for idempotency-gated retries. |
| swift/Tests/BasecampTests/IdempotencyMetadataTests.swift | Adds generator tests for parsing/emitting the idempotent flag into Metadata.swift. |
| swift/Sources/BasecampGenerator/MetadataEmitter.swift | Emits idempotentOperations + Metadata.isIdempotent(for:) into generated metadata. |
| swift/Sources/BasecampGenerator/BehaviorModel.swift | Parses idempotent from behavior-model operations into BehaviorRetryConfig. |
| swift/Sources/Basecamp/Services/BaseService.swift | Computes per-operation idempotency via Metadata and threads it to the transport. |
| swift/Sources/Basecamp/HTTP/HTTPClient.swift | Adds idempotent param and gates retries on idempotency/method allowlist. |
| swift/Sources/Basecamp/Generated/Metadata.swift | Regenerated metadata including the idempotent operation set + lookup. |
| SPEC.md | Updates spec text to reflect Swift’s corrected three-gate retry behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…mpotent-set comment - HTTPClient.retryableMethods is now a private static let so the retry gate does not allocate a Set per request on the hot path (Copilot). - MetadataEmitter: reword the comment above idempotentOperations — the set holds every idempotent-flagged op (any method), not only POSTs; naturally idempotent methods retry via the method gate regardless (Copilot).
github-actions
Bot
added
documentation
jeremy
added a commit
that referenced
this pull request
Jul 25, 2026
* Conformance: pin idempotent-POST retry liveness The idempotency.json suite pinned only the retry *safety* direction — PUT and DELETE retry, and a non-idempotent POST (CreateTodo) does NOT. It never pinned the *liveness* direction: that an idempotent-*flagged* POST MUST retry. That metadata override is the exact behavior distinguishing the three-gate SDKs (Go/TS/Kotlin/Python) from Ruby's GET-only retry, and it was only unit-tested per-SDK — it would silently regress without conformance noticing. This gap surfaced reviewing the Swift idempotency work (#411/#417). Add one fixture case: CompleteTodo (POST /todos/{todoId}/completion.json, 503 then 204, requestCount 2, noError). Dispatch it in the four three-gate runners (Go/TS/Python/Kotlin), which run and pass; skip it in Ruby, whose HTTP layer only retries GET. No SDK source, no generator, behavior-model untouched. Also correct a stale SPEC.md claim. SPEC said "Go is stricter: only GET retries … no idempotency gate," which the conformance run disproves. Go implements the full three-gate on its generated operation path: it retries operations classified idempotent at generation time (GET/HEAD or ops carrying x-basecamp-idempotent, including CompleteTodo) with exponential backoff; non-idempotent ops are single-attempt. Only Ruby is GET-only. The hand-written doRequestURL helper stays GET-only, which is the misleading path a shallow go/pkg/basecamp/ grep hits — the real gate lives in go/pkg/generated/. make conformance: Go 73p/2s, Kotlin 67p/8s, TS 92p/5s, Ruby 66p/9s, Python 72p/3s — 0 failures. make check green. * SPEC: name PUT/DELETE mutations in Go's idempotent-retry set Copilot review: the Go retry description listed only GET/HEAD as naturally-idempotent methods, obscuring that PUT/DELETE mutations (UpdateProject/TrashProject) also retry. They carry x-basecamp-idempotent {natural:true}, so they fall in the extension category — now named explicitly alongside the flagged-idempotent POSTs (CompleteTodo) in both the Cross-SDK Divergence bullet and the Retry Strategy table row.
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.
Fixes #411.
Problem
The Swift SDK's HTTP transport retried any request whose response status was in
retry_on(429/503) — and retried on network errors — without checking operation idempotency. So non-idempotent POSTs were retried, contrary to the SDK contract (SPEC §7). A 503 returned after the backend created a resource issues a second create; forCreateWormholea double-create consumes one of the scarce four-per-board slots (surfaced by #402). Documented known bug atSPEC.md:407/:1568.Exposure: 41 POST operations — 3 idempotent (
CompleteTodo,PauseQuestion,SubscribeToCardColumn), 38 non-idempotent (including all 30Create*). The 3 idempotent POSTs must keep retrying, so a method-only gate is insufficient.Fix — three-gate algorithm (mirrors Kotlin, metadata-driven)
An operation is retry-eligible iff the method is naturally idempotent (GET/HEAD/PUT/DELETE) or
behavior-model.idempotent == true. Non-idempotent POST → single attempt; everything else retries per config on both the status path and the network-error path.BehaviorModel.swift,MetadataEmitter.swift): parse the always-presentidempotentflag and emit a sortedidempotentOperationsset +Metadata.isIdempotent(for:). No service files change — onlyMetadata.swiftregenerates.HTTPClient.performRequest): new defaultedidempotent: Bool = falseparam (fail-safe). The gate allowlists the naturally-idempotent methods (so PATCH/OPTIONS/future methods stay fail-closed) and folds intomaxAttempts, covering both retry paths at once.BaseService(single caller ofperformRequest): computesMetadata.isIdempotent(for: info.operation)and threads it through all four request variants. Generated services and hand-written extensions inherit the gate unchanged.No public API break
OperationInfounchanged;performRequestgains a defaulted param (source-compatible);BaseServicerequest signatures unchanged (idempotency computed internally);Metadata.isIdempotentinternal.behavior-model.jsonunmodified (already carried the flag) →behavior-model-checkstays green. No cross-SDK change.Tests (
swift/Tests/BasecampTests)projects.create(non-idempotent POST) against 503 → 1 request;todos.complete(idempotent POSTCompleteTodo) against 503-then-204 → 2 requests, proving the metadata lookup wires through.IdempotencyMetadataTests,@testable import BasecampGenerator): flag parses true/false/absent; emitted set is deterministically sorted and contains exactly the true ops.Verification
make swift-generate→ onlyMetadata.swiftchanges;Services/diff empty. Post-commit regengit diff --exit-codeclean (drift proof).Metadata.swift:idempotentOperationscontainsCompleteTodo/PauseQuestion/SubscribeToCardColumn, excludes everyCreate*, sorted.make swift-checkgreen (216 tests).make checkgreen.Swift is macOS-only and not in the conformance matrix, so the XCTest suite is the acceptance gate.
Summary by cubic
Gate Swift transport retries on operation idempotency to stop reattempting non‑idempotent POSTs and align with SPEC §7. Non‑idempotent POSTs run once; naturally idempotent methods and POSTs marked
idempotent: truestill retry on 429/503 and network errors.HTTPClient: retry only forGET/HEAD/PUT/DELETEor whenidempotentis true; gate folded intomaxAttemptsto cover both status and network‑error retries. HoistedretryableMethodsto a private static set to avoid per‑request allocations.idempotentfrombehavior-model.jsonand emitsidempotentOperationsplusMetadata.isIdempotent(for:); emitter comment clarified to note it lists all idempotent‑flagged ops (any method).BaseServicelooks upMetadata.isIdempotent(for: operation)and threads it to the transport across all request paths.SPEC.mdupdated to document Swift’s three‑gate behavior and that Swift retries network errors, gated by idempotency.Written for commit 089abb6. Summary will update on new commits.