Merge pull request #274 from okta/OKTA-1144194-DeadlockFix

Adapt CredentialActor to use a GCD-based serial queue to avoid mixing thread isolation patterns

Author: AlexNachbaur

Sources/AuthFoundation/Migration/SDKVersion.swift

@@ -16,22 +16,18 @@ import Foundation
 import CommonSupport
 #endif
 
-#if canImport(UIKit)
-import UIKit
-#endif
-
-#if os(watchOS)
-import WatchKit
-#endif
-
 #if canImport(Android)
 import Android
 #endif
 
 private let deviceModel: String = {
     var system = utsname()
     uname(&system)
-    let model = withUnsafePointer(to: &system.machine.0) { ptr in
+    let model = withUnsafeBytes(of: &system.machine) { buf in
+        guard let ptr = buf.baseAddress?.assumingMemoryBound(to: CChar.self)
+        else {
+            return "unknown"
+        }
         return String(cString: ptr)
     }
     return model
@@ -52,22 +48,16 @@ private let systemName: String = {
         return "android"
     #elseif os(Linux)
         return "linux"
-    #elseif os(Android)
-        return "android"
+    #elseif os(Windows)
+        return "windows"
+    #else
+        return "unknown"
     #endif
 }()
 
 private let systemVersion: String = {
-    #if os(iOS) || os(tvOS) || (swift(>=5.10) && os(visionOS))
-    return MainActor.nonisolatedUnsafe {
-        UIDevice.current.systemVersion
-    }
-    #elseif os(watchOS)
-        return WKInterfaceDevice.current().systemVersion
-    #else
-        let osVersion = ProcessInfo.processInfo.operatingSystemVersion
-        return "\(osVersion.majorVersion).\(osVersion.minorVersion).\(osVersion.patchVersion)"
-    #endif
+    let osVersion = ProcessInfo.processInfo.operatingSystemVersion
+    return "\(osVersion.majorVersion).\(osVersion.minorVersion).\(osVersion.patchVersion)"
 }()
 
 /// Utility class that allows SDK components to register their name and version for use in HTTP User-Agent values.
@@ -101,47 +91,56 @@ public struct SDKVersion: Sendable {
     @discardableResult
     public static func register(sdk: SDKVersion) -> SDKVersion {
         lock.withLock {
-            guard _sdkVersions.filter({ $0.name == sdk.name }).isEmpty else {
-                return sdk
-            }
-
-            _sdkVersions.append(sdk)
-
-            let sdkVersionString = _sdkVersions
-                .sorted(by: { $0.name.rawValue < $1.name.rawValue })
-                .map(\.displayName)
-                .joined(separator: " ")
-            _userAgent = "\(sdkVersionString) \(systemName)/\(systemVersion) Device/\(deviceModel)"
+            _register(sdk: sdk)
+        }
+    }
 
+    private static func _register(sdk: SDKVersion) -> SDKVersion {
+        guard _sdkVersions.filter({ $0.name == sdk.name }).isEmpty else {
             return sdk
         }
+        
+        _sdkVersions.append(sdk)
+        
+        let sdkVersionString = _sdkVersions
+            .sorted(by: { $0.name.rawValue < $1.name.rawValue })
+            .map(\.displayName)
+            .joined(separator: " ")
+        _userAgent = "\(sdkVersionString) \(systemName)/\(systemVersion) Device/\(deviceModel)"
+        
+        return sdk
     }
 
     /// Convenience function used to register an SDK
     /// - Parameters:
     ///   - name: SDK name.
     ///   - versionString: SDK version.
     /// - Returns: The resulting SDKVersion object.
-    @inlinable
     @discardableResult
     public static func register(_ name: Name, version versionString: String) -> SDKVersion? {
-        let sdk = version(for: name) ?? register(sdk: SDKVersion(sdk: name, version: versionString))
-        guard sdk.version == versionString
-        else {
-            return nil
+        lock.withLock {
+            let sdk = _version(for: name) ?? _register(sdk: SDKVersion(sdk: name, version: versionString))
+            guard sdk.version == versionString
+            else {
+                return nil
+            }
+            return sdk
         }
-        return sdk
     }
 
     /// Returns the version information for the given SDK.
     /// - Parameter sdkName: SDK name to search for.
     /// - Returns: Version information for the given SDK name.
     public static func version(for sdkName: Name) -> SDKVersion? {
         lock.withLock {
-            _sdkVersions.first(where: { $0.name == sdkName })
+            _version(for: sdkName)
         }
     }
 
+    private static func _version(for sdkName: Name) -> SDKVersion? {
+        _sdkVersions.first(where: { $0.name == sdkName })
+    }
+
     // MARK: Private properties / methods
     private static let lock = Lock()
     nonisolated(unsafe) private static var _sdkVersions: [SDKVersion] = []

Sources/AuthFoundation/User Management/Credential.swift

@@ -31,14 +31,14 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
         get {
             assert(SDKVersion.authFoundation != nil)
 
-            return withIsolationSync { @CredentialActor in
+            return CredentialActor.sync {
                 TaskData.coordinator.default
             }
         }
         set {
             assert(SDKVersion.authFoundation != nil)
 
-            withIsolationSync { @CredentialActor in
+            CredentialActor.sync {
                 TaskData.coordinator.default = newValue
             }
         }
@@ -48,9 +48,9 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
     public static var allIDs: [String] {
         assert(SDKVersion.authFoundation != nil)
 
-        return withIsolationSync { @CredentialActor in
+        return CredentialActor.sync {
             TaskData.coordinator.allIDs
-        } ?? []
+        }
     }
 
     /// The default grace interval used when refreshing tokens using ``Credential/refreshIfNeeded(graceInterval:completion:)`` or ``Credential/refreshIfNeeded(graceInterval:)``.
@@ -74,7 +74,7 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
     public static func with(id: String, prompt: String? = nil, authenticationContext: (any TokenAuthenticationContext)? = nil) throws -> Credential? {
         assert(SDKVersion.authFoundation != nil)
 
-        return try withIsolationSyncThrowing { @CredentialActor in
+        return try CredentialActor.sync {
             try TaskData.coordinator.with(id: id,
                                           prompt: prompt,
                                           authenticationContext: authenticationContext)
@@ -102,7 +102,7 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
     public static func find(where expression: @Sendable @escaping (Token.Metadata) -> Bool, prompt: String? = nil, authenticationContext: (any TokenAuthenticationContext)? = nil) throws -> [Credential] {
         assert(SDKVersion.authFoundation != nil)
 
-        return try withIsolationSyncThrowing { @CredentialActor in
+        return try CredentialActor.sync {
             try TaskData.coordinator.find(where: expression,
                                           prompt: prompt,
                                           authenticationContext: authenticationContext)
@@ -129,7 +129,7 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
     ) throws -> Credential {
         assert(SDKVersion.authFoundation != nil)
 
-        return try withIsolationSyncThrowing { @CredentialActor in
+        return try CredentialActor.sync {
             try TaskData.coordinator.store(token: token, tags: tags, security: options)
         }
     }
@@ -181,7 +181,7 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
             throw CredentialError.missingCoordinator
         }
 
-        metadata = try withIsolationSyncThrowing { @CredentialActor in
+        metadata = try CredentialActor.sync {
             let metadata = try Token.Metadata(token: self.token, tags: tags)
             try coordinator.tokenStorage.setMetadata(metadata)
             return metadata
@@ -239,7 +239,7 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
             throw CredentialError.missingCoordinator
         }
 
-        try withIsolationSyncThrowing { @CredentialActor in
+        try CredentialActor.sync {
             try coordinator.remove(credential: self)
         }
     }
@@ -282,7 +282,7 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
         if let coordinator,
             shouldRemove(for: type)
         {
-            try withIsolationSyncThrowing { @CredentialActor in
+            try await CredentialActor.run {
                 try coordinator.remove(credential: self)
             }
         }
@@ -376,27 +376,28 @@ public final class Credential: Equatable, OAuth2ClientDelegate {
     nonisolated(unsafe) private var _metadata: Token.Metadata?
     var metadata: Token.Metadata {
         get {
-            lock.withLock {
-                if let metadata = _metadata {
-                    return metadata
-                }
-
-                let result: Token.Metadata
-
-                let id = id
-                if let coordinator,
-                   let metadata = withIsolationSync({
-                       try? await coordinator.tokenStorage.metadata(for: id)
-                   })
-                {
-                    result = metadata
-                } else {
-                    result = Token.Metadata(id: id)
-                }
+            // Return cached value under lock, if present
+            if let cached = lock.withLock({ _metadata }) {
+                return cached
+            }
 
-                _metadata = result
-                return result
+            // Fetch from storage outside the lock to avoid both re-entrant
+            // lock access (self.id -> self.token -> lock) and holding the
+            // lock while blocking on withIsolationSync.
+            let result: Token.Metadata
+            let id = id
+            if let coordinator,
+               let metadata = CredentialActor.sync({
+                   try? coordinator.tokenStorage.metadata(for: id)
+               })
+            {
+                result = metadata
+            } else {
+                result = Token.Metadata(id: id)
             }
+
+            lock.withLock { _metadata = result }
+            return result
         }
         set {
             lock.withLock {

Sources/AuthFoundation/User Management/Internal/CredentialCoordinatorImpl.swift

@@ -208,14 +208,14 @@ extension CredentialCoordinatorImpl: OAuth2ClientDelegate {
             return
         }
 
-        withIsolationSync {
-            do {
-                try await self.tokenStorage.replace(token: token.id,
-                                                    with: newToken,
-                                                    security: nil)
-            } catch {
-                print("Error happened refreshing: \(error)")
+        do {
+            try CredentialActor.sync {
+                try self.tokenStorage.replace(token: token.id,
+                                              with: newToken,
+                                              security: nil)
             }
+        } catch {
+            print("Error happened refreshing: \(error)")
         }
     }
 }

Sources/AuthFoundation/Utilities/AsyncUtilities.swift

@@ -35,16 +35,93 @@ extension Task where Success == Never, Failure == Never {
     }
 }
 
+private final class DispatchQueueExecutor: SerialExecutor {
+     private let queue: DispatchQueue
+
+     init(queue: DispatchQueue) {
+         self.queue = queue
+     }
+
+     func enqueue(_ job: UnownedJob) {
+         self.queue.async {
+             job.runSynchronously(on: self.asUnownedSerialExecutor())
+         }
+     }
+
+     func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+         UnownedSerialExecutor(ordinary: self)
+     }
+
+    func checkIsolated() {
+        dispatchPrecondition(condition: .onQueue(self.queue))
+    }
+}
+
+/// Strips `@CredentialActor` isolation from a closure's type signature.
+///
+/// Swift's type system treats `@CredentialActor () -> T` and `() -> T` as
+/// distinct types. When executing on the actor's serial GCD queue (via
+/// `queue.sync`), we are *semantically* within the actor's isolation domain
+/// — the custom ``DispatchQueueExecutor`` guarantees this, and its
+/// `checkIsolated()` method provides a runtime assertion.
+///
+/// However, the compiler can't statically verify that `queue.sync` provides
+/// the actor's isolation. There is currently no first-class Swift API to
+/// express "Trust me, I'm on this global actor's executor." Until an API exists,
+/// `unsafeBitCast` is used to reinterpret the closure type.
+///
+/// This is safe because:
+/// 1. The closure's ABI representation is identical with or without the
+///    `@CredentialActor` attribute — it only affects compile-time checking.
+/// 2. The ``sync`` guarantees execution on the
+///    actor's serial queue before invoking the result.
+@inline(__always)
+private nonisolated func stripIsolation<T>(
+    _ body: @CredentialActor @Sendable @escaping () throws -> T
+) -> @Sendable () throws -> T {
+    unsafeBitCast(body, to: (@Sendable () throws -> T).self)
+}
+
 /// Shared actor used to coordinate multithreaded interactions within the Credential storage subsystem.
 @globalActor
 @_documentation(visibility: private)
 public final actor CredentialActor {
     public static let shared = CredentialActor()
-    
+
+    private let queue = DispatchQueue(label: "com.okta.credential-actor")
+    private let executor: DispatchQueueExecutor
+
+    private init() {
+        self.executor = DispatchQueueExecutor(queue: queue)
+    }
+
+    nonisolated public var unownedExecutor: UnownedSerialExecutor {
+        executor.asUnownedSerialExecutor()
+    }
+
     /// Convenience for running a block within the context of the ``CredentialActor``.
     /// - Parameter body: Block to execute.
     /// - Returns: Result of the block.
     public static func run<T: Sendable>(_ body: @CredentialActor @Sendable () throws -> T) async rethrows -> T {
         try await body()
     }
+
+    /// Synchronously executes a block on the ``CredentialActor``'s serial queue,
+    /// blocking the caller until completion.
+    ///
+    /// This dispatches directly to the actor's GCD queue using `DispatchQueue.sync`,
+    /// bypassing the Swift cooperative thread pool. This prevents deadlocks that can occur
+    /// when all cooperative threads are blocked by `DispatchGroup.wait()`.
+    ///
+    /// > Warning: This is intended for the exclusive use of synchronous use-cases that interact with the credential storage sub-system. It may be made public at some point, but is being kept `internal` until it is required.
+    ///
+    /// - Parameter body: Block to execute on the actor's serial queue.
+    /// - Returns: The result of the block.
+    /// - Throws: Any error thrown by the block.
+    nonisolated static func sync<T: Sendable>(_ body: @CredentialActor @Sendable @escaping () throws -> T) rethrows -> T {
+        let rawBody = stripIsolation(body)
+        return try shared.queue.sync {
+            try rawBody()
+        }
+    }
 }

Tests/AuthFoundationTests/CredentialRevokeTests.swift

@@ -52,7 +52,7 @@ final class CredentialTests: XCTestCase {
 
 
     func testRemove() async throws {
-        XCTAssertNoThrow(try credential.remove())
+        try credential.remove()
 
         let hasCredential = await coordinator.credentialDataSource.hasCredential(for: token)
         XCTAssertFalse(hasCredential)