diff --git a/.github/swift-file-length-budget.tsv b/.github/swift-file-length-budget.tsv index fa65d6dcf094..e8c4fef19726 100644 --- a/.github/swift-file-length-budget.tsv +++ b/.github/swift-file-length-budget.tsv @@ -2,7 +2,7 @@ # Format: max_linesrelative path # Reduce counts as files shrink. CI fails if tracked files exceed this budget. 34499 CLI/cmux.swift -17954 Sources/AppDelegate.swift +17952 Sources/AppDelegate.swift 16427 Sources/ContentView.swift 14270 Sources/TerminalController.swift 13172 Sources/Workspace.swift @@ -24,8 +24,8 @@ 4483 Sources/cmuxApp.swift 4482 Sources/Panels/FilePreviewPanel.swift 4367 cmuxTests/BrowserPanelTests.swift -4283 Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift 4121 Sources/BrowserWindowPortal.swift +3964 Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift 3934 Sources/Feed/FeedPanelView.swift 3926 cmuxTests/TabManagerUnitTests.swift 3896 cmuxTests/WindowAndDragTests.swift diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift index b9342dc7776e..cd68684283ac 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift @@ -66,6 +66,22 @@ struct AttemptUpdateCoordinator { } } + /// Arm the coordinator to auto-confirm the update a freshly *restarted* retry check resolves, + /// entering ``Phase/awaitingResult`` directly rather than ``Phase/awaitingCheckRestart``. + /// + /// Used by the download-phase transient-retry re-arm (issue #5632). Unlike + /// ``requestInstallLatest(currentState:)`` — invoked while a stale pre-request `.updateAvailable` + /// prompt may still be on screen, which `awaitingCheckRestart` exists to ignore until a real + /// check restarts — this arm happens while the model is the retry's own `.checking` pill, so + /// there is no stale prompt to gate against. Entering `awaitingResult` directly means that if the + /// user cancels the retry before the check restarts (idling the model, e.g. during the + /// controller's readiness wait), the coordinator disarms via the `awaitingResult` idle → inactive + /// path instead of misreading the idle as check-restart progress — which would otherwise strand + /// it armed to silently auto-confirm a later, unrelated update. + mutating func armForConfirmedRetryCheck() { + phase = .awaitingResult + } + /// Feed each ``UpdateState`` change observed after a request. Returns the action the controller /// should perform. mutating func handleStateChange(_ state: UpdateState) -> Action { diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift index 1f9e09d49d96..7a3833f2b3f2 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift @@ -3,12 +3,17 @@ /// This is the dependency-inversion seam between the `CmuxUpdater` package and the app: the /// package calls up through this protocol instead of reaching `AppDelegate`/`TerminalController` /// directly. The app's delegate conforms and is injected into ``UpdateController`` (which holds -/// it `weak`). +/// it `weak`). The protocol is main-actor isolated because Sparkle callbacks and cmux update UI +/// state are both main-actor owned. @MainActor public protocol UpdateActionDelegate: AnyObject { /// The user asked to retry after an update error. The host should re-initiate a check /// through its normal entry point. - func updaterRequestsRetryCheckForUpdates() + /// + /// - Parameter preservingInstallIntent: Whether the interrupted Sparkle session had already + /// moved past update selection into an install/download phase and the retry should + /// auto-confirm the same update after Sparkle finds it again. + func updaterRequestsRetryCheckForUpdates(preservingInstallIntent: Bool) /// Sparkle is about to relaunch the app to finish installing. The host should persist /// session state, stop its terminal/runtime, and invalidate restorable state so the diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+BundleReleaseChannel.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+BundleReleaseChannel.swift new file mode 100644 index 000000000000..ffc0faed58d4 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+BundleReleaseChannel.swift @@ -0,0 +1,29 @@ +import Foundation + +extension UpdateController { + /// Which release channel a bundle identifier belongs to, decided from the identifier alone so it + /// stays unit-testable, mirroring ``TransientRetryPlan``. + /// + /// cmux DEV (`com.cmuxterm.app.debug[.]`) and staging (`com.cmuxterm.app.staging[.]`) + /// builds are produced from local source and are not on the public release train, so they must + /// never be compared against the public Sparkle appcast (#6292). + /// + /// Mirrors `SocketControlSettings.isDebugLikeBundleIdentifier` + `isStagingBundleIdentifier` (in + /// the CmuxSettings package). The classification is duplicated here deliberately to avoid + /// introducing a `CmuxUpdater → CmuxSettings` package dependency edge for a small string check. + enum BundleReleaseChannel: Equatable { + /// A DEV or staging build produced from local source; never query the public appcast. + case devLike + /// A public release-train build; the appcast applies. + case release + + nonisolated init(bundleIdentifier: String?) { + guard let bundleIdentifier else { self = .release; return } + let isDevLike = bundleIdentifier == "com.cmuxterm.app.debug" + || bundleIdentifier.hasPrefix("com.cmuxterm.app.debug.") + || bundleIdentifier == "com.cmuxterm.app.staging" + || bundleIdentifier.hasPrefix("com.cmuxterm.app.staging.") + self = isDevLike ? .devLike : .release + } + } +} diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+ReadinessWaitDecision.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+ReadinessWaitDecision.swift new file mode 100644 index 000000000000..ca490774a151 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+ReadinessWaitDecision.swift @@ -0,0 +1,27 @@ +extension UpdateController { + /// Whether the bounded readiness wait should keep polling for `canCheckForUpdates`, decided from + /// the model state observed at the top of each poll iteration. + /// + /// Kept as a pure value + decision — separate from the controller's effectful readiness wait — so + /// it is unit-testable without the live `SPUUpdater` the controller owns, mirroring + /// ``TransientRetryPlan``. + /// + /// The wait is entered on behalf of a pending check and must keep polling for readiness for any + /// pending state — a `.checking` placeholder (a plain manual check or a transient-retry pill) or + /// the `.updateAvailable` prompt that `attemptUpdate()` re-resolves to the latest version + /// (issue #6366). It stops only when the model has returned to `.idle`, the signal that the user + /// cancelled the pending check, so readiness arriving later cannot resurrect a dismissed check. + /// Stopping on every non-`.checking` state instead stranded `attemptUpdate()`'s install whenever + /// Sparkle was briefly not ready, leaving the model `.updateAvailable` so no fresh check ran + /// (autoreview follow-up to issue #5632). + enum ReadinessWaitDecision: Equatable { + /// A pending check still awaits readiness; keep polling `canCheckForUpdates`. + case keepPolling + /// The pending check was cancelled back to `.idle`; stop the wait. + case stop + + nonisolated init(modelState state: UpdateState) { + self = state.isIdle ? .stop : .keepPolling + } + } +} diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift new file mode 100644 index 000000000000..29f36a086a55 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift @@ -0,0 +1,40 @@ +extension UpdateController { + /// How ``retryAfterTransientFailure(preservingInstallIntent:)`` should react to a transient + /// Sparkle download failure. + /// + /// Kept as a pure value + decision — separate from the controller's effectful retry method — so + /// it is unit-testable without the live `SPUUpdater` the controller owns, mirroring how + /// ``AttemptUpdateCoordinator`` isolates the install re-resolution policy. + enum TransientRetryPlan: Equatable { + /// The attempt coordinator is already sequencing an install; restart its check while + /// preserving the in-flight install intent so it still auto-confirms. + case restartMonitoredCheck + /// The failure landed in a Sparkle download/extract/install phase, but the coordinator was + /// not yet monitoring (the interrupted session was Sparkle's own, not a coordinator + /// re-resolve). Re-arm the coordinator so the retried check auto-confirms the update it + /// finds; otherwise the retry surfaces a fresh prompt and the interrupted install is + /// silently stranded. + case rearmConfirmedInstall + /// A plain transient retry with no install intent to preserve; run an ordinary fresh check. + case plainCheck + + /// Pure decision for ``UpdateController/retryAfterTransientFailure(preservingInstallIntent:)``, + /// derived from the retry context alone so it stays unit-testable without the live + /// `SPUUpdater` the controller owns. + /// + /// A retry while the attempt coordinator is already monitoring just restarts its in-flight + /// check (intent preserved by the coordinator itself). A preserved-intent retry that arrives + /// *before* the coordinator is monitoring must re-arm it so the retried check auto-confirms + /// the update it resolves — the case that previously stranded the interrupted install by + /// falling through to a bare check that only surfaced a prompt (issue #5632). + nonisolated init(preservingInstallIntent: Bool, coordinatorIsMonitoring: Bool) { + if coordinatorIsMonitoring { + self = .restartMonitoredCheck + } else if preservingInstallIntent { + self = .rearmConfirmedInstall + } else { + self = .plainCheck + } + } + } +} diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index 8537b4f55819..6772f560a038 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -26,7 +26,7 @@ public final class UpdateController { private let hostBundle: Bundle private let backgroundProbeInterval: TimeInterval /// Whether the running build is a cmux DEV/staging build that must never be compared against - /// the public release appcast. See ``isDevLikeBundleIdentifier(_:)``. + /// the public release appcast. See ``BundleReleaseChannel``. private let isDevLikeBundle: Bool /// Host actions the updater delegates upward (retry, relaunch prep). Forwarded to the driver. @@ -68,7 +68,7 @@ public final class UpdateController { /// - fileManager: Filesystem access for the Sparkle installation-cache workaround; /// injectable so tests can avoid touching the real filesystem. /// - isDevLikeBundle: Overrides whether this is a DEV/staging build. Defaults to `nil`, - /// which derives it from `hostBundle.bundleIdentifier` via ``isDevLikeBundleIdentifier(_:)``. + /// which derives it from `hostBundle.bundleIdentifier` via ``BundleReleaseChannel``. /// Injectable because a `Bundle` with an arbitrary identifier cannot be constructed in tests. public init(log: any UpdateLogging, clock: any UpdateClock = SystemUpdateClock(), @@ -83,7 +83,7 @@ public final class UpdateController { self.fileManager = fileManager self.hostBundle = hostBundle self.backgroundProbeInterval = settings.scheduledCheckInterval - let isDevLikeBundle = isDevLikeBundle ?? Self.isDevLikeBundleIdentifier(hostBundle.bundleIdentifier) + let isDevLikeBundle = isDevLikeBundle ?? (BundleReleaseChannel(bundleIdentifier: hostBundle.bundleIdentifier) == .devLike) self.isDevLikeBundle = isDevLikeBundle settings.apply(to: defaults) if isDevLikeBundle { @@ -202,16 +202,79 @@ public final class UpdateController { checkForUpdatesWhenReady() } - private func performCheckForUpdates() { + /// Retry after a transient Sparkle download failure. Unlike a user-started fresh check, this + /// preserves any in-progress install/attempt intent so a retried archive download continues + /// silently after Sparkle finds the same update again. + /// + /// The reaction is chosen by the pure ``TransientRetryPlan/init(preservingInstallIntent:coordinatorIsMonitoring:)``: + /// restart the coordinator's already-monitored check, re-arm the coordinator so the retried + /// check auto-confirms (when the interrupted phase carried install intent but the coordinator + /// was not yet monitoring — issue #6366), or run a plain fresh check. + /// + /// - Parameter preservingInstallIntent: Whether the retry request came from a Sparkle + /// install/download phase whose update choice should be auto-confirmed again. + public func retryAfterTransientFailure(preservingInstallIntent: Bool = false) { + let plan = TransientRetryPlan( + preservingInstallIntent: preservingInstallIntent, + coordinatorIsMonitoring: attemptCoordinator.isMonitoring + ) + log.append("retrying update after transient download failure (plan=\(plan))") + switch plan { + case .restartMonitoredCheck: + // Preserve the interrupted session's install intent so the retried check re-resolves and + // (the coordinator being already monitoring) auto-confirms the update it finds. + checkForUpdatesWhenReady(preservingInstallIntent: true) + case .rearmConfirmedInstall: + // Arm the attempt coordinator to auto-confirm the retried check's update, matching the + // pre-failure install intent — entering the coordinator's result-awaiting phase directly + // (see `armForConfirmedRetryCheck`) rather than via `requestInstallLatest`. The model + // here is the retry's own synthetic `.checking` pill, not a stale prompt, so a user + // cancel during the readiness wait disarms the coordinator instead of stranding it armed + // to silently auto-confirm a later, unrelated update. Route the check through the + // preserving path so the synthetic pill is not torn down and the bounded-retry count is + // preserved. + attemptCoordinator.armForConfirmedRetryCheck() + checkForUpdatesWhenReady(preservingInstallIntent: true) + case .plainCheck: + checkForUpdatesWhenReady(preservingInstallIntent: false) + } + } + + private func performCheckForUpdates(preservingInstallIntent: Bool = false) { startUpdaterIfNeeded() ensureSparkleInstallationCache() // Cancel any pending deferred re-check on every path so a stale one can't fire a // duplicate checkForUpdates() after this new check starts. recheckTask?.cancel() + // A transient retry (preserved-intent or plain) only arrives from + // `retryAfterTransientFailure` after the driver's multi-second backoff, by which point + // Sparkle has already acknowledged and torn down the failed session at error time (see + // `showUpdaterError` → `acknowledgement()`). There is therefore no just-dismissed *live* + // session to coalesce with, so the non-idle teardown + 100ms delay below — which exists + // solely to let Sparkle finish aborting a session before an *immediate* re-check — does not + // apply. Routing a retry through it would also be wrong: the current non-idle state is then + // the driver's synthetic `.checking` backoff placeholder whose `cancel` aborts the retry and + // resets the bounded-retry failure count, so `cancelActiveStateForNewCheck()` would kill the + // very retry (flickering the pill to idle) and — on the readiness-delayed path, where this + // runs from `readyCheckTask` after the synchronous `isRestartingTransientRetry` window has + // closed — silently reset the `[1, 3, 8]` cap into an unbounded retry loop (autoreview P2). + // Start the fresh check directly instead. + // + // `canCheckForUpdates` was true at every caller (`checkForUpdatesWhenReady`, + // `waitForReadinessThenCheck`), so any live Sparkle session is already gone: a `.checking` + // here is only ever that synthetic placeholder, never a real in-flight check. + if preservingInstallIntent { + updater.checkForUpdates() + return + } if model.state == .idle { updater.checkForUpdates() return } + if case .checking = model.state { + updater.checkForUpdates() + return + } model.cancelActiveStateForNewCheck() @@ -228,7 +291,7 @@ public final class UpdateController { } /// Check for updates once the updater reports it can. - private func checkForUpdatesWhenReady() { + private func checkForUpdatesWhenReady(preservingInstallIntent: Bool = false) { if isDevLikeBundle { // DEV/staging builds are not on the public release train. A manual check (menu, // custom UI, or attempt-and-install) must not query the public appcast or offer the @@ -246,22 +309,24 @@ public final class UpdateController { let canCheck = updater.canCheckForUpdates log.append("checkForUpdatesWhenReady invoked (canCheck=\(canCheck))") if canCheck { - performCheckForUpdates() + performCheckForUpdates(preservingInstallIntent: preservingInstallIntent) return } if model.state.isIdle { model.setState(.checking(.init(cancel: {}))) } - waitForReadinessThenCheck() + waitForReadinessThenCheck(preservingInstallIntent: preservingInstallIntent) } - private func waitForReadinessThenCheck() { + private func waitForReadinessThenCheck(preservingInstallIntent: Bool = false) { readyCheckTask = Task { @MainActor [weak self] in guard let self else { return } var remaining = self.readyRetryCount while remaining > 0 { + // Stop the wait when the pending check was cancelled; keep polling otherwise. + guard ReadinessWaitDecision(modelState: self.model.state) == .keepPolling else { return } if self.updater.canCheckForUpdates { - self.performCheckForUpdates() + self.performCheckForUpdates(preservingInstallIntent: preservingInstallIntent) return } remaining -= 1 @@ -342,7 +407,7 @@ public final class UpdateController { if isDevLikeBundle { // DEV/staging builds are not on the public release train; never probe the public // appcast (init also disables Sparkle's own scheduled checks). Tear down any probe a - // prior path may have started. See `isDevLikeBundleIdentifier(_:)` (#6292). + // prior path may have started. See `BundleReleaseChannel` (#6292). log.append("launch update probe skipped (dev/staging build)") backgroundProbeTask?.cancel() backgroundProbeTask = nil @@ -427,23 +492,3 @@ public final class UpdateController { } } } - -extension UpdateController { - /// Whether `bundleIdentifier` is a cmux DEV (`com.cmuxterm.app.debug[.]`) or staging - /// (`com.cmuxterm.app.staging[.]`) build. - /// - /// Such builds are produced from local source and are not on the public release train, so - /// they must never be compared against the public Sparkle appcast (#6292). - /// - /// Mirrors `SocketControlSettings.isDebugLikeBundleIdentifier` + - /// `isStagingBundleIdentifier` (in the CmuxSettings package). The classification is - /// duplicated here deliberately to avoid introducing a `CmuxUpdater → CmuxSettings` package - /// dependency edge for a small string check. - static func isDevLikeBundleIdentifier(_ bundleIdentifier: String?) -> Bool { - guard let bundleIdentifier else { return false } - return bundleIdentifier == "com.cmuxterm.app.debug" - || bundleIdentifier.hasPrefix("com.cmuxterm.app.debug.") - || bundleIdentifier == "com.cmuxterm.app.staging" - || bundleIdentifier.hasPrefix("com.cmuxterm.app.staging.") - } -} diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift index f1ff0baec252..b50907c0573d 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift @@ -14,9 +14,10 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { let model: UpdateStateModel let log: any UpdateLogging private let clock: any UpdateClock + private let retryPolicy: UpdateRetryPolicy /// Whether the running build is a cmux DEV/staging build that is not on the public release /// train. When `true`, the driver must never surface the public appcast's update pill (see - /// ``UpdateController/isDevLikeBundleIdentifier(_:)``). + /// ``UpdateController/BundleReleaseChannel``). let isDevLikeBundle: Bool /// Host actions the driver delegates upward. Held weak; set by ``UpdateController``. weak var actionDelegate: (any UpdateActionDelegate)? @@ -26,12 +27,25 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { private var lastCheckStart: Date? private var pendingCheckTransitionTask: Task? private var checkTimeoutTask: Task? + private var transientErrorRetryTask: Task? + private var transientErrorRetryFailureCount = 0 + private var shouldPreserveRetryStateForNextCheck = false + // True only for the synchronous window of the retry's delegate call, where the controller may + // re-enter `cancelPendingTransientErrorRetry` via `cancelActiveStateForNewCheck()`. Scopes the + // preserve-count suppression to that internal restart so a user cancel during the later + // readiness wait still idles the pill instead of no-oping (autoreview P2). + private var isRestartingTransientRetry = false private(set) var lastFeedURLString: String? - init(model: UpdateStateModel, log: any UpdateLogging, clock: any UpdateClock, isDevLikeBundle: Bool = false) { + init(model: UpdateStateModel, + log: any UpdateLogging, + clock: any UpdateClock, + retryPolicy: UpdateRetryPolicy = UpdateRetryPolicy(), + isDevLikeBundle: Bool = false) { self.model = model self.log = log self.clock = clock + self.retryPolicy = retryPolicy self.isDevLikeBundle = isDevLikeBundle super.init() } @@ -39,6 +53,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { deinit { pendingCheckTransitionTask?.cancel() checkTimeoutTask?.cancel() + transientErrorRetryTask?.cancel() } func show(_ request: SPUUpdatePermissionRequest, @@ -79,6 +94,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { func showUpdateNotFoundWithError(_ error: any Error, acknowledgement: @escaping () -> Void) { + resetTransientErrorRetryState() log.append("show update not found: \(formatErrorForLog(error))") setStateAfterMinimumCheckDelay(.notFound(.init(acknowledgement: acknowledgement))) } @@ -87,11 +103,15 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { acknowledgement: @escaping () -> Void) { let details = formatErrorForLog(error) log.append("show updater error: \(details)") + if scheduleTransientErrorRetryIfNeeded(for: error) { + acknowledgement() + return + } setState(.error(.init( error: error, retry: { [weak self] in self?.model.setState(.idle) - self?.actionDelegate?.updaterRequestsRetryCheckForUpdates() + self?.actionDelegate?.updaterRequestsRetryCheckForUpdates(preservingInstallIntent: false) }, dismiss: { [weak self] in self?.model.setState(.idle) @@ -133,6 +153,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { } func showDownloadDidStartExtractingUpdate() { + resetTransientErrorRetryState() log.append("show extraction started") setState(.extracting(.init(progress: 0))) } @@ -158,6 +179,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { } func showUpdateInstalledAndRelaunched(_ relaunched: Bool, acknowledgement: @escaping () -> Void) { + resetTransientErrorRetryState() log.append("show update installed (relaunched=\(relaunched))") setState(.idle) acknowledgement() @@ -187,6 +209,13 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { // MARK: - State transition helpers private func beginChecking(cancel: @escaping () -> Void) { + transientErrorRetryTask?.cancel() + transientErrorRetryTask = nil + if shouldPreserveRetryStateForNextCheck { + shouldPreserveRetryStateForNextCheck = false + } else { + transientErrorRetryFailureCount = 0 + } model.setOverrideState(nil) pendingCheckTransitionTask?.cancel() pendingCheckTransitionTask = nil @@ -197,6 +226,77 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { scheduleCheckTimeout() } + private func scheduleTransientErrorRetryIfNeeded(for error: any Error) -> Bool { + guard actionDelegate != nil else { return false } + let nextFailureCount = transientErrorRetryFailureCount + 1 + guard let retryDelay = retryPolicy.delay(afterFailureNumber: nextFailureCount, for: error) else { + if retryPolicy.isTransientDownloadError(error) { + log.append("transient update download error retries exhausted (\(transientErrorRetryFailureCount)/\(retryPolicy.maximumRetryCount))") + } + return false + } + + let preservingInstallIntent = shouldPreserveInstallIntentForTransientRetry() + transientErrorRetryFailureCount = nextFailureCount + transientErrorRetryTask?.cancel() + log.append("transient update download error; retry \(nextFailureCount)/\(retryPolicy.maximumRetryCount) in \(String(format: "%.0f", retryDelay))s") + setState(.checking(.init(cancel: { [weak self] in + self?.cancelPendingTransientErrorRetry() + }))) + + transientErrorRetryTask = Task { @MainActor [weak self] in + guard let self else { return } + // Bounded, cancellable retry backoff via the injected clock. + try? await self.clock.sleep(for: .seconds(retryDelay)) + guard !Task.isCancelled else { return } + self.transientErrorRetryTask = nil + self.shouldPreserveRetryStateForNextCheck = true + // The delegate call synchronously drives the controller, which may tear down the current + // checking state via `cancelActiveStateForNewCheck()` and re-enter our cancel closure. + // Flag that re-entry so it preserves the failure count; clear it the instant the call + // returns so a subsequent user cancel (e.g. during the controller's readiness wait) is + // honored normally. + self.isRestartingTransientRetry = true + self.actionDelegate?.updaterRequestsRetryCheckForUpdates(preservingInstallIntent: preservingInstallIntent) + self.isRestartingTransientRetry = false + } + return true + } + + private func shouldPreserveInstallIntentForTransientRetry() -> Bool { + switch model.state { + case .downloading, .extracting, .installing: + return true + default: + return false + } + } + + private func cancelPendingTransientErrorRetry() { + transientErrorRetryTask?.cancel() + transientErrorRetryTask = nil + // Suppress the reset only for the *synchronous* internal restart: when the retry fires it calls + // the controller, which (on the ready path) tears down the checking state via + // `cancelActiveStateForNewCheck()` and re-enters this closure. `isRestartingTransientRetry` is + // true only for that synchronous window, so the escalating failure count survives the restart. + // A genuine user cancel — during the backoff sleep, or while the controller parks in its + // readiness-wait loop with the pill still `.checking` — sees the flag false and idles as usual, + // clearing the pending preserve so no stale count carries into a later manual check. + guard !isRestartingTransientRetry else { return } + transientErrorRetryFailureCount = 0 + shouldPreserveRetryStateForNextCheck = false + if case .checking = model.state { + model.setState(.idle) + } + } + + private func resetTransientErrorRetryState() { + transientErrorRetryTask?.cancel() + transientErrorRetryTask = nil + transientErrorRetryFailureCount = 0 + shouldPreserveRetryStateForNextCheck = false + } + private func setStateAfterMinimumCheckDelay(_ newState: UpdateState) { pendingCheckTransitionTask?.cancel() pendingCheckTransitionTask = nil diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift new file mode 100644 index 000000000000..6f50d13b487e --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift @@ -0,0 +1,106 @@ +import Foundation +@preconcurrency import Sparkle + +/// Classifies transient update download failures and maps them to bounded retry delays. +struct UpdateRetryPolicy { + /// The retry delays for consecutive transient failures. + let retryDelays: [TimeInterval] + + /// Creates a retry policy. + init(retryDelays: [TimeInterval] = [1, 3, 8]) { + self.retryDelays = retryDelays + } + + /// The number of automatic retries allowed for one update operation. + var maximumRetryCount: Int { + retryDelays.count + } + + /// Returns the retry delay for a 1-based failure number, or `nil` if this error should + /// surface immediately. + func delay(afterFailureNumber failureNumber: Int, for error: any Swift.Error) -> TimeInterval? { + guard isTransientDownloadError(error) else { return nil } + guard failureNumber > 0, failureNumber <= retryDelays.count else { return nil } + return retryDelays[failureNumber - 1] + } + + /// Whether the error is a Sparkle download failure caused by a transient network/server + /// condition such as GitHub's release CDN returning HTTP 504. + func isTransientDownloadError(_ error: any Swift.Error) -> Bool { + let errors = relatedErrors(startingAt: error as NSError) + guard errors.contains(where: isSparkleDownloadError) else { return false } + return errors.contains(where: hasTransientNetworkSignal) + } + + private func isSparkleDownloadError(_ error: NSError) -> Bool { + error.domain == SUSparkleErrorDomain && error.code == 2001 + } + + private func hasTransientNetworkSignal(_ error: NSError) -> Bool { + if isTransientURLLoadingError(error) { + return true + } + return transientHTTPStatusCode(in: error).map(isTransientHTTPStatusCode) ?? false + } + + private func isTransientURLLoadingError(_ error: NSError) -> Bool { + guard error.domain == NSURLErrorDomain else { return false } + switch error.code { + case NSURLErrorTimedOut, + NSURLErrorCannotConnectToHost, + NSURLErrorDNSLookupFailed, + NSURLErrorNetworkConnectionLost, + NSURLErrorCannotLoadFromNetwork: + return true + default: + return false + } + } + + private func isTransientHTTPStatusCode(_ statusCode: Int) -> Bool { + statusCode == 408 || statusCode == 429 || (500...599).contains(statusCode) + } + + private func transientHTTPStatusCode(in error: NSError) -> Int? { + let text = [ + error.localizedDescription, + (error.userInfo[NSLocalizedFailureReasonErrorKey] as? String) ?? "", + (error.userInfo[NSLocalizedRecoverySuggestionErrorKey] as? String) ?? "", + ].joined(separator: "\n") + + return firstTransientHTTPStatusCode(in: text) + } + + private func firstTransientHTTPStatusCode(in text: String) -> Int? { + let patterns = [ + #"\((\d{3})\)"#, + #"(?i)\bHTTP\s+(\d{3})\b"#, + ] + let nsText = text as NSString + let fullRange = NSRange(location: 0, length: nsText.length) + for pattern in patterns { + guard let regex = try? NSRegularExpression(pattern: pattern) else { continue } + let matches = regex.matches(in: text, range: fullRange) + for match in matches where match.numberOfRanges > 1 { + let status = nsText.substring(with: match.range(at: 1)) + guard let statusCode = Int(status) else { continue } + guard isTransientHTTPStatusCode(statusCode) else { continue } + return statusCode + } + } + return nil + } + + private func relatedErrors(startingAt error: NSError) -> [NSError] { + var errors = [error] + var current = error + var remainingDepth = 8 + while remainingDepth > 0, + let underlying = current.userInfo[NSUnderlyingErrorKey] as? NSError { + errors.append(underlying) + current = underlying + remainingDepth -= 1 + } + return errors + } +} diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift index bd5fff6fecbe..a58329ab0617 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift @@ -173,4 +173,40 @@ import Testing #expect(coordinator.handleStateChange(updateAvailable("0.64.16")) == .none) #expect(coordinator.handleStateChange(.notFound(.init(acknowledgement: {}))) == .none) } + + /// Regression (autoreview P1, follow-up to issue #5632): the download-phase transient-retry + /// re-arm must not strand the coordinator armed when the user cancels the retry before the check + /// restarts. + /// + /// Unlike the user-initiated install flow (`requestInstallLatest`, which enters + /// `awaitingCheckRestart` to ignore a stale on-screen prompt until a real check restarts), the + /// retry re-arm happens while the model is the retry's own `.checking` pill — there is no stale + /// prompt to gate against. If the user cancels that pill during the controller's readiness wait, + /// the model idles directly (no intervening `.checking`); the coordinator must treat that as + /// "check ended" and disarm, so a later, unrelated `.updateAvailable` is not silently + /// auto-confirmed. + @Test func retryRearmDisarmsWhenUserCancelsBeforeCheckRestarts() { + var coordinator = AttemptUpdateCoordinator() + coordinator.armForConfirmedRetryCheck() + #expect(coordinator.isMonitoring) + + // User cancels the retry pill during the readiness wait → the model idles directly, with no + // intervening `.checking` (the fresh check never started). + #expect(coordinator.handleStateChange(.idle) == .none) + #expect(!coordinator.isMonitoring) + + // A later, unrelated update must NOT be auto-confirmed — the user cancelled. + #expect(coordinator.handleStateChange(updateAvailable("0.64.16")) == .none) + } + + /// The re-arm's success path: once the retried check restarts (`.checking`) and resolves an + /// update, the coordinator auto-confirms it so the interrupted install continues silently. + @Test func retryRearmConfirmsUpdateResolvedByRestartedCheck() { + var coordinator = AttemptUpdateCoordinator() + coordinator.armForConfirmedRetryCheck() + + #expect(coordinator.handleStateChange(.checking(.init(cancel: {}))) == .none) + #expect(coordinator.handleStateChange(updateAvailable("0.64.16")) == .confirmInstall) + #expect(!coordinator.isMonitoring) + } } diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/DevStagingUpdateGatingTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/DevStagingUpdateGatingTests.swift index 06d90f1c04f3..68a59a337c93 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/DevStagingUpdateGatingTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/DevStagingUpdateGatingTests.swift @@ -44,18 +44,18 @@ import Testing } @Test func classifiesDebugAndStagingBundlesAsDevLike() { - #expect(UpdateController.isDevLikeBundleIdentifier("com.cmuxterm.app.debug")) - #expect(UpdateController.isDevLikeBundleIdentifier("com.cmuxterm.app.debug.my-tag")) - #expect(UpdateController.isDevLikeBundleIdentifier("com.cmuxterm.app.staging")) - #expect(UpdateController.isDevLikeBundleIdentifier("com.cmuxterm.app.staging.my-tag")) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: "com.cmuxterm.app.debug") == .devLike) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: "com.cmuxterm.app.debug.my-tag") == .devLike) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: "com.cmuxterm.app.staging") == .devLike) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: "com.cmuxterm.app.staging.my-tag") == .devLike) } @Test func doesNotClassifyPublicOrNightlyOrNilAsDevLike() { - #expect(!UpdateController.isDevLikeBundleIdentifier("com.cmuxterm.app")) - #expect(!UpdateController.isDevLikeBundleIdentifier(nil)) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: "com.cmuxterm.app") == .release) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: nil) == .release) // A look-alike that is neither the exact base id nor a dotted suffix must not match. - #expect(!UpdateController.isDevLikeBundleIdentifier("com.cmuxterm.app.debugger")) - #expect(!UpdateController.isDevLikeBundleIdentifier("com.cmuxterm.app.stagingx")) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: "com.cmuxterm.app.debugger") == .release) + #expect(UpdateController.BundleReleaseChannel(bundleIdentifier: "com.cmuxterm.app.stagingx") == .release) } /// A manual "Check for Updates" on a DEV/staging build must not query the public appcast or diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/ImmediateUpdateClock.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/ImmediateUpdateClock.swift new file mode 100644 index 000000000000..dc8947782f08 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/ImmediateUpdateClock.swift @@ -0,0 +1,6 @@ +import Foundation +@testable import CmuxUpdater + +struct ImmediateUpdateClock: UpdateClock { + func sleep(for duration: Duration) async throws {} +} diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/NullUpdateLog.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/NullUpdateLog.swift new file mode 100644 index 000000000000..584e54e0d1d3 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/NullUpdateLog.swift @@ -0,0 +1,6 @@ +@testable import CmuxUpdater + +struct NullUpdateLog: UpdateLogging { + func append(_ message: String) {} + func logPath() -> String { "/tmp/cmux-update-test.log" } +} diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift new file mode 100644 index 000000000000..7f58d6e887cd --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift @@ -0,0 +1,43 @@ +@testable import CmuxUpdater + +@MainActor +final class RecordingUpdateActionDelegate: UpdateActionDelegate { + private(set) var retryRequestCount = 0 + private(set) var retryPreserveInstallIntentValues: [Bool] = [] + private(set) var willRelaunchCount = 0 + + /// Continuations awaiting the next retry request. Resumed the moment + /// `updaterRequestsRetryCheckForUpdates` fires so tests wait on the actual event (causality) + /// rather than a fixed number of scheduler yields (latency). + private var retryWaiters: [CheckedContinuation] = [] + + func updaterRequestsRetryCheckForUpdates(preservingInstallIntent: Bool) { + retryRequestCount += 1 + retryPreserveInstallIntentValues.append(preservingInstallIntent) + let waiters = retryWaiters + retryWaiters.removeAll() + for waiter in waiters { + waiter.resume() + } + } + + func updaterWillRelaunchApplication() { + willRelaunchCount += 1 + } + + /// Suspend until at least `count` retry requests have been recorded. Resumes exactly when the + /// retry action fires, so a test proceeds only once the retry has actually occurred — no + /// scheduler-dependent fixed-duration wait. Both this type and the driver are `@MainActor`, so + /// registering a waiter and resuming it are serialized and race-free. + func waitForRetryRequests(atLeast count: Int) async { + while retryRequestCount < count { + await withCheckedContinuation { (continuation: CheckedContinuation) in + if retryRequestCount >= count { + continuation.resume() + } else { + retryWaiters.append(continuation) + } + } + } + } +} diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift new file mode 100644 index 000000000000..4f40d55154df --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift @@ -0,0 +1,39 @@ +import Foundation +import Testing +@preconcurrency import Sparkle +@testable import CmuxUpdater + +/// Tests for the bounded readiness-wait continuation decision, +/// ``UpdateController/ReadinessWaitDecision``. +/// +/// Regression (autoreview follow-up to issue #5632): the readiness wait is shared by every check +/// that must wait for `canCheckForUpdates`. The transient-retry work guarded it to stop unless the +/// model was `.checking`, which also aborted `attemptUpdate()`'s install-from-`.updateAvailable` +/// re-resolution (issue #6366) whenever Sparkle was briefly not ready — the user's Install then did +/// nothing until some unrelated state change occurred. The wait must keep polling for any pending +/// state and stop only when the user cancels the pending check back to `.idle`. +@MainActor +@Suite struct UpdateControllerReadinessWaitTests { + private var updateAvailable: UpdateState { + .updateAvailable(.init(appcastItem: SUAppcastItem.empty(), reply: { _ in })) + } + + /// `attemptUpdate()` enters the readiness wait while the model is still the `.updateAvailable` + /// prompt it is re-resolving (issue #6366). The wait must keep polling for readiness so the fresh + /// install check actually runs once Sparkle is ready — not abort and strand the Install action. + @Test func keepsWaitingWhileInstallPromptReResolves() { + #expect(UpdateController.ReadinessWaitDecision(modelState: updateAvailable) == .keepPolling) + } + + /// A plain manual check or a transient-retry pill parks the model in `.checking` while waiting + /// for readiness. The wait must keep polling. + @Test func keepsWaitingWhileCheckingPillAwaitsReadiness() { + #expect(UpdateController.ReadinessWaitDecision(modelState: .checking(.init(cancel: {}))) == .keepPolling) + } + + /// Cancelling the pending check returns the model to `.idle`. The wait must stop, so readiness + /// arriving later does not resurrect a check the user dismissed (the retry-pill Cancel, task #12). + @Test func stopsWaitingWhenPendingCheckIsCancelledToIdle() { + #expect(UpdateController.ReadinessWaitDecision(modelState: .idle) == .stop) + } +} diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift new file mode 100644 index 000000000000..114125d9628b --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift @@ -0,0 +1,48 @@ +import Testing +@testable import CmuxUpdater + +/// Regression coverage for the download-phase transient-retry auto-confirm contract (a follow-up to +/// issue #5632's retry work and the issue #6366 re-resolve flow). +/// +/// When a Sparkle download/extract/install phase hits a transient failure, the retry must re-arm the +/// attempt coordinator so the update the retried check finds is auto-confirmed and installation +/// continues silently — it must NOT fall back to a bare check that surfaces a fresh "Update +/// Available" prompt (which strands the interrupted install). +/// +/// ``UpdateController`` owns a live `SPUUpdater`, so the retry *decision* is factored into the pure +/// ``UpdateController/TransientRetryPlan/init(preservingInstallIntent:coordinatorIsMonitoring:)`` and +/// asserted here. The surrounding links of the chain are covered separately: the driver emitting a +/// preserve-intent retry from a download phase (`UpdateDriverRetryTests`), and an armed coordinator +/// confirming the next resolved update (`AttemptUpdateCoordinatorTests`). +@Suite struct UpdateControllerTransientRetryPlanTests { + /// THE FIX: a preserved-intent retry that arrives before the coordinator is monitoring must + /// re-arm the coordinator (auto-confirm), not fall back to a bare check that prompts. + @Test func downloadPhaseRetryWithoutMonitoringRearmsCoordinator() { + let plan = UpdateController.TransientRetryPlan( + preservingInstallIntent: true, + coordinatorIsMonitoring: false + ) + #expect(plan == .rearmConfirmedInstall) + } + + /// An already-monitored retry just restarts the coordinator's in-flight check, regardless of the + /// driver's `preservingInstallIntent` flag. + @Test func monitoredRetryRestartsMonitoredCheck() { + #expect( + UpdateController.TransientRetryPlan(preservingInstallIntent: false, coordinatorIsMonitoring: true) + == .restartMonitoredCheck + ) + #expect( + UpdateController.TransientRetryPlan(preservingInstallIntent: true, coordinatorIsMonitoring: true) + == .restartMonitoredCheck + ) + } + + /// A plain transient retry (no install intent, not monitoring) runs an ordinary fresh check. + @Test func plainRetryRunsPlainCheck() { + #expect( + UpdateController.TransientRetryPlan(preservingInstallIntent: false, coordinatorIsMonitoring: false) + == .plainCheck + ) + } +} diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift new file mode 100644 index 000000000000..9d85cd1e91a1 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift @@ -0,0 +1,141 @@ +import Foundation +import Testing +@testable import CmuxUpdater + +@MainActor +@Suite struct UpdateDriverRetryTests { + @Test func transientGitHubCDN504SchedulesRetryBeforeSurfacingError() async { + let model = UpdateStateModel() + let actionDelegate = RecordingUpdateActionDelegate() + let driver = UpdateDriver(model: model, log: NullUpdateLog(), clock: ImmediateUpdateClock()) + driver.actionDelegate = actionDelegate + driver.recordFeedURLString("https://github.com/manaflow-ai/cmux/releases/latest/download/appcast.xml", usedFallback: false) + + var didAcknowledge = false + driver.showUpdaterError( + Self.sparkleDownloadHTTPError( + statusCode: 504, + statusText: "gateway timed out", + urlString: "https://github.com/manaflow-ai/cmux/releases/download/v0.64.14/cmux-macos.dmg" + ), + acknowledgement: { didAcknowledge = true } + ) + + await actionDelegate.waitForRetryRequests(atLeast: 1) + + #expect(didAcknowledge) + #expect(actionDelegate.retryRequestCount == 1) + #expect(actionDelegate.retryPreserveInstallIntentValues == [false]) + #expect(model.state == .checking(.init(cancel: {}))) + } + + @Test func transientDownloadFailureDuringInstallPreservesInstallIntent() async { + let model = UpdateStateModel() + let actionDelegate = RecordingUpdateActionDelegate() + let driver = UpdateDriver(model: model, log: NullUpdateLog(), clock: ImmediateUpdateClock()) + driver.actionDelegate = actionDelegate + model.setState(.downloading(.init(cancel: {}, expectedLength: nil, progress: 0))) + + driver.showUpdaterError( + Self.sparkleDownloadHTTPError( + statusCode: 504, + statusText: "gateway timed out", + urlString: "https://github.com/manaflow-ai/cmux/releases/download/v0.64.14/cmux-macos.dmg" + ), + acknowledgement: {} + ) + + await actionDelegate.waitForRetryRequests(atLeast: 1) + + #expect(actionDelegate.retryPreserveInstallIntentValues == [true]) + #expect(model.state == .checking(.init(cancel: {}))) + } + + @Test func userCancelWhileRetryPillAwaitsReadinessReturnsToIdle() async { + let model = UpdateStateModel() + let actionDelegate = RecordingUpdateActionDelegate() + let driver = UpdateDriver(model: model, log: NullUpdateLog(), clock: ImmediateUpdateClock()) + driver.actionDelegate = actionDelegate + + // A transient CDN 504 schedules a retry. With the immediate clock the backoff fires at + // once and asks the controller to re-check, but the recording delegate never starts a real + // Sparkle check — so the pill stays parked in `.checking` with the driver's retry cancel + // closure, exactly as the real controller leaves it while waiting for `canCheckForUpdates` + // in `waitForReadinessThenCheck`. + driver.showUpdaterError( + Self.sparkleDownloadHTTPError( + statusCode: 504, + statusText: "gateway timed out", + urlString: "https://github.com/manaflow-ai/cmux/releases/download/v0.64.14/cmux-macos.dmg" + ), + acknowledgement: {} + ) + await actionDelegate.waitForRetryRequests(atLeast: 1) + + guard case let .checking(checking) = model.state else { + Issue.record("Expected the fired retry to leave the pill in .checking") + return + } + + // Pressing Cancel on the still-checking retry pill must abort the retry and idle the pill. + // It must not no-op just because the retry-state preserve flag is set for the pending + // restart (the readiness-wait cancel regression, UpdateDriver.swift). + checking.cancel() + + #expect(model.state == .idle) + } + + @Test func nonTransientDownload404SurfacesErrorWithoutRetry() { + let model = UpdateStateModel() + let actionDelegate = RecordingUpdateActionDelegate() + let driver = UpdateDriver(model: model, log: NullUpdateLog(), clock: ImmediateUpdateClock()) + driver.actionDelegate = actionDelegate + + driver.showUpdaterError( + Self.sparkleDownloadHTTPError( + statusCode: 404, + statusText: "not found", + urlString: "https://github.com/manaflow-ai/cmux/releases/download/v0.64.14/cmux-macos.dmg" + ), + acknowledgement: {} + ) + + #expect(actionDelegate.retryRequestCount == 0) + guard case .error = model.state else { + Issue.record("Expected a visible update error for non-transient HTTP status") + return + } + } + + @Test func laterTransientStatusIsNotShadowedByEarlierNonTransientStatus() { + let error = Self.sparkleDownloadHTTPError( + statusCode: 504, + statusText: "cached response marker (200); retryable upstream status HTTP 504", + urlString: "https://github.com/manaflow-ai/cmux/releases/latest/download/appcast.xml" + ) + + let delay = UpdateRetryPolicy().delay(afterFailureNumber: 1, for: error) + + #expect(delay == 1) + } + + private static func sparkleDownloadHTTPError(statusCode: Int, statusText: String, urlString: String) -> NSError { + let url = URL(string: urlString)! + let underlying = NSError( + domain: "SUSparkleErrorDomain", + code: 2001, + userInfo: [ + NSLocalizedDescriptionKey: "A network error occurred while downloading \(urlString). \(statusText) (\(statusCode))", + ] + ) + return NSError( + domain: "SUSparkleErrorDomain", + code: 2001, + userInfo: [ + NSLocalizedDescriptionKey: "An error occurred while downloading the update. Please try again later.", + NSUnderlyingErrorKey: underlying, + NSURLErrorFailingURLErrorKey: url, + ] + ) + } +} diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index 3c060841eac9..b10f52c5a912 100644 --- a/Sources/AppDelegate.swift +++ b/Sources/AppDelegate.swift @@ -17840,8 +17840,8 @@ private extension NSWindow { /// Conforms the composition root to updater host actions, retry, and relaunch seams. /// `checkForUpdatesInCustomUI()` is satisfied by the main `AppDelegate` declaration. extension AppDelegate: UpdateActionDelegate, UpdateActionsHost { - func updaterRequestsRetryCheckForUpdates() { - checkForUpdates(nil) + func updaterRequestsRetryCheckForUpdates(preservingInstallIntent: Bool) { + updateController.retryAfterTransientFailure(preservingInstallIntent: preservingInstallIntent) } func updaterWillRelaunchApplication() { @@ -17853,9 +17853,7 @@ extension AppDelegate: UpdateActionDelegate, UpdateActionsHost { } } - func attemptUpdate() { - attemptUpdate(nil) - } + func attemptUpdate() { attemptUpdate(nil) } var updateLogPath: String { updateLog.logPath() diff --git a/scripts/ci/run-app-host-xcodebuild.sh b/scripts/ci/run-app-host-xcodebuild.sh index ca53b723fe31..0bfbf81b5d16 100755 --- a/scripts/ci/run-app-host-xcodebuild.sh +++ b/scripts/ci/run-app-host-xcodebuild.sh @@ -11,6 +11,16 @@ max_attempts="${CMUX_APP_HOST_XCODEBUILD_ATTEMPTS:-3}" export CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TIMEOUT_SECONDS="${CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TIMEOUT_SECONDS:-${CMUX_XCODEBUILD_NONINTERACTIVE_TIMEOUT_SECONDS:-300}}" echo "App-host xcodebuild idle timeout: ${CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TIMEOUT_SECONDS}s, attempts: ${max_attempts}" +# Keep Foundation's temporaryDirectory short inside app-host XCTest. Some tests bind +# AF_UNIX sockets under temporaryDirectory, and macOS sockaddr_un paths are capped. +app_host_tmpdir="${CMUX_APP_HOST_TMPDIR:-/tmp/}" +case "$app_host_tmpdir" in + */) ;; + *) app_host_tmpdir="${app_host_tmpdir}/" ;; +esac +mkdir -p "$app_host_tmpdir" +export TMPDIR="$app_host_tmpdir" + # Principled serialization (the actual fix; the retry below is only a backstop). # Invariant: a GUI test host owns the Mac's single login session + testmanagerd # while it runs. Two hosts on one self-hosted Mac contend for that one session @@ -22,7 +32,7 @@ echo "App-host xcodebuild idle timeout: ${CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TI # script's whole lifetime. Different machines use different local lock files, so # cross-machine parallelism is preserved. if [ -z "${CMUX_APP_HOST_TEST_LOCK_ACTIVE:-}" ]; then - lock_file="${CMUX_APP_HOST_TEST_LOCK_FILE:-${TMPDIR:-/tmp}/cmux-app-host-test.lock}" + lock_file="${CMUX_APP_HOST_TEST_LOCK_FILE:-${TMPDIR%/}/cmux-app-host-test.lock}" lock_wait_seconds="${CMUX_APP_HOST_TEST_LOCK_WAIT_SECONDS:-3600}" export CMUX_APP_HOST_TEST_LOCK_ACTIVE=1 exec python3 "$(dirname "$0")/app_host_test_lock.py" \ diff --git a/tests/test_ci_app_host_xcodebuild_retry.sh b/tests/test_ci_app_host_xcodebuild_retry.sh index ecb2121a15f4..82c3bae87d6d 100755 --- a/tests/test_ci_app_host_xcodebuild_retry.sh +++ b/tests/test_ci_app_host_xcodebuild_retry.sh @@ -7,6 +7,7 @@ trap 'rm -rf "$TMP_DIR"' EXIT cat > "$TMP_DIR/xcodebuild" <<'SH' #!/usr/bin/env bash +echo "${TMPDIR:-}" >> "${CMUX_TEST_OBSERVED_TMPDIR:?}" sleep 10 SH chmod +x "$TMP_DIR/xcodebuild" @@ -14,8 +15,9 @@ chmod +x "$TMP_DIR/xcodebuild" set +e PATH="$TMP_DIR:$PATH" \ RUNNER_TEMP="$TMP_DIR" \ +CMUX_TEST_OBSERVED_TMPDIR="$TMP_DIR/observed-tmpdir.log" \ CMUX_APP_HOST_XCODEBUILD_ATTEMPTS=2 \ -CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TIMEOUT_SECONDS=0.1 \ +CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TIMEOUT_SECONDS=0.5 \ bash "$ROOT_DIR/scripts/ci/run-app-host-xcodebuild.sh" test >"$TMP_DIR/output.log" 2>&1 status=$? set -e @@ -26,13 +28,20 @@ if [ "$status" -ne 124 ]; then exit 1 fi -if ! grep -Fq "Retrying app-host xcodebuild after 0.1s idle timeout (attempt 1/2)" "$TMP_DIR/output.log"; then +if ! grep -Fq "Retrying app-host xcodebuild after 0.5s idle timeout (attempt 1/2)" "$TMP_DIR/output.log"; then cat "$TMP_DIR/output.log" echo "FAIL: wrapper did not retry after idle timeout" exit 1 fi -timeout_count="$(grep -Fc "Idle timed out after 0.1s" "$TMP_DIR/output.log")" +if ! grep -Fxq "/tmp/" "$TMP_DIR/observed-tmpdir.log"; then + cat "$TMP_DIR/output.log" + cat "$TMP_DIR/observed-tmpdir.log" 2>/dev/null || true + echo "FAIL: wrapper did not force a short app-host TMPDIR" + exit 1 +fi + +timeout_count="$(grep -Fc "Idle timed out after 0.5s" "$TMP_DIR/output.log")" if [ "$timeout_count" -ne 2 ]; then cat "$TMP_DIR/output.log" echo "FAIL: expected two timed-out attempts, got $timeout_count"