From 94236847da7e5b2474d3030a48ecec09d158dd67 Mon Sep 17 00:00:00 2001 From: willmcginnis <40506393+willmcginnis@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:28:50 -0700 Subject: [PATCH 1/4] Retry transient upstream failures in PluginHTTPClient The client already retried, but only inside the catch branch gated on isTransientNetworkError, which casts to URLError and switches on transport codes. A delivered response carrying 503, or Cloudflare's 522, never throws, so it went straight back to the plugin with no retry at all. That is why an upstream outage in front of a transcription API fails a dictation outright. Retries are on by default, with an explicit opt-out. Exponential backoff, full jitter, 0.5s base, 8s per-delay cap, bounded by both a 25s retry-scheduling budget and a 6-attempt limit. The dual bound is not belt-and-braces: full jitter draws from random(0, capped), so an endpoint that fails instantly can draw a run of near-zero delays and burn many attempts inside the budget. Which statuses retry depends on the request METHOD, because the question is not "is this a server error" but "could the origin already have applied this". 408, 503, 521, 522, 523, 525, 526 any method; the origin never processed it 502, 504, 520, 524 idempotent methods only 500 never; may have failed partway 429 one retry, and only on an explicit Retry-After that fits the budget Cloudflare documents 524 as the origin connection having been established without a timely response, so the origin may still complete the work. Repeating a POST there could duplicate it. The 2026-09-03 incident was a 522 on a POST and stays covered. Callers that must not inherit the ladder opt out with retry: .disabled, which restores the previous behaviour exactly: the Speechmatics, AssemblyAI and Gladia poll loops, which already re-issue on any non-200 up to 300 times; WebhookPlugin, which sends a user-configured method and already retries once itself; and Soniox's cleanup DELETEs, which a finished transcript is awaited behind. Retry-After is parsed as an integer, per RFC 9110 delta-seconds, and clamped to a day. This is a crash fix, not tidiness: Double("999999999999999999999999") is finite and non-negative, so it passes an isFinite guard, and Duration.seconds then traps on overflow and kills the process. A broken or hostile origin could crash the app from a response header. The first transport retry stays immediate after a session reset, but only for the stale-pooled-connection codes a reset actually fixes. A timeout has already waited the full request timeout, so it backs off instead. On exhaustion the last response is returned rather than thrown, so callers still see the real status and body. The one-argument data(for:) overload is deliberately kept rather than folded into a defaulted parameter: nine call sites pass PluginHTTPClient.data as an unapplied function reference, whose type a default does not preserve. The test harness now installs a no-op sleeper by default. Without it, mocks whose last outcome is a sticky failure drive the real ladder, and the SDK suite went from 35s to 393s with non-deterministic durations. Full SDK suite: 760 tests, 3 skipped, 0 failures. --- .../AssemblyAIPlugin/AssemblyAIPlugin.swift | 3 +- .../Plugins/GladiaPlugin/GladiaPlugin.swift | 3 +- .../Plugins/SonioxPlugin/SonioxPlugin.swift | 4 +- .../SpeechmaticsPlugin.swift | 4 +- .../Plugins/WebhookPlugin/WebhookPlugin.swift | 5 +- .../TypeWhisperPluginSDK/HostServices.swift | 368 ++++++++++++++++-- .../PluginTestSupport.swift | 13 +- .../PluginHTTPClientTests.swift | 342 ++++++++++++++++ 8 files changed, 699 insertions(+), 43 deletions(-) diff --git a/TypeWhisperPluginSDK/Plugins/AssemblyAIPlugin/AssemblyAIPlugin.swift b/TypeWhisperPluginSDK/Plugins/AssemblyAIPlugin/AssemblyAIPlugin.swift index b645fffc2..9080dfbff 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 390f4a0a3..47793f79a 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 b2e07cc3d..6610521fe 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 200d7868c..29377ad96 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 281e17501..60c28b6fd 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 e09752f1d..3b8f3c60a 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,256 @@ 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 retryAfter = retryAfterDelay(from: http) + + // 429: one retry, only on an explicit Retry-After that fits. + if isRetryAfterOnlyStatus(http.statusCode) { + guard !usedRetryAfterGrace, + 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) + + // Pre-existing behaviour, preserved under both policies: one immediate + // retry for the codes a session reset actually fixes. + if attempt == 0, isStalePooledConnectionError(error) { + attempt += 1 + logger.warning("\(method) \(url) transient failure after \(elapsed), reset session, retrying immediately: \(error.localizedDescription)") + continue + } + + guard policy.laddersTransientFailures, + 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) } + } + } + + /// The errors an immediate session reset plausibly fixes. A timeout is not one: + /// we already waited the full request timeout, so re-sending with no pause repeats + /// that wait. Being offline is not one either. + static func isStalePooledConnectionError(_ error: Error) -> Bool { + guard let urlError = error as? URLError else { return false } + switch urlError.code { + case .networkConnectionLost, .cannotConnectToHost: + return true + default: + return false + } + } + + /// 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 origin demonstrably did not process the request. 408 was + /// never received; 503 is a refusal to handle it; Cloudflare 521 (origin down), + /// 522 (connection timed out), 523 (origin unreachable), 525/526 (TLS failed) + /// all fail before the origin sees a byte. The 2026-09-03 incident was a 522 on + /// a POST, and it stays retried. + /// - Idempotent methods only: 502, 504, 520 and 524 do NOT establish that the + /// origin skipped the work. Cloudflare's own documentation of 524 says the + /// connection was established and the origin simply did not answer in time, so + /// it may still complete. Retry these only where a repeat is harmless. + /// - 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, 503, 521, 522, 523, 525, 526: + return true + case 502, 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 + } + + // 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 + } - logger.error("\(method) \(url) failed after \(elapsed): \(error.localizedDescription)") - throw error + 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. + static func retryAfterDelay(from response: HTTPURLResponse) -> Duration? { + guard let raw = response.value(forHTTPHeaderField: "Retry-After")? + .trimmingCharacters(in: .whitespaces), + let seconds = Int(raw), + seconds >= 0, + seconds <= maxHonouredRetryAfterSeconds + else { + return nil } + return .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 +513,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 +530,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 a74e7edf2..c5f42b4fd 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 502f615d6..a727e77bb 100644 --- a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift +++ b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift @@ -97,6 +97,340 @@ 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(503)), + .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(503, 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(503)), .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, 503) + XCTAssertEqual(store.sessions.first?.requestedPaths, ["/opted-out"]) + let delays = await recorder.delays + XCTAssertTrue(delays.isEmpty) + } + + func testTimeoutDoesNotGetTheImmediateRetry() async throws { + // The immediate retry exists for a stale pooled connection, which a session + // reset fixes. A timeout already waited the full request timeout, so re-sending + // with no pause just repeats that wait. + 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) }, jitterFraction: { 1.0 }) + + _ = try await PluginHTTPClient.data(for: Self.request(path: "/slow")) + + let delays = await recorder.delays + XCTAssertEqual(delays, [.milliseconds(500)], "a timeout backs off rather than retrying at once") + } + + 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(503))]) + } + PluginHTTPClient.configureRetryForTesting(sleeper: { _ in }) + + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/always-503")) + + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 503) + XCTAssertEqual(store.sessions.first?.requestedPaths.count, PluginHTTPClient.retryMaxAttempts) + } + + func testBackoffScheduleDoublesAcrossTheWholeLadder() async throws { + let store = MockHTTPSessionStore() + PluginHTTPClient.configureForTesting { _ in + store.makeSession(outcomes: [.success(Self.statusResponse(503))]) + } + 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(503, 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 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(503, 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, 503) + 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 +496,11 @@ private final class MockHTTPSession: PluginHTTPClientSession, @unchecked Sendabl } } } + +private actor DelayRecorder { + private(set) var delays: [Duration] = [] + + func record(_ delay: Duration) { + delays.append(delay) + } +} From 4d3d7553953f3a39adf527747894fc41bd82c302 Mon Sep 17 00:00:00 2001 From: willmcginnis <40506393+willmcginnis@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:59:10 -0700 Subject: [PATCH 2/4] Address review: keep .disabled byte-identical to prior behaviour Four fixes from the automated review, and the first is the important one. .disabled was NOT "the previous behaviour exactly", as its doc comment and the PR body both claimed. The old code gave ONE immediate retry to any transient URLError. Narrowing that to stale-pooled-connection codes altered pre-existing behaviour rather than adding to it, so under .disabled a timeout, DNS failure or offline error stopped being retried at all. That regressed the three poll loops the opt-out exists to protect: one timeout aborted transcription where it previously advanced to the next iteration. The compatibility retry is now unconditional again, under both policies, and only the ladder past it is new. That ladder is now gated on idempotent methods, matching what the status set already did. A POST can time out after the origin processed it, so laddering a non-idempotent transport failure risks duplicating the work. The single compatibility retry still applies to every method, as before. The 429 Retry-After grace now checks retryMaxAttempts. It previously allowed a seventh request when attempt six returned a 429 with an acceptable header. Tests: testTimeoutDoesNotGetTheImmediateRetry asserted the wrong thing and is replaced by testTimeoutStillGetsTheCompatibilityImmediateRetry. Added testDisabledPolicyStillGetsTheCompatibilityTransportRetry and testLadderedTransportRetriesAreIdempotentOnly. Full SDK suite: 762 tests, 3 skipped, 0 failures. --- .../TypeWhisperPluginSDK/HostServices.swift | 27 +++---- .../PluginHTTPClientTests.swift | 73 +++++++++++++++++-- 2 files changed, 77 insertions(+), 23 deletions(-) diff --git a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift index 3b8f3c60a..7371ca435 100644 --- a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift +++ b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift @@ -315,6 +315,7 @@ public enum PluginHTTPClient { // 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 { @@ -349,15 +350,22 @@ public enum PluginHTTPClient { resetSharedSession(matching: session, reason: "transient network error") - // Pre-existing behaviour, preserved under both policies: one immediate - // retry for the codes a session reset actually fixes. - if attempt == 0, isStalePooledConnectionError(error) { + // 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 { @@ -372,19 +380,6 @@ public enum PluginHTTPClient { } } - /// The errors an immediate session reset plausibly fixes. A timeout is not one: - /// we already waited the full request timeout, so re-sending with no pause repeats - /// that wait. Being offline is not one either. - static func isStalePooledConnectionError(_ error: Error) -> Bool { - guard let urlError = error as? URLError else { return false } - switch urlError.code { - case .networkConnectionLost, .cannotConnectToHost: - return true - default: - return false - } - } - /// 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 diff --git a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift index a727e77bb..efc011891 100644 --- a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift +++ b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift @@ -271,10 +271,12 @@ final class PluginHTTPClientTests: XCTestCase { XCTAssertTrue(delays.isEmpty) } - func testTimeoutDoesNotGetTheImmediateRetry() async throws { - // The immediate retry exists for a stale pooled connection, which a session - // reset fixes. A timeout already waited the full request timeout, so re-sending - // with no pause just repeats that wait. + 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 { @@ -283,12 +285,69 @@ final class PluginHTTPClientTests: XCTestCase { return store.makeSession(outcomes: [.success(Self.okResponse())]) } let recorder = DelayRecorder() - PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }, jitterFraction: { 1.0 }) + PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) - _ = try await PluginHTTPClient.data(for: Self.request(path: "/slow")) + let (_, response) = try await PluginHTTPClient.data(for: Self.request(path: "/slow")) + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 200) let delays = await recorder.delays - XCTAssertEqual(delays, [.milliseconds(500)], "a timeout backs off rather than retrying at once") + 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 testDoesNotRetryServerError500() async throws { From eef4e757574ad0a7b51fab073ed5bab6b393ad79 Mon Sep 17 00:00:00 2001 From: willmcginnis <40506393+willmcginnis@users.noreply.github.com> Date: Sat, 5 Sep 2026 20:11:36 -0700 Subject: [PATCH 3/4] Restrict 503 retries to idempotent methods Two independent automated reviewers flagged the same exposure, and they are right. I had argued 503 belongs with the any-method group because its semantics say the origin declined to handle the request. That is a principled reading, and it loses to a concrete one: AssemblyAIPlugin.submitTranscription sets httpMethod = "POST" and goes through the default policy, so a 503 returned after the job was created resubmits it up to five more times. LinearPlugin mutations and OpenAIVectorMemoryPlugin uploads have the same shape. A semantic argument does not outweigh a duplicate transcription job. 503 now sits with 502, 504, 520 and 524: retried for idempotent methods only. The always-safe set keeps 408 and Cloudflare 521, 522, 523, 525 and 526, which all fail before the origin sees a byte, so the outage this work exists for is unaffected. It was a 522 on a POST and it is still retried. Seven ladder tests moved from 503 to 522, which is any-method and is the status the original failure produced, so they still exercise the POST path. Added a pair asserting that a 503 is not retried on POST and is retried on GET. Full SDK suite: 764 tests, 3 skipped, 0 failures. --- .../TypeWhisperPluginSDK/HostServices.swift | 31 +++++++---- .../PluginHTTPClientTests.swift | 53 +++++++++++++++---- 2 files changed, 63 insertions(+), 21 deletions(-) diff --git a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift index 7371ca435..9741165bc 100644 --- a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift +++ b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift @@ -388,23 +388,32 @@ public enum PluginHTTPClient { /// `LinearPlugin` runs GraphQL mutations, `OpenAIVectorMemoryPlugin` uploads and /// attaches files. Duplicating those is worse than failing. /// - /// - Always safe: the origin demonstrably did not process the request. 408 was - /// never received; 503 is a refusal to handle it; Cloudflare 521 (origin down), - /// 522 (connection timed out), 523 (origin unreachable), 525/526 (TLS failed) - /// all fail before the origin sees a byte. The 2026-09-03 incident was a 522 on - /// a POST, and it stays retried. - /// - Idempotent methods only: 502, 504, 520 and 524 do NOT establish that the - /// origin skipped the work. Cloudflare's own documentation of 524 says the - /// connection was established and the origin simply did not answer in time, so - /// it may still complete. Retry these only where a repeat is harmless. + /// - 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, 503, 521, 522, 523, 525, 526: + case 408, 521, 522, 523, 525, 526: return true - case 502, 504, 520, 524: + case 502, 503, 504, 520, 524: return isIdempotentMethod(method) default: return false diff --git a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift index efc011891..f400748a1 100644 --- a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift +++ b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift @@ -107,7 +107,7 @@ final class PluginHTTPClientTests: XCTestCase { let store = MockHTTPSessionStore() PluginHTTPClient.configureForTesting { _ in store.makeSession(outcomes: [ - .success(Self.statusResponse(503)), + .success(Self.statusResponse(522)), .success(Self.okResponse()), ]) } @@ -209,7 +209,7 @@ final class PluginHTTPClientTests: XCTestCase { let store = MockHTTPSessionStore() PluginHTTPClient.configureForTesting { _ in store.makeSession(outcomes: [ - .success(Self.statusResponse(503, retryAfter: "999999999999999999999999")), + .success(Self.statusResponse(522, retryAfter: "999999999999999999999999")), .success(Self.okResponse()), ]) } @@ -256,7 +256,7 @@ final class PluginHTTPClientTests: XCTestCase { // Opt-out for pollers, self-retrying callers, and teardown. let store = MockHTTPSessionStore() PluginHTTPClient.configureForTesting { _ in - store.makeSession(outcomes: [.success(Self.statusResponse(503)), .success(Self.okResponse())]) + store.makeSession(outcomes: [.success(Self.statusResponse(522)), .success(Self.okResponse())]) } let recorder = DelayRecorder() PluginHTTPClient.configureRetryForTesting(sleeper: { await recorder.record($0) }) @@ -265,7 +265,7 @@ final class PluginHTTPClientTests: XCTestCase { for: Self.request(path: "/opted-out"), retry: .disabled ) - XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 503) + XCTAssertEqual((response as? HTTPURLResponse)?.statusCode, 522) XCTAssertEqual(store.sessions.first?.requestedPaths, ["/opted-out"]) let delays = await recorder.delays XCTAssertTrue(delays.isEmpty) @@ -350,6 +350,39 @@ final class PluginHTTPClientTests: XCTestCase { "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 @@ -374,20 +407,20 @@ final class PluginHTTPClientTests: XCTestCase { // still sees the status and body and renders its own error. let store = MockHTTPSessionStore() PluginHTTPClient.configureForTesting { _ in - store.makeSession(outcomes: [.success(Self.statusResponse(503))]) + 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, 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(503))]) + store.makeSession(outcomes: [.success(Self.statusResponse(522))]) } let recorder = DelayRecorder() // jitterFraction 1.0 gives the un-jittered upper bound, which is the readable @@ -426,7 +459,7 @@ final class PluginHTTPClientTests: XCTestCase { let store = MockHTTPSessionStore() PluginHTTPClient.configureForTesting { _ in store.makeSession(outcomes: [ - .success(Self.statusResponse(503, retryAfter: "3")), + .success(Self.statusResponse(522, retryAfter: "3")), .success(Self.okResponse()), ]) } @@ -444,14 +477,14 @@ final class PluginHTTPClientTests: XCTestCase { // 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(503, retryAfter: "600"))]) + 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, 503) + 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") From 05805cdf11052f0d01fb1dd1ca79475c541c919b Mon Sep 17 00:00:00 2001 From: Will McGinnis Date: Wed, 9 Sep 2026 10:48:05 -0700 Subject: [PATCH 4/4] Treat a Retry-After beyond the honoured ceiling as a refusal, not a delay retryAfterDelay collapsed three cases into nil: header absent, header malformed, and a well-formed delta-seconds above the one-day ceiling. The non-429 retryable path fed that nil to backoffDelay, which read it as no header and computed ordinary backoff, so a Retry-After of 86401 retried in ~0.5s, exactly as if the server had said nothing, when a value that large is a refusal that should have stopped the retries. The code comment already stated the intent: anything longer is a refusal and we do not sleep on it. Split the return into a three-case enum: none (fall through to the ladder), after (honour, subject to budget), refusal (do not retry). An oversized value like 999999999999999999999999 overflows Int and still parses as nil, so it stays on the none path and the crash-regression test is preserved. Adds a unit test on the classifier, plus an integration regression using 86401 on a GET request. The GET matters: a 503 is retryable only for idempotent methods, so with the default POST the request would return without retrying regardless of the fix and the test would pin nothing. On a GET the pre-fix code retries once at ~0.5s while the fixed code refuses, so the refusal rather than the 25s budget is what stops it. The existing overshoot test used 600, below the ceiling, which is why the ceiling case went uncovered. --- .../TypeWhisperPluginSDK/HostServices.swift | 45 ++++++++++++++++--- .../PluginHTTPClientTests.swift | 43 ++++++++++++++++++ 2 files changed, 82 insertions(+), 6 deletions(-) diff --git a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift index 9741165bc..11d03fa81 100644 --- a/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift +++ b/TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift @@ -310,7 +310,23 @@ public enum PluginHTTPClient { return (data, response) } - let retryAfter = retryAfterDelay(from: http) + 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) { @@ -483,16 +499,33 @@ public enum PluginHTTPClient { /// /// The HTTP-date form is not honoured. It needs clock-skew handling to be safe and /// falls through to the ordinary ladder instead. - static func retryAfterDelay(from response: HTTPURLResponse) -> Duration? { + /// 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, - seconds <= maxHonouredRetryAfterSeconds + seconds >= 0 else { - return nil + // 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 .seconds(seconds) + return .after(.seconds(seconds)) } /// A day. Anything longer is not a delay, it is a refusal, and we do not sleep on diff --git a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift index f400748a1..09d02cd8a 100644 --- a/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift +++ b/TypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/PluginHTTPClientTests.swift @@ -473,6 +473,49 @@ final class PluginHTTPClientTests: XCTestCase { 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()