Repository navigation
fix: add recovery for stuck Sparkle extraction updates - #1862
lawrencecchen wants to merge 1 commit into
Conversation
The Sparkle updater can get permanently stuck at "Preparing: XX%" if the XPC connection to the installer helper drops or macOS Gatekeeper stalls during validation. Previously there was no timeout, no cancel button, and no recovery path for this state. - Add 5-minute extraction timeout that transitions to error with retry - Add cancel button to the preparing/extracting popover view - Clean stale Sparkle installation cache on app launch - Log extraction elapsed time and applicationTerminated flag
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe update system now implements extraction lifecycle management, including cache cleanup, timeout scheduling with automatic error handling, and user-initiated cancellation. A new cleanup method removes stale Sparkle cache, while extraction state transitions are tracked with timing and cancellation support. Changes
Sequence DiagramsequenceDiagram
participant User
participant UpdateDriver
participant UpdateLogStore
participant Timer
User->>UpdateDriver: showDownloadDidStartExtractingUpdate()
UpdateDriver->>UpdateDriver: beginExtraction(progress)
UpdateDriver->>UpdateDriver: recordExtractionStartDate
UpdateDriver->>UpdateDriver: setState(.extracting(cancel))
UpdateDriver->>Timer: scheduleExtractionTimeout()
Note over Timer: Wait 300 seconds
alt Extraction completes before timeout
UpdateDriver->>UpdateDriver: setState(.readyToInstall)
UpdateDriver->>UpdateLogStore: Log extractionElapsed
Timer->>Timer: Cancel timeout
else Timeout reached
Timer->>UpdateDriver: scheduleExtractionTimeout()<br/>still in .extracting
UpdateDriver->>UpdateLogStore: Log timeout error
UpdateDriver->>UpdateDriver: setState(.error)
else User cancels
User->>UpdateDriver: cancelExtraction()
UpdateDriver->>UpdateLogStore: Log cancellation
UpdateDriver->>UpdateDriver: setState(.idle)
Timer->>Timer: Cancel timeout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b11e265b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private func cancelExtraction() { | ||
| UpdateLogStore.shared.append("extraction cancelled by user") | ||
| setState(.idle) |
There was a problem hiding this comment.
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 👍 / 👎.
| self.setState(.error(.init( | ||
| error: NSError( |
There was a problem hiding this comment.
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 👍 / 👎.
| error: NSError( | ||
| domain: "cmux.update", | ||
| code: 2, | ||
| userInfo: [NSLocalizedDescriptionKey: String(localized: "update.error.extractionStalled", defaultValue: "The update appears to be stuck. Try checking for updates again.")] |
There was a problem hiding this comment.
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 👍 / 👎.
Greptile SummaryThis PR adds recovery mechanisms for a real dogfooding bug where Sparkle's extraction phase gets permanently stuck: a 5-minute extraction timeout with a retry-able error state, a Cancel button in the extraction popover, and on-launch cleanup of stale installation cache. The overall approach is well-structured and follows existing patterns in the codebase, but there is one significant behavioral bug with the new Cancel button. Key issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant S as Sparkle
participant D as UpdateDriver
participant VM as UpdateViewModel
participant UI as ExtractingView
S->>D: showDownloadDidStartExtractingUpdate()
D->>D: beginExtraction(progress: 0)
D->>VM: state = .extracting(cancel:)
D->>D: scheduleExtractionTimeout() [T+300s]
VM->>UI: render ExtractingView + Cancel button
alt Normal completion
S->>D: showExtractionReceivedProgress(0.5)
D->>D: beginExtraction(progress: 0.5) [reschedules timeout]
S->>D: showReady(toInstallAndRelaunch:)
D->>D: cancelExtractionTimeout
D->>S: reply(.install)
S->>D: showInstallingUpdate(...)
D->>VM: state = .installing
else Timeout fires (stuck)
Note over D: T+300s with no completion
D->>D: extractionTimeoutWorkItem fires
D->>VM: state = .error(retry/dismiss)
UI-->>UI: show error + Retry button
else User cancels
UI->>D: extracting.cancel()
D->>VM: state = .idle ⚠️ UI only
Note over S,D: Sparkle extraction continues!
S->>D: showReady(toInstallAndRelaunch:)
D->>S: reply(.install) ⚠️ unexpected install
S->>D: showInstallingUpdate(...)
D->>VM: state = .installing ⚠️
end
Last reviewed commit: "fix: add recovery fo..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func cancelExtraction() { | ||
| UpdateLogStore.shared.append("extraction cancelled by user") | ||
| setState(.idle) | ||
| } |
There was a problem hiding this comment.
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 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( | ||
| error: NSError( | ||
| domain: "cmux.update", | ||
| code: 2, | ||
| userInfo: [NSLocalizedDescriptionKey: String(localized: "update.error.extractionStalled", defaultValue: "The update appears to be stuck. Try checking for updates again.")] | ||
| ), | ||
| 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) |
There was a problem hiding this comment.
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 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)") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Update/UpdateController.swift (1)
333-354: Consider moving file I/O off the main thread.This method performs synchronous file enumeration and deletion on the main thread during app startup. While typically fast, a slow disk or many stale files could briefly delay launch. Consider dispatching to a background queue.
That said, if profiling shows negligible impact, this is acceptable as-is given the simplicity and the fact that
ensureSparkleInstallationCache()follows synchronously anyway.♻️ Optional: Async cleanup
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") + DispatchQueue.global(qos: .utility).async { 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)") } } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Update/UpdateController.swift` around lines 333 - 354, cleanStaleInstallationCache performs synchronous file I/O on the main thread; move the heavy work to a background queue by dispatching the directory enumeration and removeItem calls (the body of cleanStaleInstallationCache) onto a background DispatchQueue (e.g., global(qos:.utility) or a dedicated serial queue) and only marshal back to the main thread (or a thread-safe logging queue) when calling UpdateLogStore.shared.append to avoid race/UI issues; keep the same guards and error handling but wrap them in the dispatched block so callers of cleanStaleInstallationCache remain non-blocking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Update/UpdateController.swift`:
- Around line 333-354: cleanStaleInstallationCache performs synchronous file I/O
on the main thread; move the heavy work to a background queue by dispatching the
directory enumeration and removeItem calls (the body of
cleanStaleInstallationCache) onto a background DispatchQueue (e.g.,
global(qos:.utility) or a dedicated serial queue) and only marshal back to the
main thread (or a thread-safe logging queue) when calling
UpdateLogStore.shared.append to avoid race/UI issues; keep the same guards and
error handling but wrap them in the dispatched block so callers of
cleanStaleInstallationCache remain non-blocking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e155efb9-cdae-4f4d-a2dd-978a824ec917
📒 Files selected for processing (5)
Sources/Update/UpdateController.swiftSources/Update/UpdateDriver.swiftSources/Update/UpdatePopoverView.swiftSources/Update/UpdateTiming.swiftSources/Update/UpdateViewModel.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Update/UpdateDriver.swift">
<violation number="1" location="Sources/Update/UpdateDriver.swift:274">
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.</violation>
<violation number="2" location="Sources/Update/UpdateDriver.swift:298">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| private func cancelExtraction() { | ||
| UpdateLogStore.shared.append("extraction cancelled by user") | ||
| setState(.idle) |
There was a problem hiding this comment.
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>
| 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( |
There was a problem hiding this comment.
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>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
The Sparkle updater can get permanently stuck at "Preparing: XX%" if the XPC connection to the installer helper drops or macOS Gatekeeper stalls during validation. Previously there was no timeout, no cancel button, and no recovery path for this state.
showReadyandshowInstallingUpdate, and logapplicationTerminatedflagTesting
xcodebuild -scheme cmux -configuration DebugpassesRelated
Summary by cubic
Prevents
Sparkleupdates from stalling at “Preparing: XX%” by adding a 5‑minute extraction timeout, a cancel action, and startup cache cleanup. This gives users a clear recovery path and avoids failures persisting across restarts.SparkleInstallation cache on app launch.Written for commit 3b11e26. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes