Fix cross-volume update failure that deletes the app without replacing it#294
Fix cross-volume update failure that deletes the app without replacing it#294augiekim wants to merge 1 commit into
Conversation
|
I know this project hasn’t been actively maintained for some time, but it’s still used by Electron for updates, and this bug is causing real pain for Mac users who install Electron apps on external volumes. Recently, more Mac users—especially Mac Mini users—have been relying on external SSDs for large app installations. Apps from the Apple App Store don’t have issues installing on external volumes, but Electron apps run into problems because of this bug. Hopefully, this can be fixed and adopted by Electron so that things become easier for everyone. |
|
@augiekim Electron floats a substantial quantity of patches on top of Squirrel.Mac in-tree for both testing & stability reasons https://github.com/electron/electron/tree/main/patches/squirrel.mac Might take some time this weekend to bring this repo into this decade but this fixing an Electron issue in this repo today is unlikely to land in electron for a while given how Electrons build system works. |
e237b51 to
bd0af2d
Compare
Thank you for the quick response and for pulling quite a few patches! Now I remember those patches in the Electron repo. The last time I looked into Electron was over five years ago, and I haven’t had a chance to revisit it since my current work isn’t related to Electron. |
…g it When the target app lives on a different volume than where ShipIt stages downloads (~/Library/Caches/ on the system volume), POSIX rename() fails with EXDEV. The old fallback called removeItemAtURL: on the target first, then NSFileManager moveItemAtURL:toURL:. If that move fails for any reason, the target slot is empty and the app is gone with no way to recover. Fix: when EXDEV fires, copy the source to a UUID-named staging path on the same volume as the target, then rename() atomically from staging to target. Both paths are on the same volume so rename() is guaranteed to succeed or leave the target untouched. The source is only deleted after a successful rename. On copy failure, the source is still intact and the existing item at the target is never touched. This correctly handles the backup step in acquireTargetBundleURLForRequest: (moving the current app off an external volume to system temp) and the rollback path in abortInstall (restoring from system temp back to the external volume). New tests: - Install an update from one external volume to another (double EXDEV) - Restore the app to an external volume after too many failed install attempts Fixes: Squirrel#201 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bd0af2d to
d7440b4
Compare
|
I just realized that this PR doesn’t correctly address the cross-volume update failure issue. The real root cause is that The best option is probably to update Electron to check the installation location and show a popup guiding users to grant permission to ShipIt, followed by opening the Settings window. I’ll also look into whether there are better ways to address this. Here is the analysis and a suggestion: Electron autoUpdater: Post-Mortem Update Failure DetectionProblemWhen a Squirrel.Mac update fails after the app has already quit (e.g. due to Root cause in the log: Why Squirrel Cannot Currently Report This
Proposed SolutionAdd post-mortem detection: on next app launch, read the ShipIt stderr log, Contribution PathEach step is an independent PR and can be pursued in parallel. Step 1 — Squirrel.MacRepo: Add a method to
Key point: This is read-only on startup — no changes to install logic, Step 2 — ElectronRepo: Apply a autoUpdater.on('update-failed-last-session', (event, details) => {
// details.reason: 'permission-denied' | 'code-signature' | 'cross-volume'
// details.message: human-readable description
})Squirrel.patch approach sidesteps the archived fork risk entirely — Step 3 — Electron AppsListen for the new autoUpdater.on('update-failed-last-session', (event, details) => {
if (details.reason === 'permission-denied') {
dialog.showMessageBox({
type: 'warning',
title: 'Update Failed',
message: 'The app could not update itself.',
detail:
'The updater (ShipIt) does not have permission to write to your ' +
'Applications folder. Please grant Full Disk Access to ShipIt in ' +
'System Settings to allow future updates.',
buttons: ['Open System Settings', 'Dismiss'],
}).then(result => {
if (result.response === 0) {
shell.openExternal(
'x-apple.systempreferences:com.apple.preference.security?Privacy_AllFiles'
);
}
});
}
});Why This Is Low Risk
Failure Patterns to Detect
References
|
|
Real-world confirmation of the TCC-permission theory above, from a MacMini + external SSD setup (same scenario @augiekim described). Setup: Element.app installed via Homebrew Cask to an external APFS volume ( Exact error chain, from This matches both the However: I granted Full Disk Access to both Happy to test any specific diagnostic steps if useful — this reproduces reliably on this machine. |
When the target app lives on a different volume than where ShipIt stages downloads (~/Library/Caches/ on the system volume), POSIX rename() fails with EXDEV. The old fallback called removeItemAtURL: on the target first, then NSFileManager moveItemAtURL:toURL:. If that move fails for any reason, the target slot is empty and the app is gone with no way to recover.
Fix: when EXDEV fires, copy the source to a UUID-named staging path on the same volume as the target, then rename() atomically from staging to target. Both paths are on the same volume so rename() is guaranteed to succeed or leave the target untouched. The source is only deleted after a successful rename. On copy failure, the source is still intact and the existing item at the target is never touched.
This correctly handles the backup step in acquireTargetBundleURLForRequest: (moving the current app off an external volume to system temp) and the rollback path in abortInstall (restoring from system temp back to the external volume).
New tests:
Fixes: #201