Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable automatic retry behavior to PostgREST requests to improve resilience against transient failures, along with test coverage for the new behavior.
Changes:
- Introduces
retryEnabledat thePostgrestClientconfiguration level (defaulting to enabled). - Adds per-request
retry(enabled:)override plus retry logic (HTTP 520 + thrown network errors) for idempotent methods (GET/HEAD), includingX-Retry-Countheader. - Extends/adjusts tests to validate retry behavior and keep request-building snapshots deterministic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Sources/PostgREST/PostgrestClient.swift |
Adds retryEnabled configuration and initializer parameter. |
Sources/PostgREST/PostgrestBuilder.swift |
Implements retry loop + per-request override + X-Retry-Count header. |
Tests/PostgRESTTests/PostgrestBuilderTests.swift |
Adds retry-focused unit tests and a no-op clock for fast retries. |
Tests/PostgRESTTests/BuildURLRequestTests.swift |
Updates test isolation primitive and disables retries to avoid impacting snapshots. |
You can also share your feedback on Copilot code review. Take the survey.
Adds transparent retry logic to the PostgREST client for transient failures (HTTP 520, network errors), mirroring supabase-js#2072. Retry behavior: - Only retries on HTTP 520 or network errors - Only retries idempotent methods (GET, HEAD) - Up to 3 retries with exponential backoff (1s, 2s, 4s, capped at 30s) - Adds X-Retry-Count header on retried requests Configuration: - Enabled by default; disable globally via `retryEnabled: false` on PostgrestClient or PostgrestClient.Configuration - Per-request override via `.retry(enabled: false/true)` on the builder Linear: SDK-771 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix bug where decode errors could be incorrectly retried: separate the network send from decoding so only transient network/HTTP errors trigger retries - Remove fetchOptions from MutableState; build the final request as a local snapshot to avoid mutable side effects on execute - Move maxRetries, retryableMethods, retryableStatusCodes to private static let constants - Mark decode closure as @sendable for StrictConcurrency correctness - Add Task.checkCancellation() at the top of each retry iteration - Refactor shouldRetry to use guard chains and Self. static references - Fix typo in log message: "Fail to decode" -> "Failed to decode" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix variable shadowing bug in PostgrestBuilder.execute where `var request = baseRequest` inside the retry loop overwrote the prepared request (with Accept/Content-Type headers), causing snapshot tests to fail. Renamed to `currentRequest` to avoid the shadowing. - Add tearDown to PostgrestBuilderTests to restore `_clock` after each test. setUp was setting _clock to ImmediateRetryTestClock() but never restoring it, causing subsequent Realtime timeout tests to fire immediately and return TimeoutError instead of expected responses. Made `_resolveClock` package-accessible to support the reset. - Fix TOCTOU force-unwrap crash in RealtimeClientV2.sendHeartbeat where pendingHeartbeatRef was set in one LockIsolated closure then force-unwrapped in a second closure, allowing a race where another thread could clear the ref between reads. Now captures and returns the ref in a single closure using an Optional. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
LockIsolated.withValue is synchronous; the await was a leftover from when the variable was ActorIsolated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8063df3 to
0790329
Compare
mandarini
left a comment
There was a problem hiding this comment.
Should we enable the retries by default? I guess it's safe since it's only for get/head, but still it's a behaviour change, we should stress it in the docs (I see you do). In any case, should we also retry on 502/503/504?
@mandarini It is enabled by default already and we should only retry on 520 as per project definition. |
Summary
PostgrestBuilderfor transient failures (HTTP 520, network errors), mirroring supabase-js#2072_clockabstraction from HelpersChanges
PostgrestClient.Configuration: newretryEnabled: Boolfield (defaulttrue); threaded through bothinitoverloadsPostgrestBuilder:retryEnabledinMutableState(propagated through copy constructor);.retry(enabled:)chainable method; privateexecutewrapped in retry loop withX-Retry-Countheader on retried requestsPostgrestBuilderTests: 10 new tests covering all retry scenarios; usesImmediateRetryTestClockto skip sleep delays in testsRetry behavior
Configuration
Test plan
X-Retry-Countheader).retry(enabled: false)disables retryretryEnabled: falsedisables retry.retry(enabled: true)overrides client-level falsePostgrestBuilderTestspassCloses: SDK-771
🤖 Generated with Claude Code
/take