Repair Sparkle updater preflight#6004
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds SparkleUpdatePreflight to remove com.apple.quarantine attributes from bundled Sparkle.framework and to prepare/remove Sparkle’s installation cache; integrates the preflight into UpdateController and adds filesystem tests and test helpers. ChangesSparkle Preflight and Integration
Sequence DiagramsequenceDiagram
participant UpdateController
participant SparkleUpdatePreflight
participant FileManager
participant Syscalls
UpdateController->>SparkleUpdatePreflight: run()
SparkleUpdatePreflight->>FileManager: find Sparkle.framework in bundle
SparkleUpdatePreflight->>FileManager: enumerator(at:) -> framework items
loop per item
SparkleUpdatePreflight->>Syscalls: removexattr(com.apple.quarantine)
end
SparkleUpdatePreflight->>FileManager: derive cache path, inspect directory
SparkleUpdatePreflight->>FileManager: removeItem(at:) if invalid or empty
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (17 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR replaces the old
Confidence Score: 4/5The preflight logic is correct and regression tests cover the intended scenarios, but the full Sparkle.framework enumeration (50–200 removexattr syscalls) runs synchronously on every update check from the @MainActor-isolated UpdateController. The new SparkleUpdatePreflight struct introduces a per-check filesystem enumeration loop over Sparkle.framework that blocks the calling thread. All three call sites in UpdateController are @MainActor-isolated, so this loop runs on the main actor before every update check. On the first call to performCheckForUpdates or checkForUpdatesWhenReady, the loop also runs twice — once inside startUpdaterIfNeeded and once at the explicit call site — doubling the blocking work. SparkleUpdatePreflight.swift and the three call sites in UpdateController.swift warrant a second look to ensure the framework enumeration is dispatched off the main actor before this merges. Important Files Changed
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func runSparkleUpdatePreflight() { | ||
| SparkleUpdatePreflight( | ||
| hostBundle: hostBundle, | ||
| fileManager: fileManager, | ||
| log: log | ||
| ).run() | ||
| } |
There was a problem hiding this comment.
@MainActor
UpdateController is @MainActor-isolated, so runSparkleUpdatePreflight() runs synchronously on the main thread. On a fresh quarantined install, SparkleUpdatePreflight.run() enumerates every file in Sparkle.framework (typically 50–200 items) and calls removexattr for each — all blocking syscalls on the main actor — before returning. This violates the cmux rule against expensive synchronous disk/syscall loads on the main actor or interactive paths. The call originates from user-triggered paths (startUpdaterIfNeeded, checkForUpdatesWhenReady, performCheckForUpdates), where a stall would be visible.
The fix is to dispatch the preflight off the main actor, e.g. via Task.detached(priority: .utility), or by making SparkleUpdatePreflight an async function that hops to a non-isolated executor before doing the syscall loop.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let result = removexattr(url.path, "com.apple.quarantine", 0) | ||
| if result == 0 { | ||
| return true | ||
| } | ||
| if errno != ENOATTR { | ||
| log.append("Failed removing Sparkle quarantine attribute at \(url.path): errno \(errno)") | ||
| } |
There was a problem hiding this comment.
url.path bridges to a C const char * via Swift's UTF-8 string encoding. For POSIX/Darwin syscalls that accept a filesystem path, the canonical spelling is (url as NSURL).fileSystemRepresentation, which uses the OS-level file-system representation and handles edge cases (non-ASCII bundle paths in unusual install locations) more robustly than the Swift String bridge.
| let result = removexattr(url.path, "com.apple.quarantine", 0) | |
| if result == 0 { | |
| return true | |
| } | |
| if errno != ENOATTR { | |
| log.append("Failed removing Sparkle quarantine attribute at \(url.path): errno \(errno)") | |
| } | |
| let result = (url as NSURL).withUnsafeFileSystemRepresentation { path -> Int32 in | |
| guard let path else { return -1 } | |
| return removexattr(path, "com.apple.quarantine", 0) | |
| } | |
| if result == 0 { | |
| return true | |
| } | |
| if errno != ENOATTR { | |
| log.append("Failed removing Sparkle quarantine attribute at \(url.path): errno \(errno)") | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| #expect(!FileManager.default.fileExists(atPath: installationURL.path)) | ||
| } | ||
| } | ||
|
|
||
| private final class CapturingUpdateLog: UpdateLogging, @unchecked Sendable { | ||
| private let lock = NSLock() | ||
| private(set) var messages: [String] = [] | ||
|
|
||
| func append(_ message: String) { | ||
| lock.withLock { | ||
| messages.append(message) | ||
| } |
There was a problem hiding this comment.
sparkleCacheURL writes into the user's real ~/Library/Caches directory. The Installation directory is only removed if the preflight under test runs successfully. If the #expect or any prior assertion fails before the preflight call, the directory leaks. Similarly, rootURL in the tmp directory is never deleted by a teardown block or defer. Adding addTeardownBlock (or a defer in PreflightFixture.init) that removes rootURL and sparkleCacheURL would make the fixture self-cleaning on any exit path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift`:
- Around line 221-223: Both checkForUpdatesWhenReady() and
performCheckForUpdates() call runSparkleUpdatePreflight(), causing the preflight
to run twice; remove the duplicate by keeping a single invocation per flow —
e.g., remove the runSparkleUpdatePreflight() call from performCheckForUpdates()
and ensure checkForUpdatesWhenReady() performs the synchronous preflight once
before delegating to performCheckForUpdates(); apply the same deduplication for
the alternate path around the code near the other occurrence (the block around
lines 248-256) so runSparkleUpdatePreflight() is only called once per check
flow.
In
`@Packages/CmuxUpdater/Tests/CmuxUpdaterTests/SparkleUpdatePreflightTests.swift`:
- Around line 29-42: Add a test covering the branch where the "Installation"
path exists as a regular file rather than a directory: in
SparkleUpdatePreflightTests create a file at
fixture.sparkleCacheURL.appendingPathComponent("Installation", isDirectory:
true) (use FileManager.default.createFile or write Data) instead of creating a
directory, call SparkleUpdatePreflight(hostBundle: fixture.bundle, fileManager:
.default, log: CapturingUpdateLog()).run(), and assert the file is removed
afterwards (use FileManager.default.fileExists(atPath:) or similar) to validate
SparkleUpdatePreflight.run() handles the non-directory Installation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e590ad79-8004-46f4-b227-8a1372430710
📒 Files selected for processing (3)
Packages/CmuxUpdater/Sources/CmuxUpdater/SparkleUpdatePreflight.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swiftPackages/CmuxUpdater/Tests/CmuxUpdaterTests/SparkleUpdatePreflightTests.swift
| private func performCheckForUpdates() { | ||
| startUpdaterIfNeeded() | ||
| ensureSparkleInstallationCache() | ||
| runSparkleUpdatePreflight() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Avoid duplicate preflight execution in a single update-check flow.
checkForUpdatesWhenReady() and performCheckForUpdates() both call runSparkleUpdatePreflight(), so the canCheck == true path performs the same synchronous filesystem preflight twice for one action. Keep a single invocation per check flow.
Suggested minimal fix
private func checkForUpdatesWhenReady() {
cancelReadinessRetry()
startUpdaterIfNeeded()
- runSparkleUpdatePreflight()
let canCheck = updater.canCheckForUpdates
log.append("checkForUpdatesWhenReady invoked (canCheck=\(canCheck))")
if canCheck {
performCheckForUpdates()
returnAlso applies to: 248-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift` around lines
221 - 223, Both checkForUpdatesWhenReady() and performCheckForUpdates() call
runSparkleUpdatePreflight(), causing the preflight to run twice; remove the
duplicate by keeping a single invocation per flow — e.g., remove the
runSparkleUpdatePreflight() call from performCheckForUpdates() and ensure
checkForUpdatesWhenReady() performs the synchronous preflight once before
delegating to performCheckForUpdates(); apply the same deduplication for the
alternate path around the code near the other occurrence (the block around lines
248-256) so runSparkleUpdatePreflight() is only called once per check flow.
| @Test func removesEmptyInstallationDirectoryBeforeSparkleCreatesInstallSession() throws { | ||
| let fixture = try PreflightFixture() | ||
| let installationURL = fixture.sparkleCacheURL | ||
| .appendingPathComponent("Installation", isDirectory: true) | ||
| try FileManager.default.createDirectory(at: installationURL, withIntermediateDirectories: true) | ||
|
|
||
| SparkleUpdatePreflight( | ||
| hostBundle: fixture.bundle, | ||
| fileManager: .default, | ||
| log: CapturingUpdateLog() | ||
| ).run() | ||
|
|
||
| #expect(!FileManager.default.fileExists(atPath: installationURL.path)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the stale-file Installation path branch.
The suite validates empty-directory cleanup, but it doesn’t cover the Installation path existing as a regular file (!isDirectory branch). Add one test that creates a file at that path and asserts preflight removes it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/CmuxUpdater/Tests/CmuxUpdaterTests/SparkleUpdatePreflightTests.swift`
around lines 29 - 42, Add a test covering the branch where the "Installation"
path exists as a regular file rather than a directory: in
SparkleUpdatePreflightTests create a file at
fixture.sparkleCacheURL.appendingPathComponent("Installation", isDirectory:
true) (use FileManager.default.createFile or write Data) instead of creating a
directory, call SparkleUpdatePreflight(hostBundle: fixture.bundle, fileManager:
.default, log: CapturingUpdateLog()).run(), and assert the file is removed
afterwards (use FileManager.default.fileExists(atPath:) or similar) to validate
SparkleUpdatePreflight.run() handles the non-directory Installation path.
1 participant
Summary
Testing
Closes #5123
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches filesystem xattrs and Sparkle cache paths on the real user cache before every update check; behavior change is intentional but could affect edge cases around in-progress installs if an empty Installation dir is removed at the wrong time (mitigated by only deleting when empty).
Overview
Replaces the old Sparkle installation-cache workaround with a dedicated
SparkleUpdatePreflightstep that runs before updater startup and update checks.Quarantine cleanup: Before Sparkle runs, the preflight walks bundled
Sparkle.frameworkand stripscom.apple.quarantineextended attributes so Autoupdate and other helpers are not blocked by stale download quarantine.Installation cache policy flip: The controller no longer pre-creates Sparkle’s
Installationcache directory under~/Library/Caches/.../org.sparkle-project.Sparkle/. Instead it removes a stale path when it is a file or an empty directory, leaving Sparkle/Autoupdate to create the directory per install session.Regression tests cover quarantine removal and empty installation-directory cleanup, with small test fixtures/helpers for bundles and xattrs.
Reviewed by Cursor Bugbot for commit a4f276a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Repairs the Sparkle updater preflight so Autoupdate starts reliably. Removes stale quarantine flags and cleans invalid Installation cache paths so updates aren’t blocked or flaky.
Bug Fixes
Refactors
Written for commit a4f276a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests