diff --git a/SPEC.md b/SPEC.md index c891052c1c..ad2b8bbaed 100644 --- a/SPEC.md +++ b/SPEC.md @@ -984,25 +984,45 @@ Downloads use a two-hop pattern: an authenticated API request that returns a red FUNCTION downloadURL(raw_url: String) → DownloadResult 1. Validate raw_url is an absolute URL with http(s) scheme. 2. Rewrite URL: replace origin with base_url origin, preserve path+query+fragment. - 3. Hop 1 — Authenticated API GET: - a. Set Authorization and User-Agent headers only (no Accept or Content-Type — this is a binary download, not a JSON API call). + 3. Hop 1 — Authenticated API GET, wrapped in the hop-1 retry loop (below): + a. Set Authorization and User-Agent headers only (no Accept or Content-Type — this is a binary download, not a JSON API call). Every attempt is authenticated — re-run the auth strategy on retry so a rotated token is picked up. b. Fetch with redirect: manual (do not follow redirects automatically). - c. If response is redirect (301, 302, 303, 307, 308): + c. If the attempt fails with a network error, or the response status is in DOWNLOAD_RETRY_ON = {429, 502, 503, 504}: retry with exponential backoff while attempts remain (honor Retry-After on 429), else surface the failure. 500 is DELIBERATELY outside the set — it is never retried. + d. If response is redirect (301, 302, 303, 307, 308): - Extract Location header. ⊥ if absent. - Resolve Location against rewritten URL (handle relative redirects). - Proceed to Hop 2. - d. If response is 2xx: + e. If response is 2xx: - Direct download (no second hop needed). - → DownloadResult from response body. - e. If response is error → ⊥ BasecampError from response. + f. If response is any other error → ⊥ BasecampError from response, without retry. 4. Hop 2 — Unauthenticated fetch (signed URL): - a. Fetch Location URL with NO auth headers. + a. Fetch Location URL with NO auth headers. Hop 2 is NEVER retried and NEVER authenticated — the signed URL is single-purpose and credentials must not leak to the storage host. b. If not 2xx → ⊥ BasecampError. c. → DownloadResult from response body. END ``` +### Hop-1 Retry `[conformance]` + +The authenticated first hop retries on **network errors plus {429, 502, 503, 504}** — never 500. The set is declared here rather than inherited from anywhere else, and it matches neither of the two sets an SDK already has to hand: it is broader than the per-operation `retry_on` in `behavior-model.json` (`{429, 503}` for all 238 operations, which never governs `DownloadURL` because it has no entry there), and narrower than the error taxonomy's "all 5xx retryable" flag, which would sweep in the 500 this policy deliberately excludes. It is the gateway-error set Go's hand-written `singleRequest` already uses for GETs. Backoff is exponential from a 1-second base with jitter; `Retry-After` is honored on 429. The second hop is exempt: no retry, no auth. + +"Network error" means a transport failure, with one carve-out that SDKs inherit from their main GET loop rather than restate: an attempt that exhausted the caller's entire per-attempt time budget (a request timeout) is not retried. The timeout is per attempt, so a retry spends another full budget on the same slowness rather than riding out a blip. Kotlin implements this explicitly; SDKs whose transports surface timeouts indistinguishably from other connection failures retry them. + +Attempt budget per SDK — disabling retry (each SDK's spelling of `enable_retry=false` or a zero cap) yields exactly ONE hop-1 attempt: + +| SDK | Budget | +|-----|--------| +| 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) | +| 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. | + +Python and Ruby carve downloads out of their ungoverned GET taxonomy (which retries 500): the download hop uses the declared `{429, 502, 503, 504}` set, in both directions — the taxonomy neither widens nor vetoes it. `DownloadURL` is deliberately absent from `behavior-model.json`; SDKs pass this policy to their retry primitive directly rather than looking it up by operation. + ### DownloadResult RECORD ``` @@ -1666,9 +1686,7 @@ logic is covered by `TestIsSameOrigin` unit tests: - "Mixed-case host and explicit default port stay on the mocked origin" — Go runner dials `configOverrides.baseUrl` directly; its `httptest` mock owns its origin, so origin-interception normalization does not apply. - "Bracketed IPv6 loopback origin stays on the mocked origin" — same as above. -**Python** (`conformance/runner/python/runner.py` `SKIPS`) — unwaivered: -- "DownloadURL retries on 503 at the auth'd first hop" — download path uses `get_no_retry`; hop-1 retry not implemented. -- "DownloadURL honors Retry-After on 429 at the auth'd first hop" — same as above. +**Python** (`conformance/runner/python/runner.py` `SKIPS`) — none. **Ruby** (`conformance/runner/ruby/runner.rb` `RUBY_SKIPS`): - "PUT operation is naturally idempotent" — GET-only retry (waiver 2B.3). @@ -1682,19 +1700,13 @@ logic is covered by `TestIsSameOrigin` unit tests: - "PrioritizeAssignment POST retries when marked idempotent" — GET-only retry (waiver 2B.3). - "DeprioritizeAssignment DELETE retries when marked idempotent" — GET-only retry (waiver 2B.3). - "Network error on an idempotent POST is retried then succeeds" — GET-only network retry (waiver 2B.3). -- "DownloadURL retries on 503 at the auth'd first hop" — download path uses `get_no_retry`; hop-1 retry not implemented (unwaivered). -- "DownloadURL honors Retry-After on 429 at the auth'd first hop" — same as above (unwaivered). **TypeScript** (`conformance/runner/typescript/runner.test.ts` `TS_SDK_SKIPS`): - "Large integer IDs preserved without precision loss" — `Number` is 53-bit (waiver 1B.6). -- "DownloadURL retries on 503 at the auth'd first hop" — `downloadURL` uses raw fetch bypassing retry (unwaivered). -- "DownloadURL honors Retry-After on 429 at the auth'd first hop" — same as above (unwaivered). -**Kotlin** (`kotlin/conformance/.../Main.kt` — `KOTLIN_SKIPS` plus one tag-based -branch ahead of it): +**Kotlin** (`kotlin/conformance/.../Main.kt` — one tag-based branch; `KOTLIN_SKIPS` +is empty): - "List operation returns first page with Link header" — skipped via the `link-header` tag branch, not `KOTLIN_SKIPS`: Kotlin auto-paginates by design, so a first-page-only requestCount assertion is inapplicable (architectural). -- "DownloadURL retries on 503 at the auth'd first hop" — Kotlin download hop 1 does not retry yet (B4) (unwaivered). -- "DownloadURL honors Retry-After on 429 at the auth'd first hop" — same as above (unwaivered). The TypeScript live canary additionally reports one placeholder skip when `BASECAMP_LIVE` is unset (`live-runner.test.ts`) — that is the opt-in gate for @@ -1901,6 +1913,8 @@ account, attachments, automation, boosts, campfires, cardColumns, cardSteps, car | `downloads.json` | DownloadURL auth'd first hop 302s to signed URL | §14 | | `downloads.json` | DownloadURL direct 2xx body | §14 | | `downloads.json` | DownloadURL retries on 503 at the auth'd first hop | §14, §7 | +| `downloads.json` | DownloadURL retries hop 1 on a network error | §14, §7 | +| `downloads.json` | DownloadURL does not retry hop 1 on 500 | §14, §7 | | `downloads.json` | DownloadURL honors Retry-After on 429 at the auth'd first hop | §14, §7 | | `downloads.json` | DownloadURL surfaces redirect with no Location | §14 | | `network-retry.json` | Network error on a non-idempotent POST is not retried | §7 (Gate 2) | diff --git a/conformance/runner/go/main.go b/conformance/runner/go/main.go index c0c27a2de4..3f99b2b549 100644 --- a/conformance/runner/go/main.go +++ b/conformance/runner/go/main.go @@ -943,11 +943,19 @@ func checkAssertion( } case "delayBetweenRequests": + // index selects a single inter-request GAP (gap i is between request i + // and i+1); it defaults to the first. A named gap that does not exist + // is a failure, not a silent pass — otherwise a dropped retry makes the + // assertion vanish instead of firing. if len(requestTimes) >= 2 { - delay := requestTimes[1].Sub(requestTimes[0]) + gap := assertionIndex(assertion) + if gap+1 >= len(requestTimes) { + return fail(tc, fmt.Sprintf("Expected a delay at gap %d, but only %d request(s) were made", gap, len(requestTimes))) + } + delay := requestTimes[gap+1].Sub(requestTimes[gap]) minDelay := time.Duration(assertion.Min) * time.Millisecond if delay < minDelay { - return fail(tc, fmt.Sprintf("Expected delay >= %v, got %v", minDelay, delay)) + return fail(tc, fmt.Sprintf("Expected delay >= %v at gap %d, got %v", minDelay, gap, delay)) } } diff --git a/conformance/runner/python/runner.py b/conformance/runner/python/runner.py index fb64e27495..29bad7bc6f 100644 --- a/conformance/runner/python/runner.py +++ b/conformance/runner/python/runner.py @@ -429,10 +429,22 @@ def _verify_assertions(self, *, result: Any, error: Exception | None) -> TestRes failures.append(f"Expected {expected} requests, got {actual}") case "delayBetweenRequests": + # Every inter-request gap must clear the minimum, unless the + # fixture names one: not all gaps are retry gaps — the + # download flow's final gap is the redirect hop to the + # signed URL, which is deliberately un-delayed — so those + # fixtures assert per-gap with an index. delays = self._tracker.delays_between_requests min_delay = assertion.get("min") - if min_delay and delays and any(d < min_delay for d in delays): - failures.append(f"Expected minimum delay of {min_delay}ms, got {min(delays)}ms") + index = assertion.get("index") + if min_delay and delays: + if index is not None: + if index < len(delays) and delays[index] < min_delay: + failures.append( + f"Expected minimum delay of {min_delay}ms at gap {index}, got {delays[index]}ms" + ) + elif any(d < min_delay for d in delays): + failures.append(f"Expected minimum delay of {min_delay}ms, got {min(delays)}ms") case "noError": if error: @@ -673,15 +685,8 @@ def _get_error_field(error: Exception, field_path: str) -> Any: class ConformanceRunner: - _DOWNLOAD_RETRY_SKIP = "Python SDK download path uses get_no_retry; retry on 5xx / Retry-After is not implemented" - SKIPS: set[str] = { - "DownloadURL retries on 503 at the auth'd first hop", - "DownloadURL honors Retry-After on 429 at the auth'd first hop", - } - SKIP_REASONS: dict[str, str] = { - "DownloadURL retries on 503 at the auth'd first hop": _DOWNLOAD_RETRY_SKIP, - "DownloadURL honors Retry-After on 429 at the auth'd first hop": _DOWNLOAD_RETRY_SKIP, - } + SKIPS: set[str] = set() + SKIP_REASONS: dict[str, str] = {} def __init__(self, tests_dir: str): self._tests_dir = Path(tests_dir) diff --git a/conformance/runner/ruby/runner.rb b/conformance/runner/ruby/runner.rb index 3193e65962..37d0c16efc 100644 --- a/conformance/runner/ruby/runner.rb +++ b/conformance/runner/ruby/runner.rb @@ -293,12 +293,9 @@ def todo_write_kwargs(body) "UpdateCalendar PUT retries when marked idempotent", "PrioritizeAssignment POST retries when marked idempotent", "DeprioritizeAssignment DELETE retries when marked idempotent", - "DownloadURL retries on 503 at the auth'd first hop", - "DownloadURL honors Retry-After on 429 at the auth'd first hop", "Network error on an idempotent POST is retried then succeeds", ].freeze) -DOWNLOAD_RETRY_SKIP = "Ruby SDK download path uses http.get_no_retry; retry on 5xx / Retry-After is not implemented".freeze RUBY_SKIP_REASONS = { "PUT operation is naturally idempotent" => "Ruby SDK only retries GET", "DELETE operation is naturally idempotent" => "Ruby SDK only retries GET", @@ -310,8 +307,6 @@ def todo_write_kwargs(body) "UpdateCalendar PUT retries when marked idempotent" => "Ruby SDK only retries GET", "PrioritizeAssignment POST retries when marked idempotent" => "Ruby SDK only retries GET", "DeprioritizeAssignment DELETE retries when marked idempotent" => "Ruby SDK only retries GET", - "DownloadURL retries on 503 at the auth'd first hop" => DOWNLOAD_RETRY_SKIP, - "DownloadURL honors Retry-After on 429 at the auth'd first hop" => DOWNLOAD_RETRY_SKIP, "Network error on an idempotent POST is retried then succeeds" => "Ruby SDK only retries GET network errors; mutations go through single_request with no retry", }.freeze @@ -531,10 +526,22 @@ def verify_assertions(result:, error:) end when "delayBetweenRequests" + # Every inter-request gap must clear the minimum, unless the fixture + # names one: not all gaps are retry gaps — the download flow's final + # gap is the redirect hop to the signed URL, which is deliberately + # un-delayed — so those fixtures assert per-gap with an index. delays = @tracker.delays_between_requests min_delay = assertion["min"] - if min_delay && delays.any? { |d| d < min_delay } - failures << "Expected minimum delay of #{min_delay}ms, got #{delays.min}ms" + index = assertion["index"] + if min_delay && delays.any? + if index + gap = delays[index] + if gap && gap < min_delay + failures << "Expected minimum delay of #{min_delay}ms at gap #{index}, got #{gap}ms" + end + elsif delays.any? { |d| d < min_delay } + failures << "Expected minimum delay of #{min_delay}ms, got #{delays.min}ms" + end end when "noError" diff --git a/conformance/runner/typescript/runner.test.ts b/conformance/runner/typescript/runner.test.ts index c9b6ac000f..9f6256f281 100644 --- a/conformance/runner/typescript/runner.test.ts +++ b/conformance/runner/typescript/runner.test.ts @@ -79,10 +79,6 @@ const TEST_ACCOUNT_ID = "999"; const TS_SDK_SKIPS: Record = { "Large integer IDs preserved without precision loss": "JavaScript loses precision on integers > Number.MAX_SAFE_INTEGER (2^53)", - "DownloadURL retries on 503 at the auth'd first hop": - "TS SDK downloadURL uses raw fetch bypassing the retry middleware; 5xx / Retry-After retry is not implemented", - "DownloadURL honors Retry-After on 429 at the auth'd first hop": - "TS SDK downloadURL uses raw fetch bypassing the retry middleware; 5xx / Retry-After retry is not implemented", }; /** @@ -804,7 +800,15 @@ function checkAssertions( case "delayBetweenRequests": { const times = tracker.requestTimes(); if (times.length >= 2) { - const delay = times[1]! - times[0]!; + // index selects a single inter-request GAP (gap i is between request + // i and i+1), defaulting to the first. A named gap that does not + // exist fails rather than passing silently. + const gap = assertion.index ?? 0; + expect( + times.length, + `[${tc.name}] expected a delay at gap ${gap}, but only ${times.length} request(s) were made`, + ).toBeGreaterThan(gap + 1); + const delay = times[gap + 1]! - times[gap]!; const minDelay = assertion.min ?? 0; // Node's timers may fire marginally BEFORE the requested delay — // libuv rounds the deadline down internally, so a 2000ms sleep can @@ -818,7 +822,7 @@ function checkAssertions( // interval (no delay at all). expect( delay, - `[${tc.name}] expected delay >= ${minDelay}ms (allowing ${TIMER_SLACK_MS}ms timer slack), got ${delay}ms`, + `[${tc.name}] expected delay >= ${minDelay}ms at gap ${gap} (allowing ${TIMER_SLACK_MS}ms timer slack), got ${delay}ms`, ).toBeGreaterThanOrEqual(minDelay - TIMER_SLACK_MS); } break; @@ -1205,11 +1209,16 @@ function shouldEnableRetry(tc: TestCase, filename: string): boolean { if ( filename === "retry.json" || filename === "idempotency.json" || - filename === "network-retry.json" + filename === "network-retry.json" || + filename === "downloads.json" ) { // network-retry.json's CreateTodo safety case must run retry-ENABLED so it // actually proves the SDK doesn't re-send a non-idempotent POST on a network // error (with retry off, the requestCount:1 assertion would be vacuous). + // downloads.json exercises the hop-1 retry policy (SPEC §14), and its + // no-retry cases (500, redirect-no-Location) stay single-attempt because + // those failures are outside the declared retry set, not because retry is + // disabled. return true; } diff --git a/conformance/schema.json b/conformance/schema.json index e4ba77d6e9..d64d40ef51 100644 --- a/conformance/schema.json +++ b/conformance/schema.json @@ -159,7 +159,7 @@ }, "index": { "type": "integer", - "description": "Request index for per-request assertions: headerPresent / headerAbsent / headerInjected / requestPath / requestMethod / requestBody / requestBodyAbsent (0-based; negative values index from the end, e.g. -1 = last request). Default 0." + "description": "Request index for per-request assertions: headerPresent / headerAbsent / headerInjected / requestPath / requestMethod / requestBody / requestBodyAbsent (0-based; negative values index from the end, e.g. -1 = last request). Default 0. For delayBetweenRequests it selects a single inter-request GAP (gap i is between request i and i+1); omit it to require the minimum on every gap." } } } diff --git a/conformance/tests/downloads.json b/conformance/tests/downloads.json index cc6ba7411c..418f362b2a 100644 --- a/conformance/tests/downloads.json +++ b/conformance/tests/downloads.json @@ -35,7 +35,7 @@ }, { "name": "DownloadURL retries on 503 at the auth'd first hop", - "description": "The first hop is a GET and must follow SDK-wide GET retry semantics (spec 2A.2): retry on 5xx with exponential backoff. After two 503s, the third attempt returns 302 and hop 2 completes.", + "description": "The authenticated first hop follows the download hop-1 retry policy (SPEC 14): network errors plus {429, 502, 503, 504} with exponential backoff — never 500. After two 503s, the third attempt returns 302 and hop 2 completes. Every hop-1 attempt carries Authorization; the signed hop 2 carries none.", "operation": "DownloadURL", "method": "GET", "path": "/999999999/blobs/abcd1234/download/logo.png", @@ -47,13 +47,53 @@ ], "assertions": [ {"type": "requestCount", "expected": 4}, - {"type": "delayBetweenRequests", "min": 1000}, + {"type": "delayBetweenRequests", "min": 1000, "index": 0}, + {"type": "delayBetweenRequests", "min": 1000, "index": 1}, {"type": "noError"}, {"type": "headerPresent", "path": "Authorization", "index": 0}, + {"type": "headerPresent", "path": "Authorization", "index": 1}, + {"type": "headerPresent", "path": "Authorization", "index": 2}, {"type": "headerAbsent", "path": "Authorization", "index": -1} ], "tags": ["download", "retry", "503"] }, + { + "name": "DownloadURL retries hop 1 on a network error", + "description": "A transport-level failure on the authenticated first hop is retried under the download hop-1 retry policy (SPEC 14). The retried attempt still carries Authorization, succeeds with 302, and hop 2 completes unauthenticated.", + "operation": "DownloadURL", + "method": "GET", + "path": "/999999999/blobs/abcd1234/download/logo.png", + "mockResponses": [ + {"networkError": true}, + {"status": 302, "headers": {"Location": "/signed/logo.png"}}, + {"status": 200, "headers": {"Content-Type": "image/png"}, "body": "pixels"} + ], + "assertions": [ + {"type": "requestCount", "expected": 3}, + {"type": "delayBetweenRequests", "min": 1000, "index": 0}, + {"type": "noError"}, + {"type": "headerPresent", "path": "Authorization", "index": 0}, + {"type": "headerPresent", "path": "Authorization", "index": 1}, + {"type": "headerAbsent", "path": "Authorization", "index": -1} + ], + "tags": ["download", "retry", "network"] + }, + { + "name": "DownloadURL does not retry hop 1 on 500", + "description": "500 is deliberately outside the download hop-1 retry set {429, 502, 503, 504} (SPEC 14): a single attempt surfaces the API error with no retry.", + "operation": "DownloadURL", + "method": "GET", + "path": "/999999999/blobs/abcd1234/download/logo.png", + "mockResponses": [ + {"status": 500, "body": {"error": "Internal server error"}} + ], + "assertions": [ + {"type": "requestCount", "expected": 1}, + {"type": "statusCode", "expected": 500}, + {"type": "headerPresent", "path": "Authorization", "index": 0} + ], + "tags": ["download", "no-retry", "500"] + }, { "name": "DownloadURL honors Retry-After on 429 at the auth'd first hop", "description": "429 Too Many Requests with Retry-After: 1 pauses for at least one second before retry. Retry succeeds with 302 → 200 body.", @@ -67,7 +107,7 @@ ], "assertions": [ {"type": "requestCount", "expected": 3}, - {"type": "delayBetweenRequests", "min": 1000}, + {"type": "delayBetweenRequests", "min": 1000, "index": 0}, {"type": "noError"}, {"type": "headerPresent", "path": "Authorization", "index": 0}, {"type": "headerAbsent", "path": "Authorization", "index": -1} diff --git a/go/pkg/basecamp/download_test.go b/go/pkg/basecamp/download_test.go index 49d8c62f5e..2f73776ca8 100644 --- a/go/pkg/basecamp/download_test.go +++ b/go/pkg/basecamp/download_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "sync/atomic" "testing" "time" @@ -859,6 +860,159 @@ func TestDownloadURL_AuthHopRetriesOnNetworkError(t *testing.T) { } } +// TestDownloadURL_AuthHopDeclaredRetrySet pins the COMPLETE declared hop-1 +// retry set (SPEC §14) status by status. The 503 and 429 cases above assert the +// timing properties (backoff growth, Retry-After); this table asserts +// membership, including the carve-out: 500 sits deliberately outside the set, +// so it is attempted exactly once and surfaces as a non-retryable API error. +func TestDownloadURL_AuthHopDeclaredRetrySet(t *testing.T) { + tests := []struct { + name string + status int + wantAttempts int32 + wantErr bool + }{ + {"429 is retried", http.StatusTooManyRequests, 2, false}, + {"502 is retried", http.StatusBadGateway, 2, false}, + {"503 is retried", http.StatusServiceUnavailable, 2, false}, + {"504 is retried", http.StatusGatewayTimeout, 2, false}, + {"500 is outside the set", http.StatusInternalServerError, 1, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fileContent := "declared-set-ok" + var attempts atomic.Int32 + + s3Server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/pdf") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(fileContent)) + })) + defer s3Server.Close() + + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if attempts.Add(1) == 1 { + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(`{"error":"transient"}`)) + return + } + w.Header().Set("Location", s3Server.URL+"/bucket/file.pdf") + w.WriteHeader(http.StatusFound) + })) + defer apiServer.Close() + + cfg := DefaultConfig() + cfg.BaseURL = apiServer.URL + token := &StaticTokenProvider{Token: "test-token"} + client := NewClient(cfg, token, + WithMaxRetries(3), + WithBaseDelay(time.Millisecond), + WithMaxJitter(time.Millisecond), + WithTransport(http.DefaultTransport), + ) + ac := client.ForAccount("12345") + + result, err := ac.DownloadURL(context.Background(), + "https://storage.3.basecamp.com/999/blobs/abc/download/doc.pdf") + + if tt.wantErr { + if err == nil { + result.Body.Close() + t.Fatalf("expected an error for %d", tt.status) + } + var sdkErr *Error + if !isSDKError(err, &sdkErr) || sdkErr.Retryable { + t.Errorf("expected a non-retryable SDK error, got: %v", err) + } + } else { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer result.Body.Close() + body, _ := io.ReadAll(result.Body) + if string(body) != fileContent { + t.Errorf("expected body %q, got %q", fileContent, string(body)) + } + } + + if got := attempts.Load(); got != tt.wantAttempts { + t.Errorf("expected %d attempts, got %d", tt.wantAttempts, got) + } + }) + } +} + +// TestDownloadURL_AuthHopAuthOnEveryAttemptNeverOnHop2 pins the credential +// boundary across a retry: every authenticated-hop attempt carries +// Authorization — the retried one included, since each attempt re-runs the auth +// strategy — and the signed hop never does. +func TestDownloadURL_AuthHopAuthOnEveryAttemptNeverOnHop2(t *testing.T) { + var mu sync.Mutex + var hop1Auth []string + var hop2Auth []string + var attempts atomic.Int32 + + s3Server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + hop2Auth = append(hop2Auth, r.Header.Get("Authorization")) + mu.Unlock() + w.Header().Set("Content-Type", "application/pdf") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("data")) + })) + defer s3Server.Close() + + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + hop1Auth = append(hop1Auth, r.Header.Get("Authorization")) + mu.Unlock() + if attempts.Add(1) == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.Header().Set("Location", s3Server.URL+"/bucket/file.pdf") + w.WriteHeader(http.StatusFound) + })) + defer apiServer.Close() + + cfg := DefaultConfig() + cfg.BaseURL = apiServer.URL + token := &StaticTokenProvider{Token: "test-token"} + client := NewClient(cfg, token, + WithMaxRetries(3), + WithBaseDelay(time.Millisecond), + WithMaxJitter(time.Millisecond), + WithTransport(http.DefaultTransport), + ) + ac := client.ForAccount("12345") + + result, err := ac.DownloadURL(context.Background(), + "https://storage.3.basecamp.com/999/blobs/abc/download/doc.pdf") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer result.Body.Close() + io.Copy(io.Discard, result.Body) + + mu.Lock() + defer mu.Unlock() + if len(hop1Auth) != 2 { + t.Fatalf("expected 2 authenticated-hop attempts, got %d", len(hop1Auth)) + } + for i, auth := range hop1Auth { + if auth != "Bearer test-token" { + t.Errorf("hop-1 attempt %d: expected %q, got %q", i+1, "Bearer test-token", auth) + } + } + if len(hop2Auth) != 1 { + t.Fatalf("expected 1 signed-hop request, got %d", len(hop2Auth)) + } + if hop2Auth[0] != "" { + t.Errorf("expected no Authorization header on the signed hop, got %q", hop2Auth[0]) + } +} + func TestDownloadURL_AuthHopNoRetryOn404(t *testing.T) { var attempts atomic.Int32 diff --git a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt index 05ee6d2fb3..9c28be7160 100644 --- a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt +++ b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt @@ -18,10 +18,7 @@ import java.util.concurrent.atomic.AtomicInteger private const val TEST_ACCOUNT_ID = "999" /** Tests where the Kotlin runner's operation dispatcher has no implementation yet. */ -private val KOTLIN_SKIPS: Map = mapOf( - "DownloadURL retries on 503 at the auth'd first hop" to "Kotlin download hop 1 does not retry yet (B4)", - "DownloadURL honors Retry-After on 429 at the auth'd first hop" to "Kotlin download hop 1 does not retry yet (B4)", -) +private val KOTLIN_SKIPS: Map = emptyMap() fun main() { val testsDir = File("../conformance/tests") @@ -414,10 +411,18 @@ private fun runTest(tc: TestCase): TestResult { "delayBetweenRequests" -> { if (requestTimes.size >= 2) { - val delay = requestTimes[1] - requestTimes[0] + // index selects a single inter-request GAP (gap i is + // between request i and i+1), defaulting to the first. A + // named gap that does not exist fails rather than passing + // silently. + val gap = assertion.index + if (gap + 1 >= requestTimes.size) { + return TestResult(false, "Expected a delay at gap $gap, but only ${requestTimes.size} request(s) were made") + } + val delay = requestTimes[gap + 1] - requestTimes[gap] val minDelay = assertion.min.toLong() if (delay < minDelay) { - return TestResult(false, "Expected delay >= ${minDelay}ms, got ${delay}ms") + return TestResult(false, "Expected delay >= ${minDelay}ms at gap $gap, got ${delay}ms") } } } diff --git a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt index fd91d63e97..893c0fe2bc 100644 --- a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt +++ b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt @@ -1,13 +1,27 @@ package com.basecamp.sdk +import com.basecamp.sdk.http.AuthPhaseFailure +import com.basecamp.sdk.http.BasecampHttpClient import com.basecamp.sdk.http.currentTimeMillis import com.basecamp.sdk.http.millisToDuration +import com.basecamp.sdk.http.safeOnRequestEnd +import com.basecamp.sdk.http.safeOnRequestStart +import com.basecamp.sdk.http.safeOnRetry import io.ktor.client.* +import io.ktor.client.plugins.HttpRequestTimeoutException import io.ktor.client.plugins.HttpTimeout import io.ktor.client.request.* import io.ktor.client.statement.* import io.ktor.http.* import kotlin.coroutines.cancellation.CancellationException +import kotlinx.coroutines.delay + +/** + * SPEC §14's declared hop-1 retry set for downloads — never 500, which stays + * aligned with the main GET loop's declared-set discipline rather than the + * error taxonomy's broader "all 5xx retryable" flag. + */ +private val DOWNLOAD_RETRY_ON = setOf(429, 502, 503, 504) /** * Result of downloading file content from a URL. @@ -71,6 +85,12 @@ fun filenameFromURL(rawURL: String): String { * authenticated first hop (which typically 302s to a signed download URL), * and unauthenticated second hop to fetch the actual file content. * + * The first hop retries under the SPEC §14 policy — network errors plus + * {429, 502, 503, 504}, never 500 — with exponential backoff (Retry-After + * honored on 429) under the public maxRetries total-attempt cap coerced to at + * least one; `enableRetry = false` collapses it to exactly one attempt. The + * second hop is exempt: no retry, no auth. + * * @param rawURL Absolute download URL (e.g., from bc-attachment elements). * @return [DownloadResult] with body, contentType, contentLength, and filename. * @throws BasecampException.Usage if rawURL is blank or not absolute. @@ -120,38 +140,14 @@ suspend fun AccountClient.downloadURL(rawURL: String): DownloadResult { } noRedirectClient.use { client -> - // Hop 1: Authenticated API request (capture redirect) - val requestInfo = RequestInfo(method = "GET", url = rewrittenURL, attempt = 1) - parent.hooks.safeOnRequestStart(requestInfo) - - val reqStart = currentTimeMillis() - val response: HttpResponse - try { - response = client.request(rewrittenURL) { - method = HttpMethod.Get - parent.authStrategy.authenticate(this) - header(HttpHeaders.UserAgent, parent.config.userAgent) - } - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - val duration = currentTimeMillis() - reqStart - parent.hooks.safeOnRequestEnd(requestInfo, RequestResult( - statusCode = 0, - duration = duration.millisToDuration(), - error = e, - )) - throw BasecampException.Network( - message = "Network error: ${e.message}", - cause = e, - ) - } - - val duration = currentTimeMillis() - reqStart - parent.hooks.safeOnRequestEnd(requestInfo, RequestResult( - statusCode = response.status.value, - duration = duration.millisToDuration(), - )) + // Hop 1: Authenticated API request (capture redirect) under the + // SPEC §14 hop-1 retry policy. The budget is the public maxRetries + // total-attempt cap coerced to at least one (an accepted + // maxRetries = 0 still sends one attempt), collapsed to exactly + // one attempt when enableRetry is false. + val maxAttempts = if (parent.config.enableRetry) parent.config.maxRetries.coerceAtLeast(1) else 1 + val baseDelayMs = parent.config.baseRetryDelay.inWholeMilliseconds + val response = downloadHop1(client, rewrittenURL, maxAttempts, baseDelayMs) val status = response.status.value @@ -245,6 +241,114 @@ suspend fun AccountClient.downloadURL(rawURL: String): DownloadResult { } } +/** + * Runs the download's authenticated hop 1 under the SPEC §14 retry policy: + * network errors plus [DOWNLOAD_RETRY_ON] — never 500 — retried with + * exponential backoff (Retry-After honored on 429) while attempts remain. + * DownloadURL is deliberately absent from the behavior model, so the policy + * lives here rather than being looked up by operation. + * + * Mirror of [BasecampHttpClient.requestWithRetry]'s directive shape (#517): + * the catch clauses only classify the attempt's outcome; retry side effects + * (on_retry, the backoff sleep, the next attempt) run outside any catch, so a + * CancellationException from the sleep propagates raw and no phantom request + * events fire for an attempt that already ended. + * + * Every attempt re-runs the auth strategy so a rotated token is picked up. A + * throwing strategy is a configuration or credential-provider fault, not a + * transport fault: it is tagged with the shared [AuthPhaseFailure], surfaces + * raw, and spends no retry budget — the same classification + * [BasecampHttpClient] applies. + */ +private suspend fun AccountClient.downloadHop1( + client: HttpClient, + url: String, + maxAttempts: Int, + baseDelayMs: Long, +): HttpResponse { + var attempt = 1 + while (true) { + val requestInfo = RequestInfo(method = "GET", url = url, attempt = attempt) + parent.hooks.safeOnRequestStart(requestInfo) + val reqStart = currentTimeMillis() + + var failure: Exception? = null + var attemptResponse: HttpResponse? = null + try { + attemptResponse = client.request(url) { + method = HttpMethod.Get + try { + parent.authStrategy.authenticate(this) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + throw AuthPhaseFailure(e) + } + header(HttpHeaders.UserAgent, parent.config.userAgent) + } + } catch (e: CancellationException) { + throw e + } catch (e: AuthPhaseFailure) { + val duration = currentTimeMillis() - reqStart + parent.hooks.safeOnRequestEnd(requestInfo, RequestResult( + statusCode = 0, + duration = duration.millisToDuration(), + error = e.original, + )) + throw e.original + } catch (e: Exception) { + failure = e + } + + if (failure != null) { + val duration = currentTimeMillis() - reqStart + parent.hooks.safeOnRequestEnd(requestInfo, RequestResult( + statusCode = 0, + duration = duration.millisToDuration(), + error = failure, + )) + val wrapped = BasecampException.Network( + message = "Network error: ${failure.message}", + cause = failure, + ) + // Same total-budget carve-out as the client loop: an attempt that + // consumed its entire per-attempt time budget is a slowness shape + // a retry tends to repeat, not a transient blip. + val retryableFailure = failure !is HttpRequestTimeoutException + if (!retryableFailure || attempt >= maxAttempts) { + throw wrapped + } + val delayMs = BasecampHttpClient.calculateBackoffDelay(baseDelayMs, attempt) + parent.hooks.safeOnRetry(requestInfo, attempt + 1, wrapped, delayMs) + delay(delayMs) + attempt += 1 + continue + } + + val response = attemptResponse!! + val duration = currentTimeMillis() - reqStart + parent.hooks.safeOnRequestEnd(requestInfo, RequestResult( + statusCode = response.status.value, + duration = duration.millisToDuration(), + )) + + val status = response.status.value + if (status !in DOWNLOAD_RETRY_ON || attempt >= maxAttempts) { + return response + } + + val retryAfter = parseRetryAfter(response.headers["Retry-After"]) + val delayMs = if (status == 429 && retryAfter != null) { + retryAfter.toLong() * 1000 + } else { + BasecampHttpClient.calculateBackoffDelay(baseDelayMs, attempt) + } + parent.hooks.safeOnRetry(requestInfo, attempt + 1, BasecampException.Api("HTTP $status", status), delayMs) + delay(delayMs) + attempt += 1 + } +} + /** * Rewrites a URL's origin (scheme + host + port) to match the base URL, * preserving the path, query, and fragment. @@ -293,13 +397,3 @@ private fun BasecampHooks.safeOnOperationStart(info: OperationInfo) { private fun BasecampHooks.safeOnOperationEnd(info: OperationInfo, result: OperationResult) { runCatching { onOperationEnd(info, result) } } - -/** Safely call onRequestStart, catching hook exceptions. */ -private fun BasecampHooks.safeOnRequestStart(info: RequestInfo) { - runCatching { onRequestStart(info) } -} - -/** Safely call onRequestEnd, catching hook exceptions. */ -private fun BasecampHooks.safeOnRequestEnd(info: RequestInfo, result: RequestResult) { - runCatching { onRequestEnd(info, result) } -} diff --git a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt index b6330691ef..371904decb 100644 --- a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt +++ b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt @@ -355,24 +355,30 @@ private sealed interface AttemptOutcome { /** * Internal tag for an exception thrown by the auth strategy while building an - * attempt. Never escapes [BasecampHttpClient]: the retry entry points unwrap - * it and rethrow [original] raw, so auth-phase faults are never classified as - * transport failures and never consume retry budget. + * attempt. Never escapes the loop that raised it: every retry entry point — + * [BasecampHttpClient]'s and the download hop-1 loop's — unwraps it and + * rethrows [original] raw, so auth-phase faults are never classified as + * transport failures and never consume retry budget. Module-internal so the + * classification lives in one place rather than being re-declared per hop. */ -private class AuthPhaseFailure(val original: Exception) : Exception(original) +internal class AuthPhaseFailure(val original: Exception) : Exception(original) -/** Safely call onRequestStart, catching hook exceptions. */ -private fun BasecampHooks.safeOnRequestStart(info: RequestInfo) { +/** + * Safely call onRequestStart, catching hook exceptions. Module-internal so the + * download hop-1 loop shares it — a hook that throws must be swallowed the + * same way on both request paths. + */ +internal fun BasecampHooks.safeOnRequestStart(info: RequestInfo) { runCatching { onRequestStart(info) } } /** Safely call onRequestEnd, catching hook exceptions. */ -private fun BasecampHooks.safeOnRequestEnd(info: RequestInfo, result: RequestResult) { +internal fun BasecampHooks.safeOnRequestEnd(info: RequestInfo, result: RequestResult) { runCatching { onRequestEnd(info, result) } } /** Safely call onRetry, catching hook exceptions. */ -private fun BasecampHooks.safeOnRetry(info: RequestInfo, attempt: Int, error: Throwable, delayMs: Long) { +internal fun BasecampHooks.safeOnRetry(info: RequestInfo, attempt: Int, error: Throwable, delayMs: Long) { runCatching { onRetry(info, attempt, error, delayMs) } } diff --git a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt index 1f44ced13d..f16709260b 100644 --- a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt +++ b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt @@ -12,6 +12,7 @@ class DownloadTest { handler: MockRequestHandler, hooks: BasecampHooks = NoopHooks, enableRetry: Boolean = false, + maxRetries: Int? = null, ): BasecampClient { val mockEngine = MockEngine(handler) return testBasecampClient { @@ -20,6 +21,9 @@ class DownloadTest { engine = mockEngine this.enableRetry = enableRetry this.hooks = hooks + if (maxRetries != null) { + this.maxRetries = maxRetries + } } } @@ -464,30 +468,325 @@ class DownloadTest { client.close() } - // -- No retry on 429 -- + // -- Hop-1 retry policy (SPEC §14) -- + + private val hop1URL = "http://localhost:3000/12345/attachments/abc/download/file.txt" + + // The COMPLETE declared retry set, pinned status by status (the shared + // conformance fixtures cover 429/503): hop 1 retries {429, 502, 503, 504} + // under the public maxRetries total-attempt cap coerced to at least one. + @Test + fun downloadURL_retriesDeclaredStatusesThenFollowsRedirect() = runTest { + for (status in listOf(429, 502, 503, 504)) { + var hop1Attempts = 0 + var hop2Requests = 0 + val client = mockClient( + handler = { request -> + if (request.url.encodedPath.startsWith("/signed/")) { + hop2Requests++ + respond( + content = ByteReadChannel("data"), + status = HttpStatusCode.OK, + headers = headersOf(HttpHeaders.ContentType to listOf("application/octet-stream")), + ) + } else { + hop1Attempts++ + if (hop1Attempts == 1) { + respond( + content = ByteReadChannel("{}"), + status = HttpStatusCode.fromValue(status), + headers = headersOf(HttpHeaders.ContentType to listOf("application/json")), + ) + } else { + respond( + content = ByteReadChannel(""), + status = HttpStatusCode.Found, + headers = headersOf(HttpHeaders.Location to listOf("http://localhost:3000/signed/file")), + ) + } + } + }, + enableRetry = true, + ) + val account = client.forAccount("12345") + + val result = account.downloadURL(hop1URL) + + assertEquals("data", result.body.decodeToString(), "status $status") + assertEquals(2, hop1Attempts, "status $status") + assertEquals(1, hop2Requests, "status $status") + client.close() + } + } @Test - fun downloadURL_noRetryOn429() = runTest { + fun downloadURL_neverRetries500() = runTest { var requestCount = 0 val client = mockClient( handler = { _ -> requestCount++ respond( - content = ByteReadChannel("""{"error":"Rate limited"}"""), - status = HttpStatusCode.TooManyRequests, - headers = headersOf( - HttpHeaders.ContentType to listOf("application/json"), - "Retry-After" to listOf("30"), - ) + content = ByteReadChannel("""{"error":"Server error"}"""), + status = HttpStatusCode.InternalServerError, + headers = headersOf(HttpHeaders.ContentType to listOf("application/json")), ) }, - enableRetry = true, // Retry is on but downloadURL shouldn't use it + enableRetry = true, ) val account = client.forAccount("12345") - assertFailsWith { - account.downloadURL("http://localhost:3000/12345/attachments/abc/download/file.txt") + + assertFailsWith { account.downloadURL(hop1URL) } + + // 500 is deliberately outside the declared set {429, 502, 503, 504}. + assertEquals(1, requestCount) + client.close() + } + + @Test + fun downloadURL_retriesNetworkErrorThenSucceeds() = runTest { + var requestCount = 0 + val client = mockClient( + handler = { _ -> + requestCount++ + if (requestCount == 1) { + throw java.io.IOException("Connection refused") + } + respond( + content = ByteReadChannel("content"), + status = HttpStatusCode.OK, + headers = headersOf(HttpHeaders.ContentType to listOf("text/plain")), + ) + }, + enableRetry = true, + ) + val account = client.forAccount("12345") + + val result = account.downloadURL(hop1URL) + + assertEquals("content", result.body.decodeToString()) + assertEquals(2, requestCount) + client.close() + } + + @Test + fun downloadURL_exhaustsCapThenSurfacesError() = runTest { + var requestCount = 0 + val client = mockClient( + handler = { _ -> + requestCount++ + respond( + content = ByteReadChannel("{}"), + status = HttpStatusCode.ServiceUnavailable, + headers = headersOf(HttpHeaders.ContentType to listOf("application/json")), + ) + }, + enableRetry = true, + maxRetries = 3, + ) + val account = client.forAccount("12345") + + assertFailsWith { account.downloadURL(hop1URL) } + + assertEquals(3, requestCount) + client.close() + } + + @Test + fun downloadURL_zeroMaxRetriesStillSendsOneAttempt() = runTest { + var requestCount = 0 + val client = mockClient( + handler = { _ -> + requestCount++ + respond( + content = ByteReadChannel("{}"), + status = HttpStatusCode.ServiceUnavailable, + headers = headersOf(HttpHeaders.ContentType to listOf("application/json")), + ) + }, + enableRetry = true, + maxRetries = 0, + ) + val account = client.forAccount("12345") + + // The download attempt budget is the public cap coerced to at least + // one: an accepted maxRetries = 0 still sends exactly one attempt. + assertFailsWith { account.downloadURL(hop1URL) } + + assertEquals(1, requestCount) + client.close() + } + + @Test + fun downloadURL_enableRetryFalseSendsExactlyOneAttempt() = runTest { + var requestCount = 0 + val client = mockClient( + handler = { _ -> + requestCount++ + respond( + content = ByteReadChannel("{}"), + status = HttpStatusCode.ServiceUnavailable, + headers = headersOf(HttpHeaders.ContentType to listOf("application/json")), + ) + }, + enableRetry = false, + ) + val account = client.forAccount("12345") + + assertFailsWith { account.downloadURL(hop1URL) } + + assertEquals(1, requestCount) + client.close() + } + + @Test + fun downloadURL_authOnEveryHop1AttemptNeverOnHop2() = runTest { + val hop1AuthHeaders = mutableListOf() + var hop2AuthHeader: String? = "unset" + var hop1Attempts = 0 + val client = mockClient( + handler = { request -> + if (request.url.encodedPath.startsWith("/signed/")) { + hop2AuthHeader = request.headers[HttpHeaders.Authorization] + respond( + content = ByteReadChannel("data"), + status = HttpStatusCode.OK, + headers = headersOf(HttpHeaders.ContentType to listOf("application/octet-stream")), + ) + } else { + hop1Attempts++ + hop1AuthHeaders.add(request.headers[HttpHeaders.Authorization]) + if (hop1Attempts == 1) { + respond( + content = ByteReadChannel("{}"), + status = HttpStatusCode.ServiceUnavailable, + headers = headersOf(HttpHeaders.ContentType to listOf("application/json")), + ) + } else { + respond( + content = ByteReadChannel(""), + status = HttpStatusCode.Found, + headers = headersOf(HttpHeaders.Location to listOf("http://localhost:3000/signed/file")), + ) + } + } + }, + enableRetry = true, + ) + val account = client.forAccount("12345") + + account.downloadURL(hop1URL) + + assertEquals(listOf("Bearer test-token", "Bearer test-token"), hop1AuthHeaders) + assertNull(hop2AuthHeader) + client.close() + } + + @Test + fun downloadURL_balancedHooksAcrossRetries() = runTest { + val starts = mutableListOf() + val ends = mutableListOf() + val retries = mutableListOf() + val hooks = object : BasecampHooks { + override fun onRequestStart(info: RequestInfo) { + starts.add(info.attempt) + } + + override fun onRequestEnd(info: RequestInfo, result: RequestResult) { + ends.add(info.attempt) + } + + override fun onRetry(info: RequestInfo, attempt: Int, error: Throwable, delayMs: Long) { + retries.add(attempt) + } } - // Only one request — no retry + var requestCount = 0 + val client = mockClient( + handler = { _ -> + requestCount++ + if (requestCount < 3) { + respond( + content = ByteReadChannel("{}"), + status = HttpStatusCode.ServiceUnavailable, + headers = headersOf(HttpHeaders.ContentType to listOf("application/json")), + ) + } else { + respond( + content = ByteReadChannel("content"), + status = HttpStatusCode.OK, + headers = headersOf(HttpHeaders.ContentType to listOf("text/plain")), + ) + } + }, + hooks = hooks, + enableRetry = true, + ) + val account = client.forAccount("12345") + + account.downloadURL(hop1URL) + + assertEquals(listOf(1, 2, 3), starts) + assertEquals(listOf(1, 2, 3), ends) + // onRetry receives the UPCOMING attempt (SPEC §7 attempt semantics). + assertEquals(listOf(2, 3), retries) + client.close() + } + + @Test + fun downloadURL_honorsRetryAfterOn429() = runTest { + var requestCount = 0 + val requestTimestamps = mutableListOf() + val client = mockClient( + handler = { _ -> + requestCount++ + requestTimestamps.add(testScheduler.currentTime) + if (requestCount == 1) { + respond( + content = ByteReadChannel("{}"), + status = HttpStatusCode.TooManyRequests, + headers = headersOf( + HttpHeaders.ContentType to listOf("application/json"), + "Retry-After" to listOf("7"), + ), + ) + } else { + respond( + content = ByteReadChannel("content"), + status = HttpStatusCode.OK, + headers = headersOf(HttpHeaders.ContentType to listOf("text/plain")), + ) + } + }, + enableRetry = true, + ) + val account = client.forAccount("12345") + + account.downloadURL(hop1URL) + + assertEquals(2, requestCount) + val elapsed = requestTimestamps[1] - requestTimestamps[0] + assertTrue(elapsed >= 7000, "Expected delay >= 7000ms from Retry-After: 7, got $elapsed") + client.close() + } + + @Test + fun downloadURL_cancellationIsTerminal() = runTest { + var requestCount = 0 + val client = mockClient( + handler = { _ -> + requestCount++ + throw kotlin.coroutines.cancellation.CancellationException("cancelled") + }, + enableRetry = true, + ) + val account = client.forAccount("12345") + + // Cancellation is not a transport failure: it must propagate raw with + // NO further attempts, even though a network error would have been + // retried under the same budget. + assertFailsWith { + account.downloadURL(hop1URL) + } + assertEquals(1, requestCount) client.close() } diff --git a/python/src/basecamp/_async_http.py b/python/src/basecamp/_async_http.py index 2797bf8a4a..f6b4e2cdda 100644 --- a/python/src/basecamp/_async_http.py +++ b/python/src/basecamp/_async_http.py @@ -119,9 +119,16 @@ async def request_multipart( return await self._request_with_retry(method, url, files=files, operation=operation) return await self._single_request(method, url, files=files) - async def get_no_retry(self, url: str) -> httpx.Response: + async def get_download(self, url: str) -> httpx.Response: + """Authenticated hop-1 GET for the download flow (SPEC section 14). + + Retries network errors plus the declared DOWNLOAD_RETRY_ON statuses — + never 500 — under the public max_retries total-attempt cap, floored at + one. DownloadURL has no behavior-model entry, so the policy is passed + directly rather than looked up by operation. + """ url = self._build_url(url) - return await self._single_request("GET", url) + return await self._request_with_retry("GET", url, retry_on=self.DOWNLOAD_RETRY_ON, accept=None) async def close(self) -> None: await self._client.aclose() @@ -147,6 +154,8 @@ async def _request_with_retry( files: dict | None = None, allow_cross_origin: bool = False, operation: str | None = None, + retry_on: frozenset[int] | None = None, + accept: str | None = "application/json", ) -> httpx.Response: # max_retries is a TOTAL attempt count (config validation guarantees it # is >= 0). 0 is accepted as a compatibility exception and means a single @@ -172,9 +181,10 @@ async def _request_with_retry( files=files, attempt=attempt, allow_cross_origin=allow_cross_origin, + accept=accept, ) except (RateLimitError, NetworkError, ApiError) as e: - if not self._is_retryable_error(e, operation): + if not self._is_retryable_error(e, operation, retry_on=retry_on): raise last_error = e if attempt >= max_attempts: @@ -207,6 +217,7 @@ async def _single_request( attempt: int = 1, _retry_count: int = 0, allow_cross_origin: bool = False, + accept: str | None = "application/json", ) -> httpx.Response: if not allow_cross_origin and not ( _security.is_localhost(url) or _security.same_origin(url, self._config.base_url) @@ -219,7 +230,7 @@ async def _single_request( start = time.monotonic() try: - headers = self._request_headers() + headers = self._request_headers(accept) if content_type: headers["Content-Type"] = content_type await self._auth.authenticate(headers) @@ -251,6 +262,7 @@ async def _single_request( attempt=attempt, _retry_count=_retry_count + 1, allow_cross_origin=allow_cross_origin, + accept=accept, ) raise error @@ -278,11 +290,14 @@ def _handle_error(self, response: httpx.Response) -> BasecampError: dict(response.headers), ) - def _request_headers(self) -> dict[str, str]: - return { - "User-Agent": self._user_agent, - "Accept": "application/json", - } + def _request_headers(self, accept: str | None = "application/json") -> dict[str, str]: + # accept=None is the binary-download carve-out (SPEC section 14): hop 1 + # sends Authorization and User-Agent only, because it is not a JSON API + # call. Every other caller keeps the JSON Accept. + headers = {"User-Agent": self._user_agent} + if accept is not None: + headers["Accept"] = accept + return headers def _build_url(self, path: str) -> str: # Schemes are case-insensitive (RFC 3986): detect absolute URLs on a @@ -315,12 +330,23 @@ def _is_retryable_operation(self, operation: str) -> bool: # retry_on of its own. DEFAULT_RETRY_ON = frozenset({429, 503}) - def _is_retryable_error(self, error: BasecampError, operation: str | None) -> bool: + # SPEC section 14's declared hop-1 set for downloads: a carve-out from the + # ungoverned GET taxonomy (which retries 500). Authoritative in BOTH + # directions, like an operation's declared retry_on. + DOWNLOAD_RETRY_ON = frozenset({429, 502, 503, 504}) + + def _is_retryable_error( + self, error: BasecampError, operation: str | None, *, retry_on: frozenset[int] | None = 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 + # An explicit declared set (the download flow) is authoritative in both + # directions, exactly like an operation's declared retry_on below. + if retry_on is not None: + return error.http_status in retry_on # 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 diff --git a/python/src/basecamp/_http.py b/python/src/basecamp/_http.py index 771ed8180e..abf9b2be36 100644 --- a/python/src/basecamp/_http.py +++ b/python/src/basecamp/_http.py @@ -119,9 +119,16 @@ def request_multipart( return self._request_with_retry(method, url, files=files, operation=operation) return self._single_request(method, url, files=files) - def get_no_retry(self, url: str) -> httpx.Response: + def get_download(self, url: str) -> httpx.Response: + """Authenticated hop-1 GET for the download flow (SPEC section 14). + + Retries network errors plus the declared DOWNLOAD_RETRY_ON statuses — + never 500 — under the public max_retries total-attempt cap, floored at + one. DownloadURL has no behavior-model entry, so the policy is passed + directly rather than looked up by operation. + """ url = self._build_url(url) - return self._single_request("GET", url) + return self._request_with_retry("GET", url, retry_on=self.DOWNLOAD_RETRY_ON, accept=None) def close(self) -> None: self._client.close() @@ -147,6 +154,8 @@ def _request_with_retry( files: dict | None = None, allow_cross_origin: bool = False, operation: str | None = None, + retry_on: frozenset[int] | None = None, + accept: str | None = "application/json", ) -> httpx.Response: # max_retries is a TOTAL attempt count (config validation guarantees it # is >= 0). 0 is accepted as a compatibility exception and means a single @@ -172,9 +181,10 @@ def _request_with_retry( files=files, attempt=attempt, allow_cross_origin=allow_cross_origin, + accept=accept, ) except (RateLimitError, NetworkError, ApiError) as e: - if not self._is_retryable_error(e, operation): + if not self._is_retryable_error(e, operation, retry_on=retry_on): raise last_error = e if attempt >= max_attempts: @@ -207,6 +217,7 @@ def _single_request( attempt: int = 1, _retry_count: int = 0, allow_cross_origin: bool = False, + accept: str | None = "application/json", ) -> httpx.Response: if not allow_cross_origin and not ( _security.is_localhost(url) or _security.same_origin(url, self._config.base_url) @@ -219,7 +230,7 @@ def _single_request( start = time.monotonic() try: - headers = self._request_headers() + headers = self._request_headers(accept) if content_type: headers["Content-Type"] = content_type self._auth.authenticate(headers) @@ -251,6 +262,7 @@ def _single_request( attempt=attempt, _retry_count=_retry_count + 1, allow_cross_origin=allow_cross_origin, + accept=accept, ) raise error @@ -278,11 +290,14 @@ def _handle_error(self, response: httpx.Response) -> BasecampError: dict(response.headers), ) - def _request_headers(self) -> dict[str, str]: - return { - "User-Agent": self._user_agent, - "Accept": "application/json", - } + def _request_headers(self, accept: str | None = "application/json") -> dict[str, str]: + # accept=None is the binary-download carve-out (SPEC section 14): hop 1 + # sends Authorization and User-Agent only, because it is not a JSON API + # call. Every other caller keeps the JSON Accept. + headers = {"User-Agent": self._user_agent} + if accept is not None: + headers["Accept"] = accept + return headers def _build_url(self, path: str) -> str: # Schemes are case-insensitive (RFC 3986): detect absolute URLs on a @@ -315,12 +330,23 @@ def _is_retryable_operation(self, operation: str) -> bool: # retry_on of its own. DEFAULT_RETRY_ON = frozenset({429, 503}) - def _is_retryable_error(self, error: BasecampError, operation: str | None) -> bool: + # SPEC section 14's declared hop-1 set for downloads: a carve-out from the + # ungoverned GET taxonomy (which retries 500). Authoritative in BOTH + # directions, like an operation's declared retry_on. + DOWNLOAD_RETRY_ON = frozenset({429, 502, 503, 504}) + + def _is_retryable_error( + self, error: BasecampError, operation: str | None, *, retry_on: frozenset[int] | None = 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 + # An explicit declared set (the download flow) is authoritative in both + # directions, exactly like an operation's declared retry_on below. + if retry_on is not None: + return error.http_status in retry_on # 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 diff --git a/python/src/basecamp/download.py b/python/src/basecamp/download.py index 812ebfbb6e..bd1d9df564 100644 --- a/python/src/basecamp/download.py +++ b/python/src/basecamp/download.py @@ -45,11 +45,11 @@ def _parse_content_length(value: str | None) -> int: def download_sync(raw_url: str, *, http_client, config) -> DownloadResult: - """Perform sync download: URL rewrite → authenticated hop 1 → redirect → unauthenticated hop 2.""" + """Perform sync download: URL rewrite → authenticated hop 1 (SPEC §14 retry) → redirect → unauthenticated hop 2.""" _validate_url(raw_url) rewritten_url = _rewrite_url(raw_url, config.base_url) - response = http_client.get_no_retry(rewritten_url) + response = http_client.get_download(rewritten_url) if response.status_code in {301, 302, 303, 307, 308}: location = response.headers.get("Location") or response.headers.get("location") @@ -76,11 +76,11 @@ def download_sync(raw_url: str, *, http_client, config) -> DownloadResult: async def download_async(raw_url: str, *, http_client, config) -> DownloadResult: - """Perform async download: URL rewrite → authenticated hop 1 → redirect → unauthenticated hop 2.""" + """Perform async download: URL rewrite → authenticated hop 1 (SPEC §14 retry) → redirect → unauthenticated hop 2.""" _validate_url(raw_url) rewritten_url = _rewrite_url(raw_url, config.base_url) - response = await http_client.get_no_retry(rewritten_url) + response = await http_client.get_download(rewritten_url) if response.status_code in {301, 302, 303, 307, 308}: location = response.headers.get("Location") or response.headers.get("location") diff --git a/python/tests/test_download.py b/python/tests/test_download.py index 1b7a9701bd..f96e5154d1 100644 --- a/python/tests/test_download.py +++ b/python/tests/test_download.py @@ -1,14 +1,19 @@ from __future__ import annotations +import asyncio + import httpx import pytest import respx +from basecamp._async_http import AsyncHttpClient from basecamp._http import HttpClient +from basecamp.async_auth import AsyncBearerAuth, AsyncStaticTokenProvider from basecamp.auth import BearerAuth, StaticTokenProvider from basecamp.config import Config -from basecamp.download import _rewrite_url, download_sync, filename_from_url -from basecamp.errors import UsageError +from basecamp.download import _rewrite_url, download_async, download_sync, filename_from_url +from basecamp.errors import ApiError, BasecampError, UsageError +from basecamp.hooks import BasecampHooks def make_config(): @@ -107,3 +112,369 @@ def test_empty_url_raises(self): def test_relative_url_raises(self): with pytest.raises(UsageError, match="absolute URL"): download_sync("/just/a/path", http_client=make_http(), config=make_config()) + + +def make_fast_config(**overrides): + """Millisecond backoff so the retry tables run without real sleeps.""" + defaults = dict(base_url="https://3.basecampapi.com", base_delay=0.001, max_jitter=0.0) + defaults.update(overrides) + return Config(**defaults) + + +def make_fast_http(config, hooks=None): + auth = BearerAuth(StaticTokenProvider("test-token")) + return HttpClient(config, auth, hooks) + + +class RecordingHooks(BasecampHooks): + def __init__(self): + self.starts: list[int] = [] + self.ends: list[int] = [] + self.retries: list[int] = [] + + def on_request_start(self, info): + self.starts.append(info.attempt) + + def on_request_end(self, info, result): + self.ends.append(info.attempt) + + def on_retry(self, info, attempt, error, delay): + self.retries.append(attempt) + + +class TestHop1Retry: + """SPEC §14 hop-1 retry policy: network errors plus {429, 502, 503, 504}, + never 500, under the public max_retries cap floored at one.""" + + HOP1 = "https://3.basecampapi.com/files/doc.pdf" + SIGNED = "https://signed.storage.com/doc.pdf" + RAW = "https://original.com/files/doc.pdf" + + # The COMPLETE declared retry set, pinned status by status (the shared + # conformance fixtures cover 429/503). + @respx.mock + @pytest.mark.parametrize("status", [429, 502, 503, 504]) + def test_retries_declared_status_then_succeeds(self, status): + hop1 = respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(status), + httpx.Response(302, headers={"Location": self.SIGNED}), + ] + ) + respx.get(self.SIGNED).mock( + return_value=httpx.Response(200, content=b"data", headers={"content-type": "application/pdf"}) + ) + + config = make_fast_config() + result = download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + assert hop1.call_count == 2 + assert result.body == b"data" + + @respx.mock + def test_never_retries_500(self): + hop1 = respx.get(self.HOP1).mock(return_value=httpx.Response(500)) + + config = make_fast_config() + with pytest.raises(ApiError): + download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + assert hop1.call_count == 1 + + @respx.mock + def test_retries_network_error_then_succeeds(self): + hop1 = respx.get(self.HOP1).mock( + side_effect=[ + httpx.ConnectError("simulated network error"), + httpx.Response(200, content=b"content", headers={"content-type": "text/plain"}), + ] + ) + + config = make_fast_config() + result = download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + assert hop1.call_count == 2 + assert result.body == b"content" + + @respx.mock + def test_exhausts_cap_then_surfaces_error(self): + hop1 = respx.get(self.HOP1).mock(return_value=httpx.Response(503)) + + config = make_fast_config(max_retries=3) + with pytest.raises(ApiError): + download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + assert hop1.call_count == 3 + + @respx.mock + def test_zero_max_retries_still_sends_one_attempt(self): + hop1 = respx.get(self.HOP1).mock(return_value=httpx.Response(503)) + + config = make_fast_config(max_retries=0) + with pytest.raises(ApiError): + download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + assert hop1.call_count == 1 + + @respx.mock + def test_auth_on_every_hop1_attempt_never_on_hop2(self): + hop1 = respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(503), + httpx.Response(302, headers={"Location": self.SIGNED}), + ] + ) + hop2 = respx.get(self.SIGNED).mock( + return_value=httpx.Response(200, content=b"data", headers={"content-type": "application/pdf"}) + ) + + config = make_fast_config() + download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + for call in hop1.calls: + assert call.request.headers.get("Authorization") == "Bearer test-token" + assert "Authorization" not in hop2.calls[0].request.headers + + @respx.mock + def test_refreshable_401_replays_once_independent_of_the_retry_cap(self): + """SPEC §4: refresh is attempted at most once PER REQUEST. + + §4 tracks it with a boolean, not a counter, and does not subordinate + it to the transient-retry budget — so a refreshable 401 replays once + even with retries disabled. That is deliberately orthogonal to §14's + hop-1 attempt cap, which governs transient-failure retries. Whether + the two should be reconciled is tracked in #565; this pins what the + spec says today so a future change has to be a decision, not a drift. + """ + + class RefreshableProvider(StaticTokenProvider): + refreshable = True + + def __init__(self): + super().__init__("test-token") + self.refreshes = 0 + + def refresh(self): + self.refreshes += 1 + return True + + hop1 = respx.get(self.HOP1).mock(return_value=httpx.Response(401)) + + provider = RefreshableProvider() + config = make_fast_config(max_retries=0) + http = HttpClient(config, BearerAuth(provider)) + with pytest.raises(BasecampError): + download_sync(self.RAW, http_client=http, config=config) + + # One transient attempt, plus §4's single refresh replay. + assert hop1.call_count == 2 + assert provider.refreshes == 1 + + @respx.mock + def test_hop1_sends_no_accept_header_on_any_attempt(self): + """SPEC section 14: hop 1 carries Authorization and User-Agent only. + + A binary download is not a JSON API call, so the generic path's + ``Accept: application/json`` must not ride along — on the first + attempt or on a retry. + """ + hop1 = respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(503), + httpx.Response(302, headers={"Location": self.SIGNED}), + ] + ) + hop2 = respx.get(self.SIGNED).mock( + return_value=httpx.Response(200, content=b"data", headers={"content-type": "application/pdf"}) + ) + + config = make_fast_config() + download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + # httpx supplies its own transport-level "*/*" default when no Accept + # is set, exactly as it does for the bare hop-2 client. Pin hop 1 + # against that baseline: identical to hop 2, and never the JSON Accept. + hop2_accept = hop2.calls[0].request.headers.get("Accept") + assert hop1.call_count == 2 + for call in hop1.calls: + assert call.request.headers.get("Accept") == hop2_accept + assert call.request.headers.get("Accept") != "application/json" + assert "Content-Type" not in call.request.headers + assert call.request.headers.get("User-Agent") is not None + + @respx.mock + def test_balanced_hooks_across_retries(self): + respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(503), + httpx.Response(503), + httpx.Response(200, content=b"content", headers={"content-type": "text/plain"}), + ] + ) + + config = make_fast_config() + hooks = RecordingHooks() + download_sync(self.RAW, http_client=make_fast_http(config, hooks), config=config) + + assert hooks.starts == [1, 2, 3] + assert hooks.ends == [1, 2, 3] + # on_retry receives the UPCOMING attempt (SPEC §7 attempt semantics). + assert hooks.retries == [2, 3] + + @respx.mock + def test_honors_retry_after_on_429(self, monkeypatch): + delays: list[float] = [] + monkeypatch.setattr("basecamp._http.time.sleep", lambda d: delays.append(d)) + + respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(429, headers={"Retry-After": "7"}), + httpx.Response(200, content=b"content", headers={"content-type": "text/plain"}), + ] + ) + + config = make_fast_config() + download_sync(self.RAW, http_client=make_fast_http(config), config=config) + + assert delays == [7.0] + + +class TestHop1RetryAsync: + HOP1 = "https://3.basecampapi.com/files/doc.pdf" + SIGNED = "https://signed.storage.com/doc.pdf" + RAW = "https://original.com/files/doc.pdf" + + def make_async_http(self, config): + auth = AsyncBearerAuth(AsyncStaticTokenProvider("test-token")) + return AsyncHttpClient(config, auth) + + @respx.mock + @pytest.mark.asyncio + @pytest.mark.parametrize("status", [429, 502, 503, 504]) + async def test_retries_declared_status_then_succeeds(self, status): + hop1 = respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(status), + httpx.Response(302, headers={"Location": self.SIGNED}), + ] + ) + respx.get(self.SIGNED).mock( + return_value=httpx.Response(200, content=b"data", headers={"content-type": "application/pdf"}) + ) + + config = make_fast_config() + result = await download_async(self.RAW, http_client=self.make_async_http(config), config=config) + + assert hop1.call_count == 2 + assert result.body == b"data" + + @respx.mock + @pytest.mark.asyncio + async def test_never_retries_500(self): + hop1 = respx.get(self.HOP1).mock(return_value=httpx.Response(500)) + + config = make_fast_config() + with pytest.raises(ApiError): + await download_async(self.RAW, http_client=self.make_async_http(config), config=config) + + assert hop1.call_count == 1 + + @respx.mock + @pytest.mark.asyncio + async def test_retries_network_error_then_succeeds(self): + hop1 = respx.get(self.HOP1).mock( + side_effect=[ + httpx.ConnectError("simulated network error"), + httpx.Response(200, content=b"content", headers={"content-type": "text/plain"}), + ] + ) + + config = make_fast_config() + result = await download_async(self.RAW, http_client=self.make_async_http(config), config=config) + + assert hop1.call_count == 2 + assert result.body == b"content" + + @respx.mock + @pytest.mark.asyncio + async def test_zero_max_retries_still_sends_one_attempt(self): + hop1 = respx.get(self.HOP1).mock(return_value=httpx.Response(503)) + + config = make_fast_config(max_retries=0) + with pytest.raises(ApiError): + await download_async(self.RAW, http_client=self.make_async_http(config), config=config) + + assert hop1.call_count == 1 + + @respx.mock + @pytest.mark.asyncio + async def test_cancellation_is_terminal(self): + # Cancellation is not a transport failure: it must propagate raw with + # NO further attempts, even though a 503 would have been retried. + # (A counting callable side effect: respx refuses to raise BaseException + # types itself — CancelledError is a BaseException since Python 3.8 — + # and does not record calls whose side effect raises one.) + attempts = [] + + def cancel(request): + attempts.append(request) + raise asyncio.CancelledError() + + respx.get(self.HOP1).mock(side_effect=cancel) + + config = make_fast_config() + with pytest.raises(asyncio.CancelledError): + await download_async(self.RAW, http_client=self.make_async_http(config), config=config) + + assert len(attempts) == 1 + + @respx.mock + @pytest.mark.asyncio + async def test_honors_retry_after_on_429(self, monkeypatch): + """The async transport's backoff is pinned too, not just the sync one.""" + delays: list[float] = [] + + async def record(d): + delays.append(d) + + monkeypatch.setattr("basecamp._async_http.asyncio.sleep", record) + + respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(429, headers={"Retry-After": "7"}), + httpx.Response(200, content=b"content", headers={"content-type": "text/plain"}), + ] + ) + + config = make_fast_config() + await download_async(self.RAW, http_client=self.make_async_http(config), config=config) + + assert delays == [7.0] + + @respx.mock + @pytest.mark.asyncio + async def test_hop1_sends_no_accept_header_on_any_attempt(self): + """SPEC section 14 header scope, async transport (see the sync twin).""" + hop1 = respx.get(self.HOP1).mock( + side_effect=[ + httpx.Response(503), + httpx.Response(302, headers={"Location": self.SIGNED}), + ] + ) + hop2 = respx.get(self.SIGNED).mock( + return_value=httpx.Response(200, content=b"data", headers={"content-type": "application/pdf"}) + ) + + config = make_fast_config() + await download_async(self.RAW, http_client=self.make_async_http(config), config=config) + + # httpx supplies its own transport-level "*/*" default when no Accept + # is set, exactly as it does for the bare hop-2 client. Pin hop 1 + # against that baseline: identical to hop 2, and never the JSON Accept. + hop2_accept = hop2.calls[0].request.headers.get("Accept") + assert hop1.call_count == 2 + for call in hop1.calls: + assert call.request.headers.get("Accept") == hop2_accept + assert call.request.headers.get("Accept") != "application/json" + assert "Content-Type" not in call.request.headers + assert call.request.headers.get("User-Agent") is not None diff --git a/ruby/lib/basecamp/client.rb b/ruby/lib/basecamp/client.rb index 1e0259b3a9..d992c05a43 100644 --- a/ruby/lib/basecamp/client.rb +++ b/ruby/lib/basecamp/client.rb @@ -275,8 +275,9 @@ def download_url(raw_url) rewritten.port = base.port rewritten_url = rewritten.to_s - # Hop 1: Authenticated API request (no retry, captures redirect) - response = http.get_no_retry(rewritten_url) + # Hop 1: Authenticated API request under the SPEC §14 hop-1 retry + # policy (captures redirect; every attempt is authenticated) + response = http.get_download(rewritten_url) result = case response.status when 301, 302, 303, 307, 308 diff --git a/ruby/lib/basecamp/http.rb b/ruby/lib/basecamp/http.rb index e630377156..1a175f48a4 100644 --- a/ruby/lib/basecamp/http.rb +++ b/ruby/lib/basecamp/http.rb @@ -161,12 +161,22 @@ def put_raw(path, body:, content_type:) single_request_raw(:put, url, body: body, content_type: content_type, attempt: 1) end - # Performs a GET request without retry logic. - # Used for the download flow where retry is not appropriate. + # SPEC §14's declared hop-1 retry set for downloads: a carve-out from the + # ungoverned GET taxonomy (which retries all retryable 5xx, including 500). + # Authoritative in BOTH directions, like an operation's declared retryOn. + DOWNLOAD_RETRY_ON = [ 429, 502, 503, 504 ].freeze + + # 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. # @param url [String] absolute URL # @return [Response] - def get_no_retry(url) - single_request(:get, url, params: {}, body: nil, attempt: 1) + def get_download(url) + request_with_retry(:get, url, retry_on: DOWNLOAD_RETRY_ON, accept: nil) end # Fetches all pages of a paginated resource. @@ -354,10 +364,21 @@ def request(method, path, params: {}, body: nil, allow_cross_origin: false, oper end end - def request_with_retry(method, url, params: {}, allow_cross_origin: false, operation: nil) + 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) caller_cap = [ @config.max_retries, 1 ].max - max_attempts = op_retry ? [ caller_cap, op_retry.fetch("maxAttempts") ].min : @config.max_retries + # 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 attempt = 0 last_error = nil @@ -366,9 +387,10 @@ def request_with_retry(method, url, params: {}, allow_cross_origin: false, opera break if attempt > max_attempts begin - return single_request(method, url, params: params, body: nil, attempt: attempt, allow_cross_origin: allow_cross_origin) + return single_request(method, url, params: params, body: nil, attempt: attempt, + allow_cross_origin: allow_cross_origin, accept: accept) rescue Basecamp::RateLimitError, Basecamp::NetworkError, Basecamp::ApiError => e - raise e unless retry_eligible?(e, op_retry) + raise e unless retry_eligible?(e, op_retry, retry_on) last_error = e @@ -387,14 +409,16 @@ def request_with_retry(method, url, params: {}, allow_cross_origin: false, opera raise last_error || Basecamp::ApiError.new("Request failed after #{max_attempts} #{noun}") end - # For a governed request (operation given), a status-bearing error retries - # exactly when the operation's declared retryOn set says so — the error + # For a governed request — an operation's declared retry block, or an + # explicit declared set such as the download flow's — a status-bearing + # error retries exactly when the declared retryOn set says so: the error # taxonomy's retryable flag neither widens the set (500 is retryable in # errors.rb but not declared) nor vetoes it. Status-less errors (network # failures) and all ungoverned traffic keep the taxonomy's judgment. - def retry_eligible?(error, op_retry) - if op_retry && error.http_status - op_retry.fetch("retryOn").include?(error.http_status) + def retry_eligible?(error, op_retry, retry_on) + declared = retry_on || op_retry&.fetch("retryOn") + if declared && error.http_status + declared.include?(error.http_status) else error.retryable? end @@ -409,7 +433,8 @@ def self.operation_retry(operation) @operation_metadata.dig(operation, "retry") end - def single_request(method, url, params:, body:, attempt:, retry_count: 0, allow_cross_origin: false) + def single_request(method, url, params:, body:, attempt:, retry_count: 0, allow_cross_origin: false, + accept: "application/json") assert_credential_origin!(url, allow_cross_origin) info = RequestInfo.new(method: method.to_s.upcase, url: url, attempt: attempt) @hooks.on_request_start(info) @@ -417,7 +442,7 @@ def single_request(method, url, params:, body:, attempt:, retry_count: 0, allow_ start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) begin - response = @faraday.run_request(method, url, body, request_headers) do |req| + response = @faraday.run_request(method, url, body, request_headers(accept: accept)) do |req| req.params.merge!(params) if params.any? end @@ -444,7 +469,8 @@ def single_request(method, url, params:, body:, attempt:, retry_count: 0, allow_ # After a successful token refresh on 401, retry the request once if error.is_a?(Basecamp::AuthError) && error.http_status == 401 && retry_count < 1 && @token_refreshed @token_refreshed = false - return single_request(method, url, params: params, body: body, attempt: attempt, retry_count: retry_count + 1, allow_cross_origin: allow_cross_origin) + return single_request(method, url, params: params, body: body, attempt: attempt, retry_count: retry_count + 1, + allow_cross_origin: allow_cross_origin, accept: accept) end raise error @@ -457,11 +483,12 @@ def single_request(method, url, params:, body:, attempt:, retry_count: 0, allow_ end end - def request_headers - headers = { - "User-Agent" => USER_AGENT, - "Accept" => "application/json" - } + # accept: nil is the binary-download carve-out (SPEC §14): hop 1 sends + # Authorization and User-Agent only, because it is not a JSON API call. + # Every other caller keeps the JSON Accept. + def request_headers(accept: "application/json") + headers = { "User-Agent" => USER_AGENT } + headers["Accept"] = accept if accept @auth_strategy.authenticate(headers) headers end diff --git a/ruby/test/basecamp/download_test.rb b/ruby/test/basecamp/download_test.rb index 9de0470f61..93648d4939 100644 --- a/ruby/test/basecamp/download_test.rb +++ b/ruby/test/basecamp/download_test.rb @@ -299,7 +299,9 @@ def test_download_url_hop1_network_failure define_method(:on_request_end) { |info, result| requests_ended << [ info, result ] } end.new - account = create_account_client(hooks: hooks_impl) + # max_retries: 1 pins the per-attempt hook contract without backoff; the + # retry-enabled hook shape is pinned by the balanced-hooks test below. + account = create_account_client(config: fast_download_config(max_retries: 1), hooks: hooks_impl) stub_request(:get, "#{base_url}/12345/attachments/abc/download/file.txt") .to_timeout @@ -333,20 +335,208 @@ def test_download_url_hop2_network_failure assert_equal "network", error.code end - # -- No retry on 429 -- + # -- Hop-1 retry policy (SPEC §14) -- - def test_download_url_no_retry_on_429 - stub_request(:get, "#{base_url}/12345/attachments/abc/download/file.txt") + HOP1_URL = "https://3.basecampapi.com/12345/attachments/abc/download/file.txt" + HOP1_PATH = "/12345/attachments/abc/download/file.txt" + SIGNED_URL = "https://s3.amazonaws.com/bucket/signed-file" + + # Millisecond backoff so the retry tables run without real one-second sleeps. + def fast_download_config(**overrides) + Basecamp::Config.new( + **{ base_url: base_url, timeout: 5, max_retries: 3, base_delay: 0.001, max_jitter: 0.0 }.merge(overrides) + ) + end + + # The COMPLETE declared retry set, pinned status by status (the shared + # conformance fixtures cover 429/503). Hop 1 retries {429, 502, 503, 504} + # under the public max_retries total-attempt cap, floored at one. + [ 429, 502, 503, 504 ].each do |status| + define_method("test_download_url_retries_#{status}_then_follows_redirect") do + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: status, body: "{}", headers: { "Content-Type" => "application/json" }) + .then.to_return(status: 302, headers: { "Location" => SIGNED_URL }) + + stub_request(:get, SIGNED_URL) + .to_return(status: 200, body: "data", headers: { "Content-Type" => "application/octet-stream" }) + + account = create_account_client(config: fast_download_config) + result = account.download_url(HOP1_URL) + + assert_equal "data", result.body + assert_requested(:get, "#{base_url}#{HOP1_PATH}", times: 2) + end + end + + def test_download_url_never_retries_500 + stub_request(:get, "#{base_url}#{HOP1_PATH}") .with(headers: { "Authorization" => "Bearer #{access_token}" }) - .to_return(status: 429, body: '{"error":"Rate limited"}', headers: { "Content-Type" => "application/json", "Retry-After" => "30" }) + .to_return(status: 500, body: '{"error":"Server error"}', headers: { "Content-Type" => "application/json" }) - error = assert_raises(Basecamp::RateLimitError) do - @account.download_url("https://3.basecampapi.com/12345/attachments/abc/download/file.txt") + account = create_account_client(config: fast_download_config) + assert_raises(Basecamp::ApiError) { account.download_url(HOP1_URL) } + + # 500 is deliberately outside the declared set {429, 502, 503, 504} + assert_requested(:get, "#{base_url}#{HOP1_PATH}", times: 1) + end + + def test_download_url_retries_network_error_then_succeeds + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_timeout + .then.to_return(status: 200, body: "content", headers: { "Content-Type" => "text/plain" }) + + account = create_account_client(config: fast_download_config) + result = account.download_url(HOP1_URL) + + assert_equal "content", result.body + assert_requested(:get, "#{base_url}#{HOP1_PATH}", times: 2) + end + + def test_download_url_exhausts_cap_then_surfaces_error + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 503, body: "{}", headers: { "Content-Type" => "application/json" }) + + account = create_account_client(config: fast_download_config(max_retries: 3)) + assert_raises(Basecamp::ApiError) { account.download_url(HOP1_URL) } + + assert_requested(:get, "#{base_url}#{HOP1_PATH}", times: 3) + end + + def test_download_url_zero_max_retries_still_sends_one_attempt + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .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.) + account = create_account_client(config: fast_download_config(max_retries: 0)) + assert_raises(Basecamp::ApiError) { account.download_url(HOP1_URL) } + + assert_requested(:get, "#{base_url}#{HOP1_PATH}", times: 1) + end + + def test_download_url_auth_on_every_hop1_attempt_never_on_hop2 + # The .with(headers:) matcher applies to every attempt: an unauthenticated + # retry would not match this stub and would raise as unstubbed. + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 503, body: "{}", headers: { "Content-Type" => "application/json" }) + .then.to_return(status: 302, headers: { "Location" => SIGNED_URL }) + + s3_stub = stub_request(:get, SIGNED_URL) + .with { |req| req.headers["Authorization"].nil? } + .to_return(status: 200, body: "data", headers: { "Content-Type" => "application/octet-stream" }) + + account = create_account_client(config: fast_download_config) + account.download_url(HOP1_URL) + + assert_requested(:get, "#{base_url}#{HOP1_PATH}", times: 2) + assert_requested(s3_stub) + end + + # SPEC §4: refresh is attempted at most once PER REQUEST — tracked with a + # boolean, not a counter, and not subordinate to the transient-retry budget. + # So a refreshable 401 replays once even with retries disabled, deliberately + # orthogonal to §14's hop-1 attempt cap. Whether the two should be + # reconciled is tracked in #565; this pins what the spec says today. + def test_download_url_refreshable_401_replays_once_independent_of_the_retry_cap + provider = Class.new do + attr_reader :refreshes + + def initialize = @refreshes = 0 + def access_token = "test-token" + def refreshable? = true + + def refresh + @refreshes += 1 + true + end + end.new + + stub_request(:get, "#{base_url}#{HOP1_PATH}").to_return(status: 401) + + account = create_account_client(config: fast_download_config(max_retries: 0), token_provider: provider) + assert_raises(Basecamp::Error) { account.download_url(HOP1_URL) } + + # One transient attempt, plus §4's single refresh replay. + assert_requested(:get, "#{base_url}#{HOP1_PATH}", times: 2) + end + + # SPEC §14: hop 1 carries Authorization and User-Agent only. A binary + # download is not a JSON API call, so the generic request path's + # "Accept: application/json" must not ride along — on the first attempt or + # on a retry. + def test_download_url_hop1_sends_no_json_accept_on_any_attempt + hop1_accepts = [] + + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with { |req| hop1_accepts << req.headers["Accept"]; true } + .to_return(status: 503, body: "{}", headers: { "Content-Type" => "application/json" }) + .then.to_return(status: 302, headers: { "Location" => SIGNED_URL }) + + stub_request(:get, SIGNED_URL) + .to_return(status: 200, body: "data", headers: { "Content-Type" => "application/octet-stream" }) + + account = create_account_client(config: fast_download_config) + account.download_url(HOP1_URL) + + # Faraday supplies its own "*/*" default once the SDK stops setting an + # Accept, so the header cannot be absent outright without overriding the + # HTTP library. Pin the exact value rather than merely "not JSON": that + # catches both a reintroduced application/json and any other Accept the + # SDK might start attaching. + assert_equal 2, hop1_accepts.length + hop1_accepts.each do |accept| + assert_equal "*/*", accept, + "hop 1 must carry only Faraday's default Accept, never one the SDK sets (SPEC §14)" end + end - assert_equal "rate_limit", error.code + def test_download_url_balanced_hooks_across_retries + starts = [] + ends = [] + retries = [] - # Should have been called exactly once (no retry) - assert_requested(:get, "#{base_url}/12345/attachments/abc/download/file.txt", times: 1) + hooks_impl = Class.new do + include Basecamp::Hooks + define_method(:on_request_start) { |info| starts << info.attempt } + define_method(:on_request_end) { |info, _result| ends << info.attempt } + define_method(:on_retry) { |_info, attempt, _error, _delay| retries << attempt } + end.new + + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 503, body: "{}", headers: { "Content-Type" => "application/json" }) + .then.to_return(status: 503, body: "{}", headers: { "Content-Type" => "application/json" }) + .then.to_return(status: 200, body: "content", headers: { "Content-Type" => "text/plain" }) + + account = create_account_client(config: fast_download_config, hooks: hooks_impl) + account.download_url(HOP1_URL) + + assert_equal [ 1, 2, 3 ], starts + assert_equal [ 1, 2, 3 ], ends + # on_retry receives the UPCOMING attempt (SPEC §7 attempt semantics). + assert_equal [ 2, 3 ], retries + end + + def test_download_url_honors_retry_after_on_429 + delays = [] + + stub_request(:get, "#{base_url}#{HOP1_PATH}") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 429, body: "{}", headers: { "Content-Type" => "application/json", "Retry-After" => "7" }) + .then.to_return(status: 200, body: "content", headers: { "Content-Type" => "text/plain" }) + + http = Basecamp::Http.new(config: fast_download_config, token_provider: token_provider) + http.define_singleton_method(:sleep) { |delay| delays << delay } + + response = http.get_download("#{base_url}#{HOP1_PATH}") + + assert_equal 200, response.status + assert_equal [ 7 ], delays end end diff --git a/scripts/check-retry-metadata-parity.py b/scripts/check-retry-metadata-parity.py index 8ab919972b..6ee0e1a313 100755 --- a/scripts/check-retry-metadata-parity.py +++ b/scripts/check-retry-metadata-parity.py @@ -236,8 +236,11 @@ def check_max_only(name: str, emitted: dict[str, int], model: dict[str, tuple]) ["retryOn.includes", "maxAttempts", "baseDelayMs ??"], [], "base.ts: retryOn.includes(status), attempt vs maxAttempts, baseDelayMs backoff"), ("TypeScript", "full tuple", "typescript/src/client.ts", - ["retryOn.includes", "attempt >= maxAttempts", "calculateBackoffDelay", "config.baseDelayMs"], [], - "client.ts createRetryingFetch: retryOn.includes(status), attempt >= maxAttempts, backoff from baseDelayMs"), + ["getRetryConfigForRequest", "retryConfig.maxAttempts", "executeWithRetry"], [], + "client.ts createRetryingFetch resolves the per-operation tuple and hands it to executeWithRetry"), + ("TypeScript", "full tuple", "typescript/src/retry.ts", + ["config.retryOn.includes", "attempt >= config.maxAttempts", "calculateBackoffDelay", "config.baseDelayMs"], [], + "retry.ts executeWithRetry: retryOn.includes(status), attempt >= maxAttempts, backoff from baseDelayMs"), ("Swift", "full tuple", "swift/Sources/Basecamp/HTTP/HTTPClient.swift", ["retryOn.contains", "< maxAttempts", "baseDelayMs"], [], "HTTPClient.swift: retryOn.contains(status), attempt < maxAttempts, baseDelayMs"), @@ -253,7 +256,7 @@ def check_max_only(name: str, emitted: dict[str, int], model: dict[str, tuple]) ['.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", "max + retry_on", "ruby/lib/basecamp/http.rb", - ['fetch("maxAttempts")', 'fetch("retryOn").include?', "operation_retry"], [], + ['fetch("maxAttempts")', 'fetch("retryOn")', "declared.include?", "operation_retry"], [], "http.rb retries governed GETs with min(caller cap, maxAttempts) and gates status retry on the declared retryOn; base_delay/backoff emitted-but-inert"), ] diff --git a/swift/Sources/Basecamp/Download.swift b/swift/Sources/Basecamp/Download.swift index 893e534717..fdb9fa2bc2 100644 --- a/swift/Sources/Basecamp/Download.swift +++ b/swift/Sources/Basecamp/Download.swift @@ -45,6 +45,13 @@ extension AccountClient { /// authenticated first hop (which typically 302s to a signed download URL), /// and unauthenticated second hop to fetch the actual file content. /// + /// The first hop retries under the SPEC §14 policy — network errors plus + /// {429, 502, 503, 504}, never 500 — with exponential backoff, honoring + /// `Retry-After` on 429, over a fixed three attempts when + /// ``BasecampConfig/enableRetry`` is true and exactly one when it is false. + /// Every attempt is authenticated. The second hop is exempt: no retry, and + /// no credentials on the signed URL. + /// /// - Parameter rawURL: Absolute download URL (e.g., from bc-attachment elements). /// - Returns: A ``DownloadResult`` with body, content type, content length, and filename. /// - Throws: ``BasecampError/usage(message:hint:)`` if rawURL is empty or not absolute. diff --git a/swift/Sources/Basecamp/HTTP/HTTPClient.swift b/swift/Sources/Basecamp/HTTP/HTTPClient.swift index be5f149b49..93bfc1543c 100644 --- a/swift/Sources/Basecamp/HTTP/HTTPClient.swift +++ b/swift/Sources/Basecamp/HTTP/HTTPClient.swift @@ -23,6 +23,28 @@ package final class HTTPClient: Sendable { /// on the hot path does not allocate a `Set` per request. private static let retryableMethods: Set = ["GET", "HEAD", "PUT", "DELETE"] + /// The download hop-1 retry policy (SPEC §14): a fixed three attempts when + /// retry is enabled, retrying network errors plus these statuses — never + /// 500, which stays aligned with the main GET loop's declared-set + /// discipline rather than the error taxonomy's broader "all 5xx retryable" + /// flag. Backoff is exponential from ``defaultBaseDelayMs``, honoring + /// `Retry-After` on 429. The signed second hop is exempt: no retry, no auth. + private static let downloadMaxAttempts = 3 + private static let downloadRetryOn: Set = [429, 502, 503, 504] + + /// Converts a backoff interval to nanoseconds without trapping. + /// + /// `UInt64(_:)` on an out-of-range `Double` is a runtime trap, not an + /// error, and a hostile or simply buggy `Retry-After` can name a delay + /// whose nanosecond product overflows `UInt64` — `Retry-After: 99999999999` + /// is 9.9e19 ns against a 1.8e19 ceiling. Clamp to a day instead: no SDK + /// retry is worth sleeping longer, and a crash is never the right answer + /// to a response header. + private static func sleepNanoseconds(_ seconds: TimeInterval) -> UInt64 { + guard seconds.isFinite, seconds > 0 else { return 0 } + return UInt64(min(seconds, 86_400) * 1_000_000_000) + } + package init( transport: any Transport, authStrategy: any AuthStrategy, @@ -176,7 +198,14 @@ package final class HTTPClient: Sendable { $0.onRequestEnd(info, result: RequestResult(statusCode: 0, durationMs: durationMs)) } - if attempt < maxAttempts { + if error is CancellationError { + // Cooperative cancellation is terminal, not a transport + // blip: retrying would announce and start an attempt the + // caller has already abandoned. It propagates raw rather + // than wrapped, and the attempt is finalized above so + // start/end stay paired. + directive = .fail(error) + } else if attempt < maxAttempts { let delaySeconds = calculateDelay( attempt: attempt, baseDelayMs: effectiveConfig.baseDelayMs, @@ -203,7 +232,7 @@ package final class HTTPClient: Sendable { case .retry(let error, let delaySeconds): safeInvokeHooks { $0.onRetry(info, attempt: attempt + 1, error: error, delaySeconds: delaySeconds) } - try await Task.sleep(nanoseconds: UInt64(delaySeconds * 1_000_000_000)) + try await Task.sleep(nanoseconds: Self.sleepNanoseconds(delaySeconds)) // Re-authenticate for retry (e.g. refresh expired token) try await authStrategy.authenticate(&request) @@ -238,8 +267,14 @@ package final class HTTPClient: Sendable { return (data, httpResponse) } - /// Authenticated GET without redirect following. Fires request hooks. - /// Used by downloadURL for the first hop. + /// Authenticated GET without redirect following, under the SPEC §14 hop-1 + /// retry policy. Fires request hooks. Used by downloadURL for the first hop. + /// + /// The policy is fixed and passed here directly rather than looked up by + /// operation: `DownloadURL` is deliberately absent from + /// `behavior-model.json`, so there is no per-operation `RetryConfig` to + /// resolve and no public numeric knob for the attempt count. Disabling + /// retry collapses the hop to exactly one attempt. package func performDownloadRequest(url: String) async throws -> (Data, HTTPURLResponse) { guard let requestURL = URL(string: url) else { throw BasecampError.usage(message: "Invalid URL: \(url)", hint: nil) @@ -249,37 +284,108 @@ package final class HTTPClient: Sendable { request.httpMethod = "GET" request.timeoutInterval = config.timeoutInterval + // Attempt 1's auth runs here, outside the loop, matching performRequest: + // a failing strategy surfaces raw, before any request hook fires. try await authStrategy.authenticate(&request) request.setValue(config.userAgent, forHTTPHeaderField: "User-Agent") - let info = RequestInfo(method: "GET", url: url, attempt: 1) - safeInvokeHooks { $0.onRequestStart(info) } + let maxAttempts = config.enableRetry ? Self.downloadMaxAttempts : 1 - let startTime = CFAbsoluteTimeGetCurrent() + /// Outcome of a single attempt, computed inside the do/catch and acted + /// on at the loop tail — the same directive shape performRequest uses, + /// so retry side effects never run inside a catch clause. + enum AttemptDirective { + case done(Data, HTTPURLResponse) + case retry(error: any Error, delaySeconds: TimeInterval) + case fail(any Error) + } - do { - let (data, response) = try await transport.dataNoRedirect(for: request) + for attempt in 1...maxAttempts { + let info = RequestInfo(method: "GET", url: url, attempt: attempt) + safeInvokeHooks { $0.onRequestStart(info) } - guard let httpResponse = response as? HTTPURLResponse else { - throw BasecampError.network(message: "Invalid response type", cause: nil) - } + let startTime = CFAbsoluteTimeGetCurrent() + let directive: AttemptDirective + + do { + let (data, response) = try await transport.dataNoRedirect(for: request) + + guard let httpResponse = response as? HTTPURLResponse else { + throw BasecampError.network(message: "Invalid response type", cause: nil) + } + + let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) + safeInvokeHooks { + $0.onRequestEnd(info, result: RequestResult( + statusCode: httpResponse.statusCode, durationMs: durationMs)) + } + + let statusCode = httpResponse.statusCode + if Self.downloadRetryOn.contains(statusCode), attempt < maxAttempts { + let delaySeconds = calculateDelay( + attempt: attempt, + baseDelayMs: Self.defaultBaseDelayMs, + backoff: .exponential, + retryAfterHeader: httpResponse.value(forHTTPHeaderField: "Retry-After"), + statusCode: statusCode + ) + let error = BasecampError.fromHTTPResponse( + status: statusCode, data: data, + headers: httpResponse.allHeaderFields as? [String: String] ?? [:], + requestId: httpResponse.value(forHTTPHeaderField: "X-Request-Id") + ) + directive = .retry(error: error, delaySeconds: delaySeconds) + } else { + directive = .done(data, httpResponse) + } + } catch let error as BasecampError { + directive = .fail(error) + } catch { + let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) + safeInvokeHooks { + $0.onRequestEnd(info, result: RequestResult(statusCode: 0, durationMs: durationMs)) + } - let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) - safeInvokeHooks { - $0.onRequestEnd(info, result: RequestResult( - statusCode: httpResponse.statusCode, durationMs: durationMs)) + if error is CancellationError { + // Cooperative cancellation is terminal, not a transport + // blip: retrying would announce and start an attempt the + // caller has already abandoned. It propagates raw rather + // than wrapped, and the attempt is finalized above so + // start/end stay paired. + directive = .fail(error) + } else if attempt < maxAttempts { + let delaySeconds = calculateDelay( + attempt: attempt, + baseDelayMs: Self.defaultBaseDelayMs, + backoff: .exponential, + retryAfterHeader: nil, + statusCode: nil + ) + directive = .retry(error: error, delaySeconds: delaySeconds) + } else { + directive = .fail(BasecampError.network(message: "Network error", cause: error)) + } } - return (data, httpResponse) - } catch let error as BasecampError { - throw error - } catch { - let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) - safeInvokeHooks { - $0.onRequestEnd(info, result: RequestResult(statusCode: 0, durationMs: durationMs)) + // Loop tail, outside any catch: a CancellationError from the backoff + // sleep or an error from re-authentication propagates raw — no + // phantom request events for an attempt that already ended. + switch directive { + case .done(let data, let response): + return (data, response) + case .fail(let error): + throw error + case .retry(let error, let delaySeconds): + safeInvokeHooks { $0.onRetry(info, attempt: attempt + 1, error: error, delaySeconds: delaySeconds) } + + try await Task.sleep(nanoseconds: Self.sleepNanoseconds(delaySeconds)) + + // Re-authenticate every attempt so a rotated token is picked up. + try await authStrategy.authenticate(&request) } - throw BasecampError.network(message: "Network error", cause: error) } + + throw BasecampError.network(message: "Download failed after \(maxAttempts) attempts", cause: nil) } /// Unauthenticated GET via bare transport. No hooks. diff --git a/swift/Tests/BasecampTests/DownloadTests.swift b/swift/Tests/BasecampTests/DownloadTests.swift index 87b388f762..1f10a77327 100644 --- a/swift/Tests/BasecampTests/DownloadTests.swift +++ b/swift/Tests/BasecampTests/DownloadTests.swift @@ -461,34 +461,438 @@ final class DownloadTests: XCTestCase { } } - // MARK: - No Retry on 429 + // MARK: - Hop-1 Retry Policy (SPEC §14) - func testDownloadURL_noRetryOn429() async throws { + private static let hop1URL = "https://3.basecampapi.com/999999999/attachments/abc/download/file.txt" + + /// Records the request lifecycle so the hop-1 loop's hook emission can be + /// asserted attempt by attempt. + private final class RetryHookSpy: BasecampHooks, @unchecked Sendable { + private let lock = NSLock() + private var _starts: [Int] = [] + private var _ends: [Int] = [] + private var _retries: [Int] = [] + + var starts: [Int] { lock.withLock { _starts } } + var ends: [Int] { lock.withLock { _ends } } + var retries: [Int] { lock.withLock { _retries } } + + func onRequestStart(_ info: RequestInfo) { + lock.withLock { _starts.append(info.attempt) } + } + + func onRequestEnd(_ info: RequestInfo, result: RequestResult) { + lock.withLock { _ends.append(info.attempt) } + } + + func onRetry(_ info: RequestInfo, attempt: Int, error: any Error, delaySeconds: TimeInterval) { + lock.withLock { _retries.append(attempt) } + } + } + + /// The COMPLETE declared retry set, pinned status by status (the shared + /// conformance fixtures only cover 429 and 503): hop 1 retries + /// {429, 502, 503, 504} and then follows the redirect to the signed hop. + func testDownloadURL_retriesDeclaredStatusesThenFollowsRedirect() async throws { + for status in [429, 502, 503, 504] { + let hop1Attempts = Counter() + let hop2Requests = Counter() + let transport = MockTransport { request in + if request.url!.path.hasPrefix("/signed/") { + hop2Requests.increment() + return ( + Data("data".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 200, + headers: ["Content-Type": "application/octet-stream"] + ) + ) + } + if hop1Attempts.increment() == 1 { + return ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: status, + headers: ["Content-Type": "application/json"] + ) + ) + } + return ( + Data(), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 302, + headers: ["Location": "/signed/file.txt"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true) + + let result = try await account.downloadURL(Self.hop1URL) + + XCTAssertEqual(String(data: result.body, encoding: .utf8), "data", "status \(status)") + XCTAssertEqual(hop1Attempts.value, 2, "status \(status)") + XCTAssertEqual(hop2Requests.value, 1, "status \(status)") + } + } + + /// 500 is deliberately outside the declared set: the download hop keeps the + /// main GET loop's declared-set discipline rather than the error taxonomy's + /// broader "all 5xx retryable" flag. + func testDownloadURL_neverRetries500() async throws { let counter = Counter() let transport = MockTransport { request in counter.increment() return ( - Data(#"{"error":"Rate limited"}"#.utf8), + Data(#"{"error":"Internal server error"}"#.utf8), makeHTTPResponse( url: request.url!.absoluteString, - statusCode: 429, - headers: ["Content-Type": "application/json", "Retry-After": "30"] + statusCode: 500, + headers: ["Content-Type": "application/json"] ) ) } let account = makeTestAccountClient(transport: transport, enableRetry: true) do { - _ = try await account.downloadURL("https://3.basecampapi.com/999999999/attachments/abc/download/file.txt") - XCTFail("Expected rate limit error") - } catch let error as BasecampError { - guard case .rateLimit = error else { - XCTFail("Expected rateLimit, got \(error)") - return + _ = try await account.downloadURL(Self.hop1URL) + XCTFail("Expected api error") + } catch is BasecampError { + // Expected. + } + + XCTAssertEqual(counter.value, 1) + } + + func testDownloadURL_retriesNetworkErrorThenSucceeds() async throws { + struct TestError: Error {} + + let counter = Counter() + let transport = MockTransport { request in + if counter.increment() == 1 { + throw TestError() } + return ( + Data("content".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 200, + headers: ["Content-Type": "text/plain"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true) + + let result = try await account.downloadURL(Self.hop1URL) + + XCTAssertEqual(String(data: result.body, encoding: .utf8), "content") + XCTAssertEqual(counter.value, 2) + } + + /// The enabled policy is a fixed three attempts — there is no public + /// numeric knob for the download hop. + func testDownloadURL_exhaustsThreeAttemptsThenSurfaces() async throws { + let counter = Counter() + let transport = MockTransport { request in + counter.increment() + return ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 503, + headers: ["Content-Type": "application/json"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true) + + do { + _ = try await account.downloadURL(Self.hop1URL) + XCTFail("Expected error after the attempt budget is spent") + } catch is BasecampError { + // Expected. + } + + XCTAssertEqual(counter.value, 3) + } + + func testDownloadURL_enableRetryFalseSendsExactlyOneAttempt() async throws { + let counter = Counter() + let transport = MockTransport { request in + counter.increment() + return ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 503, + headers: ["Content-Type": "application/json"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: false) + + do { + _ = try await account.downloadURL(Self.hop1URL) + XCTFail("Expected error") + } catch is BasecampError { + // Expected. } - // Only one request — no retry XCTAssertEqual(counter.value, 1) } + + /// Every hop-1 attempt is authenticated — including the retried one — and + /// the signed hop never is. + func testDownloadURL_authOnEveryHop1AttemptNeverOnHop2() async throws { + let hop1Auth = AuthRecorder() + let hop2Auth = AuthRecorder() + let hop1Attempts = Counter() + let transport = MockTransport { request in + if request.url!.path.hasPrefix("/signed/") { + hop2Auth.record(request.value(forHTTPHeaderField: "Authorization")) + return ( + Data("data".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 200, + headers: ["Content-Type": "application/octet-stream"] + ) + ) + } + hop1Auth.record(request.value(forHTTPHeaderField: "Authorization")) + if hop1Attempts.increment() == 1 { + return ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 503, + headers: ["Content-Type": "application/json"] + ) + ) + } + return ( + Data(), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 302, + headers: ["Location": "/signed/file.txt"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true) + + _ = try await account.downloadURL(Self.hop1URL) + + XCTAssertEqual(hop1Auth.values, ["Bearer test-token", "Bearer test-token"]) + XCTAssertEqual(hop2Auth.values.count, 1) + XCTAssertNil(hop2Auth.values[0]) + } + + func testDownloadURL_balancedHooksAcrossRetries() async throws { + let spy = RetryHookSpy() + let counter = Counter() + let transport = MockTransport { request in + if counter.increment() < 3 { + return ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 503, + headers: ["Content-Type": "application/json"] + ) + ) + } + return ( + Data("content".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 200, + headers: ["Content-Type": "text/plain"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true, hooks: spy) + + _ = try await account.downloadURL(Self.hop1URL) + + XCTAssertEqual(spy.starts, [1, 2, 3]) + XCTAssertEqual(spy.ends, [1, 2, 3]) + // onRetry names the UPCOMING attempt (SPEC §7 attempt semantics). + XCTAssertEqual(spy.retries, [2, 3]) + } + + func testDownloadURL_honorsRetryAfterOn429() async throws { + let counter = Counter() + let transport = MockTransport { request in + if counter.increment() == 1 { + return ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 429, + headers: ["Content-Type": "application/json", "Retry-After": "2"] + ) + ) + } + return ( + Data("content".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 200, + headers: ["Content-Type": "text/plain"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true) + + let start = CFAbsoluteTimeGetCurrent() + _ = try await account.downloadURL(Self.hop1URL) + let elapsed = CFAbsoluteTimeGetCurrent() - start + + XCTAssertEqual(counter.value, 2) + // Retry-After: 2 wins over the 1-second exponential base. + XCTAssertGreaterThanOrEqual(elapsed, 2.0, "Expected the Retry-After pause, got \(elapsed)s") + } + + /// Cancelling during the hop-1 backoff propagates CancellationError raw: + /// no phantom onRequestEnd for the attempt that already ended, no second + /// onRetry, and no further attempt. + func testDownloadURL_cancellationDuringBackoffPropagatesRaw() async throws { + let spy = RetryHookSpy() + let transport = MockTransport { request in + ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 503, + headers: ["Content-Type": "application/json"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true, hooks: spy) + + let url = Self.hop1URL + let task = Task { _ = try await account.downloadURL(url) } + + // Bounded poll: if the first attempt never fires, fail the test rather + // than spinning here forever. + var waited = 0 + while transport.requests.isEmpty { + guard waited < 5_000 else { + task.cancel() + XCTFail("First hop-1 attempt never reached the transport") + return + } + try await Task.sleep(nanoseconds: 1_000_000) + waited += 1 + } + task.cancel() + + do { + _ = try await task.value + XCTFail("Expected CancellationError") + } catch is CancellationError { + // Expected: cooperative cancellation propagates raw. + } catch { + XCTFail("Expected CancellationError, got \(error)") + } + + XCTAssertEqual(transport.requests.count, 1, "A cancelled backoff must not start another attempt") + XCTAssertEqual(spy.ends, [1], "A cancelled backoff must not emit a phantom onRequestEnd") + XCTAssertEqual(spy.retries, [2], "A cancelled backoff must not emit a second onRetry") + } + + /// Cancellation raised by the transport itself — mid-flight, before any + /// response — is terminal too. Classifying it as a transport failure would + /// announce a retry for an attempt that can never start. + func testDownloadURL_cancellationInFlightIsTerminalAndSilent() async throws { + let spy = RetryHookSpy() + let counter = Counter() + let transport = MockTransport { _ in + counter.increment() + throw CancellationError() + } + let account = makeTestAccountClient(transport: transport, enableRetry: true, hooks: spy) + + do { + _ = try await account.downloadURL(Self.hop1URL) + XCTFail("Expected CancellationError") + } catch is CancellationError { + // Expected. + } catch { + XCTFail("Expected CancellationError, got \(error)") + } + + XCTAssertEqual(counter.value, 1, "Cancellation must not spend another attempt") + XCTAssertEqual(spy.retries, [], "Cancellation must not announce a retry") + // Hook balance is the actual contract, not merely "no crash": the + // cancelled attempt is finalized exactly once, so an observer pairing + // start/end spans never leaks one. + XCTAssertEqual(spy.starts, [1]) + XCTAssertEqual(spy.ends, [1]) + } + + /// A `Retry-After` large enough to overflow the nanosecond conversion must + /// not trap the process. `UInt64(_:)` on an out-of-range `Double` is a + /// runtime trap, so an unclamped conversion crashes here rather than + /// failing. + func testDownloadURL_absurdRetryAfterDoesNotTrap() async throws { + let counter = Counter() + let transport = MockTransport { request in + if counter.increment() == 1 { + return ( + Data("{}".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 429, + headers: ["Retry-After": "99999999999"] + ) + ) + } + return ( + Data("content".utf8), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 200, + headers: ["Content-Type": "text/plain"] + ) + ) + } + let account = makeTestAccountClient(transport: transport, enableRetry: true) + + let url = Self.hop1URL + let task = Task { _ = try await account.downloadURL(url) } + + // The clamped sleep is a day, so the call cannot finish here. Reaching + // this line at all is the assertion: an unclamped UInt64 conversion + // would have trapped inside the first backoff. + var waited = 0 + while counter.value < 1 { + guard waited < 5_000 else { + task.cancel() + XCTFail("First hop-1 attempt never reached the transport") + return + } + try await Task.sleep(nanoseconds: 1_000_000) + waited += 1 + } + try await Task.sleep(nanoseconds: 50_000_000) + task.cancel() + + XCTAssertEqual(counter.value, 1, "The retry must still be waiting out the clamped delay") + } +} + +/// Thread-safe recorder for per-attempt Authorization header values. +private final class AuthRecorder: @unchecked Sendable { + private let lock = NSLock() + private var _values: [String?] = [] + + var values: [String?] { lock.withLock { _values } } + + func record(_ value: String?) { + lock.withLock { _values.append(value) } + } } diff --git a/typescript/src/client.ts b/typescript/src/client.ts index 69a2fb4f50..e935060aa9 100644 --- a/typescript/src/client.ts +++ b/typescript/src/client.ts @@ -16,6 +16,14 @@ import { isLocalhost, requireSameOrigin } from "./security.js"; import { parseNextLink, resolveURL, isSameOrigin } from "./pagination-utils.js"; import { type AuthStrategy, bearerAuth } from "./auth-strategy.js"; import { createDownloadURL, type DownloadResult } from "./download.js"; +import { + DEFAULT_RETRY_CONFIG, + NO_RETRY_CONFIG, + TerminalRetryError, + executeWithRetry, + type RetryConfig, + type RetryEmit, +} from "./retry.js"; // ============================================================================ // Services - Generated from OpenAPI spec (spec-driven, not hand-written) @@ -415,7 +423,7 @@ export function createBasecampClient(options: BasecampClientOptions): BasecampCl // Wire downloadURL — raw fetch, not openapi-fetch (like fetchPage). // Defined before service factories so UploadsService can inject it. const downloadURLFn = createDownloadURL({ - authStrategy, userAgent, baseUrl, hooks, requestTimeoutMs, + authStrategy, userAgent, baseUrl, hooks, requestTimeoutMs, enableRetry, }); Object.defineProperty(enhancedClient, "downloadURL", { value: downloadURLFn, @@ -811,33 +819,9 @@ function getCacheKey(url: string, tokenHash: string): string { // Retrying Fetch (the retry loop, beneath the middleware chain) // ============================================================================= -/** - * Retry configuration matching x-basecamp-retry extension schema. - */ -interface RetryConfig { - maxAttempts: number; - baseDelayMs: number; - backoff: "exponential" | "linear" | "constant"; - retryOn: number[]; -} - -/** Default retry config used when no operation-specific config is available */ -const DEFAULT_RETRY_CONFIG: RetryConfig = { - maxAttempts: 3, - baseDelayMs: 1000, - backoff: "exponential", - retryOn: [429, 503], -}; - -/** No-retry config for non-idempotent POST operations */ -const NO_RETRY_CONFIG: RetryConfig = { - maxAttempts: 1, - baseDelayMs: 0, - backoff: "constant", - retryOn: [], -}; - -const MAX_JITTER_MS = 100; +// The retry loop itself lives in retry.ts (executeWithRetry) so the raw-fetch +// download hop 1 shares it; this section owns what is client-specific — the +// operation-metadata config resolution and the openapi-fetch integration. // PATH_TO_OPERATION is imported from generated/path-mapping.js @@ -1021,7 +1005,10 @@ function createRetryingFetch( return async (request) => { const { method, url } = request; const retryConfig = getRetryConfigForRequest(method, url); - const maxAttempts = enableRetry ? retryConfig.maxAttempts : 1; + const effectiveConfig: RetryConfig = { + ...retryConfig, + maxAttempts: enableRetry ? retryConfig.maxAttempts : 1, + }; // Serialize the body once, before the first send, because Request.body is // a stream that can only be consumed once and a retry needs to replay it. @@ -1031,18 +1018,15 @@ function createRetryingFetch( const upperMethod = method.toUpperCase(); let bodyBuffer: ArrayBuffer | null = null; if ( - maxAttempts > 1 && + effectiveConfig.maxAttempts > 1 && (upperMethod === "POST" || upperMethod === "PUT" || upperMethod === "PATCH") && request.body ) { bodyBuffer = await request.clone().arrayBuffer(); } - let attempt = 1; let attemptRequest = request; - lifecycle.begin(request, method, url, attempt); - - for (;;) { + const makeAttempt = async (attempt: number): Promise => { if (attempt > 1) { // Rebuild from the original request: the headers carry everything the // onRequest middleware attached (auth, User-Agent, If-None-Match), and @@ -1057,147 +1041,37 @@ function createRetryingFetch( // Refresh auth (the token may have rotated since the last attempt). // The attempt is already begun, so a throwing refresh lands on a live - // attempt and propagates to the lifecycle middleware's onError. - await authStrategy.authenticate(attemptRequest.headers); - } - - let response: Response; - try { - // globalThis.fetch resolved per attempt rather than captured at client - // creation, so test interceptors that patch it (MSW) are honored. - response = await globalThis.fetch(attemptRequest); - } catch (error) { - // An abort is terminal no matter what the budget says: a caller - // cancellation must not re-send, and the per-request timeout is one - // budget shared by every attempt and backoff — once it fires, a retry - // would instantly re-reject. Terminal errors are rethrown as-is so - // their identity survives to the lifecycle middleware's onError (and - // to the caller). - // - // The signal is the authoritative abort test: a caller can abort with - // a CUSTOM reason — AbortController.abort(reason) — and fetch then - // rejects with that reason, not a DOMException named AbortError. The - // DOMException check remains for abort-shaped rejections that arrive - // without an aborted request signal. - const isAbort = - request.signal?.aborted === true || - (error instanceof DOMException && - (error.name === "AbortError" || error.name === "TimeoutError")); - if (isAbort || attempt >= maxAttempts) { - throw error; + // attempt; the terminal marker carries it raw past the loop's retry + // classification to the lifecycle middleware's onError. + try { + await authStrategy.authenticate(attemptRequest.headers); + } catch (error) { + throw new TerminalRetryError(error); } - - // Network-error retry rides the same per-operation gate as status - // retry: a non-idempotent POST resolves NO_RETRY_CONFIG (maxAttempts - // 1), so it is rethrown above after its single attempt. - const cause = error instanceof Error ? error : new Error(String(error)); - const delay = calculateBackoffDelay(retryConfig, attempt - 1); - - lifecycle.finalize(request, method, url, { statusCode: 0, error: cause }); - lifecycle.retrying(method, url, attempt, cause, delay); - - await sleep(delay, request.signal); - - // Same placement rationale as the status path below. - attempt += 1; - lifecycle.begin(request, method, url, attempt); - continue; } - // Terminal: a status outside the operation's declared retryOn set, or a - // spent budget — maxAttempts is a total attempt count, so attempt N is - // terminal when it equals the cap. - if (!retryConfig.retryOn.includes(response.status) || attempt >= maxAttempts) { - return response; - } + // globalThis.fetch resolved per attempt rather than captured at client + // creation, so test interceptors that patch it (MSW) are honored. + return globalThis.fetch(attemptRequest); + }; - // For 429, respect Retry-After; otherwise back off. - let delay: number; - const retryAfter = - response.status === 429 ? response.headers.get("Retry-After") : null; - const retryAfterSeconds = retryAfter ? parseInt(retryAfter, 10) : NaN; - if (!isNaN(retryAfterSeconds)) { - delay = retryAfterSeconds * 1000; - } else { - delay = calculateBackoffDelay(retryConfig, attempt - 1); - } + const emit: RetryEmit = { + begin: (attempt) => lifecycle.begin(request, method, url, attempt), + finalize: (outcome) => lifecycle.finalize(request, method, url, outcome), + retrying: (failedAttempt, error, delayMs) => + lifecycle.retrying(method, url, failedAttempt, error, delayMs), + }; - const statusError = new Error( - `HTTP ${response.status}: ${response.statusText || "Request failed"}`, - ); - - // End the failed attempt before sleeping, so a slow backoff cannot leave - // an attempt open, then announce the upcoming one. - lifecycle.finalize(request, method, url, { statusCode: response.status }); - lifecycle.retrying(method, url, attempt, statusError, delay); - - // This response is being discarded, so release its stream before we sleep - // rather than leaving it open across the backoff — otherwise a throttled - // client holds a connection per in-flight retry and cannot reuse any of - // them. The multipart transport in services/base.ts already does this. - // Errors are ignored: the body may already be consumed or closed. - void response.body?.cancel().catch(() => {}); - - await sleep(delay, request.signal); - - // Begun after the backoff but before any work that can throw. After, so - // the attempt's duration measures the request rather than the sleep; - // before, so that if the auth refresh or the fetch throws, the lifecycle - // middleware's onError still finds a live attempt to finalize. Starting - // it later would let onRetry announce an attempt and then never account - // for it. - attempt += 1; - lifecycle.begin(request, method, url, attempt); + try { + return await executeWithRetry(makeAttempt, effectiveConfig, emit, request.signal); + } catch (error) { + // Unwrap the terminal marker so the original error's identity survives + // to the lifecycle middleware's onError (and to the caller). + throw error instanceof TerminalRetryError ? error.reason : error; } }; } -function calculateBackoffDelay(config: RetryConfig, attempt: number): number { - const base = config.baseDelayMs; - let delay: number; - - switch (config.backoff) { - case "exponential": - delay = base * Math.pow(2, attempt); - break; - case "linear": - delay = base * (attempt + 1); - break; - case "constant": - default: - delay = base; - } - - // Add jitter (0-100ms) - const jitter = Math.random() * MAX_JITTER_MS; - return delay + jitter; -} - -/** - * Signal-aware sleep for backoff waits: resolves after `ms`, or rejects with - * the signal's abort reason the moment it fires. Without this, a caller abort - * or the request-timeout budget expiring during a backoff would leave the - * request pending for the full delay and then start another attempt — begin, - * auth refresh, fetch — against an already-aborted signal. - */ -function sleep(ms: number, signal?: AbortSignal): Promise { - return new Promise((resolve, reject) => { - if (signal?.aborted) { - reject(signal.reason as Error); - return; - } - const onAbort = () => { - clearTimeout(timer); - reject(signal!.reason as Error); - }; - const timer = setTimeout(() => { - signal?.removeEventListener("abort", onAbort); - resolve(); - }, ms); - signal?.addEventListener("abort", onAbort, { once: true }); - }); -} - // ============================================================================= // Pagination Helper // ============================================================================= diff --git a/typescript/src/download.ts b/typescript/src/download.ts index a40ea58a65..a29436bbcd 100644 --- a/typescript/src/download.ts +++ b/typescript/src/download.ts @@ -2,6 +2,24 @@ import type { AuthStrategy } from "./auth-strategy.js"; import type { BasecampHooks, OperationInfo, RequestInfo } from "./hooks.js"; import { BasecampError, Errors, errorFromResponse } from "./errors.js"; import { safeInvoke } from "./hooks.js"; +import { + TerminalRetryError, + executeWithRetry, + type RetryConfig, + type RetryEmit, +} from "./retry.js"; + +/** + * The fixed hop-1 retry policy (SPEC §14): three total attempts when retry is + * enabled, retrying network errors plus {429, 502, 503, 504} — never 500 — + * with exponential backoff, honoring Retry-After on 429. DownloadURL is + * deliberately absent from behavior-model.json, so the policy is passed to + * the retry primitive directly rather than looked up by operation. There is + * no public knob for the attempt count. + */ +const DOWNLOAD_MAX_ATTEMPTS = 3; +const DOWNLOAD_RETRY_ON = [429, 502, 503, 504]; +const DOWNLOAD_RETRY_BASE_DELAY_MS = 1000; /** * Result of downloading file content from a URL. @@ -53,6 +71,13 @@ interface DownloadDeps { baseUrl: string; hooks?: BasecampHooks; requestTimeoutMs: number; + /** false collapses hop 1 to exactly one attempt (SPEC §14). */ + enableRetry: boolean; + /** + * Test seam for the fixed policy's backoff base. Not wired to any client + * option — production callers omit it and get the 1-second base. + */ + retryBaseDelayMs?: number; } /** @@ -65,7 +90,7 @@ interface DownloadDeps { * other signed-download URL that routes through the API. */ export function createDownloadURL(deps: DownloadDeps): (rawURL: string) => Promise { - const { authStrategy, userAgent, baseUrl, hooks, requestTimeoutMs } = deps; + const { authStrategy, userAgent, baseUrl, hooks, requestTimeoutMs, enableRetry, retryBaseDelayMs } = deps; return async (rawURL: string): Promise => { // Validation @@ -99,26 +124,72 @@ export function createDownloadURL(deps: DownloadDeps): (rawURL: string) => Promi const base = new URL(baseUrl); const rewrittenURL = `${base.origin}${parsed.pathname}${parsed.search}${parsed.hash}`; - // Hop 1: Authenticated API request (capture redirect) + // Hop 1: Authenticated API request (capture redirect), under the fixed + // hop-1 retry policy. Attempt 1's auth runs here, outside the loop, so + // a failing strategy surfaces raw without request hooks — matching the + // client path, where the middleware authenticates before the loop. const headers = new Headers({ "User-Agent": userAgent, }); await authStrategy.authenticate(headers); - const requestInfo: RequestInfo = { + const downloadRetryConfig: RetryConfig = { + maxAttempts: enableRetry ? DOWNLOAD_MAX_ATTEMPTS : 1, + baseDelayMs: retryBaseDelayMs ?? DOWNLOAD_RETRY_BASE_DELAY_MS, + backoff: "exponential", + retryOn: DOWNLOAD_RETRY_ON, + }; + + const requestInfoFor = (attempt: number): RequestInfo => ({ method: "GET", url: rewrittenURL, - attempt: 1, + attempt, + }); + + // The download path has no lifecycle middleware, so the emit seams fire + // the hooks directly. executeWithRetry finalizes only the attempts it + // abandons; the terminal outcome is finalized after the loop below. + let currentAttempt = 1; + let attemptStart = performance.now(); + const emit: RetryEmit = { + begin: (attempt) => { + currentAttempt = attempt; + attemptStart = performance.now(); + safeInvoke(hooks, "onRequestStart", requestInfoFor(attempt)); + }, + finalize: (outcome) => { + const durationMs = Math.round(performance.now() - attemptStart); + safeInvoke(hooks, "onRequestEnd", requestInfoFor(currentAttempt), { + statusCode: outcome.statusCode, + durationMs, + fromCache: false, + ...(outcome.error ? { error: outcome.error } : {}), + }); + }, + retrying: (failedAttempt, error, delayMs) => { + safeInvoke(hooks, "onRetry", requestInfoFor(failedAttempt), failedAttempt + 1, error, delayMs); + }, }; - safeInvoke(hooks, "onRequestStart", requestInfo); - const reqStart = performance.now(); - let response: Response; - try { + const makeAttempt = async (attempt: number): Promise => { + if (attempt > 1) { + // Refresh auth (the token may have rotated since the last attempt), + // so EVERY hop-1 attempt goes out authenticated. A throwing refresh + // is terminal: the marker carries it raw past retry classification. + try { + await authStrategy.authenticate(headers); + } catch (error) { + throw new TerminalRetryError(error); + } + } + // Per-attempt timeout: the controller aborts only its own fetch, and + // an abort-shaped rejection is terminal in the loop — a request that + // consumed its whole time budget is a slowness shape a retry tends to + // repeat, not a transient blip. const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), requestTimeoutMs); try { - response = await fetch(rewrittenURL, { + return await fetch(rewrittenURL, { method: "GET", headers, redirect: "manual", @@ -127,24 +198,28 @@ export function createDownloadURL(deps: DownloadDeps): (rawURL: string) => Promi } finally { clearTimeout(timeoutId); } + }; + + let response: Response; + try { + response = await executeWithRetry(makeAttempt, downloadRetryConfig, emit); } catch (err) { - const durationMs = Math.round(performance.now() - reqStart); + if (err instanceof TerminalRetryError) { + // A retry's auth refresh failed: finalize the live attempt, then + // surface the strategy's own error raw — auth faults are neither + // transport failures nor API errors. + const reason = err.reason; + const error = reason instanceof Error ? reason : new Error(String(reason)); + emit.finalize({ statusCode: 0, error }); + throw reason; + } const error = err instanceof Error ? err : new Error(String(err)); - safeInvoke(hooks, "onRequestEnd", requestInfo, { - statusCode: 0, - durationMs, - fromCache: false, - error, - }); + emit.finalize({ statusCode: 0, error }); throw Errors.network(error.message, error); } - const durationMs = Math.round(performance.now() - reqStart); - safeInvoke(hooks, "onRequestEnd", requestInfo, { - statusCode: response.status, - durationMs, - fromCache: false, - }); + // Terminal attempt's end — the loop deliberately leaves it to us. + emit.finalize({ statusCode: response.status }); // Dispatch on response status const isRedirect = [301, 302, 303, 307, 308].includes(response.status); diff --git a/typescript/src/retry.ts b/typescript/src/retry.ts new file mode 100644 index 0000000000..ff210fa8b2 --- /dev/null +++ b/typescript/src/retry.ts @@ -0,0 +1,222 @@ +/** + * The SDK's retry primitive: the attempt loop extracted from the client's + * retrying fetch so that both the middleware-chained JSON path (client.ts) + * and the raw-fetch download hop 1 (download.ts) share one budget/backoff/ + * abort policy. The loop is transport-agnostic — callers own attempt + * preparation (auth, request construction) via `makeAttempt` and hook + * emission via `RetryEmit`. + */ + +/** + * Retry configuration matching x-basecamp-retry extension schema. + */ +export interface RetryConfig { + maxAttempts: number; + baseDelayMs: number; + backoff: "exponential" | "linear" | "constant"; + retryOn: number[]; +} + +/** Default retry config used when no operation-specific config is available */ +export const DEFAULT_RETRY_CONFIG: RetryConfig = { + maxAttempts: 3, + baseDelayMs: 1000, + backoff: "exponential", + retryOn: [429, 503], +}; + +/** No-retry config for non-idempotent POST operations */ +export const NO_RETRY_CONFIG: RetryConfig = { + maxAttempts: 1, + baseDelayMs: 0, + backoff: "constant", + retryOn: [], +}; + +const MAX_JITTER_MS = 100; + +/** + * Lifecycle seams the retry loop emits through. The loop begins EVERY attempt + * and finalizes the attempts it abandons (before the backoff sleep); the + * terminal outcome — the response it returns, or the error it throws — is + * deliberately NOT finalized here. The caller owns the terminal attempt's end + * so it can record post-transform results (the client's cache middleware may + * rewrite a 304 into a cached 200 before its lifecycle middleware finalizes). + */ +export interface RetryEmit { + /** An attempt is beginning (fired for attempt 1 and every retry). */ + begin(attempt: number): void; + /** An attempt was abandoned in favor of a retry (never the terminal one). */ + finalize(outcome: { statusCode: number; error?: Error }): void; + /** A retry is coming: `failedAttempt` just ended, `failedAttempt + 1` is next. */ + retrying(failedAttempt: number, error: Error, delayMs: number): void; +} + +/** + * Marks an error thrown during attempt PREPARATION (e.g. a retry's auth + * refresh) rather than by the transport. The retry loop rethrows it as-is — + * no retry classification, no budget spent — and the caller unwraps `reason` + * so the original error's identity survives to its error path. + */ +export class TerminalRetryError extends Error { + constructor(readonly reason: unknown) { + super("attempt preparation failed terminally"); + this.name = "TerminalRetryError"; + } +} + +/** + * Runs `makeAttempt` under the retry policy in `config`. + * + * `config.maxAttempts` is a total attempt count — the caller passes the + * effective budget (e.g. 1 when retry is disabled). Status retry is gated on + * the declared `retryOn` set; 429 honors Retry-After. Transport errors retry + * on the same budget, except aborts, which are terminal no matter what the + * budget says: a caller cancellation must not re-send, and a request-timeout + * budget is shared by every attempt and backoff — once it fires, a retry + * would instantly re-reject. Terminal errors are rethrown as-is so their + * identity survives to the caller. + * + * `signal`, when given, is the authoritative abort test: a caller can abort + * with a CUSTOM reason — AbortController.abort(reason) — and fetch then + * rejects with that reason, not a DOMException named AbortError. The + * DOMException check remains for abort-shaped rejections that arrive without + * an aborted signal (e.g. a per-attempt timeout controller). + */ +export async function executeWithRetry( + makeAttempt: (attempt: number) => Promise, + config: RetryConfig, + emit: RetryEmit, + signal?: AbortSignal | null, +): Promise { + let attempt = 1; + emit.begin(attempt); + + for (;;) { + let response: Response; + try { + response = await makeAttempt(attempt); + } catch (error) { + // Attempt preparation failed — the caller marked it terminal. Rethrow + // the marker itself; the caller unwraps it at its own boundary. + if (error instanceof TerminalRetryError) { + throw error; + } + + const isAbort = + signal?.aborted === true || + (error instanceof DOMException && + (error.name === "AbortError" || error.name === "TimeoutError")); + if (isAbort || attempt >= config.maxAttempts) { + throw error; + } + + // Network-error retry rides the same budget as status retry: a caller + // that resolves a no-retry policy (maxAttempts 1) is rethrown above + // after its single attempt. + const cause = error instanceof Error ? error : new Error(String(error)); + const delay = calculateBackoffDelay(config, attempt - 1); + + emit.finalize({ statusCode: 0, error: cause }); + emit.retrying(attempt, cause, delay); + + await sleep(delay, signal ?? undefined); + + // Same placement rationale as the status path below. + attempt += 1; + emit.begin(attempt); + continue; + } + + // Terminal: a status outside the declared retryOn set, or a spent + // budget — maxAttempts is a total attempt count, so attempt N is + // terminal when it equals the cap. + if (!config.retryOn.includes(response.status) || attempt >= config.maxAttempts) { + return response; + } + + // For 429, respect Retry-After; otherwise back off. + let delay: number; + const retryAfter = + response.status === 429 ? response.headers.get("Retry-After") : null; + const retryAfterSeconds = retryAfter ? parseInt(retryAfter, 10) : NaN; + if (!isNaN(retryAfterSeconds)) { + delay = retryAfterSeconds * 1000; + } else { + delay = calculateBackoffDelay(config, attempt - 1); + } + + const statusError = new Error( + `HTTP ${response.status}: ${response.statusText || "Request failed"}`, + ); + + // End the failed attempt before sleeping, so a slow backoff cannot leave + // an attempt open, then announce the upcoming one. + emit.finalize({ statusCode: response.status }); + emit.retrying(attempt, statusError, delay); + + // This response is being discarded, so release its stream before we sleep + // rather than leaving it open across the backoff — otherwise a throttled + // client holds a connection per in-flight retry and cannot reuse any of + // them. The multipart transport in services/base.ts already does this. + // Errors are ignored: the body may already be consumed or closed. + void response.body?.cancel().catch(() => {}); + + await sleep(delay, signal ?? undefined); + + // Begun after the backoff but before any work that can throw. After, so + // the attempt's duration measures the request rather than the sleep; + // before, so that if the caller's attempt preparation or the fetch + // throws, its error path still finds a live attempt to finalize. + // Starting it later would let onRetry announce an attempt and then never + // account for it. + attempt += 1; + emit.begin(attempt); + } +} + +export function calculateBackoffDelay(config: RetryConfig, attempt: number): number { + const base = config.baseDelayMs; + let delay: number; + + switch (config.backoff) { + case "exponential": + delay = base * Math.pow(2, attempt); + break; + case "linear": + delay = base * (attempt + 1); + break; + case "constant": + default: + delay = base; + } + + // Add jitter (0-100ms) + const jitter = Math.random() * MAX_JITTER_MS; + return delay + jitter; +} + +/** + * Signal-aware sleep for backoff waits: resolves after `ms`, or rejects with + * the signal's abort reason the moment it fires. Without this, a caller abort + * or the request-timeout budget expiring during a backoff would leave the + * request pending for the full delay and then start another attempt — begin, + * auth refresh, fetch — against an already-aborted signal. + */ +export function sleep(ms: number, signal?: AbortSignal): Promise { + return new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(signal.reason as Error); + return; + } + const onAbort = () => { + clearTimeout(timer); + reject(signal!.reason as Error); + }; + const timer = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(); + }, ms); + signal?.addEventListener("abort", onAbort, { once: true }); + }); +} diff --git a/typescript/tests/download.test.ts b/typescript/tests/download.test.ts index b690112246..56f65b04f2 100644 --- a/typescript/tests/download.test.ts +++ b/typescript/tests/download.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from "vitest"; import { http, HttpResponse } from "msw"; import { server } from "./setup.js"; import { createBasecampClient } from "../src/client.js"; -import { filenameFromURL } from "../src/download.js"; +import { createDownloadURL, filenameFromURL } from "../src/download.js"; import type { BasecampHooks, RequestInfo, RequestResult, OperationInfo } from "../src/hooks.js"; import { BasecampError } from "../src/errors.js"; @@ -10,12 +10,38 @@ const BASE_URL = "https://3.basecampapi.com/12345"; const API_ORIGIN = "https://3.basecampapi.com"; const S3_URL = "https://s3.amazonaws.com/bucket/signed-file.png"; -function makeClient(hooks?: BasecampHooks) { +function makeClient(hooks?: BasecampHooks, enableRetry?: boolean) { return createBasecampClient({ accountId: "12345", accessToken: "test-token", baseUrl: BASE_URL, hooks, + ...(enableRetry === undefined ? {} : { enableRetry }), + }); +} + +/** + * Direct factory construction: exercises the fixed hop-1 retry policy with a + * millisecond backoff base (the internal test seam), so the retry tables run + * without real one-second sleeps. Client-level tests keep the default base. + */ +function makeDownloadURL(overrides?: { + hooks?: BasecampHooks; + enableRetry?: boolean; + requestTimeoutMs?: number; +}) { + return createDownloadURL({ + authStrategy: { + authenticate: async (headers) => { + headers.set("Authorization", "Bearer test-token"); + }, + }, + userAgent: "basecamp-sdk-test", + baseUrl: BASE_URL, + hooks: overrides?.hooks, + requestTimeoutMs: overrides?.requestTimeoutMs ?? 30_000, + enableRetry: overrides?.enableRetry ?? true, + retryBaseDelayMs: 1, }); } @@ -307,25 +333,233 @@ describe("downloadURL", () => { ).rejects.toMatchObject({ code: "network" }); }); - it("does not retry on 429", async () => { + }); + + describe("hop-1 retry policy (SPEC §14)", () => { + const RAW_URL = "https://storage.3.basecamp.com/999/blobs/abc/download/file.png"; + + // The COMPLETE declared retry set, pinned status by status: hop 1 retries + // {429, 502, 503, 504} (the shared conformance fixtures cover 429/503). + it.each([[429], [502], [503], [504]] as const)( + "retries hop 1 on %d, then follows the redirect", + async (status) => { + let apiAttempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, () => { + apiAttempts++; + if (apiAttempts === 1) { + return new HttpResponse(null, { status }); + } + return new HttpResponse(null, { + status: 302, + headers: { Location: S3_URL }, + }); + }), + http.get(S3_URL, () => { + return new HttpResponse("data", { + headers: { "Content-Type": "application/octet-stream" }, + }); + }), + ); + + const downloadURL = makeDownloadURL(); + const result = await downloadURL(RAW_URL); + result.body.cancel(); + + expect(apiAttempts).toBe(2); + }, + ); + + it("never retries 500 — deliberately outside the declared set", async () => { + let attempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, () => { + attempts++; + return new HttpResponse(null, { status: 500 }); + }), + ); + + const downloadURL = makeDownloadURL(); + await expect(downloadURL(RAW_URL)).rejects.toMatchObject({ code: "api_error" }); + + expect(attempts).toBe(1); + }); + + it("retries hop 1 on a network error", async () => { let attempts = 0; server.use( http.get(`${API_ORIGIN}/*`, () => { attempts++; + if (attempts === 1) { + return HttpResponse.error(); + } + return new HttpResponse("content", { + headers: { "Content-Type": "text/plain" }, + }); + }), + ); + + const downloadURL = makeDownloadURL(); + const result = await downloadURL(RAW_URL); + result.body.cancel(); + + expect(attempts).toBe(2); + }); + + it("exhausts the three-attempt budget and surfaces the final error", async () => { + let attempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, () => { + attempts++; + return new HttpResponse(null, { status: 503 }); + }), + ); + + const downloadURL = makeDownloadURL(); + await expect(downloadURL(RAW_URL)).rejects.toThrow(BasecampError); + + expect(attempts).toBe(3); + }); + + it("makes exactly one attempt when retry is disabled", async () => { + let attempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, () => { + attempts++; + return new HttpResponse(null, { status: 503 }); + }), + ); + + const downloadURL = makeDownloadURL({ enableRetry: false }); + await expect(downloadURL(RAW_URL)).rejects.toThrow(BasecampError); + + expect(attempts).toBe(1); + }); + + it("treats a per-attempt timeout abort as terminal — no retry after abort", async () => { + let attempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, async () => { + attempts++; + await new Promise((resolve) => setTimeout(resolve, 200)); + return new HttpResponse(null, { status: 503 }); + }), + ); + + const downloadURL = makeDownloadURL({ requestTimeoutMs: 50 }); + await expect(downloadURL(RAW_URL)).rejects.toMatchObject({ code: "network" }); + + expect(attempts).toBe(1); + }); + + it("sends Authorization on every hop-1 attempt and never on hop 2", async () => { + const apiAuthHeaders: (string | null)[] = []; + let s3AuthHeader: string | null = "unset"; + let apiAttempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, ({ request }) => { + apiAttempts++; + apiAuthHeaders.push(request.headers.get("Authorization")); + if (apiAttempts === 1) { + return new HttpResponse(null, { status: 503 }); + } return new HttpResponse(null, { - status: 429, - headers: { "Retry-After": "1" }, + status: 302, + headers: { Location: S3_URL }, + }); + }), + http.get(S3_URL, ({ request }) => { + s3AuthHeader = request.headers.get("Authorization"); + return new HttpResponse("data", { + headers: { "Content-Type": "application/octet-stream" }, + }); + }), + ); + + const downloadURL = makeDownloadURL(); + const result = await downloadURL(RAW_URL); + result.body.cancel(); + + expect(apiAuthHeaders).toEqual(["Bearer test-token", "Bearer test-token"]); + expect(s3AuthHeader).toBeNull(); + }); + + it("fires balanced start/end hooks and one onRetry per backoff", async () => { + const events: { kind: string; attempt: number }[] = []; + let apiAttempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, () => { + apiAttempts++; + if (apiAttempts < 3) { + return new HttpResponse(null, { status: 503 }); + } + return new HttpResponse("content", { + headers: { "Content-Type": "text/plain" }, + }); + }), + ); + + const hooks: BasecampHooks = { + onRequestStart: (info) => { + events.push({ kind: "start", attempt: info.attempt }); + }, + onRequestEnd: (info) => { + events.push({ kind: "end", attempt: info.attempt }); + }, + onRetry: (_info, upcomingAttempt) => { + events.push({ kind: "retry", attempt: upcomingAttempt }); + }, + }; + + const downloadURL = makeDownloadURL({ hooks }); + const result = await downloadURL(RAW_URL); + result.body.cancel(); + + const byKind = (kind: string) => + events.filter((e) => e.kind === kind).map((e) => e.attempt); + expect(byKind("start")).toEqual([1, 2, 3]); + expect(byKind("end")).toEqual([1, 2, 3]); + // onRetry's argument is the UPCOMING attempt (SPEC §7 attempt semantics). + expect(byKind("retry")).toEqual([2, 3]); + }); + + it("honors Retry-After on 429 at the client level", async () => { + let attempts = 0; + + server.use( + http.get(`${API_ORIGIN}/*`, () => { + attempts++; + if (attempts === 1) { + return new HttpResponse(null, { + status: 429, + headers: { "Retry-After": "1" }, + }); + } + return new HttpResponse("content", { + headers: { "Content-Type": "text/plain" }, }); }), ); const client = makeClient(); - await expect( - client.downloadURL("https://storage.3.basecamp.com/999/blobs/abc/download/file.txt"), - ).rejects.toMatchObject({ code: "rate_limit" }); + const start = performance.now(); + const result = await client.downloadURL(RAW_URL); + result.body.cancel(); + const elapsed = performance.now() - start; - expect(attempts).toBe(1); + expect(attempts).toBe(2); + // Node timers may fire marginally early; require all but a sliver of + // the requested second so a dropped Retry-After (millisecond backoff + // would miss by ~999ms) still fails loudly. + expect(elapsed).toBeGreaterThanOrEqual(990); }); }); @@ -478,7 +712,7 @@ describe("downloadURL", () => { expect(capturedResult!.statusCode).toBe(404); }); - it("fires onRequestEnd with statusCode 0 on network failure", async () => { + it("fires onRequestEnd with statusCode 0 on network failure (retry disabled)", async () => { let reqStartCount = 0; let reqEndCount = 0; let capturedResult: RequestResult | null = null; @@ -499,7 +733,10 @@ describe("downloadURL", () => { }, }; - const client = makeClient(hooks); + // Retry disabled: exactly one hop-1 attempt (SPEC §14 attempt budget), + // so the hook contract is pinned per attempt without real backoff. The + // retry-enabled hook shape is pinned by the balanced-hooks test above. + const client = makeClient(hooks, false); await expect( client.downloadURL("https://storage.3.basecamp.com/999/blobs/abc/download/file.txt"), ).rejects.toMatchObject({ code: "network" });