Fix crash in clearDownloadedUpdate when no downloaded update is tracked#2863
Conversation
Replace assert(downloadedUpdateObject != nil) with an early return and SULog warning. The assertion causes a SIGABRT in production when clearDownloadedUpdate is called but both _resumableUpdate and _downloadedUpdateForRemoval are nil. Multiple code paths can trigger this: delta update application failure (installerDidFailToApplyDeltaUpdate), extraction errors, and the UI driver's skip-update path. The code after the assertion already handled nil gracefully via an if-check, so the assert was the sole cause of the crash. Observed in production: 519 occurrences across 279 users since Sparkle 2.9.0.
|
I don't think this code has changed recently (probably has worked this way since 2021 maybe with no other reports?). This doesn't explain why the assert happens, nor fixes the core issue if there is a problem. The call sites you mention should only call the method when there is a downloaded/resumable method. The nil check was probably added in conjunction with the isKindOfClass check, separate from the assert. Do you have a repro case? |
Cannot reproduce this. Modify/add a file in the original app bundle to force delta failure.
"Non-auth" is not a correct description. Cannot reproduce this. Require auth by doing
Cannot reproduce this. Turn automatic downloading on,
On side note I don't understand how you got this old PR template. |
…er messages sendInstallationData kicks off the installer process, but completionHandler was deferred until the probe callback fired. Installer messages like SPUExtractionStarted could arrive on the main queue in the gap, triggering installerDidStartExtracting -> clearDownloadedUpdate before the caller set _downloadedUpdateForRemoval. A subsequent clearDownloadedUpdate (e.g. from installerDidFailToApplyDeltaUpdate) then found both ivars nil and hit the assertion. The probe result is only logged, so completing before the probe is safe.
|
Thanks for testing those paths. I dug deeper and found the actual root cause -- it's a timing issue, not a logic issue in the callers. The race is in Here's the sequence that crashes for delta updates:
The probe result is only used for logging ( I've pushed a second commit (688d278) with this fix alongside the defensive early-return in This explains why you couldn't reproduce: the crash only happens when installer messages arrive on the main queue before the probe completes. The probe typically finishes fast on a local dev machine, so the window is narrow. In production across hundreds of machines with varying XPC/status service latencies, it hits often enough (519 times across 279 users). |
|
Thanks that makes sense. That probe message was added recently to try to speed up launching that agent process. It is still quite worrying the probe was slower than after a delta update was downloaded and failed to extract.. but that's a separate issue. |
|
Secondary separate concern is the rate you may be seeing delta update failures, if you're using generate_appcast and the optional |
|
Thanks for the quick merge. On the delta failure rate -- we actually only started shipping deltas with our most recent release (2.15.3, about 10 days ago). The crash has been showing up since December 2025, so the bulk of the volume (370+ out of 519 events) was on versions that never had delta entries in the appcast. The probe race explains the delta crashes cleanly, but there's likely another path hitting clearDownloadedUpdate with both ivars nil that we haven't pinned down yet. The defensive early-return in SPUCoreBasedUpdateDriver covers it either way, but figured it's worth flagging in case it rings a bell. We've set --maximum-deltas 0 on our end for now. Will look into the deltaFromSparkleExecutableSize / deltaFromSparkleLocales attributes before turning them back on -- good call on that. |
This would be before 2.9.0 was released which added the probe check, but we've not had any reports about this before then. That is not to rule out there could be an issue somewhere though.
Actually in this case the update was already downloaded, so this may be within expectations. |
clearDownloadedUpdateasserts that either_resumableUpdateor_downloadedUpdateForRemovalis non-nil, but several code paths can reach it when both are nil. The code after the assert already handles nil correctly (theifcheck on line 203), so the assert is the sole cause of the crash.We're seeing this in production as a SIGABRT — 519 occurrences across 279 users on Sparkle 2.9.0, macOS 14 through 26. The call paths that trigger it:
installerDidFailToApplyDeltaUpdate(delta application failure)extractUpdate:completion handler (extraction error, non-auth)SPUUIBasedUpdateDriver(user skips a downloaded update)The fix replaces the assert with an early return +
SULogwarning, so the condition is still logged for diagnostics without taking down the app.Checklist
Type of change
Testing