-
Notifications
You must be signed in to change notification settings - Fork 5.6k
fix(desktop): preserve AppImages when updates fail #10782
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.
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
Open
Gigioxx
wants to merge
2
commits into
pingdotgg:main
Choose a base branch
from
Gigioxx:t3code/fix-issue-10685
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| // @effect-diagnostics nodeBuiltinImport:off -- Real files exercise the CommonJS updater installation boundary. | ||
| import * as NodeCrypto from "node:crypto"; | ||
| import * as NodeModule from "node:module"; | ||
| import * as NodeOS from "node:os"; | ||
| import * as NodePath from "node:path"; | ||
|
|
||
| import { AppImageUpdater } from "electron-updater/out/AppImageUpdater.js"; | ||
| import { DownloadedUpdateHelper } from "electron-updater/out/DownloadedUpdateHelper.js"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; | ||
|
|
||
| // The dependency uses CommonJS; spy on the same Node modules it loads. | ||
| const require = NodeModule.createRequire(import.meta.url); | ||
| const fs = require("node:fs") as typeof import("node:fs"); | ||
| const childProcess = require("node:child_process") as typeof import("node:child_process"); | ||
| const roots: string[] = []; | ||
| const oldBinary = "working AppImage"; | ||
| const newBinary = "verified replacement AppImage"; | ||
|
|
||
| class TestUpdater extends AppImageUpdater { | ||
| override spawnLog = vi.fn(async () => true); | ||
|
|
||
| async prepare(installer: string, sha512: string) { | ||
| this.downloadedUpdateHelper = new DownloadedUpdateHelper(NodePath.dirname(installer)); | ||
| await this.downloadedUpdateHelper.setDownloadedFile( | ||
| installer, | ||
| null, | ||
| { version: "1.1.0", files: [], path: "", sha512, releaseDate: "2026-09-08" }, | ||
| { | ||
| url: new URL("https://example.com/update.AppImage"), | ||
| info: { url: "update.AppImage", sha512 }, | ||
| }, | ||
| NodePath.basename(installer), | ||
| false, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| async function fixture(name = "t3code.AppImage") { | ||
| const root = fs.mkdtempSync(NodePath.join(NodeOS.tmpdir(), "t3-appimage-")); | ||
| roots.push(root); | ||
| const installed = NodePath.join(root, name); | ||
| const cache = NodePath.join(root, "cache"); | ||
| fs.mkdirSync(cache); | ||
| const installer = NodePath.join(cache, "T3-Code-1.1.0.AppImage"); | ||
| fs.writeFileSync(installed, oldBinary, { mode: 0o755 }); | ||
| fs.writeFileSync(installer, newBinary, { mode: 0o755 }); | ||
| vi.stubEnv("APPIMAGE", installed); | ||
| const updater = new TestUpdater(null, { version: "1.0.0" }); | ||
| updater.logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; | ||
| const errors = vi.fn(); | ||
| updater.on("error", errors); | ||
| await updater.prepare( | ||
| installer, | ||
| NodeCrypto.createHash("sha512").update(newBinary).digest("base64"), | ||
| ); | ||
| return { root, installed, installer, updater, errors }; | ||
| } | ||
|
|
||
| beforeEach(() => vi.useFakeTimers({ toFake: ["setImmediate"] })); | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| vi.restoreAllMocks(); | ||
| vi.unstubAllEnvs(); | ||
| for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| describe("AppImage installation", () => { | ||
| // eslint-disable-next-line t3code/no-global-process-runtime -- Directory fsync is a Linux installation boundary. | ||
| it.skipIf(NodeOS.platform() !== "linux").each([false, true])( | ||
| "syncs the renamed directory before cleanup and relaunch (sync failure: %s)", | ||
| async (failSync) => { | ||
| const { installed, installer, updater, errors, root } = | ||
| await fixture("T3-Code-1.0.0.AppImage"); | ||
| const destination = NodePath.join(root, NodePath.basename(installer)); | ||
| const sync = fs.fsyncSync; | ||
| let directoryFd: number | undefined; | ||
| vi.spyOn(fs, "fsyncSync").mockImplementation((fd) => { | ||
| if (fs.fstatSync(fd).isDirectory()) { | ||
| directoryFd = fd; | ||
| expect(fs.fstatSync(fd).ino).toBe(fs.statSync(root).ino); | ||
| expect(fs.readFileSync(destination, "utf8")).toBe(newBinary); | ||
| expect(fs.readFileSync(installed, "utf8")).toBe(oldBinary); | ||
| expect(fs.readFileSync(installer, "utf8")).toBe(newBinary); | ||
| expect(updater.spawnLog).not.toHaveBeenCalled(); | ||
| expect(updater.logger?.info).not.toHaveBeenCalledWith( | ||
| expect.stringContaining("Installed verified"), | ||
| ); | ||
| if (failSync) throw new Error("directory sync failed"); | ||
| } | ||
| sync(fd); | ||
| }); | ||
|
|
||
| updater.quitAndInstall(true, true); | ||
|
|
||
| const closedFd = directoryFd; | ||
| expect(closedFd).toBeDefined(); | ||
| if (closedFd !== undefined) expect(() => fs.fstatSync(closedFd)).toThrow(); | ||
| expect(errors).toHaveBeenCalledTimes(failSync ? 1 : 0); | ||
| expect(updater.spawnLog).toHaveBeenCalledTimes(failSync ? 0 : 1); | ||
| expect(vi.getTimerCount()).toBe(failSync ? 0 : 1); | ||
| expect(fs.existsSync(installed)).toBe(failSync); | ||
| expect(fs.existsSync(installer)).toBe(failSync); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(["empty", "corrupt", "ENOSPC"])( | ||
| "preserves the old executable after a %s copy", | ||
| async (fault) => { | ||
| const { installed, installer, updater, errors, root } = await fixture(); | ||
| const badCopy = (destination: import("node:fs").PathLike) => { | ||
| fs.writeFileSync(destination, fault === "corrupt" ? "x".repeat(newBinary.length) : ""); | ||
| if (fault === "ENOSPC") | ||
| throw Object.assign(new Error("No space left on device"), { code: "ENOSPC" }); | ||
| }; | ||
| vi.spyOn(fs, "copyFileSync").mockImplementation((_source, destination) => | ||
| badCopy(destination), | ||
| ); | ||
| // Reproduce the reported successful-but-empty cross-filesystem mv on the unpatched updater. | ||
| vi.spyOn(childProcess, "execFileSync").mockImplementation((command, args) => { | ||
| if (command !== "mv" || !Array.isArray(args)) throw new Error("Unexpected process launch"); | ||
| badCopy(String(args[2])); | ||
| fs.unlinkSync(installer); | ||
| return Buffer.alloc(0); | ||
| }); | ||
|
|
||
| updater.quitAndInstall(true, true); | ||
|
|
||
| expect(fs.readFileSync(installed, "utf8")).toBe(oldBinary); | ||
| expect(fs.readFileSync(installer, "utf8")).toBe(newBinary); | ||
| expect(updater.spawnLog).not.toHaveBeenCalled(); | ||
| expect(vi.getTimerCount()).toBe(0); | ||
| expect(errors).toHaveBeenCalledOnce(); | ||
| expect(fs.readdirSync(root).sort()).toEqual(["cache", "t3code.AppImage"]); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(["t3code.AppImage", "T3-Code-1.0.0.AppImage"])( | ||
| "atomically installs and relaunches %s", | ||
| async (name) => { | ||
| const { installed, installer, updater } = await fixture(name); | ||
| const destination = | ||
| name === "t3code.AppImage" | ||
| ? installed | ||
| : NodePath.join(NodePath.dirname(installed), NodePath.basename(installer)); | ||
| const rename = fs.renameSync; | ||
| const swapped = vi.spyOn(fs, "renameSync").mockImplementation((source, target) => { | ||
| expect(fs.readFileSync(installed, "utf8")).toBe(oldBinary); | ||
| expect(fs.readFileSync(source, "utf8")).toBe(newBinary); | ||
| expect(fs.statSync(source).dev).toBe(fs.statSync(NodePath.dirname(String(target))).dev); | ||
| rename(source, target); | ||
| }); | ||
| const renamed = vi.fn(); | ||
| updater.on("appimage-filename-updated", renamed); | ||
|
|
||
| expect(updater.install(true, true)).toBe(true); | ||
|
|
||
| expect(swapped).toHaveBeenCalledOnce(); | ||
| expect(fs.readFileSync(destination, "utf8")).toBe(newBinary); | ||
| // eslint-disable-next-line t3code/no-global-process-runtime -- Check permissions on the real fixture filesystem. | ||
| if (NodeOS.platform() !== "win32") expect(fs.statSync(destination).mode & 0o777).toBe(0o755); | ||
| expect(updater.spawnLog).toHaveBeenCalledWith( | ||
| destination, | ||
| [], | ||
| expect.objectContaining({ APPIMAGE_SILENT_INSTALL: "true" }), | ||
| ); | ||
| expect(renamed.mock.calls).toEqual(destination === installed ? [] : [[destination]]); | ||
| }, | ||
| ); | ||
|
|
||
| it("keeps the executable and download retryable when the atomic rename fails", async () => { | ||
| const { installed, installer, updater, errors } = await fixture(); | ||
| const rename = vi.spyOn(fs, "renameSync").mockImplementationOnce(() => { | ||
| throw Object.assign(new Error("Permission denied"), { code: "EACCES" }); | ||
| }); | ||
|
|
||
| updater.quitAndInstall(true, true); | ||
|
|
||
| expect(fs.readFileSync(installed, "utf8")).toBe(oldBinary); | ||
| expect(fs.readFileSync(installer, "utf8")).toBe(newBinary); | ||
| expect(errors).toHaveBeenCalledOnce(); | ||
| expect(updater.spawnLog).not.toHaveBeenCalled(); | ||
| expect(vi.getTimerCount()).toBe(0); | ||
| rename.mockRestore(); | ||
| expect(updater.install(true, true)).toBe(true); | ||
| expect(fs.readFileSync(installed, "utf8")).toBe(newBinary); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| diff --git a/out/AppImageUpdater.js b/out/AppImageUpdater.js | ||
| index 25cef3598cd278cc2a7f1432377c7a50048824b9..a6132439a056671777e10fcfdd96820e331e489b 100644 | ||
| --- a/out/AppImageUpdater.js | ||
| +++ b/out/AppImageUpdater.js | ||
| @@ -6,6 +6,7 @@ const child_process_1 = require("child_process"); | ||
| const fs_extra_1 = require("fs-extra"); | ||
| const fs_1 = require("fs"); | ||
| const path = require("path"); | ||
| +const crypto = require("crypto"); | ||
| const BaseUpdater_1 = require("./BaseUpdater"); | ||
| const FileWithEmbeddedBlockMapDifferentialDownloader_1 = require("./differentialDownloader/FileWithEmbeddedBlockMapDifferentialDownloader"); | ||
| const Provider_1 = require("./providers/Provider"); | ||
| @@ -74,8 +75,6 @@ class AppImageUpdater extends BaseUpdater_1.BaseUpdater { | ||
| if (appImageFile == null) { | ||
| throw (0, builder_util_runtime_1.newError)("APPIMAGE env is not defined", "ERR_UPDATER_OLD_FILE_NOT_FOUND"); | ||
| } | ||
| - // https://stackoverflow.com/a/1712051/1910191 | ||
| - (0, fs_1.unlinkSync)(appImageFile); | ||
| let destination; | ||
| const existingBaseName = path.basename(appImageFile); | ||
| const installerPath = this.installerPath; | ||
| @@ -92,7 +91,62 @@ class AppImageUpdater extends BaseUpdater_1.BaseUpdater { | ||
| else { | ||
| destination = path.join(path.dirname(appImageFile), path.basename(installerPath)); | ||
| } | ||
| - (0, child_process_1.execFileSync)("mv", ["-f", installerPath, destination]); | ||
| + // Stage beside the destination: cache and AppImage may be on different filesystems. | ||
| + const stageDir = fs_1.mkdtempSync(path.join(path.dirname(destination), ".t3-update-")); | ||
| + const staged = path.join(stageDir, "update.AppImage"); | ||
| + this._logger.info(`Staging AppImage update for ${destination}`); | ||
| + try { | ||
| + const size = fs_1.statSync(installerPath).size; | ||
| + fs_1.copyFileSync(installerPath, staged); | ||
| + if (size === 0 || fs_1.statSync(staged).size !== size) { | ||
| + throw new Error("Staged AppImage size mismatch"); | ||
| + } | ||
| + const fd = fs_1.openSync(staged, "r+"); | ||
| + try { | ||
| + const hash = crypto.createHash("sha512"); | ||
| + const buffer = Buffer.allocUnsafe(1024 * 1024); | ||
| + let bytes; | ||
| + while ((bytes = fs_1.readSync(fd, buffer, 0, buffer.length, null)) > 0) { | ||
| + hash.update(buffer.subarray(0, bytes)); | ||
| + } | ||
| + if (hash.digest("base64") !== this.downloadedUpdateHelper.downloadedFileInfo.sha512) { | ||
| + throw new Error("Staged AppImage sha512 mismatch"); | ||
| + } | ||
| + fs_1.fchmodSync(fd, 0o755); | ||
| + fs_1.fsyncSync(fd); | ||
| + } | ||
| + finally { | ||
| + fs_1.closeSync(fd); | ||
| + } | ||
| + fs_1.renameSync(staged, destination); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| + if (process.platform === "linux") { | ||
| + const directoryFd = fs_1.openSync(path.dirname(destination), "r"); | ||
| + try { | ||
| + fs_1.fsyncSync(directoryFd); | ||
| + } | ||
| + finally { | ||
| + fs_1.closeSync(directoryFd); | ||
| + } | ||
| + } | ||
| + this._logger.info(`Installed verified AppImage at ${destination}`); | ||
| + } | ||
| + finally { | ||
| + try { | ||
| + fs_1.rmSync(stageDir, { recursive: true, force: true }); | ||
| + } | ||
| + catch (error) { | ||
| + this._logger.warn(`Could not remove AppImage staging directory: ${error}`); | ||
| + } | ||
| + } | ||
| + // Cleanup must not turn a committed replacement into a failed install. | ||
| + for (const obsolete of destination === appImageFile ? [installerPath] : [installerPath, appImageFile]) { | ||
| + try { | ||
| + fs_1.unlinkSync(obsolete); | ||
| + } | ||
| + catch (error) { | ||
| + this._logger.warn(`Could not remove obsolete AppImage ${obsolete}: ${error}`); | ||
| + } | ||
| + } | ||
| if (destination !== appImageFile) { | ||
| this.emit("appimage-filename-updated", destination); | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 High
patches/electron-updater@6.8.3.patch:54After
renameSync(staged, destination), a power loss can leave the AppImage missing even though its contents were flushed, so the application cannot relaunch.fsyncSync(fd)only persists the file; the parent directory entry is never synced. Open andfsyncthe destination directory after the rename on Linux.🤖 Copy this AI Prompt to have your agent fix this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid finding, fixed in 178dec1. On Linux, installation now opens and fsyncs the destination directory after rename, closes the descriptor in finally, and only then logs success and cleans up. Two regression cases failed before the change and now pass: successful directory sync and injected sync failure. The failure case verifies descriptor closure, retained old version/download, and no relaunch or quit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.