Fix @MainActor isolation crashes in OAuth + signal listeners - #23
Fix @MainActor isolation crashes in OAuth + signal listeners#23mathiasrscom wants to merge 1 commit into
Conversation
On macOS (Swift 6 runtime), two paths trapped with EXC_BREAKPOINT in libdispatch's _dispatch_assert_queue_fail (main-actor isolation check): - OAuthHandler.authenticate: ASWebAuthenticationSession invokes its completion handler on a background XPC queue, but the completion closure was @MainActor-isolated (it's a literal inside a @mainactor type and captured self), so the runtime's entry isolation check trapped. Fixed by marking the closure @sendable (so it carries no actor isolation), capturing the continuation directly, and using a nonisolated static cookie extractor. (A DispatchQueue.main.async inside the closure is not enough — the assertion fires at closure entry, before the body runs.) - SignalBus.listen: the Combine sink spawned Task { ... } on the global executor, but listeners (e.g. SessionStore) mutate @mainactor @published state. Hop to the main actor with Task { @mainactor in ... }.
WalkthroughThis PR refines async patterns in two independent areas: signal handlers now execute on the main actor via explicit ChangesSignal Handler Main Actor Isolation
OAuth Authentication Async Continuation Refactoring
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Core/Session/OAuthHandler.swift`:
- Line 57: The call to session.start() in OAuthHandler.swift ignores its Bool
return and can leave the withCheckedThrowingContinuation in authenticate(...)
hanging if start() returns false; modify the authenticate method to capture the
Bool from session.start(), and if it returns false immediately resume (or
resumeThrowing) the continuation with a meaningful error (e.g.,
OAuthError.sessionFailedToStart) and cancel/clean up the
ASWebAuthenticationSession; ensure any existing completion handler still guards
against double-resume and that the continuation is only resumed once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 806c47cd-c808-40ed-ba00-dadd610424a0
📒 Files selected for processing (2)
Sources/Core/Plugins/Events.swiftSources/Core/Session/OAuthHandler.swift
| session.presentationContextProvider = self | ||
| session.prefersEphemeralWebBrowserSession = false | ||
| self.webAuthSession = session | ||
| session.start() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Apple AuthenticationServices documentation, for ASWebAuthenticationSession, when start() returns false, is the completion handler guaranteed to be called or not? What is the recommended error-handling pattern for this case?
💡 Result:
When ASWebAuthenticationSession.start returns false, the documentation indicates that the session did not start successfully (it returns a Boolean “whether the web authentication session started successfully”) [1][2]. In that failure case, the completion handler is not guaranteed to be called, because there is no started session to complete and deliver either a callback URL or an error via that handler [1][2]. Recommended error-handling pattern: 1) Check the Bool return from start. If it’s false, treat it as an immediate failure and handle it synchronously (e.g., update UI/state and don’t wait for the completion handler). 2) For the cases where the session actually starts, handle all outcomes in the completion handler closure by checking the error first, then inspecting callbackURL when error is nil (Apple’s example pattern uses “guard error == nil, let callbackURL = callbackURL else { return }”) [3]. In practice (Swift): let session = ASWebAuthenticationSession(url: authURL, callbackURLScheme: scheme) { callbackURL, error in // Only called for started/completing sessions guard error == nil, let callbackURL = callbackURL else { return } // Process callbackURL } if session.start == false { // Session failed to start: handle immediately here // e.g., show an error / abort flow / log return } Apple also exposes a canStart property to determine whether a session can begin; if you already know you can’t start, handle that up-front rather than waiting for the completion handler [4][5].
Citations:
- 1: https://apple-docs.everest.mt/docs/authenticationservices/aswebauthenticationsession/start()/
- 2: https://developer.apple.com/documentation/authenticationservices/aswebauthenticationsession/start()
- 3: https://apple-docs.everest.mt/docs/authenticationservices/authenticating-a-user-through-a-web-service/
- 4: https://developer.apple.com/documentation/AuthenticationServices/ASWebAuthenticationSession?changes=latest_minor
- 5: https://developer.apple.com/documentation/authenticationservices/aswebauthenticationsession
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around line 57
sed -n '1,120p' Sources/Core/Session/OAuthHandler.swift | nl -ba | sed -n '35,95p'
# Find continuation usage in this file
rg -n "withCheckedThrowingContinuation|continuation\.resume|ASWebAuthenticationSession|start\(\)" Sources/Core/Session/OAuthHandler.swift
# Check BetterAuthSwiftError definition for initializer used in suggested fix
rg -n "struct BetterAuthSwiftError|enum BetterAuthSwiftError|BetterAuthSwiftError" Sources/Core | headRepository: ouwargui/BetterAuthSwift
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print relevant section with line numbers (cat -n is available)
cat -n Sources/Core/Session/OAuthHandler.swift | sed -n '35,95p'
# Locate the continuation and session.start usage
rg -n "withCheckedThrowingContinuation|continuation\.resume|ASWebAuthenticationSession|start\(\)" Sources/Core/Session/OAuthHandler.swift
# Find BetterAuthSwiftError definition / shape for the suggested initializer
rg -n "BetterAuthSwiftError" Sources/Core | head -n 50Repository: ouwargui/BetterAuthSwift
Length of output: 6614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check BetterAuthSwiftError definition and initializers
cat -n Sources/Core/Network/Errors.swift | sed -n '1,120p'Repository: ouwargui/BetterAuthSwift
Length of output: 2560
Handle ASWebAuthenticationSession.start() returning false to avoid hanging authenticate.
Sources/Core/Session/OAuthHandler.swift calls session.start() and ignores its Bool. When start() returns false, the completion handler isn’t guaranteed to run, so the withCheckedThrowingContinuation may never be resumed, potentially hanging OAuth/sign-in.
💡 Suggested fix
session.presentationContextProvider = self
session.prefersEphemeralWebBrowserSession = false
self.webAuthSession = session
- session.start()
+ if !session.start() {
+ self.webAuthSession = nil
+ continuation.resume(
+ throwing: BetterAuthSwiftError(
+ message: "Failed to start web authentication session"
+ )
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Core/Session/OAuthHandler.swift` at line 57, The call to
session.start() in OAuthHandler.swift ignores its Bool return and can leave the
withCheckedThrowingContinuation in authenticate(...) hanging if start() returns
false; modify the authenticate method to capture the Bool from session.start(),
and if it returns false immediately resume (or resumeThrowing) the continuation
with a meaningful error (e.g., OAuthError.sessionFailedToStart) and cancel/clean
up the ASWebAuthenticationSession; ensure any existing completion handler still
guards against double-resume and that the continuation is only resumed once.
1 participant
On macOS (Swift 6 runtime), two paths trapped with EXC_BREAKPOINT in libdispatch's _dispatch_assert_queue_fail (main-actor isolation check):
OAuthHandler.authenticate: ASWebAuthenticationSession invokes its completion handler on a background XPC queue, but the completion closure was @MainActor-isolated (it's a literal inside a @mainactor type and captured self), so the runtime's entry isolation check trapped. Fixed by marking the closure @sendable (so it carries no actor isolation), capturing the continuation directly, and using a nonisolated static cookie extractor. (A DispatchQueue.main.async inside the closure is not enough — the assertion fires at closure entry, before the body runs.)
SignalBus.listen: the Combine sink spawned Task { ... } on the global executor, but listeners (e.g. SessionStore) mutate @mainactor @published state. Hop to the main actor with Task { @mainactor in ... }.
Summary by CodeRabbit