diff --git a/electron/updater-coordinator.mjs b/electron/updater-coordinator.mjs new file mode 100644 index 00000000..90661c45 --- /dev/null +++ b/electron/updater-coordinator.mjs @@ -0,0 +1,81 @@ +export function createUpdaterCoordinator(updater, setState) { + let checkOperation = null; + let downloadOperation = null; + + function handleRejectedOperation(manual, error) { + if (!manual) { + setState({ status: "idle" }); + return; + } + setState({ status: "error", message: String(error?.message ?? error) }); + } + + function checkOwnsState() { + return !downloadOperation && !checkOperation?.supersededByDownload; + } + + updater.on("checking-for-update", () => { + if (checkOwnsState()) setState({ status: "checking" }); + }); + updater.on("update-available", (info) => { + if (checkOwnsState()) { + setState({ status: "available", version: info?.version, message: undefined }); + } + }); + updater.on("update-not-available", () => { + if (checkOwnsState()) setState({ status: "idle" }); + }); + updater.on("download-progress", (progress) => + setState({ status: "downloading", percent: Math.round(progress?.percent ?? 0) }), + ); + updater.on("update-downloaded", (info) => + setState({ status: "downloaded", version: info?.version }), + ); + + function check(manual = false) { + if (checkOperation) { + // A manual caller upgrades the shared operation; a timer never downgrades it. + if (manual) checkOperation.manual = true; + return checkOperation.promise; + } + + const operation = { manual, supersededByDownload: Boolean(downloadOperation), promise: null }; + checkOperation = operation; + try { + operation.promise = Promise.resolve(updater.checkForUpdates()) + .catch((error) => { + if (!operation.supersededByDownload) handleRejectedOperation(operation.manual, error); + }) + .finally(() => { + if (checkOperation === operation) checkOperation = null; + }); + } catch (error) { + if (!operation.supersededByDownload) handleRejectedOperation(operation.manual, error); + checkOperation = null; + operation.promise = Promise.resolve(); + } + return operation.promise; + } + + function download() { + if (checkOperation) checkOperation.supersededByDownload = true; + if (downloadOperation) return downloadOperation.promise; + + const operation = { promise: null }; + downloadOperation = operation; + try { + operation.promise = Promise.resolve(updater.downloadUpdate()) + .catch((error) => handleRejectedOperation(true, error)) + .finally(() => { + if (downloadOperation === operation) downloadOperation = null; + }); + } catch (error) { + handleRejectedOperation(true, error); + downloadOperation = null; + operation.promise = Promise.resolve(); + } + return operation.promise; + } + + return { check, download }; +} diff --git a/electron/updater-coordinator.node-test.mjs b/electron/updater-coordinator.node-test.mjs new file mode 100644 index 00000000..68bac5e6 --- /dev/null +++ b/electron/updater-coordinator.node-test.mjs @@ -0,0 +1,272 @@ +import assert from "node:assert/strict"; +import { EventEmitter } from "node:events"; +import test from "node:test"; + +import { createUpdaterCoordinator } from "./updater-coordinator.mjs"; + +function deferred() { + let resolve; + let reject; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +function harness() { + const updater = new EventEmitter(); + // electron-updater has its own error listener; model that without routing it. + updater.on("error", () => {}); + let state = { status: "idle" }; + const states = []; + const coordinator = createUpdaterCoordinator(updater, (patch) => { + state = { ...state, ...patch }; + states.push({ ...state }); + }); + return { updater, coordinator, states, getState: () => state }; +} + +function errorStates(states) { + return states.filter((entry) => entry.status === "error"); +} + +test("automatic check rejection is handled and returns to idle", async () => { + const { updater, coordinator, getState } = harness(); + updater.checkForUpdates = () => Promise.reject(new Error("offline")); + + await assert.doesNotReject(coordinator.check()); + + assert.equal(getState().status, "idle"); +}); + +test("manual check rejection is handled as a user-visible error", async () => { + const { updater, coordinator, getState } = harness(); + updater.checkForUpdates = () => Promise.reject(new Error("feed failed")); + + await assert.doesNotReject(coordinator.check(true)); + + assert.deepEqual(getState(), { status: "error", message: "feed failed" }); +}); + +test("download rejection is handled as a user-visible error", async () => { + const { updater, coordinator, getState } = harness(); + updater.downloadUpdate = () => Promise.reject(new Error("download failed")); + + await assert.doesNotReject(coordinator.download()); + + assert.deepEqual(getState(), { status: "error", message: "download failed" }); +}); + +test("synchronous check and download throws are handled", async () => { + const { updater, coordinator, getState } = harness(); + updater.checkForUpdates = () => { + throw new Error("check threw"); + }; + + await assert.doesNotReject(coordinator.check(true)); + assert.deepEqual(getState(), { status: "error", message: "check threw" }); + + updater.downloadUpdate = () => { + throw new Error("download threw"); + }; + + await assert.doesNotReject(coordinator.download()); + assert.deepEqual(getState(), { status: "error", message: "download threw" }); +}); + +test("a concurrent background check cannot downgrade a manual check", async () => { + const { updater, coordinator, getState } = harness(); + const pending = deferred(); + let calls = 0; + updater.checkForUpdates = () => { + calls += 1; + return pending.promise; + }; + + const manual = coordinator.check(true); + const background = coordinator.check(); + assert.strictEqual(background, manual); + assert.equal(calls, 1); + + pending.reject(new Error("manual failure")); + await manual; + + assert.deepEqual(getState(), { status: "error", message: "manual failure" }); +}); + +test("a manual request during a background check preserves user-visible errors", async () => { + const { updater, coordinator, getState } = harness(); + const pending = deferred(); + let calls = 0; + updater.checkForUpdates = () => { + calls += 1; + return pending.promise; + }; + + const background = coordinator.check(); + const manual = coordinator.check(true); + assert.strictEqual(manual, background); + assert.equal(calls, 1); + + pending.reject(new Error("background request failed")); + await background; + + assert.deepEqual(getState(), { status: "error", message: "background request failed" }); +}); + +test("an active download state survives a later background check failure", async () => { + const { updater, coordinator, getState } = harness(); + const downloadPending = deferred(); + const checkPending = deferred(); + updater.downloadUpdate = () => { + updater.emit("download-progress", { percent: 42 }); + return downloadPending.promise; + }; + updater.checkForUpdates = () => { + updater.emit("checking-for-update"); + updater.emit("update-available", { version: "2.1.0" }); + updater.emit("update-not-available"); + return checkPending.promise; + }; + + const download = coordinator.download(); + assert.deepEqual(getState(), { status: "downloading", percent: 42 }); + + const background = coordinator.check(); + checkPending.reject(new Error("background check failed")); + await background; + assert.deepEqual(getState(), { status: "downloading", percent: 42 }); + + downloadPending.resolve(); + await download; +}); + +test("a download error remains authoritative after a later background failure", async () => { + const { updater, coordinator, getState } = harness(); + const downloadPending = deferred(); + const checkPending = deferred(); + updater.downloadUpdate = () => { + updater.emit("download-progress", { percent: 75 }); + return downloadPending.promise; + }; + updater.checkForUpdates = () => checkPending.promise; + + const download = coordinator.download(); + const background = coordinator.check(); + + const downloadError = new Error("download failed first"); + downloadPending.reject(downloadError); + await download; + assert.deepEqual(getState(), { + status: "error", + percent: 75, + message: "download failed first", + }); + + updater.emit("checking-for-update"); + updater.emit("update-available", { version: "2.1.0" }); + updater.emit("update-not-available"); + checkPending.reject(new Error("background check failed later")); + await background; + + assert.deepEqual(getState(), { + status: "error", + percent: 75, + message: "download failed first", + }); +}); + +test("a background failure stays silent before a later download failure", async () => { + const { updater, coordinator, getState, states } = harness(); + const downloadPending = deferred(); + const checkPending = deferred(); + updater.checkForUpdates = () => checkPending.promise; + updater.downloadUpdate = () => { + updater.emit("download-progress", { percent: 18 }); + return downloadPending.promise; + }; + + const background = coordinator.check(); + const download = coordinator.download(); + + updater.emit("checking-for-update"); + updater.emit("update-available", { version: "2.1.0" }); + updater.emit("update-not-available"); + checkPending.reject(new Error("background check failed first")); + await background; + assert.deepEqual(getState(), { status: "downloading", percent: 18 }); + assert.equal(errorStates(states).length, 0); + + const downloadError = new Error("download failed later"); + downloadPending.reject(downloadError); + await download; + + assert.deepEqual(getState(), { + status: "error", + percent: 18, + message: "download failed later", + }); +}); + +test("available, not-available, progress, and downloaded events preserve success behavior", async () => { + const available = harness(); + available.updater.checkForUpdates = () => { + available.updater.emit("checking-for-update"); + queueMicrotask(() => available.updater.emit("update-available", { version: "2.0.0" })); + return Promise.resolve({ isUpdateAvailable: true }); + }; + await available.coordinator.check(true); + assert.equal(available.getState().status, "available"); + assert.equal(available.getState().version, "2.0.0"); + + available.updater.downloadUpdate = () => { + available.updater.emit("download-progress", { percent: 42.4 }); + return Promise.resolve().then(() => { + available.updater.emit("update-downloaded", { version: "2.0.0" }); + return ["update.zip"]; + }); + }; + await available.coordinator.download(); + assert.deepEqual(available.getState(), { + status: "downloaded", + version: "2.0.0", + message: undefined, + percent: 42, + }); + + const notAvailable = harness(); + notAvailable.updater.checkForUpdates = () => { + notAvailable.updater.emit("checking-for-update"); + notAvailable.updater.emit("update-not-available"); + return Promise.resolve({ isUpdateAvailable: false }); + }; + await notAvailable.coordinator.check(); + assert.equal(notAvailable.getState().status, "idle"); +}); + +test("an updater error event and rejected promise produce one deterministic state", async () => { + const check = harness(); + const checkError = new Error("check failed once"); + check.updater.checkForUpdates = () => + Promise.reject(checkError).catch((error) => { + check.updater.emit("error", error); + throw error; + }); + + await check.coordinator.check(true); + assert.equal(errorStates(check.states).length, 1); + assert.deepEqual(check.getState(), { status: "error", message: "check failed once" }); + + const download = harness(); + const downloadError = new Error("download failed once"); + download.updater.downloadUpdate = () => + Promise.reject(downloadError).catch((error) => { + download.updater.emit("error", error); + throw error; + }); + + await download.coordinator.download(); + assert.equal(errorStates(download.states).length, 1); + assert.deepEqual(download.getState(), { status: "error", message: "download failed once" }); +}); diff --git a/electron/updater.mjs b/electron/updater.mjs index 866b9b97..dca2a84e 100644 --- a/electron/updater.mjs +++ b/electron/updater.mjs @@ -9,6 +9,7 @@ // the packaged app ships no node_modules. import { app, ipcMain } from "electron"; import { createRequire } from "node:module"; +import { createUpdaterCoordinator } from "./updater-coordinator.mjs"; const require = createRequire(import.meta.url); @@ -16,12 +17,7 @@ let autoUpdater = null; let win = null; // status: idle | checking | available | downloading | downloaded | error let state = { status: "idle" }; -// Whether the in-flight check came from the user's button. Background checks -// fail for reasons that are none of the user's business — no feed published -// for this platform yet, offline, a GitHub blip — and a popup for those on -// every launch is pure noise. Only a check the user asked for may surface an -// error; automatic ones fall back to idle. -let userInitiated = false; +let updaterCoordinator = null; function setState(patch) { state = { ...state, ...patch }; @@ -32,31 +28,10 @@ function setState(patch) { } } -function check(manual = false) { - if (!autoUpdater) return; - userInitiated = manual; - try { - autoUpdater.checkForUpdates(); - } catch (e) { - reportError(e); - } -} - -function reportError(e) { - if (!userInitiated) return setState({ status: "idle" }); - setState({ status: "error", message: String(e?.message ?? e) }); -} - export function registerUpdaterIpc() { ipcMain.handle("update:get-state", () => state); - ipcMain.handle("update:check", () => check(true)); - ipcMain.handle("update:download", () => { - try { - autoUpdater?.downloadUpdate(); - } catch (e) { - setState({ status: "error", message: String(e?.message ?? e) }); - } - }); + ipcMain.handle("update:check", () => updaterCoordinator?.check(true)); + ipcMain.handle("update:download", () => updaterCoordinator?.download()); ipcMain.handle("update:install", () => { // isSilent, isForceRunAfter — relaunch straight into the new version try { @@ -71,12 +46,14 @@ export function startUpdater(mainWindow) { win = mainWindow; // dev / unsigned builds can't auto-update — leave the banner dormant if (!app.isPackaged) { + updaterCoordinator = null; setState({ status: "idle" }); return; } try { ({ autoUpdater } = require("./vendor/electron-updater.cjs")); } catch { + updaterCoordinator = null; setState({ status: "error", message: "updater unavailable" }); return; } @@ -84,22 +61,11 @@ export function startUpdater(mainWindow) { autoUpdater.autoInstallOnAppQuit = false; // button-driven install autoUpdater.logger = null; - autoUpdater.on("checking-for-update", () => setState({ status: "checking" })); - autoUpdater.on("update-available", (info) => - setState({ status: "available", version: info?.version, message: undefined }), - ); - autoUpdater.on("update-not-available", () => setState({ status: "idle" })); - autoUpdater.on("download-progress", (p) => - setState({ status: "downloading", percent: Math.round(p?.percent ?? 0) }), - ); - autoUpdater.on("update-downloaded", (info) => - setState({ status: "downloaded", version: info?.version }), - ); - autoUpdater.on("error", reportError); + updaterCoordinator = createUpdaterCoordinator(autoUpdater, setState); // first check ~15s after launch (let the app settle), then hourly — both // silent on failure, hence the arrow: a bare `check` would receive the // timer's argument as `manual` and start reporting errors again. - setTimeout(() => check(), 15_000).unref?.(); - setInterval(() => check(), 60 * 60 * 1000).unref?.(); + setTimeout(() => void updaterCoordinator?.check(), 15_000).unref?.(); + setInterval(() => void updaterCoordinator?.check(), 60 * 60 * 1000).unref?.(); } diff --git a/package.json b/package.json index ad703f7e..0621dad9 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,8 @@ "dev:desktop": "electron .", "build": "tsc -b && tsc -p tsconfig.server.json && vite build", "typecheck": "tsc -b && tsc -p tsconfig.server.json", - "test": "vitest run", + "test": "vitest run && pnpm test:updater", + "test:updater": "node --test electron/updater-coordinator.node-test.mjs", "bench:observation": "node --experimental-strip-types scripts/bench-observation.ts", "test:watch": "vitest", "test:cua": "pnpm build:cua && node scripts/smoke-cua.mjs",