-
-
Notifications
You must be signed in to change notification settings - Fork 142
fix(auth): do not remove session in case of network failure #712
New issue
Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “No Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? No Sign in to your account
base: main
Are you sure you want to change the base?
fix(auth): do not remove session in case of network failure #712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the issue of incorrectly removing the session when a cancellation error occurs during token refresh.
- Adds explicit checks to rethrow errors when they are cancellation-related.
- Ensures that session removal only occurs for non-retryable errors.
Pull Request Test Coverage Report for Build 14714501160Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the authentication logic so that the session is not removed when a cancellation error or other retryable error occurs. It also updates the error handling and retry logic by removing the RetryableError protocol and refactoring the default error code sets.
- Removed the RetryableError protocol and its associated implementations.
- Updated RetryRequestInterceptor with static default retryable error and status code sets.
- Modified SessionManager to avoid deleting the session on error and removed AuthError’s conformance to RetryableError.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
Sources/Helpers/RetryableError.swift | Removed the RetryableError protocol and its extensions. |
Sources/Helpers/HTTP/RetryRequestInterceptor.swift | Added static default error and status code sets and reformatted the exponential backoff. |
Sources/Auth/Internal/SessionManager.swift | Removed the try/catch block that deleted the session on error, ensuring retryable errors pass through. |
Sources/Auth/AuthError.swift | Removed conformance of AuthError to RetryableError. |
Comments suppressed due to low confidence (3)
Sources/Auth/Internal/SessionManager.swift:80
- Removing the try/catch block in refreshSession changes error handling behavior; please ensure non-retryable errors are managed appropriately without needing to explicitly remove the session.
do {
Sources/Auth/AuthError.swift:298
- Removing RetryableError conformance from AuthError alters error propagation; confirm that the error handling strategy across the codebase aligns with this change.
extension AuthError: RetryableError {
Sources/Helpers/HTTP/RetryRequestInterceptor.swift:34
- Verify that the error codes listed in the migrated defaultRetryableURLErrorCodes exactly match the originally intended set to ensure consistent retry behavior.
package static let defaultRetryableURLErrorCodes: Set<URLError.Code> = [
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses issues with session removal during network failures by altering the error‐handling behavior of the auth session refresh flow and related error protocols. The key changes include:
- Removing the deletion of sessions on non‐retryable errors in SessionManager.
- Dropping the RetryableError conformance from AuthError.
- Updating integration tests to reflect changes in identity comparisons and provider usage.
Reviewed Changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
Tests/IntegrationTests/supabase/config.toml | Added configuration values for testing local instances. |
Tests/IntegrationTests/DotEnv.swift | Introduced local environment constants. |
Tests/IntegrationTests/AuthClientIntegrationTests.swift | Updated tests to compare identity IDs and changed use of OAuth provider from GitHub to Apple. |
Sources/Helpers/RetryableError.swift | Removed the RetryableError protocol implementation. |
Sources/Helpers/HTTP/RetryRequestInterceptor.swift | Updated default retryable error and HTTP status code definitions. |
Sources/Auth/Internal/SessionManager.swift | Removed the catch block’s session-deletion logic to prevent session removal on error, modifying error flow. |
Sources/Auth/AuthError.swift | Removed conformance from AuthError to RetryableError. |
Sources/Auth/AuthClient.swift | Minor update to the lifecycle task invocation. |
Files not reviewed (4)
- Tests/IntegrationTests/.vscode/extensions.json: Language not supported
- Tests/IntegrationTests/.vscode/settings.json: Language not supported
- Tests/IntegrationTests/supabase/.gitignore: Language not supported
- Tests/IntegrationTests/supabase/.temp/cli-latest: Language not supported
What changes does this PR introduce?
This PR addresses two issues:
refresh_token_already_used
error.What is the current behavior?
When a non-retryable error occurs, the session is automatically removed. This leads to a few issues:
URLError(.cancelled)
, which should not cause session removal, are also removed.refresh_token_already_used
error, as reported in Issue Session deleted from auth local storage while refreshing token due to "refresh token already used" when sharing auth local storage with multiple processes #703.What is the new behavior?
The new behavior is to avoid removing sessions when an error occurs.
Additional context
Please provide any additional context or screenshots relevant to the changes introduced in this PR.