From dabcb477de2b42ad755d6e2ca46b9d63698a2be5 Mon Sep 17 00:00:00 2001 From: cmux Date: Fri, 26 Jun 2026 16:34:13 -0700 Subject: [PATCH 01/24] Add Sparkle transient download retry regression test --- .../UpdateDriverRetryTests.swift | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift new file mode 100644 index 000000000000..53c7e103f5b1 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift @@ -0,0 +1,97 @@ +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 } + ) + + for _ in 0..<5 { + await Task.yield() + } + + #expect(didAcknowledge) + #expect(actionDelegate.retryRequestCount == 1) + #expect(model.state == .checking(.init(cancel: {}))) + } + + @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 + } + } + + 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, + ] + ) + } +} + +private struct ImmediateUpdateClock: UpdateClock { + func sleep(for duration: Duration) async throws {} +} + +private struct NullUpdateLog: UpdateLogging { + func append(_ message: String) {} + func logPath() -> String { "/tmp/cmux-update-test.log" } +} + +@MainActor +private final class RecordingUpdateActionDelegate: UpdateActionDelegate { + private(set) var retryRequestCount = 0 + private(set) var willRelaunchCount = 0 + + func updaterRequestsRetryCheckForUpdates() { + retryRequestCount += 1 + } + + func updaterWillRelaunchApplication() { + willRelaunchCount += 1 + } +} From 22def7e16180697af2a64b439335bfd252dd86b4 Mon Sep 17 00:00:00 2001 From: cmux Date: Fri, 26 Jun 2026 16:38:47 -0700 Subject: [PATCH 02/24] Retry transient Sparkle download failures --- .../CmuxUpdater/UpdateController.swift | 25 +++- .../Sources/CmuxUpdater/UpdateDriver.swift | 71 +++++++++++- .../CmuxUpdater/UpdateRetryPolicy.swift | 107 ++++++++++++++++++ .../ImmediateUpdateClock.swift | 6 + .../CmuxUpdaterTests/NullUpdateLog.swift | 6 + .../RecordingUpdateActionDelegate.swift | 15 +++ .../UpdateDriverRetryTests.swift | 23 ---- Sources/AppDelegate.swift | 2 +- 8 files changed, 224 insertions(+), 31 deletions(-) create mode 100644 Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift create mode 100644 Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/ImmediateUpdateClock.swift create mode 100644 Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/NullUpdateLog.swift create mode 100644 Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index c3114782eda6..277fc37e260d 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -218,12 +218,25 @@ 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 can + /// continue silently after Sparkle finds the same update again. + public func retryAfterTransientFailure() { + let shouldPreserveInstallIntent = isForceInstalling || isAttemptingUpdate + log.append("retrying update after transient download failure (preserveInstallIntent=\(shouldPreserveInstallIntent))") + checkForUpdatesWhenReady(preservingInstallIntent: shouldPreserveInstallIntent) + } + + 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() + if preservingInstallIntent { + updater.checkForUpdates() + return + } if model.state == .idle { updater.checkForUpdates() return @@ -245,29 +258,29 @@ public final class UpdateController { } /// Check for updates once the updater reports it can. - private func checkForUpdatesWhenReady() { + private func checkForUpdatesWhenReady(preservingInstallIntent: Bool = false) { cancelReadinessRetry() startUpdaterIfNeeded() ensureSparkleInstallationCache() 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 { if self.updater.canCheckForUpdates { - self.performCheckForUpdates() + self.performCheckForUpdates(preservingInstallIntent: preservingInstallIntent) return } remaining -= 1 diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift index 72073a717eb9..07580daceab8 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift @@ -14,6 +14,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { let model: UpdateStateModel let log: any UpdateLogging private let clock: any UpdateClock + private let retryPolicy: UpdateRetryPolicy /// Host actions the driver delegates upward. Held weak; set by ``UpdateController``. weak var actionDelegate: (any UpdateActionDelegate)? @@ -22,18 +23,26 @@ 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 private(set) var lastFeedURLString: String? - init(model: UpdateStateModel, log: any UpdateLogging, clock: any UpdateClock) { + init(model: UpdateStateModel, + log: any UpdateLogging, + clock: any UpdateClock, + retryPolicy: UpdateRetryPolicy = UpdateRetryPolicy()) { self.model = model self.log = log self.clock = clock + self.retryPolicy = retryPolicy super.init() } deinit { pendingCheckTransitionTask?.cancel() checkTimeoutTask?.cancel() + transientErrorRetryTask?.cancel() } func show(_ request: SPUUpdatePermissionRequest, @@ -74,6 +83,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))) } @@ -82,6 +92,10 @@ 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 @@ -128,6 +142,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { } func showDownloadDidStartExtractingUpdate() { + resetTransientErrorRetryState() log.append("show extraction started") setState(.extracting(.init(progress: 0))) } @@ -153,6 +168,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() @@ -182,6 +198,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 @@ -192,6 +215,52 @@ 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 + } + + 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 + self.actionDelegate?.updaterRequestsRetryCheckForUpdates() + } + return true + } + + private func cancelPendingTransientErrorRetry() { + transientErrorRetryTask?.cancel() + transientErrorRetryTask = nil + guard !shouldPreserveRetryStateForNextCheck else { return } + transientErrorRetryFailureCount = 0 + 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..6584e6470d5c --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift @@ -0,0 +1,107 @@ +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 firstHTTPStatusCode(in: text).flatMap { statusCode in + isTransientHTTPStatusCode(statusCode) ? statusCode : nil + } + } + + private func firstHTTPStatusCode(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 } + guard let match = regex.firstMatch(in: text, range: fullRange) else { continue } + guard match.numberOfRanges > 1 else { continue } + let status = nsText.substring(with: match.range(at: 1)) + if let statusCode = Int(status) { + 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/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..50932488a337 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift @@ -0,0 +1,15 @@ +@testable import CmuxUpdater + +@MainActor +final class RecordingUpdateActionDelegate: UpdateActionDelegate { + private(set) var retryRequestCount = 0 + private(set) var willRelaunchCount = 0 + + func updaterRequestsRetryCheckForUpdates() { + retryRequestCount += 1 + } + + func updaterWillRelaunchApplication() { + willRelaunchCount += 1 + } +} diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift index 53c7e103f5b1..6f2bb7563d02 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift @@ -72,26 +72,3 @@ import Testing ) } } - -private struct ImmediateUpdateClock: UpdateClock { - func sleep(for duration: Duration) async throws {} -} - -private struct NullUpdateLog: UpdateLogging { - func append(_ message: String) {} - func logPath() -> String { "/tmp/cmux-update-test.log" } -} - -@MainActor -private final class RecordingUpdateActionDelegate: UpdateActionDelegate { - private(set) var retryRequestCount = 0 - private(set) var willRelaunchCount = 0 - - func updaterRequestsRetryCheckForUpdates() { - retryRequestCount += 1 - } - - func updaterWillRelaunchApplication() { - willRelaunchCount += 1 - } -} diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index 2a00ae4d7545..0b2902351be1 100644 --- a/Sources/AppDelegate.swift +++ b/Sources/AppDelegate.swift @@ -17725,7 +17725,7 @@ private extension NSWindow { /// `checkForUpdatesInCustomUI()` is satisfied by the main `AppDelegate` declaration. extension AppDelegate: UpdateActionDelegate, UpdateActionsHost { func updaterRequestsRetryCheckForUpdates() { - checkForUpdates(nil) + updateController.retryAfterTransientFailure() } func updaterWillRelaunchApplication() { From fccabfc34d7f43024570b2734717d8c9b9b621eb Mon Sep 17 00:00:00 2001 From: cmux Date: Fri, 26 Jun 2026 16:55:03 -0700 Subject: [PATCH 03/24] Handle shadowed transient Sparkle status codes --- .../Sources/CmuxUpdater/UpdateRetryPolicy.swift | 15 +++++++-------- .../CmuxUpdaterTests/UpdateDriverRetryTests.swift | 12 ++++++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift index 6584e6470d5c..6f50d13b487e 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift @@ -68,12 +68,10 @@ struct UpdateRetryPolicy { (error.userInfo[NSLocalizedRecoverySuggestionErrorKey] as? String) ?? "", ].joined(separator: "\n") - return firstHTTPStatusCode(in: text).flatMap { statusCode in - isTransientHTTPStatusCode(statusCode) ? statusCode : nil - } + return firstTransientHTTPStatusCode(in: text) } - private func firstHTTPStatusCode(in text: String) -> Int? { + private func firstTransientHTTPStatusCode(in text: String) -> Int? { let patterns = [ #"\((\d{3})\)"#, #"(?i)\bHTTP\s+(\d{3})\b"#, @@ -82,10 +80,11 @@ struct UpdateRetryPolicy { let fullRange = NSRange(location: 0, length: nsText.length) for pattern in patterns { guard let regex = try? NSRegularExpression(pattern: pattern) else { continue } - guard let match = regex.firstMatch(in: text, range: fullRange) else { continue } - guard match.numberOfRanges > 1 else { continue } - let status = nsText.substring(with: match.range(at: 1)) - if let statusCode = Int(status) { + 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 } } diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift index 6f2bb7563d02..344cd022ed31 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift @@ -52,6 +52,18 @@ import Testing } } + @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( From 01fccf96e153f0e886c7fd76811b55285f190758 Mon Sep 17 00:00:00 2001 From: cmux Date: Fri, 26 Jun 2026 22:36:35 -0700 Subject: [PATCH 04/24] Fix tmux compat test socket path length --- cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift b/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift index 5effed516529..cc27f7cdde50 100644 --- a/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift +++ b/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift @@ -9432,7 +9432,7 @@ extension CLINotifyProcessIntegrationRegressionTests { .appendingPathComponent("cmux-spawn-id-e2e-\(UUID().uuidString)", isDirectory: true) try FileManager.default.createDirectory(at: tmpDir, withIntermediateDirectories: true) defer { try? FileManager.default.removeItem(at: tmpDir) } - let socketPath = tmpDir.appendingPathComponent("sock").path + let socketPath = makeSocketPath("tmux-env") let listenerFD = try bindUnixSocket(at: socketPath) defer { Darwin.close(listenerFD); unlink(socketPath) } let state = MockSocketServerState() From e10fbade800c2336ade6238acc2ff702dead2990 Mon Sep 17 00:00:00 2001 From: cmux Date: Fri, 26 Jun 2026 22:44:53 -0700 Subject: [PATCH 05/24] Preserve update install intent on retry --- .../CmuxUpdater/UpdateActionDelegate.swift | 6 ++++- .../CmuxUpdater/UpdateController.swift | 7 ++++-- .../Sources/CmuxUpdater/UpdateDriver.swift | 14 +++++++++-- .../RecordingUpdateActionDelegate.swift | 4 ++- .../UpdateDriverRetryTests.swift | 25 +++++++++++++++++++ .../CmuxUpdaterUI/UpdateActionsHost.swift | 3 +++ .../CmuxUpdaterUI/UpdatePopoverView.swift | 5 ++-- Sources/AppDelegate.swift | 8 ++++-- 8 files changed, 62 insertions(+), 10 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift index 1f9e09d49d96..4e395c1f113d 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift @@ -8,7 +8,11 @@ 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.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index 277fc37e260d..d424310df1bf 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -221,8 +221,11 @@ public final class UpdateController { /// 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 can /// continue silently after Sparkle finds the same update again. - public func retryAfterTransientFailure() { - let shouldPreserveInstallIntent = isForceInstalling || isAttemptingUpdate + /// + /// - 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 shouldPreserveInstallIntent = preservingInstallIntent || isForceInstalling || isAttemptingUpdate log.append("retrying update after transient download failure (preserveInstallIntent=\(shouldPreserveInstallIntent))") checkForUpdatesWhenReady(preservingInstallIntent: shouldPreserveInstallIntent) } diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift index 07580daceab8..496ecb1d342e 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift @@ -100,7 +100,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { 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) @@ -225,6 +225,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { 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") @@ -239,11 +240,20 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { guard !Task.isCancelled else { return } self.transientErrorRetryTask = nil self.shouldPreserveRetryStateForNextCheck = true - self.actionDelegate?.updaterRequestsRetryCheckForUpdates() + self.actionDelegate?.updaterRequestsRetryCheckForUpdates(preservingInstallIntent: preservingInstallIntent) } 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 diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift index 50932488a337..5ab1526049d6 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift @@ -3,10 +3,12 @@ @MainActor final class RecordingUpdateActionDelegate: UpdateActionDelegate { private(set) var retryRequestCount = 0 + private(set) var retryPreserveInstallIntentValues: [Bool] = [] private(set) var willRelaunchCount = 0 - func updaterRequestsRetryCheckForUpdates() { + func updaterRequestsRetryCheckForUpdates(preservingInstallIntent: Bool) { retryRequestCount += 1 + retryPreserveInstallIntentValues.append(preservingInstallIntent) } func updaterWillRelaunchApplication() { diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift index 344cd022ed31..d3825b10e060 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift @@ -27,6 +27,31 @@ import Testing #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: {} + ) + + for _ in 0..<5 { + await Task.yield() + } + + #expect(actionDelegate.retryPreserveInstallIntentValues == [true]) #expect(model.state == .checking(.init(cancel: {}))) } diff --git a/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdateActionsHost.swift b/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdateActionsHost.swift index 32cc1f412aa3..cd23c9b6df40 100644 --- a/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdateActionsHost.swift +++ b/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdateActionsHost.swift @@ -14,6 +14,9 @@ public protocol UpdateActionsHost: AnyObject { /// "Install and Relaunch" action on the detected-update popover). func attemptUpdate() + /// Install the currently available update through the shared update controller path. + func installUpdate() + /// The filesystem path of the update log, shown in the error popover's details block. var updateLogPath: String { get } } diff --git a/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdatePopoverView.swift b/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdatePopoverView.swift index c2a0322a48ad..c3de0f71a9c2 100644 --- a/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdatePopoverView.swift +++ b/Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdatePopoverView.swift @@ -43,7 +43,7 @@ public struct UpdatePopoverView: View { CheckingView(checking: checking, dismiss: dismiss) case .updateAvailable(let update): - UpdateAvailableView(update: update, dismiss: dismiss) + UpdateAvailableView(update: update, actions: actions, dismiss: dismiss) case .downloading(let download): DownloadingView(download: download, dismiss: dismiss) @@ -273,6 +273,7 @@ private struct CheckingView: View { private struct UpdateAvailableView: View { let update: UpdateState.UpdateAvailable + let actions: any UpdateActionsHost let dismiss: () -> Void private let labelWidth: CGFloat = 60 @@ -304,7 +305,7 @@ private struct UpdateAvailableView: View { Spacer() Button(String(localized: "common.installAndRelaunch", defaultValue: "Install and Relaunch")) { - update.reply(.install) + actions.installUpdate() dismiss() } .keyboardShortcut(.defaultAction) diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index 0e87948061bd..aae83de9a4c5 100644 --- a/Sources/AppDelegate.swift +++ b/Sources/AppDelegate.swift @@ -17740,8 +17740,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() { - updateController.retryAfterTransientFailure() + func updaterRequestsRetryCheckForUpdates(preservingInstallIntent: Bool) { + updateController.retryAfterTransientFailure(preservingInstallIntent: preservingInstallIntent) } func updaterWillRelaunchApplication() { @@ -17757,6 +17757,10 @@ extension AppDelegate: UpdateActionDelegate, UpdateActionsHost { attemptUpdate(nil) } + func installUpdate() { + updateController.installUpdate() + } + var updateLogPath: String { updateLog.logPath() } From eb6328ac74bf2a3eb28ffd890e6eaeb23f9f94cd Mon Sep 17 00:00:00 2001 From: cmux Date: Fri, 26 Jun 2026 22:49:04 -0700 Subject: [PATCH 06/24] Avoid AppDelegate update wrapper growth --- Sources/AppDelegate.swift | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index aae83de9a4c5..5c756d5cef8c 100644 --- a/Sources/AppDelegate.swift +++ b/Sources/AppDelegate.swift @@ -17753,13 +17753,9 @@ extension AppDelegate: UpdateActionDelegate, UpdateActionsHost { } } - func attemptUpdate() { - attemptUpdate(nil) - } + func attemptUpdate() { attemptUpdate(nil) } - func installUpdate() { - updateController.installUpdate() - } + func installUpdate() { applyUpdateIfAvailable(nil) } var updateLogPath: String { updateLog.logPath() From 5751e0bf4839e06ae7c01f7aefb2d13adb912d7a Mon Sep 17 00:00:00 2001 From: cmux Date: Fri, 26 Jun 2026 22:52:44 -0700 Subject: [PATCH 07/24] Use short app-host tmpdir in CI --- ...INotifyProcessIntegrationRegressionTests.swift | 2 +- scripts/ci/run-app-host-xcodebuild.sh | 12 +++++++++++- tests/test_ci_app_host_xcodebuild_retry.sh | 15 ++++++++++++--- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift b/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift index cc27f7cdde50..5effed516529 100644 --- a/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift +++ b/cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift @@ -9432,7 +9432,7 @@ extension CLINotifyProcessIntegrationRegressionTests { .appendingPathComponent("cmux-spawn-id-e2e-\(UUID().uuidString)", isDirectory: true) try FileManager.default.createDirectory(at: tmpDir, withIntermediateDirectories: true) defer { try? FileManager.default.removeItem(at: tmpDir) } - let socketPath = makeSocketPath("tmux-env") + let socketPath = tmpDir.appendingPathComponent("sock").path let listenerFD = try bindUnixSocket(at: socketPath) defer { Darwin.close(listenerFD); unlink(socketPath) } let state = MockSocketServerState() 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" From 5cb984e286565aca58c0dcadb1794dfcf8bb4ae2 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Mon, 29 Jun 2026 17:35:05 -0700 Subject: [PATCH 08/24] Wait on retry causality instead of fixed yields in update retry tests CodeRabbit flagged the `for _ in 0..<5 { await Task.yield() }` waits in UpdateDriverRetryTests as scheduler-dependent (they can race the retry task on slower CI), violating the repo guideline "assert on causality, not latency." Add an async completion signal to RecordingUpdateActionDelegate (`waitForRetryRequests(atLeast:)`) that resumes the moment `updaterRequestsRetryCheckForUpdates` fires, and await it in the two transient tests instead of yielding a fixed number of times. Both the mock and the driver are @MainActor, so registering a waiter and resuming it are serialized and race-free. The non-transient 404 test stays synchronous (it asserts no retry is scheduled). Co-Authored-By: Claude Opus 4.8 --- .../RecordingUpdateActionDelegate.swift | 26 +++++++++++++++++++ .../UpdateDriverRetryTests.swift | 8 ++---- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift index 5ab1526049d6..7f58d6e887cd 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift @@ -6,12 +6,38 @@ final class RecordingUpdateActionDelegate: UpdateActionDelegate { 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/UpdateDriverRetryTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift index d3825b10e060..64c37fdcf2d0 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift @@ -21,9 +21,7 @@ import Testing acknowledgement: { didAcknowledge = true } ) - for _ in 0..<5 { - await Task.yield() - } + await actionDelegate.waitForRetryRequests(atLeast: 1) #expect(didAcknowledge) #expect(actionDelegate.retryRequestCount == 1) @@ -47,9 +45,7 @@ import Testing acknowledgement: {} ) - for _ in 0..<5 { - await Task.yield() - } + await actionDelegate.waitForRetryRequests(atLeast: 1) #expect(actionDelegate.retryPreserveInstallIntentValues == [true]) #expect(model.state == .checking(.init(cancel: {}))) From 5a0406c016432a8a0f1d92dca2afb69c17272a54 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Wed, 1 Jul 2026 23:03:42 -0700 Subject: [PATCH 09/24] Add failing test for download-phase transient retry auto-confirm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Sparkle transient download failure during a download/extract/install phase asks the host to retry with preservingInstallIntent=true. When the attempt coordinator (issue #6366) is not yet monitoring, the controller must re-arm it so the retried check auto-confirms whatever update it resolves — otherwise the retry surfaces a fresh "Update Available" prompt and the interrupted install is silently stranded (the very failure mode issue #5632's retry work is meant to recover from). Factor the controller's retry decision into a pure UpdateController.transientRetryPlan(...) in its own file, mirroring AttemptUpdateCoordinator's pure-policy split so the decision is testable without the live SPUUpdater the controller owns, and dispatch retryAfterTransientFailure through it. This commit deliberately keeps today's buggy mapping (a preserved-intent retry that is not yet monitored still collapses into the monitored-restart path and never re-arms the coordinator), so the new UpdateControllerTransientRetryPlanTests regression fails. The one-line mapping fix follows in the next commit. Co-Authored-By: Claude Opus 4.8 --- .../UpdateController+TransientRetry.swift | 34 +++++++++++++ .../CmuxUpdater/UpdateController.swift | 34 +++++++++---- ...ateControllerTransientRetryPlanTests.swift | 48 +++++++++++++++++++ 3 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift create mode 100644 Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift 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..01ffe2a9dd48 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift @@ -0,0 +1,34 @@ +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 ``retryAfterTransientFailure(preservingInstallIntent:)``; see + /// ``TransientRetryPlan`` for what each case means. + nonisolated static func transientRetryPlan( + preservingInstallIntent: Bool, + coordinatorIsMonitoring: Bool + ) -> TransientRetryPlan { + // BUG (issue #5632 follow-up): a preserved-intent retry that is not already being monitored + // is collapsed into the monitored-restart path, so the coordinator is never armed and the + // retried check surfaces a prompt instead of auto-confirming the update it finds. + if preservingInstallIntent || coordinatorIsMonitoring { return .restartMonitoredCheck } + return .plainCheck + } +} diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index e266c551511e..46eb3d43f44b 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -202,21 +202,35 @@ public final class UpdateController { checkForUpdatesWhenReady() } - /// 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 can - /// continue silently after Sparkle finds the same update again. + /// 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 driver passes `preservingInstallIntent: true` when the failure occurred during a - /// download/extract/install phase. We additionally OR in ``AttemptUpdateCoordinator/isMonitoring`` - /// so intent is preserved when the failure lands during the coordinator's own re-resolve - /// check (issue #6366), before the install state machine reaches a downloading phase. + /// The reaction is chosen by the pure ``transientRetryPlan(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 shouldPreserveInstallIntent = preservingInstallIntent || attemptCoordinator.isMonitoring - log.append("retrying update after transient download failure (preserveInstallIntent=\(shouldPreserveInstallIntent))") - checkForUpdatesWhenReady(preservingInstallIntent: shouldPreserveInstallIntent) + let plan = Self.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 on the retried check via the shared attempt path so the + // update it finds is auto-confirmed, matching the pre-failure install intent. + performAttemptAction(attemptCoordinator.requestInstallLatest(currentState: model.state)) + case .plainCheck: + checkForUpdatesWhenReady(preservingInstallIntent: false) + } } private func performCheckForUpdates(preservingInstallIntent: Bool = false) { diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift new file mode 100644 index 000000000000..eb463eaefe07 --- /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(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 + ) + } +} From d5ee061a4249e1b7327f0c3f66a04c60f145c547 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Wed, 1 Jul 2026 23:16:12 -0700 Subject: [PATCH 10/24] Re-arm attempt coordinator on preserved-intent transient retry Flip the one remaining branch of transientRetryPlan: a transient retry carrying install intent that arrives before the attempt coordinator is monitoring now returns .rearmConfirmedInstall, which routes through the shared attempt path (requestInstallLatest) so the coordinator is armed and the retried check auto-confirms the update it resolves. Previously this case fell through to a bare fresh check that only surfaced an "Update Available" prompt, silently stranding the interrupted install. Turns UpdateControllerTransientRetryPlanTests green. Co-Authored-By: Claude Opus 4.8 --- .../CmuxUpdater/UpdateController+TransientRetry.swift | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift index 01ffe2a9dd48..b759a6228220 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift @@ -25,10 +25,13 @@ extension UpdateController { preservingInstallIntent: Bool, coordinatorIsMonitoring: Bool ) -> TransientRetryPlan { - // BUG (issue #5632 follow-up): a preserved-intent retry that is not already being monitored - // is collapsed into the monitored-restart path, so the coordinator is never armed and the - // retried check surfaces a prompt instead of auto-confirming the update it finds. - if preservingInstallIntent || coordinatorIsMonitoring { return .restartMonitoredCheck } + // 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). + if coordinatorIsMonitoring { return .restartMonitoredCheck } + if preservingInstallIntent { return .rearmConfirmedInstall } return .plainCheck } } From 46c12ddc9254e279dbf42f19bdacd65f508ee05c Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 02:18:12 -0700 Subject: [PATCH 11/24] Document why preserved-intent retry skips the Sparkle teardown delay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preserved-intent branch in `performCheckForUpdates` returns early and calls `updater.checkForUpdates()` immediately, bypassing the non-idle `cancelActiveStateForNewCheck()` + 100ms delay used elsewhere. That looks like it could let Sparkle coalesce/drop the re-check, but it is safe and intentional: - This path is only reached from `retryAfterTransientFailure` after the driver's multi-second retry backoff, and the failed Sparkle session was already acknowledged/torn down at error time (`showUpdaterError` → `acknowledgement()`). There is no just-dismissed live session to coalesce with — the 100ms delay only guards an *immediate* re-check against a session Sparkle is still aborting. - Routing it through the teardown path would be wrong: the current non-idle state is the driver's synthetic `.checking` backoff placeholder whose `cancel` aborts the retry, so `cancelActiveStateForNewCheck()` would kill the retry and flicker the pill to idle. Comment-only; no behavior change. Documents the invariant an autoreview pass flagged so it is legible to future readers. Co-Authored-By: Claude Opus 4.8 --- .../Sources/CmuxUpdater/UpdateController.swift | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index 46eb3d43f44b..e3d67f54b016 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -239,6 +239,15 @@ public final class UpdateController { // 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 preserved-intent retry 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 this path through it + // would also be wrong: the current non-idle state is the driver's synthetic `.checking` + // backoff placeholder whose `cancel` aborts the retry, so `cancelActiveStateForNewCheck()` + // would kill the very retry and flicker the pill to idle. Start the fresh check directly. if preservingInstallIntent { updater.checkForUpdates() return From 8fd40d797adf13b4cc064591a73febaee7f9247d Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 03:05:02 -0700 Subject: [PATCH 12/24] Document why the transient-retry cancel guard preserves user cancellation The guard in cancelPendingTransientErrorRetry preserves the escalating failure count across the synchronous internal restart teardown, and does not strand a user-initiated cancel: at retry time Sparkle has already acknowledged/ended the failed session and the backoff has elapsed, so the controller performs the check immediately rather than parking in the uncancellable readiness-wait loop. Documents the invariant behind autoreview's low-confidence UpdateDriver.swift:266 finding (no behavior change). Co-Authored-By: Claude Opus 4.8 --- .../CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift index 6502d5604a10..a242ae305422 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift @@ -263,6 +263,16 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { private func cancelPendingTransientErrorRetry() { transientErrorRetryTask?.cancel() transientErrorRetryTask = nil + // The guard preserves the escalating failure count across the *synchronous* internal restart + // teardown: on the plain-recheck path the controller calls `cancelActiveStateForNewCheck()`, + // which reruns this same closure via `state.cancel()` while `shouldPreserveRetryStateForNextCheck` + // is briefly set (from just before the delegate call until `beginChecking` consumes it). It does + // not swallow a user-initiated cancel: during the multi-second backoff sleep the flag is still + // false, so tapping Cancel then resets and idles here as usual. The flag's true-window never + // becomes a long uncancellable checking pill because at retry time Sparkle has already + // acknowledged/ended the failed session (see `showUpdaterError` → `acknowledgement()`) and the + // >=1s backoff has elapsed, so `checkForUpdatesWhenReady` sees `canCheckForUpdates == true` and + // starts the check immediately instead of parking in the readiness-wait loop. guard !shouldPreserveRetryStateForNextCheck else { return } transientErrorRetryFailureCount = 0 if case .checking = model.state { From 2031c151b66cf8323dc299491ab4d2edc8d9af9a Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 03:17:58 -0700 Subject: [PATCH 13/24] Add failing test: retry-pill Cancel during readiness wait must idle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproduces the readiness-wait cancel regression (UpdateDriver.swift): after a transient download failure schedules a retry and the backoff fires, the pill stays in .checking; pressing Cancel then hits the shouldPreserveRetryStateForNextCheck guard and no-ops, so the pill is stuck instead of returning to idle. This commit adds the test only (CLAUDE.md two-commit regression policy) — it fails until the follow-up scopes the preserve guard to the synchronous internal restart. Co-Authored-By: Claude Opus 4.8 --- .../UpdateDriverRetryTests.swift | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift index 64c37fdcf2d0..9d85cd1e91a1 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift @@ -51,6 +51,40 @@ import Testing #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() From e624abd6f7b2c99f5a6ecce46d590f495393813f Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 03:23:29 -0700 Subject: [PATCH 14/24] Fix retry-pill Cancel no-op during Sparkle readiness wait Scope the transient-retry preserve-state suppression to the synchronous internal restart so a user cancel during the controller's readiness wait idles the pill instead of no-oping. - UpdateDriver: add isRestartingTransientRetry, set true only around the synchronous updaterRequestsRetryCheckForUpdates delegate call (the window where the controller may re-enter the cancel closure via cancelActiveStateForNewCheck). The cancel guard now keys off this flag instead of shouldPreserveRetryStateForNextCheck, which stays true across the async readiness wait; also clear the pending preserve on user cancel. - UpdateController: waitForReadinessThenCheck bails when the model has left .checking, so an idled pill isn't resurrected once readiness arrives and the orphaned readiness task stops promptly. Fixes the readiness-wait regression covered by userCancelWhileRetryPillAwaitsReadinessReturnsToIdle (commit 2031c151b6). Co-Authored-By: Claude Opus 4.8 --- .../CmuxUpdater/UpdateController.swift | 3 ++ .../Sources/CmuxUpdater/UpdateDriver.swift | 32 ++++++++++++------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index e3d67f54b016..8354004b9379 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -304,6 +304,9 @@ public final class UpdateController { guard let self else { return } var remaining = self.readyRetryCount while remaining > 0 { + // A user cancel on the retry/checking pill idles the model. Stop waiting rather than + // resurrecting the check when readiness eventually arrives (autoreview P2). + guard case .checking = self.model.state else { return } if self.updater.canCheckForUpdates { self.performCheckForUpdates(preservingInstallIntent: preservingInstallIntent) return diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift index a242ae305422..0330f20959dc 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift @@ -30,6 +30,11 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { 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, @@ -246,7 +251,14 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { 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 } @@ -263,18 +275,16 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { private func cancelPendingTransientErrorRetry() { transientErrorRetryTask?.cancel() transientErrorRetryTask = nil - // The guard preserves the escalating failure count across the *synchronous* internal restart - // teardown: on the plain-recheck path the controller calls `cancelActiveStateForNewCheck()`, - // which reruns this same closure via `state.cancel()` while `shouldPreserveRetryStateForNextCheck` - // is briefly set (from just before the delegate call until `beginChecking` consumes it). It does - // not swallow a user-initiated cancel: during the multi-second backoff sleep the flag is still - // false, so tapping Cancel then resets and idles here as usual. The flag's true-window never - // becomes a long uncancellable checking pill because at retry time Sparkle has already - // acknowledged/ended the failed session (see `showUpdaterError` → `acknowledgement()`) and the - // >=1s backoff has elapsed, so `checkForUpdatesWhenReady` sees `canCheckForUpdates == true` and - // starts the check immediately instead of parking in the readiness-wait loop. - guard !shouldPreserveRetryStateForNextCheck else { return } + // 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) } From 3a53e504a9123f37e3f2e5b19815bf48d2060c45 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 03:48:24 -0700 Subject: [PATCH 15/24] Refactor transient-retry decision into a TransientRetryPlan initializer Convert the pure `UpdateController.transientRetryPlan(preservingInstallIntent: coordinatorIsMonitoring:)` static factory into a `nonisolated init` on the nested `TransientRetryPlan` value type. Behavior-preserving: the decision logic (monitored -> restart, preserved-intent-before-monitoring -> re-arm, else -> plain check) is byte-identical and stays covered by the existing UpdateControllerTransientRetryPlanTests. Constructing the plan by value (`TransientRetryPlan(...)`) rather than calling a static method on the stateful `@MainActor UpdateController` removes the static-as-namespace shape flagged by package-design review, without adding any new type or cross-object reference. Co-Authored-By: Claude Opus 4.8 --- .../UpdateController+TransientRetry.swift | 33 ++++++++++--------- .../CmuxUpdater/UpdateController.swift | 4 +-- ...ateControllerTransientRetryPlanTests.swift | 10 +++--- 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift index b759a6228220..29f36a086a55 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift @@ -17,21 +17,24 @@ extension UpdateController { case rearmConfirmedInstall /// A plain transient retry with no install intent to preserve; run an ordinary fresh check. case plainCheck - } - /// Pure decision for ``retryAfterTransientFailure(preservingInstallIntent:)``; see - /// ``TransientRetryPlan`` for what each case means. - nonisolated static func transientRetryPlan( - preservingInstallIntent: Bool, - coordinatorIsMonitoring: Bool - ) -> TransientRetryPlan { - // 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). - if coordinatorIsMonitoring { return .restartMonitoredCheck } - if preservingInstallIntent { return .rearmConfirmedInstall } - return .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 8354004b9379..b3246da34b65 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -206,7 +206,7 @@ public final class UpdateController { /// 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(preservingInstallIntent:coordinatorIsMonitoring:)``: + /// 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. @@ -214,7 +214,7 @@ public final class UpdateController { /// - 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 = Self.transientRetryPlan( + let plan = TransientRetryPlan( preservingInstallIntent: preservingInstallIntent, coordinatorIsMonitoring: attemptCoordinator.isMonitoring ) diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift index eb463eaefe07..114125d9628b 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerTransientRetryPlanTests.swift @@ -10,7 +10,7 @@ import Testing /// Available" prompt (which strands the interrupted install). /// /// ``UpdateController`` owns a live `SPUUpdater`, so the retry *decision* is factored into the pure -/// ``UpdateController/transientRetryPlan(preservingInstallIntent:coordinatorIsMonitoring:)`` and +/// ``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`). @@ -18,7 +18,7 @@ import Testing /// 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( + let plan = UpdateController.TransientRetryPlan( preservingInstallIntent: true, coordinatorIsMonitoring: false ) @@ -29,11 +29,11 @@ import Testing /// driver's `preservingInstallIntent` flag. @Test func monitoredRetryRestartsMonitoredCheck() { #expect( - UpdateController.transientRetryPlan(preservingInstallIntent: false, coordinatorIsMonitoring: true) + UpdateController.TransientRetryPlan(preservingInstallIntent: false, coordinatorIsMonitoring: true) == .restartMonitoredCheck ) #expect( - UpdateController.transientRetryPlan(preservingInstallIntent: true, coordinatorIsMonitoring: true) + UpdateController.TransientRetryPlan(preservingInstallIntent: true, coordinatorIsMonitoring: true) == .restartMonitoredCheck ) } @@ -41,7 +41,7 @@ import Testing /// A plain transient retry (no install intent, not monitoring) runs an ordinary fresh check. @Test func plainRetryRunsPlainCheck() { #expect( - UpdateController.transientRetryPlan(preservingInstallIntent: false, coordinatorIsMonitoring: false) + UpdateController.TransientRetryPlan(preservingInstallIntent: false, coordinatorIsMonitoring: false) == .plainCheck ) } From 229a9cfda45b546b8786e02c7692ae7dfb574638 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 04:14:55 -0700 Subject: [PATCH 16/24] Add failing test: retry re-arm strands coordinator armed after cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The download-phase transient-retry re-arm (`.rearmConfirmedInstall`) currently arms the attempt coordinator via `requestInstallLatest`, which enters `awaitingCheckRestart`. That phase exists to ignore a stale on-screen prompt until a real check restarts, so a state change to `.idle` there is read as "check restarted" and advances to `awaitingResult` (still armed). But a transient-retry re-arm has no stale prompt: the model is the retry's own synthetic `.checking` pill. If the user cancels it during the controller's readiness wait, the model idles directly (no intervening `.checking`) and the coordinator is stranded armed — later silently auto-confirming an unrelated update the user never asked to install (autoreview P1). Introduce an `armForConfirmedRetryCheck()` seam for this re-arm and wire the controller to it (routing the check through the preserving path so the synthetic pill is not torn down and the bounded-retry count is preserved). This commit gives the seam the status-quo `awaitingCheckRestart` body so the new `retryRearmDisarmsWhenUserCancelsBeforeCheckRestarts` regression test fails, proving it catches the strand before the fix lands in the next commit. Co-Authored-By: Claude Opus 4.8 --- .../AttemptUpdateCoordinator.swift | 7 ++++ .../CmuxUpdater/UpdateController.swift | 13 +++++-- .../AttemptUpdateCoordinatorTests.swift | 36 +++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift index 1df6ae66597a..b9eb1e44cb06 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift @@ -62,6 +62,13 @@ struct AttemptUpdateCoordinator { } } + /// Arm the coordinator to auto-confirm the update a freshly *restarted* retry check resolves, + /// used by the download-phase transient-retry re-arm (issue #5632) instead of routing through + /// ``requestInstallLatest(currentState:)``. + mutating func armForConfirmedRetryCheck() { + phase = .awaitingCheckRestart + } + /// 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/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index b3246da34b65..53e4ab0461f7 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -225,9 +225,16 @@ public final class UpdateController { // (the coordinator being already monitoring) auto-confirms the update it finds. checkForUpdatesWhenReady(preservingInstallIntent: true) case .rearmConfirmedInstall: - // Arm the attempt coordinator on the retried check via the shared attempt path so the - // update it finds is auto-confirmed, matching the pre-failure install intent. - performAttemptAction(attemptCoordinator.requestInstallLatest(currentState: model.state)) + // 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) } diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift index a3e3ea8413d1..aad7c06579c4 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/AttemptUpdateCoordinatorTests.swift @@ -156,4 +156,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) + } } From 0472e3aa5d0dc377854e4e2c7c60bc39fc114377 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 04:15:19 -0700 Subject: [PATCH 17/24] Disarm retry re-arm coordinator when user cancels before check restarts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enter `awaitingResult` directly in `armForConfirmedRetryCheck()` instead of `awaitingCheckRestart`. The retry re-arm has no stale on-screen prompt to gate against (the model is the retry's own synthetic `.checking` pill), so the `awaitingCheckRestart` stage is unnecessary and actively harmful here: a user cancel during the readiness wait idles the model directly, and in `awaitingCheckRestart` that idle is misread as check-restart progress (→ `awaitingResult`, still armed), later silently auto-confirming an unrelated update. Arming straight into `awaitingResult` routes that idle through the `awaitingResult` idle → inactive path, disarming the coordinator (autoreview P1). `retryRearmDisarmsWhenUserCancelsBeforeCheckRestarts` now passes; the #6366 user-initiated install flow is untouched (still goes through `requestInstallLatest`/`awaitingCheckRestart`). Co-Authored-By: Claude Opus 4.8 --- .../CmuxUpdater/AttemptUpdateCoordinator.swift | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift index b9eb1e44cb06..6451bc5d0e53 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift @@ -63,10 +63,19 @@ struct AttemptUpdateCoordinator { } /// Arm the coordinator to auto-confirm the update a freshly *restarted* retry check resolves, - /// used by the download-phase transient-retry re-arm (issue #5632) instead of routing through - /// ``requestInstallLatest(currentState:)``. + /// 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 = .awaitingCheckRestart + phase = .awaitingResult } /// Feed each ``UpdateState`` change observed after a request. Returns the action the controller From 725575cf9bde6c40eae43919b7c27784a438873a Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 04:16:33 -0700 Subject: [PATCH 18/24] Preserve bounded-retry count on readiness-delayed plain retries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `performCheckForUpdates` already bypasses the non-idle teardown + `cancelActiveStateForNewCheck()` for preserved-intent retries and for the idle case, precisely because tearing down the driver's synthetic `.checking` backoff placeholder fires its `cancel` — which aborts the retry and resets the `[1, 3, 8]` bounded-retry failure count. But the guard keyed on `preservingInstallIntent`, so a *plain* transient retry (no install intent) whose model is that same `.checking` pill fell through to the teardown. On the readiness-delayed path this runs from `readyCheckTask`, after the synchronous `isRestartingTransientRetry` window has closed, so `cancelPendingTransientErrorRetry` resets the failure count to 0 every attempt — turning the bounded policy into an unbounded ~1s retry loop while a transient appcast/download error persists (autoreview P2). Extend the bypass to any synthetic `.checking` state. Every caller reaches `performCheckForUpdates` only after observing `canCheckForUpdates == true`, so no live Sparkle session exists and a `.checking` here is always the synthetic placeholder — safe to start the fresh check directly and keep the count. Not unit-tested: the repro requires toggling `SPUUpdater.canCheckForUpdates`, which `UpdateController` owns as a concrete, non-injectable updater; the fix mirrors the two adjacent (likewise unit-untested) bypasses with an explicit rationale and is covered at compile time by CI. Co-Authored-By: Claude Opus 4.8 --- .../CmuxUpdater/UpdateController.swift | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index 53e4ab0461f7..770de407954b 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -246,15 +246,23 @@ public final class UpdateController { // 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 preserved-intent retry 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 this path through it - // would also be wrong: the current non-idle state is the driver's synthetic `.checking` - // backoff placeholder whose `cancel` aborts the retry, so `cancelActiveStateForNewCheck()` - // would kill the very retry and flicker the pill to idle. Start the fresh check directly. + // 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 @@ -263,6 +271,10 @@ public final class UpdateController { updater.checkForUpdates() return } + if case .checking = model.state { + updater.checkForUpdates() + return + } model.cancelActiveStateForNewCheck() From 9ce713a02cf63dda7a1be1f780da65bac0a6b73c Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 04:31:29 -0700 Subject: [PATCH 19/24] Add failing test: readiness wait aborts install-from-prompt when Sparkle not ready MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the readiness-wait continuation guard into a pure static predicate (readinessWaitShouldContinue) so it can be unit-tested, preserving the current (too-broad) `.checking`-only behavior for now. Add UpdateControllerReadinessWaitTests asserting the wait must keep polling while the model is the `.updateAvailable` prompt that attemptUpdate() re-resolves (issue #6366). That case fails today: the guard bails on any non-`.checking` state, so an Install triggered while Sparkle is briefly not ready (canCheckForUpdates == false) exits the readiness wait before it can run the fresh re-resolution check — stranding the coordinator armed so the user's Install does nothing until some unrelated state change occurs (autoreview follow-up to issue #5632). Red half of the two-commit regression proof; the fix follows. Co-Authored-By: Claude Opus 4.8 --- .../CmuxUpdater/UpdateController.swift | 13 +++++-- .../UpdateControllerReadinessWaitTests.swift | 39 +++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index 770de407954b..5d19e95e9d7d 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -318,14 +318,21 @@ public final class UpdateController { waitForReadinessThenCheck(preservingInstallIntent: preservingInstallIntent) } + /// Whether the bounded readiness wait should keep polling for `canCheckForUpdates`, given the + /// model state observed at the top of each poll iteration. Stops the wait only when the model is + /// no longer the `.checking` pill (a user cancel idles it; see task #12). + static func readinessWaitShouldContinue(whileModelState state: UpdateState) -> Bool { + if case .checking = state { return true } + return false + } + private func waitForReadinessThenCheck(preservingInstallIntent: Bool = false) { readyCheckTask = Task { @MainActor [weak self] in guard let self else { return } var remaining = self.readyRetryCount while remaining > 0 { - // A user cancel on the retry/checking pill idles the model. Stop waiting rather than - // resurrecting the check when readiness eventually arrives (autoreview P2). - guard case .checking = self.model.state else { return } + // Stop the wait when the pending check was cancelled; keep polling otherwise. + guard UpdateController.readinessWaitShouldContinue(whileModelState: self.model.state) else { return } if self.updater.canCheckForUpdates { self.performCheckForUpdates(preservingInstallIntent: preservingInstallIntent) return diff --git a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift new file mode 100644 index 000000000000..fe0efa8c8beb --- /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 predicate, +/// ``UpdateController/readinessWaitShouldContinue(whileModelState:)``. +/// +/// 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.readinessWaitShouldContinue(whileModelState: updateAvailable)) + } + + /// 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.readinessWaitShouldContinue(whileModelState: .checking(.init(cancel: {})))) + } + + /// 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.readinessWaitShouldContinue(whileModelState: .idle)) + } +} From 377ee13bb6a7f403448e2f8b25529579e1d5d7d9 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 04:37:18 -0700 Subject: [PATCH 20/24] Keep readiness wait polling for any pending check; stop only on idle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transient-retry work guarded the shared readiness wait to stop unless the model was `.checking`. But `attemptUpdate()` (install from an on-screen prompt) enters the readiness wait while the model is still `.updateAvailable` — the #6366 re-resolve keeps the prompt visible, it does not switch to `.checking`. When Sparkle was briefly not ready (canCheckForUpdates == false), the guard bailed immediately, no fresh check ran, and the armed coordinator left the user's Install doing nothing until an unrelated state change. Scope the guard to the actual cancellation signal: stop only when the pending check has returned to `.idle` (the retry/checking pill's Cancel), and keep polling for every other pending state — `.checking` placeholders and the `.updateAvailable` install re-resolve alike. Turns UpdateControllerReadinessWaitTests green. Green half of the two-commit regression proof (red: prior commit). Co-Authored-By: Claude Opus 4.8 --- .../Sources/CmuxUpdater/UpdateController.swift | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index 5d19e95e9d7d..13551e8cb56c 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -319,11 +319,20 @@ public final class UpdateController { } /// Whether the bounded readiness wait should keep polling for `canCheckForUpdates`, given the - /// model state observed at the top of each poll iteration. Stops the wait only when the model is - /// no longer the `.checking` pill (a user cancel idles it; see task #12). + /// model state observed at the top of each poll iteration. + /// + /// 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 (e.g. Cancel on the retry/checking pill idles the model), so + /// readiness arriving later does not resurrect a dismissed check. Aborting on every non-`.checking` + /// state instead stranded `attemptUpdate()`'s install whenever Sparkle was briefly not ready, + /// leaving the model `.updateAvailable` — not `.checking` — so no fresh check ran (autoreview + /// follow-up to issue #5632). static func readinessWaitShouldContinue(whileModelState state: UpdateState) -> Bool { - if case .checking = state { return true } - return false + if case .idle = state { return false } + return true } private func waitForReadinessThenCheck(preservingInstallIntent: Bool = false) { From ba540c67b8c823432ad1bf42c098db6ba8bf2a62 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 04:39:49 -0700 Subject: [PATCH 21/24] Extract UpdateController pure decision helpers to satisfy file-length budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transient-retry and readiness fixes grew UpdateController.swift past the 500-line budget (workflow-guard-tests / scripts/swift_file_length_budget.py, which fails any untracked cmux-owned Swift file at or over the threshold). Per the swift-file-package-boundaries review rule, restore the budget by splitting a responsibility rather than expanding the TSV: move the two pure, self-free static decision helpers — readinessWaitShouldContinue (readiness-wait continuation) and isDevLikeBundleIdentifier (dev/staging bundle classification) — into a new UpdateController+Decisions.swift extension, mirroring UpdateController+TransientRetry. Both remain reachable at their unchanged UpdateController. call sites and their existing tests; UpdateController.swift returns to 494 lines. Co-Authored-By: Claude Opus 4.8 --- .../UpdateController+Decisions.swift | 38 +++++++++++++++++++ .../CmuxUpdater/UpdateController.swift | 37 ------------------ 2 files changed, 38 insertions(+), 37 deletions(-) create mode 100644 Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift new file mode 100644 index 000000000000..9f2a134f7b80 --- /dev/null +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift @@ -0,0 +1,38 @@ +import Foundation + +extension UpdateController { + /// Whether the bounded readiness wait should keep polling for `canCheckForUpdates`, given the + /// model state observed at the top of each poll iteration. + /// + /// 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 (e.g. Cancel on the retry/checking pill idles the model), so + /// readiness arriving later does not resurrect a dismissed check. Aborting on every non-`.checking` + /// state instead stranded `attemptUpdate()`'s install whenever Sparkle was briefly not ready, + /// leaving the model `.updateAvailable` — not `.checking` — so no fresh check ran (autoreview + /// follow-up to issue #5632). + static func readinessWaitShouldContinue(whileModelState state: UpdateState) -> Bool { + if case .idle = state { return false } + return true + } + + /// 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/UpdateController.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index 13551e8cb56c..e6bf99544a30 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift @@ -318,23 +318,6 @@ public final class UpdateController { waitForReadinessThenCheck(preservingInstallIntent: preservingInstallIntent) } - /// Whether the bounded readiness wait should keep polling for `canCheckForUpdates`, given the - /// model state observed at the top of each poll iteration. - /// - /// 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 (e.g. Cancel on the retry/checking pill idles the model), so - /// readiness arriving later does not resurrect a dismissed check. Aborting on every non-`.checking` - /// state instead stranded `attemptUpdate()`'s install whenever Sparkle was briefly not ready, - /// leaving the model `.updateAvailable` — not `.checking` — so no fresh check ran (autoreview - /// follow-up to issue #5632). - static func readinessWaitShouldContinue(whileModelState state: UpdateState) -> Bool { - if case .idle = state { return false } - return true - } - private func waitForReadinessThenCheck(preservingInstallIntent: Bool = false) { readyCheckTask = Task { @MainActor [weak self] in guard let self else { return } @@ -509,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.") - } -} From 1fd8b373f0474955b7057c8b56b9930f4c9cef37 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 06:13:56 -0700 Subject: [PATCH 22/24] Model UpdateController decisions as value types (Aziz package-design policy) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two pure decision helpers extracted into `UpdateController+Decisions.swift` were `static func`s called as `UpdateController.method(...)`, which the package design policy flags as a "static-as-namespace" anti-pattern (a caseless namespace on a type that holds no relevant stored state). Re-express both as value-type enums constructed via a `nonisolated init`, mirroring the sibling `TransientRetryPlan` that the same policy already accepts: - `ReadinessWaitDecision(modelState:)` → `.keepPolling` / `.stop` - `BundleReleaseChannel(bundleIdentifier:)` → `.devLike` / `.release` These stay unit-testable without the live `SPUUpdater` the controller owns (the reason they were extracted), while replacing the `Type.staticMethod()` call shape with instance construction. No behavior change; the readiness-wait continuation and dev/staging gating logic are identical. Existing unit tests are retargeted to the new types, and the two call sites plus doc links updated. UpdateController.swift stays within the 500-line file-length budget. Co-Authored-By: Claude Opus 4.8 --- .../UpdateController+Decisions.swift | 65 ++++++++++++------- .../CmuxUpdater/UpdateController.swift | 10 +-- .../Sources/CmuxUpdater/UpdateDriver.swift | 2 +- .../DevStagingUpdateGatingTests.swift | 16 ++--- .../UpdateControllerReadinessWaitTests.swift | 10 +-- 5 files changed, 60 insertions(+), 43 deletions(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift index 9f2a134f7b80..77e7a7c6097e 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift @@ -1,38 +1,55 @@ import Foundation extension UpdateController { - /// Whether the bounded readiness wait should keep polling for `canCheckForUpdates`, given the - /// model state observed at the top of each poll iteration. + /// 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 (e.g. Cancel on the retry/checking pill idles the model), so - /// readiness arriving later does not resurrect a dismissed check. Aborting on every non-`.checking` - /// state instead stranded `attemptUpdate()`'s install whenever Sparkle was briefly not ready, - /// leaving the model `.updateAvailable` — not `.checking` — so no fresh check ran (autoreview - /// follow-up to issue #5632). - static func readinessWaitShouldContinue(whileModelState state: UpdateState) -> Bool { - if case .idle = state { return false } - return true + /// 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 + } } - /// Whether `bundleIdentifier` is a cmux DEV (`com.cmuxterm.app.debug[.]`) or staging - /// (`com.cmuxterm.app.staging[.]`) build. + /// Which release channel a bundle identifier belongs to, decided from the identifier alone so it + /// stays unit-testable, mirroring ``TransientRetryPlan``. /// - /// 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). + /// 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. - 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.") + /// 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.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift index e6bf99544a30..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 { @@ -324,7 +324,7 @@ public final class UpdateController { var remaining = self.readyRetryCount while remaining > 0 { // Stop the wait when the pending check was cancelled; keep polling otherwise. - guard UpdateController.readinessWaitShouldContinue(whileModelState: self.model.state) else { return } + guard ReadinessWaitDecision(modelState: self.model.state) == .keepPolling else { return } if self.updater.canCheckForUpdates { self.performCheckForUpdates(preservingInstallIntent: preservingInstallIntent) return @@ -407,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 diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift index 0330f20959dc..b50907c0573d 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift @@ -17,7 +17,7 @@ final class UpdateDriver: NSObject, @preconcurrency SPUUserDriver { 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)? 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/UpdateControllerReadinessWaitTests.swift b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift index fe0efa8c8beb..4f40d55154df 100644 --- a/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift +++ b/Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateControllerReadinessWaitTests.swift @@ -3,8 +3,8 @@ import Testing @preconcurrency import Sparkle @testable import CmuxUpdater -/// Tests for the bounded readiness-wait continuation predicate, -/// ``UpdateController/readinessWaitShouldContinue(whileModelState:)``. +/// 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 @@ -22,18 +22,18 @@ import Testing /// 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.readinessWaitShouldContinue(whileModelState: updateAvailable)) + #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.readinessWaitShouldContinue(whileModelState: .checking(.init(cancel: {})))) + #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.readinessWaitShouldContinue(whileModelState: .idle)) + #expect(UpdateController.ReadinessWaitDecision(modelState: .idle) == .stop) } } From 372b5a4182d4b7c832636c59daef44261c7ed6b9 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Thu, 2 Jul 2026 06:21:11 -0700 Subject: [PATCH 23/24] Split UpdateController decision enums into one-type-per-file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior commit re-expressed the two pure decision helpers as value-type enums (ReadinessWaitDecision, BundleReleaseChannel) to satisfy Aziz's static-as-namespace policy, but placed both in a single UpdateController+Decisions.swift. Aziz's file-organization policy wants one major type per file (precedent: UpdateController+TransientRetry.swift holds only TransientRetryPlan). Split into UpdateController+ReadinessWaitDecision.swift and UpdateController+BundleReleaseChannel.swift, one nested enum each. Pure file reorganization — no type, signature, or behavior change; all call sites and tests already reference the types unqualified / via UpdateController. and are untouched. BundleReleaseChannel keeps `import Foundation` for `hasPrefix`; ReadinessWaitDecision needs no import (matches +TransientRetry.swift). Co-Authored-By: Claude Opus 4.8 --- ...pdateController+BundleReleaseChannel.swift | 29 +++++++++++++++++++ ...ateController+ReadinessWaitDecision.swift} | 28 ------------------ 2 files changed, 29 insertions(+), 28 deletions(-) create mode 100644 Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+BundleReleaseChannel.swift rename Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/{UpdateController+Decisions.swift => UpdateController+ReadinessWaitDecision.swift} (51%) 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+Decisions.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+ReadinessWaitDecision.swift similarity index 51% rename from Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift rename to Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+ReadinessWaitDecision.swift index 77e7a7c6097e..ca490774a151 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+ReadinessWaitDecision.swift @@ -1,5 +1,3 @@ -import Foundation - 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. @@ -26,30 +24,4 @@ extension UpdateController { self = state.isIdle ? .stop : .keepPolling } } - - /// 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 - } - } } From b7920ded29602e4a3431dc0a78d96c6f35f380df Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Fri, 3 Jul 2026 14:18:04 -0700 Subject: [PATCH 24/24] Clarify updater delegate main actor isolation --- .../CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift index 4e395c1f113d..7a3833f2b3f2 100644 --- a/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift +++ b/Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift @@ -3,7 +3,8 @@ /// 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