Skip to content

fix: add recovery for stuck Sparkle extraction updates #1862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lawrencecchen wants to merge 1 commit into from
Closed

fix: add recovery for stuck Sparkle extraction updates #1862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Sources/Update/UpdateController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ class UpdateController {
/// Start the updater. If startup fails, the error is shown via the custom UI.
func startUpdaterIfNeeded() {
guard !didStartUpdater else { return }
cleanStaleInstallationCache()
ensureSparkleInstallationCache()
#if DEBUG
// Keep the permission-related defaults resettable for UI tests even though the
Expand Down Expand Up @@ -329,6 +330,29 @@ class UpdateController {
#endif
}

private func cleanStaleInstallationCache() {
guard let bundleIdentifier = Bundle.main.bundleIdentifier else { return }
guard let cachesURL = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first else { return }

let installURL = cachesURL
.appendingPathComponent(bundleIdentifier)
.appendingPathComponent("org.sparkle-project.Sparkle")
.appendingPathComponent("Installation")

let fm = FileManager.default
guard let contents = try? fm.contentsOfDirectory(at: installURL, includingPropertiesForKeys: nil),
!contents.isEmpty else { return }

for item in contents {
do {
try fm.removeItem(at: item)
UpdateLogStore.shared.append("cleaned stale installation cache: \(item.lastPathComponent)")
} catch {
UpdateLogStore.shared.append("failed to clean installation cache item \(item.lastPathComponent): \(error)")
}
}
}
Comment on lines +333 to +354

@greptile-apps greptile-apps Bot Mar 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 All Installation cache items are deleted indiscriminately on every launch

cleanStaleInstallationCache() removes everything in the Sparkle Installation directory unconditionally, with no staleness check (e.g., modification date, file age). If Sparkle ever legitimately places something in that directory during a normal background update flow that involves a relaunch — for example an update staged for silent install on next launch — deleting it on startup would silently break that flow without any user-visible error.

Adding a minimum-age guard (e.g., only remove items older than 1 hour) would make this safer while still recovering from the 12-hour stuck extractions the PR targets:

let oneHour: TimeInterval = 3600
for item in contents {
    let attrs = try? fm.attributesOfItem(atPath: item.path)
    let modified = attrs?[.modificationDate] as? Date ?? .distantPast
    guard Date().timeIntervalSince(modified) > oneHour else { continue }
    // ... existing remove + log
}


private func ensureSparkleInstallationCache() {
guard let bundleIdentifier = Bundle.main.bundleIdentifier else { return }
guard let cachesURL = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first else { return }
Expand Down
70 changes: 66 additions & 4 deletions Sources/Update/UpdateDriver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ class UpdateDriver: NSObject, SPUUserDriver {
private var pendingCheckTransition: DispatchWorkItem?
private var checkTimeoutWorkItem: DispatchWorkItem?
private var lastFeedURLString: String?
private var extractionTimeoutWorkItem: DispatchWorkItem?
private var extractionStartDate: Date?

init(viewModel: UpdateViewModel, hostBundle _: Bundle) {
self.viewModel = viewModel
Expand Down Expand Up @@ -117,21 +119,23 @@ class UpdateDriver: NSObject, SPUUserDriver {

func showDownloadDidStartExtractingUpdate() {
UpdateLogStore.shared.append("show extraction started")
setState(.extracting(.init(progress: 0)))
beginExtraction(progress: 0)
}

func showExtractionReceivedProgress(_ progress: Double) {
UpdateLogStore.shared.append(String(format: "show extraction progress: %.2f", progress))
setState(.extracting(.init(progress: progress)))
beginExtraction(progress: progress)
}

func showReady(toInstallAndRelaunch reply: @escaping @Sendable (SPUUserUpdateChoice) -> Void) {
UpdateLogStore.shared.append("show ready to install")
let elapsed = extractionStartDate.map { String(format: "%.1fs", Date().timeIntervalSince($0)) } ?? "unknown"
UpdateLogStore.shared.append("show ready to install (extractionElapsed=\(elapsed))")
reply(.install)
}

func showInstallingUpdate(withApplicationTerminated applicationTerminated: Bool, retryTerminatingApplication: @escaping () -> Void) {
UpdateLogStore.shared.append("show installing update")
let elapsed = extractionStartDate.map { String(format: "%.1fs", Date().timeIntervalSince($0)) } ?? "unknown"
UpdateLogStore.shared.append("show installing update (appTerminated=\(applicationTerminated), extractionElapsed=\(elapsed))")
setState(.installing(.init(
retryTerminatingApplication: retryTerminatingApplication,
dismiss: { [weak viewModel] in
Expand Down Expand Up @@ -222,6 +226,9 @@ class UpdateDriver: NSObject, SPUUserDriver {
checkTimeoutWorkItem?.cancel()
checkTimeoutWorkItem = nil
lastCheckStart = nil
extractionTimeoutWorkItem?.cancel()
extractionTimeoutWorkItem = nil
extractionStartDate = nil
applyState(newState)
}
}
Expand All @@ -236,6 +243,61 @@ class UpdateDriver: NSObject, SPUUserDriver {
DispatchQueue.main.asyncAfter(deadline: .now() + UpdateTiming.checkTimeoutDuration, execute: workItem)
}

private func beginExtraction(progress: Double) {
runOnMain { [weak self] in
guard let self else { return }
pendingCheckTransition?.cancel()
pendingCheckTransition = nil
checkTimeoutWorkItem?.cancel()
checkTimeoutWorkItem = nil
lastCheckStart = nil
extractionTimeoutWorkItem?.cancel()
extractionTimeoutWorkItem = nil
if extractionStartDate == nil {
extractionStartDate = Date()
}
let cancel: () -> Void = { [weak self] in
self?.cancelExtraction()
}
applyState(.extracting(.init(progress: progress, cancel: cancel)))
scheduleExtractionTimeout()
}
}

private func scheduleExtractionTimeout() {
extractionTimeoutWorkItem?.cancel()
let workItem = DispatchWorkItem { [weak self] in
guard let self else { return }
guard case .extracting = self.viewModel.state else { return }
let elapsed = self.extractionStartDate.map { String(format: "%.0fs", Date().timeIntervalSince($0)) } ?? "unknown"
UpdateLogStore.shared.append("extraction timed out after \(elapsed)")
self.setState(.error(.init(

@cubic-dev-ai cubic-dev-ai Bot Mar 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The extraction-timeout path only switches to .error UI. Invalidate or ignore the current extraction session before showing retry, otherwise stale extraction callbacks can still drive install during the timeout/retry flow.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Update/UpdateDriver.swift, line 274:

<comment>The extraction-timeout path only switches to `.error` UI. Invalidate or ignore the current extraction session before showing retry, otherwise stale extraction callbacks can still drive install during the timeout/retry flow.</comment>

<file context>
@@ -236,6 +243,61 @@ class UpdateDriver: NSObject, SPUUserDriver {
+            guard case .extracting = self.viewModel.state else { return }
+            let elapsed = self.extractionStartDate.map { String(format: "%.0fs", Date().timeIntervalSince($0)) } ?? "unknown"
+            UpdateLogStore.shared.append("extraction timed out after \(elapsed)")
+            self.setState(.error(.init(
+                error: NSError(
+                    domain: "cmux.update",
</file context>
Fix with Cubic

error: NSError(
Comment on lines +274 to +275

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Cancel timed-out extraction before surfacing retry

When the 5-minute timeout fires, the code only swaps UI state to .error but does not abort or invalidate the in-flight Sparkle extraction job. In slow-but-eventually-successful extractions, the stale job can still call showReady and auto-install while the user is already seeing a timeout/retry flow (or has started a retry), causing conflicting update actions.

Useful? React with 👍 / 👎.

domain: "cmux.update",
code: 2,
userInfo: [NSLocalizedDescriptionKey: String(localized: "update.error.extractionStalled", defaultValue: "The update appears to be stuck. Try checking for updates again.")]

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add localization entries for extraction-stalled error

This introduces a new user-visible localization key (update.error.extractionStalled) but there is no corresponding entry in Resources/Localizable.xcstrings (repo search only finds this call site). That makes this error message fall back to English in non-English locales, violating the repo’s requirement that all UI strings be fully localized.

Useful? React with 👍 / 👎.

),
retry: { [weak viewModel = self.viewModel] in
viewModel?.state = .idle
DispatchQueue.main.async {
guard let delegate = NSApp.delegate as? AppDelegate else { return }
delegate.checkForUpdates(nil)
}
},
dismiss: { [weak viewModel = self.viewModel] in
viewModel?.state = .idle
}
)))
}
extractionTimeoutWorkItem = workItem
DispatchQueue.main.asyncAfter(deadline: .now() + UpdateTiming.extractionTimeoutDuration, execute: workItem)
Comment on lines +267 to +293

@greptile-apps greptile-apps Bot Mar 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Timeout resets on every progress callback, making it an idle timeout, not an absolute one

scheduleExtractionTimeout() is called inside beginExtraction(progress:), which is invoked on every showExtractionReceivedProgress callback. Each call cancels the previous work item and posts a fresh 300-second timer. This means the timeout fires only if no progress callbacks arrive for 5 minutes — not 5 minutes from the start of extraction.

For the exact failure mode described in the PR (XPC drops → progress freezes entirely), this works fine. However, a very slow extraction that emits one callback per 4m59s would never time out, which diverges from the PR description's implied "5-minute extraction timeout."

Consider posting the timeout once, keyed off extractionStartDate, and not rescheduling it on subsequent progress callbacks:

private func beginExtraction(progress: Double) {
    runOnMain { [weak self] in
        guard let self else { return }
        // ... existing cancellations ...
        let isFirstCall = extractionStartDate == nil
        if isFirstCall {
            extractionStartDate = Date()
        }
        let cancel: () -> Void = { [weak self] in self?.cancelExtraction() }
        applyState(.extracting(.init(progress: progress, cancel: cancel)))
        if isFirstCall {
            scheduleExtractionTimeout()
        }
    }
}

}

private func cancelExtraction() {
UpdateLogStore.shared.append("extraction cancelled by user")
setState(.idle)
Comment on lines +296 to +298

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Cancel Sparkle extraction when user taps Cancel

cancelExtraction() only sets the view model back to .idle and never cancels the underlying Sparkle update session, so the extraction can continue in the background. If that session later reaches showReady, this driver still auto-replies .install, which can relaunch unexpectedly after the user explicitly pressed Cancel.

Useful? React with 👍 / 👎.

@cubic-dev-ai cubic-dev-ai Bot Mar 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The new extraction cancel action is UI-only (setState(.idle)) and does not abort the underlying Sparkle update, so the update may still reach showReady and auto-install after the user clicks cancel.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Update/UpdateDriver.swift, line 298:

<comment>The new extraction cancel action is UI-only (`setState(.idle)`) and does not abort the underlying Sparkle update, so the update may still reach `showReady` and auto-install after the user clicks cancel.</comment>

<file context>
@@ -236,6 +243,61 @@ class UpdateDriver: NSObject, SPUUserDriver {
+
+    private func cancelExtraction() {
+        UpdateLogStore.shared.append("extraction cancelled by user")
+        setState(.idle)
+    }
+
</file context>
Fix with Cubic

}
Comment on lines +296 to +299

@greptile-apps greptile-apps Bot Mar 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Cancel button doesn't actually stop the Sparkle extraction

cancelExtraction() sets the UI state to .idle, but the underlying Sparkle extraction continues running in the background. Because showReady(toInstallAndRelaunch:) unconditionally calls reply(.install) (line 133), Sparkle will still trigger the install and then call showInstallingUpdate, pushing the state back to .installing — surprising the user who believed they cancelled.

Unlike the download phase (where Sparkle provides a cancellation callback in showDownloadInitiated), the extraction phase has no Sparkle-side cancellation mechanism. To make the cancel genuinely effective, showReady needs to check whether the extraction was cancelled before replying .install:

private var extractionCancelled = false

private func cancelExtraction() {
    UpdateLogStore.shared.append("extraction cancelled by user")
    extractionCancelled = true
    setState(.idle)
}

func showReady(toInstallAndRelaunch reply: @escaping @Sendable (SPUUserUpdateChoice) -> Void) {
    let elapsed = extractionStartDate.map { String(format: "%.1fs", Date().timeIntervalSince($0)) } ?? "unknown"
    UpdateLogStore.shared.append("show ready to install (extractionElapsed=\(elapsed))")
    if extractionCancelled {
        reply(.dismiss)
    } else {
        reply(.install)
    }
}

extractionCancelled should be reset to false at the start of a new extraction (beginExtraction) and inside setState (alongside the existing extractionStartDate = nil reset).


private func applyState(_ newState: UpdateState) {
viewModel.state = newState
UpdateLogStore.shared.append("state -> \(describe(newState))")
Expand Down
31 changes: 22 additions & 9 deletions Sources/Update/UpdatePopoverView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ struct UpdatePopoverView: View {
DownloadingView(download: download, dismiss: dismiss)

case .extracting(let extracting):
ExtractingView(extracting: extracting)
ExtractingView(extracting: extracting, dismiss: dismiss)

case .installing(let installing):
InstallingView(installing: installing, dismiss: dismiss)
Expand Down Expand Up @@ -247,17 +247,30 @@ fileprivate struct DownloadingView: View {

fileprivate struct ExtractingView: View {
let extracting: UpdateState.Extracting
let dismiss: DismissAction

var body: some View {
VStack(alignment: .leading, spacing: 8) {
Text(String(localized: "update.popover.preparingUpdate", defaultValue: "Preparing Update"))
.font(.system(size: 13, weight: .semibold))
VStack(alignment: .leading, spacing: 16) {
VStack(alignment: .leading, spacing: 8) {
Text(String(localized: "update.popover.preparingUpdate", defaultValue: "Preparing Update"))
.font(.system(size: 13, weight: .semibold))

VStack(alignment: .leading, spacing: 6) {
ProgressView(value: min(1, max(0, extracting.progress)), total: 1.0)
Text(String(format: "%.0f%%", min(1, max(0, extracting.progress)) * 100))
.font(.system(size: 11))
.foregroundColor(.secondary)
VStack(alignment: .leading, spacing: 6) {
ProgressView(value: min(1, max(0, extracting.progress)), total: 1.0)
Text(String(format: "%.0f%%", min(1, max(0, extracting.progress)) * 100))
.font(.system(size: 11))
.foregroundColor(.secondary)
}
}

HStack {
Spacer()
Button(String(localized: "common.cancel", defaultValue: "Cancel")) {
extracting.cancel()
dismiss()
}
.keyboardShortcut(.cancelAction)
.controlSize(.small)
}
}
.padding(16)
Expand Down
1 change: 1 addition & 0 deletions Sources/Update/UpdateTiming.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ enum UpdateTiming {
static let minimumCheckDisplayDuration: TimeInterval = 2.0
static let noUpdateDisplayDuration: TimeInterval = 5.0
static let checkTimeoutDuration: TimeInterval = 10.0
static let extractionTimeoutDuration: TimeInterval = 300.0
}
3 changes: 3 additions & 0 deletions Sources/Update/UpdateViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,8 @@ enum UpdateState: Equatable {
available.reply(.dismiss)
case .downloading(let downloading):
downloading.cancel()
case .extracting(let extracting):
extracting.cancel()
case .notFound(let notFound):
notFound.acknowledgement()
case .error(let err):
Expand Down Expand Up @@ -568,6 +570,7 @@ enum UpdateState: Equatable {

struct Extracting {
let progress: Double
let cancel: () -> Void
}

struct Installing {
Expand Down
Loading