diff --git a/TypeWhisperPluginSDK/Plugins/AssemblyAIPlugin/AssemblyAIPlugin.swift b/TypeWhisperPluginSDK/Plugins/AssemblyAIPlugin/AssemblyAIPlugin.swift index b645fffc..9080dfbf 100644 --- a/TypeWhisperPluginSDK/Plugins/AssemblyAIPlugin/AssemblyAIPlugin.swift +++ b/TypeWhisperPluginSDK/Plugins/AssemblyAIPlugin/AssemblyAIPlugin.swift @@ -352,7 +352,8 @@ final class AssemblyAIPlugin: NSObject, StructuredTranscriptionEnginePlugin, Dic for _ in 0..<300 { try await Task.sleep(for: .seconds(1)) - let (data, response) = try await PluginHTTPClient.data(for: request) + // Same shape as the other pollers: the loop IS the retry, so it opts out. + let (data, response) = try await PluginHTTPClient.data(for: request, retry: .disabled) guard let httpResponse = response as? HTTPURLResponse, httpResponse.statusCode == 200 else { continue diff --git a/TypeWhisperPluginSDK/Plugins/GladiaPlugin/GladiaPlugin.swift b/TypeWhisperPluginSDK/Plugins/GladiaPlugin/GladiaPlugin.swift index 390f4a0a..47793f79 100644 --- a/TypeWhisperPluginSDK/Plugins/GladiaPlugin/GladiaPlugin.swift +++ b/TypeWhisperPluginSDK/Plugins/GladiaPlugin/GladiaPlugin.swift @@ -418,7 +418,8 @@ final class GladiaPlugin: NSObject, TranscriptionEnginePlugin, LanguageHintTrans for _ in 0..<300 { try await Task.sleep(for: .seconds(1)) - let (data, response) = try await PluginHTTPClient.data(for: request) + // The 300-iteration loop is already the retry. + let (data, response) = try await PluginHTTPClient.data(for: request, retry: .disabled) guard let httpResponse = response as? HTTPURLResponse, httpResponse.statusCode == 200 else { continue } diff --git a/TypeWhisperPluginSDK/Plugins/SonioxPlugin/SonioxPlugin.swift b/TypeWhisperPluginSDK/Plugins/SonioxPlugin/SonioxPlugin.swift index b2e07cc3..6610521f 100644 --- a/TypeWhisperPluginSDK/Plugins/SonioxPlugin/SonioxPlugin.swift +++ b/TypeWhisperPluginSDK/Plugins/SonioxPlugin/SonioxPlugin.swift @@ -1571,7 +1571,9 @@ final class SonioxPlugin: NSObject, request.timeoutInterval = 10 do { - let (_, response) = try await PluginHTTPClient.data(for: request) + // Teardown that a finished transcript is awaited behind. Retrying here would + // delay a result the user already has. + let (_, response) = try await PluginHTTPClient.data(for: request, retry: .disabled) guard let httpResponse = response as? HTTPURLResponse else { cleanupLogger.warning("Soniox transcription cleanup received a non-HTTP response") return .failed diff --git a/TypeWhisperPluginSDK/Plugins/SpeechmaticsPlugin/SpeechmaticsPlugin.swift b/TypeWhisperPluginSDK/Plugins/SpeechmaticsPlugin/SpeechmaticsPlugin.swift index 200d7868..29377ad9 100644 --- a/TypeWhisperPluginSDK/Plugins/SpeechmaticsPlugin/SpeechmaticsPlugin.swift +++ b/TypeWhisperPluginSDK/Plugins/SpeechmaticsPlugin/SpeechmaticsPlugin.swift @@ -329,7 +329,9 @@ final class SpeechmaticsPlugin: NSObject, TranscriptionEnginePlugin, DictionaryT for _ in 0..<300 { try await Task.sleep(for: .seconds(1)) - let (data, response) = try await PluginHTTPClient.data(for: statusRequest) + // This loop already re-issues on any non-200, up to 300 times. A ladder here + // would multiply the loop's own bound rather than add resilience. + let (data, response) = try await PluginHTTPClient.data(for: statusRequest, retry: .disabled) guard let httpResponse = response as? HTTPURLResponse, httpResponse.statusCode == 200 else { continue diff --git a/TypeWhisperPluginSDK/Plugins/WebhookPlugin/WebhookPlugin.swift b/TypeWhisperPluginSDK/Plugins/WebhookPlugin/WebhookPlugin.swift index 281e1750..60c28b6f 100644 --- a/TypeWhisperPluginSDK/Plugins/WebhookPlugin/WebhookPlugin.swift +++ b/TypeWhisperPluginSDK/Plugins/WebhookPlugin/WebhookPlugin.swift @@ -396,7 +396,10 @@ final class ExampleWebhookService: ObservableObject, @unchecked Sendable { do { request.httpBody = try JSONEncoder().encode(payload) - let (_, response) = try await PluginHTTPClient.data(for: request) + // Opted out for two reasons: the method here is user-configured and often + // side-effecting, and this caller already retries once below. Laddering + // underneath that would multiply deliveries. + let (_, response) = try await PluginHTTPClient.data(for: request, retry: .disabled) let statusCode = (response as? HTTPURLResponse)?.statusCode ?? 0 let success = (200...299).contains(statusCode) diff --git a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift index e09752f1..11d03fa8 100644 --- a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift +++ b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift @@ -84,6 +84,30 @@ public extension HostServices { @_spi(Testing) extension URLSession: PluginHTTPClientSession {} +/// Whether a request rides the transient-failure retry ladder. +/// +/// Retries are ON BY DEFAULT, so an ordinary plugin call rides out a brief upstream +/// failure without changing. `.disabled` restores exactly the behaviour that existed +/// before the ladder: one immediate retry after a session reset on a stale-connection +/// error, and nothing else. +/// +/// Opt out where the caller is already looping, already retrying, or is holding +/// something the user is waiting on: +/// - polling loops that re-issue on a non-200 anyway, where a ladder multiplies the +/// loop's own bound; +/// - callers with their own retry, where two layers compound; +/// - teardown that a finished result is blocked behind. +public struct PluginHTTPRetryPolicy: Sendable, Equatable { + public let laddersTransientFailures: Bool + + public static let `default` = PluginHTTPRetryPolicy(laddersTransientFailures: true) + public static let disabled = PluginHTTPRetryPolicy(laddersTransientFailures: false) + + public init(laddersTransientFailures: Bool) { + self.laddersTransientFailures = laddersTransientFailures + } +} + /// Drop-in replacement for `URLSession.shared.data(for:)` that reuses one ephemeral /// session so fast plugin requests can keep DNS/TLS/HTTP connections warm. public enum PluginHTTPClient { @@ -91,14 +115,70 @@ public enum PluginHTTPClient { private static let defaultRequestTimeout: TimeInterval = 30 private static let longRunningResourceTimeout: TimeInterval = 600 private static let lock = NSLock() + + /// Budget for retry SCHEDULING, not for the whole operation. + /// + /// It bounds the sum of the backoff sleeps: no retry sleep begins after it. It + /// does NOT bound elapsed time, and calling it a wall-clock budget would be wrong. + /// A request started just inside the deadline still runs its own timeout, so the + /// true worst case is this budget plus one request timeout (30 s by default, and + /// some callers set 120 s or 600 s). Bounding in-flight time would mean cancelling + /// live requests, which is a larger change than this one. + /// + /// 25 s sits above Nielsen's 10 s "you owe a progress indicator" threshold and + /// below the roughly 30 s at which users report frustration. No primary source + /// gives a ceiling for a user-facing retry, so this is a synthesis, and it is + /// deliberately conservative because a failed dictation preserves its recording. + static let retrySchedulingBudget: Duration = .seconds(25) + static let retryBaseDelay: Duration = .milliseconds(500) + /// Per-delay ceiling. Note the ladder `retryMaxAttempts` permits ends at + /// exactly this value (0.5s * 2^4), so under the current bound the cap never + /// actually binds and is carried defensively, for if that bound is raised. + static let retryMaxDelay: Duration = .seconds(8) + /// Total attempts, initial included, so at most five retries. + /// + /// The budget alone is not a sufficient bound: full jitter draws from + /// `random(0, capped)`, so an endpoint that fails instantly can draw a run of + /// near-zero delays and burn a great many attempts inside 25 s. Un-jittered the + /// ladder here is 0.5 + 1 + 2 + 4 + 8 = 15.5 s, comfortably inside the budget, so + /// in practice attempts bind first and the budget catches the slow cases: a + /// long-running request, or a `Retry-After` that would overshoot. + static let retryMaxAttempts = 6 + + /// Injectable so tests assert the SCHEDULE without sleeping through it. + nonisolated(unsafe) private static var _sleeper: @Sendable (Duration) async throws -> Void = { + try await Task.sleep(for: $0) + } + private static var sleeper: @Sendable (Duration) async throws -> Void { + lock.withLock { _sleeper } + } + /// Injectable so tests see a deterministic ladder instead of jittered values. + nonisolated(unsafe) private static var _jitterFraction: @Sendable () -> Double = { + Double.random(in: 0...1) + } + private static var jitterFraction: @Sendable () -> Double { + lock.withLock { _jitterFraction } + } nonisolated(unsafe) private static var sharedSession: (any PluginHTTPClientSession)? nonisolated(unsafe) private static var sessionFactory: (URLSessionConfiguration) -> any PluginHTTPClientSession = { URLSession(configuration: $0) } + /// Kept as a distinct one-argument overload, NOT collapsed into a defaulted + /// parameter on the call below. Nine call sites pass `PluginHTTPClient.data` as an + /// unapplied function reference typed + /// `@Sendable (URLRequest) async throws -> (Data, URLResponse)`, and a defaulted + /// parameter does not preserve that type. public static func data(for request: URLRequest) async throws -> (Data, URLResponse) { + try await data(for: request, retry: .default) + } + + public static func data( + for request: URLRequest, + retry policy: PluginHTTPRetryPolicy + ) async throws -> (Data, URLResponse) { try ensureNetworkAccessIsAllowed() - return try await data(for: request, allowsRetry: true) + return try await dataWithRetries(for: request, policy: policy) } public static func data( @@ -107,7 +187,7 @@ public enum PluginHTTPClient { ) async throws -> (Data, URLResponse) { try ensureNetworkAccessIsAllowed() guard let resourceTimeout, resourceTimeout > longRunningResourceTimeout else { - return try await data(for: request, allowsRetry: true) + return try await dataWithRetries(for: request, policy: .default) } let config = URLSessionConfiguration.ephemeral @@ -164,42 +244,293 @@ public enum PluginHTTPClient { } } + /// Replaces the sleep and jitter sources so a test asserts the retry SCHEDULE + /// deterministically instead of sleeping through it. `jitterFraction` returning 1 + /// gives the un-jittered upper bound of the ladder, which is the readable case to + /// assert against. + @_spi(Testing) public static func configureRetryForTesting( + sleeper newSleeper: @escaping @Sendable (Duration) async throws -> Void, + jitterFraction newJitter: @escaping @Sendable () -> Double = { 1.0 } + ) { + lock.withLock { + _sleeper = newSleeper + _jitterFraction = newJitter + } + } + @_spi(Testing) public static func resetTestingHooks() { resetSharedSession(reason: "test cleanup") lock.withLock { sessionFactory = { URLSession(configuration: $0) } + _sleeper = { try await Task.sleep(for: $0) } + _jitterFraction = { Double.random(in: 0...1) } } } - private static func data( + /// Runs `request` against the shared session, retrying transient failures. + /// + /// Two failure shapes reach this and they are not the same: + /// + /// - A thrown `URLError`. The first retry is IMMEDIATE after resetting the shared + /// session, and only for the stale-pooled-connection codes that reset actually + /// fixes. This is the behaviour that existed before the ladder and is preserved + /// verbatim, including under `.disabled`. + /// - A delivered response carrying a retryable status. This never threw, so before + /// the ladder it went straight back to the plugin. That is the gap that let a + /// Cloudflare 522 in front of a transcription API fail a dictation with no retry. + /// + /// On exhaustion the last response is RETURNED, not thrown, so the caller still + /// sees the real status and body. Note two in-repo callers ignore the response + /// entirely, so an exhausted 503 reads to them as success; that predates this and + /// is called out in the pull request rather than silently relied upon. + private static func dataWithRetries( for request: URLRequest, - allowsRetry: Bool + policy: PluginHTTPRetryPolicy ) async throws -> (Data, URLResponse) { - let session = sharedOrCreateSession() + let deadline = ContinuousClock.now + retrySchedulingBudget let method = request.httpMethod ?? "GET" let url = request.url?.absoluteString ?? "unknown" - logger.info("\(method) \(url)") - let start = ContinuousClock.now + var attempt = 0 + var usedRetryAfterGrace = false + + while true { + let session = sharedOrCreateSession() + logger.info("\(method) \(url) (attempt \(attempt + 1))") + let start = ContinuousClock.now + + do { + let (data, response) = try await session.data(for: request) + let elapsed = ContinuousClock.now - start + let status = (response as? HTTPURLResponse)?.statusCode ?? 0 + logger.info("\(method) \(url) -> \(status) (\(elapsed))") + + guard policy.laddersTransientFailures, + let http = response as? HTTPURLResponse + else { + return (data, response) + } + + let decision = retryAfterDecision(from: http) + + // A well-formed Retry-After beyond the honoured ceiling is a refusal, not + // a delay: the server is asking us to stay away for longer than we are + // ever willing to sleep. Stop, rather than falling through to the ladder + // and retrying almost immediately. + if case .refusal = decision { + logger.warning("\(method) \(url) -> \(status), Retry-After beyond the honoured ceiling, treating as a refusal and not retrying") + return (data, response) + } + + let retryAfter: Duration? + if case let .after(delay) = decision { + retryAfter = delay + } else { + retryAfter = nil + } + + // 429: one retry, only on an explicit Retry-After that fits. + if isRetryAfterOnlyStatus(http.statusCode) { + guard !usedRetryAfterGrace, + attempt + 1 < retryMaxAttempts, + let retryAfter, + retryAfter <= deadline - ContinuousClock.now + else { + return (data, response) + } + usedRetryAfterGrace = true + attempt += 1 + logger.warning("\(method) \(url) -> 429, honouring Retry-After \(retryAfter) once") + try await sleeper(retryAfter) + continue + } + + guard isRetryableStatus(http.statusCode, method: method) else { + return (data, response) + } + guard attempt + 1 < retryMaxAttempts, + let delay = backoffDelay(forAttempt: attempt, deadline: deadline, retryAfter: retryAfter) + else { + logger.warning("\(method) \(url) -> \(status), retries exhausted after \(attempt + 1) attempt(s)") + return (data, response) + } + + attempt += 1 + logger.warning("\(method) \(url) -> \(status), retrying in \(delay) (attempt \(attempt + 1))") + try await sleeper(delay) + } catch { + let elapsed = ContinuousClock.now - start + guard isTransientNetworkError(error) else { + logger.error("\(method) \(url) failed after \(elapsed): \(error.localizedDescription)") + throw error + } - do { - let (data, response) = try await session.data(for: request) - let elapsed = ContinuousClock.now - start - let status = (response as? HTTPURLResponse)?.statusCode ?? 0 - logger.info("\(method) \(url) -> \(status) (\(elapsed))") - return (data, response) - } catch { - let elapsed = ContinuousClock.now - start - if allowsRetry, isTransientNetworkError(error) { - logger.warning("\(method) \(url) transient failure after \(elapsed), resetting session and retrying once: \(error.localizedDescription)") resetSharedSession(matching: session, reason: "transient network error") - return try await data(for: request, allowsRetry: false) + + // Compatibility, and it applies under BOTH policies: one immediate + // retry after the reset, for ANY transient error. This is exactly what + // this client did before the ladder existed, and narrowing it would be + // an alteration rather than an addition. A poll loop that opts out with + // `.disabled` therefore behaves precisely as it did before. + if attempt == 0 { + attempt += 1 + logger.warning("\(method) \(url) transient failure after \(elapsed), reset session, retrying immediately: \(error.localizedDescription)") + continue + } + + // Everything past that first retry is new, and is gated the same way + // the status ladder is. A POST can time out AFTER the origin processed + // it, so laddering a non-idempotent request risks duplicating the work. + guard policy.laddersTransientFailures, + isIdempotentMethod(method), + attempt + 1 < retryMaxAttempts, + let delay = backoffDelay(forAttempt: attempt, deadline: deadline, retryAfter: nil) + else { + logger.error("\(method) \(url) transient failure after \(elapsed), not retrying further: \(error.localizedDescription)") + throw error + } + + attempt += 1 + logger.warning("\(method) \(url) transient failure after \(elapsed), retrying in \(delay) (attempt \(attempt + 1)): \(error.localizedDescription)") + try await sleeper(delay) } + } + } + + /// Whether a delivered status is worth retrying FOR THIS REQUEST'S METHOD. + /// + /// The axis is not "is this a server error" but "could the origin already have + /// applied the request". This client is shared by plugins that POST + /// side-effecting requests: `WebhookPlugin` delivers a user-configured method, + /// `LinearPlugin` runs GraphQL mutations, `OpenAIVectorMemoryPlugin` uploads and + /// attaches files. Duplicating those is worse than failing. + /// + /// - Always safe: the request provably never reached a working origin. 408 was + /// never received; Cloudflare 521 (origin down), 522 (connection timed out), + /// 523 (origin unreachable) and 525/526 (TLS handshake failed) all fail before + /// the origin sees a byte. The failure that motivated this work was a 522 on a + /// POST, and it stays retried for every method. + /// - Idempotent methods only: 502, 503, 504, 520 and 524 do NOT establish that + /// the origin skipped the work. Cloudflare documents 524 as the connection + /// having been established with no timely answer, so the origin may still + /// complete it. + /// + /// 503 sits here rather than above, which is a change of mind. Its semantics do + /// say the origin declined to handle the request, and on that reasoning it was + /// originally any-method. But two independent reviewers pointed at the same + /// concrete exposure: `AssemblyAIPlugin.submitTranscription` POSTs job creation + /// through the default policy, so a 503 returned after the job was created would + /// resubmit it, and Linear mutations and vector-store uploads have the same + /// shape. A semantic argument does not outweigh a duplicate transcription job, + /// and the outage case this work exists for is a 52x, which is unaffected. + /// - Never: 500, which can mean the origin accepted the work and then failed + /// partway, and 429, which is a deliberate refusal the origin explained. See + /// `retryAfterOnlyStatuses` for how 429 is handled instead. + static func isRetryableStatus(_ status: Int, method: String) -> Bool { + switch status { + case 408, 521, 522, 523, 525, 526: + return true + case 502, 503, 504, 520, 524: + return isIdempotentMethod(method) + default: + return false + } + } + + /// RFC 9110 section 9.2.2: these are safe to repeat. POST and PATCH are not. + static func isIdempotentMethod(_ method: String) -> Bool { + switch method.uppercased() { + case "GET", "HEAD", "PUT", "DELETE", "OPTIONS", "TRACE": + return true + default: + return false + } + } + + /// 429 gets exactly one retry, and only when the origin said when to come back. + /// + /// Not laddered. The plugins above already map 429 to a rate-limit or quota error, + /// and a quota will not clear inside this budget. But a provider that sends + /// `Retry-After: 2` on a burst throttle is telling us something actionable, and + /// ignoring it is pessimistic. No header means no retry. + static func isRetryAfterOnlyStatus(_ status: Int) -> Bool { + status == 429 + } + + /// Full jitter: `random(0, min(cap, base * 2^attempt))`, the shipped consensus. + /// Returns nil when nothing more fits inside the budget, which is the signal to + /// stop. A `Retry-After` longer than the remaining budget also stops rather than + /// sleeping past the deadline. + static func backoffDelay( + forAttempt attempt: Int, + deadline: ContinuousClock.Instant, + retryAfter: Duration? + ) -> Duration? { + let remaining = deadline - ContinuousClock.now + guard remaining > .zero else { return nil } + + if let retryAfter { + return retryAfter <= remaining ? retryAfter : nil + } - logger.error("\(method) \(url) failed after \(elapsed): \(error.localizedDescription)") - throw error + // Arithmetic in seconds rather than on Duration: explicit, and it keeps the + // jitter multiply off Duration's operator surface. + let base = seconds(of: retryBaseDelay) + let cap = seconds(of: retryMaxDelay) + // Bounded shift so a long-lived ladder cannot overflow; the cap makes it moot. + let growth = Double(1 << min(max(attempt, 0), 20)) + let capped = min(base * growth, cap) + let jittered = Duration.seconds(capped * jitterFraction()) + guard jittered <= remaining else { return nil } + return jittered + } + + static func seconds(of duration: Duration) -> Double { + let parts = duration.components + return Double(parts.seconds) + Double(parts.attoseconds) * 1e-18 + } + + /// Parses the delta-seconds form of `Retry-After`. + /// + /// Parsed as an INTEGER, which is what RFC 9110 defines delta-seconds to be, and + /// clamped. That is not tidiness: `Double("999999999999999999999999")` is finite + /// and non-negative, passes an `isFinite` guard, and then TRAPS inside + /// `Duration.seconds(_:)` with an overflow in multiplication, killing the process. + /// A hostile or merely broken origin could crash the app from a response header. + /// + /// The HTTP-date form is not honoured. It needs clock-skew handling to be safe and + /// falls through to the ordinary ladder instead. + /// How to treat a `Retry-After` on a retryable response. + enum RetryAfterDecision: Equatable { + /// No usable header: absent, empty, non-integer, or negative. Fall through to + /// the ordinary backoff ladder. + case none + /// A usable delay. Honour it, subject to the remaining budget. + case after(Duration) + /// A well-formed delta-seconds beyond `maxHonouredRetryAfterSeconds`. This is a + /// refusal, not a delay, so we do not retry at all. + case refusal + } + + static func retryAfterDecision(from response: HTTPURLResponse) -> RetryAfterDecision { + guard let raw = response.value(forHTTPHeaderField: "Retry-After")? + .trimmingCharacters(in: .whitespaces), + let seconds = Int(raw), + seconds >= 0 + else { + // Absent, empty, non-integer, or negative. Note an oversized value like + // "999999999999999999999999" also lands here: it overflows Int and parses + // as nil, so it is treated as an unusable header, not a refusal. + return .none + } + if seconds > maxHonouredRetryAfterSeconds { + return .refusal } + return .after(.seconds(seconds)) } + /// A day. Anything longer is not a delay, it is a refusal, and we do not sleep on + /// it. Also keeps the value far below the range where Duration arithmetic traps. + static let maxHonouredRetryAfterSeconds = 86_400 private static func sharedOrCreateSession() -> any PluginHTTPClientSession { lock.withLock { if let sharedSession { @@ -219,23 +550,6 @@ public enum PluginHTTPClient { return config } - private static func resetSharedSession(matching session: any PluginHTTPClientSession, reason: String) { - let didRemoveSharedSession = lock.withLock { - guard let current = sharedSession, current === session else { - return false - } - sharedSession = nil - return true - } - - session.finishTasksAndInvalidate() - if didRemoveSharedSession { - logger.info("Reset shared plugin HTTP session: \(reason)") - } else { - logger.info("Invalidated plugin HTTP session after \(reason)") - } - } - private static func isTransientNetworkError(_ error: Error) -> Bool { guard let urlError = error as? URLError else { return false @@ -253,6 +567,23 @@ public enum PluginHTTPClient { return false } } + private static func resetSharedSession(matching session: any PluginHTTPClientSession, reason: String) { + let didRemoveSharedSession = lock.withLock { + guard let current = sharedSession, current === session else { + return false + } + sharedSession = nil + return true + } + + session.finishTasksAndInvalidate() + if didRemoveSharedSession { + logger.info("Reset shared plugin HTTP session: \(reason)") + } else { + logger.info("Invalidated plugin HTTP session after \(reason)") + } + } + } // MARK: - WAV Encoder Utility diff --git a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDKTesting/PluginTestSupport.swift b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDKTesting/PluginTestSupport.swift index a74e7edf..c5f42b4f 100644 --- a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDKTesting/PluginTestSupport.swift +++ b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDKTesting/PluginTestSupport.swift @@ -259,10 +259,21 @@ public final class PluginTestHostServices: HostServices, HostModelLifecyclePolic } @_spi(Testing) public enum PluginHTTPClientTestHarness { + /// Installs a mock session AND a no-op sleeper. + /// + /// The sleeper matters: a mock whose last outcome is a sticky transient failure or + /// a sticky 503 now drives the real retry ladder, so without this a single plugin + /// test can sleep for tens of seconds and its duration becomes non-deterministic + /// in CI. Pass `laddersTransientFailures: true` only when the test is deliberately + /// exercising retry timing. public static func configure( - _ factory: @escaping (URLSessionConfiguration) -> PluginHTTPClientMockSession + _ factory: @escaping (URLSessionConfiguration) -> PluginHTTPClientMockSession, + sleepsForRealBackoff: Bool = false ) { PluginHTTPClient.configureForTesting(factory) + if !sleepsForRealBackoff { + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + } } public static func reset() { diff --git a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift index 502f615d..09d02cd8 100644 --- a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift +++ b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift @@ -97,6 +97,475 @@ final class PluginHTTPClientTests: XCTestCase { ) } + // MARK: - Transient HTTP STATUS retry + // + // Before this ladder existed the client retried only THROWN URLErrors. A + // delivered response carrying 503, or Cloudflare's 522, never threw, so it was + // handed straight back to the plugin and failed the dictation outright. + + func testRetriesRetryableStatusThenSucceeds() async throws { + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(522)), + .success(Self.okResponse()), + ]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (data, response) = try await PluginHTTPClient.data(for: Self.request(path: "/flaky")) + + XCTAssertEqual(String(data: data, encoding: .utf8), "ok") + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/flaky", "/flaky"]) + let delays = await recorder.delays + XCTAssertEqual(delays, [.milliseconds(500)]) + } + + func testCloudflare522IsRetried() async throws { + // The 2026-09-03 incident shape: Cloudflare in front of a transcription API + // answering 522 while the origin was unreachable. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(522)), + .success(Self.okResponse()), + ]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/522")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) + XCTAssertEqual(store.sessions.first?.requestedPaths.count, 2) + } + + func testDoesNotRetryNonRetryableStatus() async throws { + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(400)), + .success(Self.okResponse()), + ]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/bad")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 400) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/bad"]) + } + + func testDoesNotRetryHeaderless429() async throws { + // No header means the origin told us nothing actionable, and the plugins above + // already map 429 to a quota or rate-limit error. A quota will not clear inside + // this budget. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(429)), + .success(Self.okResponse()), + ]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/429")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 429) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/429"]) + let delays = await recorder.delays + XCTAssertTrue(delays.isEmpty, "a headerless rate limit must reach the plugin at once") + } + + func testRetries429ExactlyOnceWhenRetryAfterSaysWhen() async throws { + // A provider throttling a burst sends Retry-After with a small value. Honouring + // it once is actionable; laddering is not, so the grace is single-shot. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(429, retryAfter: "2")), + .success(Self.statusResponse(429, retryAfter: "2")), + .success(Self.okResponse()), + ]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/429ra")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 429, + "the grace is one retry, so the second 429 is returned") + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/429ra", "/429ra"]) + let delays = await recorder.delays + XCTAssertEqual(delays, [.seconds(2)]) + } + + func testOversizedRetryAfterIsIgnoredRatherThanCrashing() async throws { + // Regression. Double("999999999999999999999999") is finite and non-negative, so + // an isFinite guard passes it, and Duration.seconds then TRAPS on overflow, + // killing the process. Reachable from any provider's proxy, mid-dictation. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(522, retryAfter: "999999999999999999999999")), + .success(Self.okResponse()), + ]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }, jitterFraction: { 1.0 }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/huge-ra")) + + // Survives, ignores the unusable header, and falls back to the ordinary ladder. + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) + let delays = await recorder.delays + XCTAssertEqual(delays, [.milliseconds(500)]) + } + + func testGatewayStatusesAreRetriedOnlyForIdempotentMethods() async throws { + // 504 and Cloudflare 524 do NOT establish that the origin skipped the work, so + // repeating a POST could duplicate it. + let post = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + post.makeSession(outcomes: [.success(Self.statusResponse(504)), .success(Self.okResponse())]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + var postRequest = Self.request(path: "/504") + postRequest.httpMethod = "POST" + let (_, postResponse) = try await PluginHTTPClient.data(for: postRequest) + XCTAssertEqual((postResponse as? HTTPURLResponse)?.statusCode, 504) + XCTAssertEqual(post.sessions.first?.requestedPaths, ["/504"]) + + PluginHTTPClient.resetTestingHooks() + let get = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + get.makeSession(outcomes: [.success(Self.statusResponse(504)), .success(Self.okResponse())]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + var getRequest = Self.request(path: "/504") + getRequest.httpMethod = "GET" + let (_, getResponse) = try await PluginHTTPClient.data(for: getRequest) + XCTAssertEqual((getResponse as? HTTPURLResponse)?.statusCode, 200, + "a GET is safe to repeat, so it rides the ladder") + XCTAssertEqual(get.sessions.first?.requestedPaths, ["/504", "/504"]) + } + + func testDisabledPolicySkipsTheLadder() async throws { + // Opt-out for pollers, self-retrying callers, and teardown. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [.success(Self.statusResponse(522)), .success(Self.okResponse())]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (_, response) = try await PluginHTTPClient.data( + for: Self.request(path: "/opted-out"), retry: .disabled + ) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 522) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/opted-out"]) + let delays = await recorder.delays + XCTAssertTrue(delays.isEmpty) + } + + func testTimeoutStillGetsTheCompatibilityImmediateRetry() async throws { + // This asserted the OPPOSITE until CodeRabbit caught it. Narrowing the + // immediate retry to stale-pool codes ALTERED pre-existing behaviour rather + // than adding to it, and it meant one timeout aborted the very poll loops that + // opt out with `.disabled`. Any transient error still gets one immediate retry + // after the session reset, exactly as before the ladder existed. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + if store.sessions.isEmpty { + return store.makeSession(outcomes: [.failure(URLError(.timedOut))]) + } + return store.makeSession(outcomes: [.success(Self.okResponse())]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/slow")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) + let delays = await recorder.delays + XCTAssertTrue(delays.isEmpty, "the compatibility retry is immediate, not backed off") + } + + func testDisabledPolicyStillGetsTheCompatibilityTransportRetry() async throws { + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + if store.sessions.isEmpty { + return store.makeSession(outcomes: [.failure(URLError(.timedOut))]) + } + return store.makeSession(outcomes: [.success(Self.okResponse())]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + + let (_, response) = try await PluginHTTPClient.data( + for: Self.request(path: "/opted-out-transient"), retry: .disabled + ) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200, + "an opted-out poll loop must survive a transient error as it did before") + } + + func testLadderedTransportRetriesAreIdempotentOnly() async throws { + // A POST can time out AFTER the origin processed it, so it gets the single + // compatibility retry and no ladder. + let post = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + post.makeSession(outcomes: [.failure(URLError(.timedOut))]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + var postRequest = Self.request(path: "/post-timeout") + postRequest.httpMethod = "POST" + do { + _ = try await PluginHTTPClient.data(for: postRequest) + XCTFail("a POST must not ride the transport ladder") + } catch { + XCTAssertEqual((error as? URLError)?.code, .timedOut) + } + XCTAssertEqual(post.sessions.flatMap(\.requestedPaths).count, 2, + "initial attempt plus the one compatibility retry") + + PluginHTTPClient.resetTestingHooks() + let get = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + get.makeSession(outcomes: [.failure(URLError(.timedOut))]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + var getRequest = Self.request(path: "/get-timeout") + getRequest.httpMethod = "GET" + do { + _ = try await PluginHTTPClient.data(for: getRequest) + XCTFail("expected exhaustion") + } catch { + XCTAssertEqual((error as? URLError)?.code, .timedOut) + } + XCTAssertEqual(get.sessions.flatMap(\.requestedPaths).count, + PluginHTTPClient.retryMaxAttempts, + "a GET is safe to repeat, so it uses the whole ladder") + } + + func testDoesNotRetry503OnANonIdempotentRequest() async throws { + // Conceded after two independent reviewers pointed at the same exposure: + // AssemblyAIPlugin.submitTranscription POSTs job creation through the default + // policy, so a 503 returned after the job was created would resubmit it. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [.success(Self.statusResponse(503)), .success(Self.okResponse())]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + var post = Self.request(path: "/503") + post.httpMethod = "POST" + + let (_, response) = try await PluginHTTPClient.data(for: post) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 503) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/503"]) + } + + func testRetries503OnAnIdempotentRequest() async throws { + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [.success(Self.statusResponse(503)), .success(Self.okResponse())]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + var get = Self.request(path: "/503") + get.httpMethod = "GET" + + let (_, response) = try await PluginHTTPClient.data(for: get) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/503", "/503"]) + } + + func testDoesNotRetryServerError500() async throws { + // Deliberate exclusion: this client is shared by plugins that POST + // side-effecting requests, and a 500 can mean the origin accepted the work + // and then failed partway. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(500)), + .success(Self.okResponse()), + ]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/500")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 500) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/500"]) + } + + func testReturnsLastResponseWhenAttemptsAreExhausted() async throws { + // Exhaustion RETURNS the real response rather than throwing, so the caller + // still sees the status and body and renders its own error. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [.success(Self.statusResponse(522))]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/always-503")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 522) + XCTAssertEqual(store.sessions.first?.requestedPaths.count, PluginHTTPClient.retryMaxAttempts) + } + + func testBackoffScheduleDoublesAcrossTheWholeLadder() async throws { + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [.success(Self.statusResponse(522))]) + } + let recorder = DelayRecorder() + // jitterFraction 1.0 gives the un-jittered upper bound, which is the readable + // thing to assert; full jitter draws uniformly below each of these. + PluginHTTPClient.configureRetryForTesting( + sleeper: { await recorder.record($0) }, + jitterFraction: { 1.0 } + ) + + _ = try await PluginHTTPClient.data(for: Self.request(path: "/ladder")) + + let delays = await recorder.delays + XCTAssertEqual( + delays, + [.milliseconds(500), .seconds(1), .seconds(2), .seconds(4), .seconds(8)] + ) + } + + func testBackoffIsCappedAtMaxDelayBeyondTheLadder() async throws { + // The ladder that `retryMaxAttempts` permits tops out at exactly + // `retryMaxDelay`, so the cap is a no-op for every delay the loop actually + // takes and the schedule test above cannot exercise it. Mutation testing + // caught that: deleting `min(..., retryMaxDelay)` left the whole suite green. + // This asserts the cap directly, at an attempt the current bound never + // reaches, so the cap stays honest if that bound is ever raised. + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }, jitterFraction: { 1.0 }) + let deadline = ContinuousClock.now + .seconds(600) + + let capped = PluginHTTPClient.backoffDelay(forAttempt: 10, deadline: deadline, retryAfter: nil) + + // Uncapped this would be 0.5s * 2^10 = 512s. + XCTAssertEqual(capped, PluginHTTPClient.retryMaxDelay) + } + + func testHonoursRetryAfterHeaderWithinBudget() async throws { + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(522, retryAfter: "3")), + .success(Self.okResponse()), + ]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/503-retry-after")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) + let delays = await recorder.delays + XCTAssertEqual(delays, [.seconds(3)], "Retry-After must win over the ladder") + } + + func testRetryAfterDecisionClassifiesAboveCeilingAsRefusal() { + // Unit-level pin on the classifier itself: a well-formed delta-seconds one second past + // the one-day ceiling is a refusal, not an absent header. This is what distinguishes the + // fix from the old behaviour at the decision boundary, independent of the retry loop. + let response = Self.statusResponse(503, retryAfter: "86401").1 as! HTTPURLResponse + XCTAssertEqual(PluginHTTPClient.retryAfterDecision(from: response), .refusal) + } + + func testValidRetryAfterAboveCeilingRefusesRatherThanRetryingFast() async throws { + // Regression for the clamp bug. A well-formed Retry-After of 86401 (one second past the + // one-day ceiling) is a refusal, not a delay. The old code collapsed it into the same nil + // as an absent header, so a retryable status fell through to the ordinary ladder and + // retried with the same backoff an absent header would produce, when a Retry-After that + // large is a refusal that should stop the retries. + // + // The request MUST be a GET: this test exercises the isRetryableStatus ladder, and a 503 + // is retryable only for idempotent methods. With the default POST it would return without + // retrying regardless of the fix, so the test would pass either way and pin nothing. + // With a GET, the pre-fix code retries once at ~0.5s (two requests, one sleep); the fixed + // code refuses (one request, no sleep). The 25s deadline is not the limiter here: a single + // 0.5s retry fits it, so what stops the retry is the refusal, not the budget. + var request = Self.request(path: "/refusal") + request.httpMethod = "GET" + request.httpBody = nil + + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [ + .success(Self.statusResponse(503, retryAfter: "86401")), + .success(Self.okResponse()), + ]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }, jitterFraction: { 1.0 }) + + let (_, response) = try await PluginHTTPClient.data(for: request) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 503, "must not retry past a refusal") + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/refusal"], "exactly one request") + let delays = await recorder.delays + XCTAssertTrue(delays.isEmpty, "must not sleep, and must not fall through to the ordinary ladder") + } + + func testStopsWhenRetryAfterExceedsRemainingBudget() async throws { + // Sleeping past the deadline is worse than giving up: the user is waiting. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [.success(Self.statusResponse(522, retryAfter: "600"))]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/503-long")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 522) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/503-long"]) + let delays = await recorder.delays + XCTAssertTrue(delays.isEmpty, "must not sleep at all when Retry-After overshoots") + } + + func testFirstTransportRetryStaysImmediate() async throws { + // Preserves the pre-existing behaviour: the usual cause is a stale pooled + // connection, which the session reset has just fixed, so sleeping would only + // add latency. + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + if store.sessions.isEmpty { + return store.makeSession(outcomes: [.failure(URLError(.networkConnectionLost))]) + } + return store.makeSession(outcomes: [.success(Self.okResponse())]) + } + let recorder = DelayRecorder() + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/transient")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) + let delays = await recorder.delays + XCTAssertTrue(delays.isEmpty, "the first transport retry must not back off") + } + + private static func statusResponse(_ code: Int, retryAfter: String? = nil) -> (Data, URLResponse) { + let url = URL(string: "https://example.test/status")! + var headers: [String: String] = [:] + if let retryAfter { + headers["Retry-After"] = retryAfter + } + let response = HTTPURLResponse( + url: url, statusCode: code, httpVersion: nil, headerFields: headers + )! + return (Data("status-\(code)".utf8), response) + } + private static func request(path: String) -> URLRequest { var request = URLRequest(url: URL(string: "https://example.test\(path)")!) request.httpMethod = "POST" @@ -162,3 +631,11 @@ private final class MockHTTPSession: PluginHTTPClientSession, @unchecked Sendabl } } } + +private actor DelayRecorder { + private(set) var delays: [Duration] = [] + + func record(_ delay: Duration) { + delays.append(delay) + } +}