From c16f893fc94f9036b64fdcc02999b26cac4b6d05 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:19:24 -0400 Subject: [PATCH] fix(desktop): let a newer release supersede a staged update without relaunch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem A staged update blocked discovery of later releases, so the top-right pill kept offering an older download for hours after a newer version shipped. Cause Automatic checks returned early while status was ready, and the Settings button recorded the newer version as metadata without replacing the staged archive. Change and boundary Every check now runs while ready: same-or-older is ignored, a strictly newer feed answer supersedes and downloads in place, and a failed check leaves the staged update untouched. An in-flight quitAndInstall still blocks a check so it cannot delete the archive about to be handed to Squirrel or NSIS. IPC, CLI, TUI, and snapshot shape are unchanged; userInitiated only labels the log. Verification npx vitest run src/main/services/updates — 99 passed (77 autoUpdateService, 8 updateTransaction, 14 runtimeRestartVerification). Authored with Cursor Grok 4.6 via ADE. Co-authored-by: Cursor --- .../src/main/services/ipc/registerIpc.ts | 4 +- .../updates/autoUpdateService.test.ts | 332 +++++++++++++++--- .../services/updates/autoUpdateService.ts | 132 +++---- docs/ARCHITECTURE.md | 2 +- .../onboarding-and-settings/README.md | 29 +- .../desktop-auto-update.md | 42 ++- 6 files changed, 418 insertions(+), 123 deletions(-) diff --git a/apps/desktop/src/main/services/ipc/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index c1e2321b2..caff5f132 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -11451,8 +11451,8 @@ export function registerIpc({ }); ipcMain.handle(IPC.updateCheckForUpdates, () => { - // Only reachable from the Settings button, so it always counts as - // user-initiated: it must run even when an update is already staged. + // Only reachable from the Settings button. Every entry point now runs the + // same check, so `userInitiated` only labels the log line. getCtx().autoUpdateService?.checkForUpdates({ userInitiated: true }); }); diff --git a/apps/desktop/src/main/services/updates/autoUpdateService.test.ts b/apps/desktop/src/main/services/updates/autoUpdateService.test.ts index a1f456d71..ccd78180a 100644 --- a/apps/desktop/src/main/services/updates/autoUpdateService.test.ts +++ b/apps/desktop/src/main/services/updates/autoUpdateService.test.ts @@ -2,7 +2,7 @@ import { EventEmitter } from "node:events"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi, type Mock } from "vitest"; import { createAutoUpdateService } from "./autoUpdateService"; import { buildGithubReleaseUrl, @@ -76,6 +76,80 @@ function plentyOfDisk(): { availableBytes: number; volumePath: string } { }; } +// The release the supersede tests answer the feed with. Every one of them +// needs the same shape, and a drifting `files` entry would silently change +// which preflight the download runs through. +const NEWER_UPDATE = { + version: "1.2.63", + files: [{ url: "ADE-1.2.63-mac.zip", size: 100 * 1024 * 1024, sha512: "new" }], +}; + +function newerUpdateArchivePath(updaterCacheDir: string): string { + return path.join(updaterCacheDir, "pending", "ADE-1.2.63-universal-mac.zip"); +} + +// electron-updater emits `update-downloaded` from inside downloadUpdate(), so +// the fake writes the archive and emits before it resolves. A mock that only +// resolves cannot see the supersede path at all. +function downloadsNewerUpdate( + updater: FakeAutoUpdater, + updaterCacheDir: string, +): () => Promise { + const archivePath = newerUpdateArchivePath(updaterCacheDir); + return async () => { + fs.mkdirSync(path.dirname(archivePath), { recursive: true }); + fs.writeFileSync(archivePath, "newer update", "utf8"); + updater.emit("update-downloaded", { ...NEWER_UPDATE, downloadedFile: archivePath }); + }; +} + +type StagedReadyUpdate = { + service: ReturnType; + updater: FakeAutoUpdater & { downloadUpdate: Mock<[], Promise> }; + logger: Logger; + updaterCacheDir: string; +}; + +/** + * Builds a service that already has 1.2.61 downloaded and waiting for a + * restart, which is the starting state of every "check while staged" test. + * `downloadUpdate` defaults to a fake that downloads nothing; pass + * `downloadsNewerUpdate` to cover the supersede path. + */ +function stageReadyUpdate( + overrides: Partial[0]> & { + downloadUpdate?: ( + updater: FakeAutoUpdater, + updaterCacheDir: string, + ) => () => Promise; + } = {}, +): StagedReadyUpdate { + const { downloadUpdate, ...serviceOverrides } = overrides; + const updaterCacheDir = makeUpdaterCacheDir(); + const logger = makeLogger(); + const updater = new FakeAutoUpdater() as StagedReadyUpdate["updater"]; + updater.downloadUpdate = vi.fn<[], Promise>( + downloadUpdate?.(updater, updaterCacheDir) ?? (async () => undefined), + ); + const service = createAutoUpdateService({ + logger, + currentVersion: "1.2.60", + globalStatePath: makeStatePath(), + updaterCacheDir, + getDiskSpace: plentyOfDisk, + autoCheckEnabled: false, + autoApplyEnabled: false, + updater, + ...serviceOverrides, + }); + + updater.emit("update-available", { version: "1.2.61" }); + updater.emit("update-downloaded", { version: "1.2.61" }); + expect(service.getSnapshot()).toMatchObject({ status: "ready", version: "1.2.61" }); + + return { service, updater, logger, updaterCacheDir }; +} + function readState(globalStatePath: string): Record { try { return JSON.parse(fs.readFileSync(globalStatePath, "utf8")); @@ -468,80 +542,242 @@ describe("createAutoUpdateService", () => { service.dispose(); }); - it("still checks when an update is already staged, but only when the user asked", async () => { - // The Settings button was a silent no-op in exactly this state: an update - // downloaded and waiting for a restart. The automatic timers must keep - // standing down so they cannot disturb the staged download. - const updater = new FakeAutoUpdater(); - const service = createAutoUpdateService({ - logger: makeLogger(), - currentVersion: "1.2.60", - globalStatePath: makeStatePath(), + // A staged update used to block discovery of every later release until the + // app was relaunched: the automatic checks returned early while `ready`, and + // a manual check recorded the newer version as metadata only. The pill kept + // offering the older release for hours. Every check now behaves the same way. + it("ignores a same-version feed answer on the periodic check while an update is staged", async () => { + vi.useFakeTimers(); + const { service, updater, logger, updaterCacheDir } = stageReadyUpdate({ startupDelayMs: 60_000, - periodicCheckMs: 60_000, - now: () => "2026-08-19T21:00:00.000Z", - updater, + periodicCheckMs: 1_000, + autoCheckEnabled: true, }); - updater.emit("update-available", { version: "1.2.61" }); - updater.emit("update-downloaded", { version: "1.2.61" }); - expect(service.getSnapshot()).toMatchObject({ status: "ready", version: "1.2.61" }); + updater.checkForUpdates.mockImplementationOnce(async () => { + updater.emit("checking-for-update"); + updater.emit("update-available", { version: "1.2.61" }); + return { updateInfo: { version: "1.2.61" } }; + }); - updater.checkForUpdates.mockClear(); - service.checkForUpdates(); - // Flush the microtask queue rather than waitFor: a `not.toHaveBeenCalled` - // inside waitFor passes on its first tick and would assert nothing. - await Promise.resolve(); - await Promise.resolve(); - expect(updater.checkForUpdates).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(1_000); + await vi.waitFor(() => expect(updater.checkForUpdates).toHaveBeenCalled()); - // electron-updater emits these BEFORE checkForUpdates() resolves. A mock - // that only resolves cannot see the bug this test exists for: the - // update-available handler is what supersedes the staged update, deletes - // the finished download, and frees the resolve path to start a new one. - const downloadUpdate = vi.fn(async () => null); - (updater as unknown as { downloadUpdate: unknown }).downloadUpdate = downloadUpdate; - updater.checkForUpdates.mockImplementation(async () => { + expect(service.getSnapshot()).toMatchObject({ + status: "ready", + version: "1.2.61", + latestKnownVersion: "1.2.61", + }); + expect(updater.downloadUpdate).not.toHaveBeenCalled(); + expect(fs.readdirSync(updaterCacheDir).sort()).toEqual(["pending", "update.zip"]); + expect(logger.info).not.toHaveBeenCalledWith( + "autoUpdate.cache_cleaned", + expect.objectContaining({ reason: "superseded_ready_update" }), + ); + expect(logger.info).toHaveBeenCalledWith( + "autoUpdate.update_available_ignored", + expect.objectContaining({ version: "1.2.61", reason: "same_ready_version" }), + ); + + service.dispose(); + }); + + it("supersedes a staged update with a newer release found by the periodic check", async () => { + vi.useFakeTimers(); + const { service, updater, logger, updaterCacheDir } = stageReadyUpdate({ + startupDelayMs: 60_000, + periodicCheckMs: 1_000, + autoCheckEnabled: true, + downloadUpdate: downloadsNewerUpdate, + }); + + updater.checkForUpdates.mockImplementationOnce(async () => { + updater.emit("checking-for-update"); + updater.emit("update-available", NEWER_UPDATE); + return { updateInfo: NEWER_UPDATE }; + }); + + await vi.advanceTimersByTimeAsync(1_000); + await vi.waitFor(() => { + expect(service.getSnapshot()).toMatchObject({ + status: "ready", + version: "1.2.63", + latestKnownVersion: "1.2.63", + }); + }); + + expect(updater.downloadUpdate).toHaveBeenCalledTimes(1); + expect(logger.info).toHaveBeenCalledWith( + "autoUpdate.cache_cleaned", + expect.objectContaining({ reason: "superseded_ready_update" }), + ); + expect(fs.readFileSync(newerUpdateArchivePath(updaterCacheDir), "utf8")).toBe("newer update"); + + service.dispose(); + }); + + it("supersedes a staged update with a newer release found by a user-initiated check", async () => { + const { service, updater, logger } = stageReadyUpdate({ + downloadUpdate: downloadsNewerUpdate, + }); + + // electron-updater emits these BEFORE checkForUpdates() resolves, so the + // supersede happens in the update-available handler. + updater.checkForUpdates.mockImplementationOnce(async () => { updater.emit("checking-for-update"); - updater.emit("update-available", { version: "1.2.63" }); - return { updateInfo: { version: "1.2.63" } }; + updater.emit("update-available", NEWER_UPDATE); + return { updateInfo: NEWER_UPDATE }; }); + service.checkForUpdates({ userInitiated: true }); await vi.waitFor(() => { - expect(updater.checkForUpdates).toHaveBeenCalledTimes(1); - // The newest version is reported, and the staged 1.2.61 is left alone. expect(service.getSnapshot()).toMatchObject({ status: "ready", - version: "1.2.61", + version: "1.2.63", latestKnownVersion: "1.2.63", }); }); - // Asking what the newest version is must not throw away the update the - // user already downloaded and is one restart away from installing. - expect(downloadUpdate).not.toHaveBeenCalled(); + expect(updater.checkForUpdates).toHaveBeenCalledTimes(1); + expect(updater.downloadUpdate).toHaveBeenCalledTimes(1); + expect(logger.info).toHaveBeenCalledWith( + "autoUpdate.cache_cleaned", + expect.objectContaining({ reason: "superseded_ready_update" }), + ); - // Let the first check's promise chain settle: while `checkPromise` is - // still set, a second call is swallowed by the in-flight guard and would - // assert nothing. - await new Promise((resolve) => setTimeout(resolve, 0)); + service.dispose(); + }); - // ...and neither must a check that fails. A feed error here means "nothing - // new to tell you", not "discard the finished download". - updater.checkForUpdates.mockImplementation(async () => { + it("leaves a staged update untouched when the check itself fails", async () => { + const { service, updater, logger, updaterCacheDir } = stageReadyUpdate(); + + // A feed error means "nothing new to report", not "discard the finished + // download". electron-updater emits `error` and rejects, so cover both. + updater.checkForUpdates.mockImplementationOnce(async () => { updater.emit("checking-for-update"); updater.emit("error", new Error("net::ERR_INTERNET_DISCONNECTED")); throw new Error("net::ERR_INTERNET_DISCONNECTED"); }); + + service.checkForUpdates(); + + await vi.waitFor(() => expect(updater.checkForUpdates).toHaveBeenCalledTimes(1)); + expect(service.getSnapshot()).toMatchObject({ + status: "ready", + version: "1.2.61", + latestKnownVersion: "1.2.61", + error: null, + errorDetails: null, + }); + expect(updater.downloadUpdate).not.toHaveBeenCalled(); + expect(fs.readdirSync(updaterCacheDir).sort()).toEqual(["pending", "update.zip"]); + expect(logger.info).not.toHaveBeenCalledWith( + "autoUpdate.cache_cleaned", + expect.anything(), + ); + expect(logger.warn).toHaveBeenCalledWith( + "autoUpdate.ready_check_failed", + expect.objectContaining({ readyVersion: "1.2.61" }), + ); + + service.dispose(); + }); + + it("refuses to check the feed while a quit-and-install transaction is running", async () => { + // The snapshot stays `ready` for the whole transaction, across the + // `beforeQuitAndInstall` service uninstall, and only flips to `installing` + // afterwards. A check started in that window would let a strictly newer + // feed answer supersede and delete the archive the install is one call away + // from handing to Squirrel/NSIS. + let releasePrepare!: () => void; + let signalPrepareStarted!: () => void; + const prepareStarted = new Promise((resolve) => { + signalPrepareStarted = resolve; + }); + const { service, updater, logger, updaterCacheDir } = stageReadyUpdate({ + downloadUpdate: downloadsNewerUpdate, + beforeQuitAndInstall: () => { + signalPrepareStarted(); + return new Promise((resolve) => { + releasePrepare = resolve; + }); + }, + }); + const cacheBefore = fs.readdirSync(updaterCacheDir).sort(); + + const installPromise = service.quitAndInstall(); + await prepareStarted; + + // The pre-install refresh already ran and finished; only the guard can + // stand between the next check and the staged archive now. + updater.checkForUpdates.mockClear(); + updater.checkForUpdates.mockImplementation(async () => { + updater.emit("checking-for-update"); + updater.emit("update-available", NEWER_UPDATE); + return { updateInfo: NEWER_UPDATE }; + }); + + service.checkForUpdates(); service.checkForUpdates({ userInitiated: true }); + await Promise.resolve(); + await Promise.resolve(); + + expect(updater.checkForUpdates).not.toHaveBeenCalled(); + expect(updater.downloadUpdate).not.toHaveBeenCalled(); + expect(fs.readdirSync(updaterCacheDir).sort()).toEqual(cacheBefore); + expect(fs.existsSync(newerUpdateArchivePath(updaterCacheDir))).toBe(false); + expect(logger.info).not.toHaveBeenCalledWith( + "autoUpdate.cache_cleaned", + expect.objectContaining({ reason: "superseded_ready_update" }), + ); + expect(service.getSnapshot()).toMatchObject({ status: "ready", version: "1.2.61" }); + + releasePrepare(); + await expect(installPromise).resolves.toBe(true); + expect(service.getSnapshot()).toMatchObject({ status: "installing", version: "1.2.61" }); + expect(updater.quitAndInstall).toHaveBeenCalledWith(false, true); - await vi.waitFor(() => expect(updater.checkForUpdates).toHaveBeenCalledTimes(2)); + service.dispose(); + }); + + it("aborts the install when the check it piggybacks on fails", async () => { + // The pre-install refresh reuses an in-flight periodic check instead of + // starting its own. The failure guards must test the refresh first, or the + // feed error is swallowed as "nothing new" and the install proceeds on a + // version it never managed to verify. + const { service, updater, logger, updaterCacheDir } = stageReadyUpdate({ + downloadUpdate: downloadsNewerUpdate, + }); + const cacheBefore = fs.readdirSync(updaterCacheDir).sort(); + + let rejectFeed!: (error: Error) => void; + updater.checkForUpdates.mockImplementationOnce(() => { + updater.emit("checking-for-update"); + return new Promise((_resolve, reject) => { + rejectFeed = reject; + }); + }); + + service.checkForUpdates(); + await vi.waitFor(() => expect(updater.checkForUpdates).toHaveBeenCalledTimes(1)); + const installPromise = service.quitAndInstall(); + + const feedError = new Error("net::ERR_INTERNET_DISCONNECTED"); + updater.emit("error", feedError); + rejectFeed(feedError); + + await expect(installPromise).resolves.toBe(false); + expect(updater.quitAndInstall).not.toHaveBeenCalled(); expect(service.getSnapshot()).toMatchObject({ status: "ready", version: "1.2.61", - latestKnownVersion: "1.2.63", + parked: { reason: "refresh_failed" }, }); - expect(downloadUpdate).not.toHaveBeenCalled(); + expect(fs.readdirSync(updaterCacheDir).sort()).toEqual(cacheBefore); + expect(logger.warn).toHaveBeenCalledWith( + "autoUpdate.refresh_ready_before_install_failed", + expect.objectContaining({ version: "1.2.61" }), + ); service.dispose(); }); diff --git a/apps/desktop/src/main/services/updates/autoUpdateService.ts b/apps/desktop/src/main/services/updates/autoUpdateService.ts index ddf22388a..ffa10bddb 100644 --- a/apps/desktop/src/main/services/updates/autoUpdateService.ts +++ b/apps/desktop/src/main/services/updates/autoUpdateService.ts @@ -527,11 +527,35 @@ export function createAutoUpdateService({ let ignoredDownloadVersion: string | null = null; let readyRefreshInProgress = false; /** - * A user-initiated check while an update is already staged. The answer they - * want is "what is the newest version" — throwing away the update they have - * already downloaded in order to answer it is not a service. + * A feed check that started while an update was already staged, from any + * entry point. A strictly newer release supersedes the staged one, so this + * flag only protects the failure path: a check that cannot answer must leave + * the staged update, its version and its archive exactly as it found them. + * It is never set for the pre-install refresh, which owns its own failure + * handling through `readyRefreshInProgress`. */ - let readyMetadataRefreshInProgress = false; + let readyCheckInProgress = false; + /** + * The shared failure guard for a check that started while an update was + * staged. The check asked what the newest version is and the feed did not + * answer. That is a reason to report nothing new, not a reason to throw away + * the update already downloaded: setErrorSnapshot would replace the `ready` + * snapshot and, with currentPhase already moved to "download" by + * `checking-for-update`, take the finished download with it. Once a supersede + * has started the status is no longer `ready`, so a failure of the new + * download flows through the normal error path instead. + * + * Returns true when the caller must return without touching the snapshot. + */ + function preserveStagedUpdateOnCheckFailure(err: unknown): boolean { + if (!readyCheckInProgress || snapshot.status !== "ready") return false; + logger.warn("autoUpdate.ready_check_failed", { + message: formatErrorMessage(err), + kind: classifyUpdateError(err, currentPhase).kind, + readyVersion: snapshot.version, + }); + return true; + } const readyRefreshFailure: { current: { error: unknown; phase: AutoUpdatePhase } | null; } = { current: null }; @@ -840,20 +864,10 @@ export function createAutoUpdateService({ }); return; } - if (readyMetadataRefreshInProgress) { - // electron-updater emits `update-available` BEFORE checkForUpdates() - // resolves, so without this the newer version would supersede the - // staged one here — deleting the finished download from the cache and - // leaving the resolve path free to start a fresh one — purely because - // someone pressed a button to ask a question. - ignoredDownloadVersion = info.version; - patchSnapshot({ latestKnownVersion: info.version }); - logger.info("autoUpdate.update_available_metadata_only", { - version: info.version, - readyVersion: snapshot.version, - }); - return; - } + // A strictly newer release supersedes the staged one, whichever check + // found it. Keeping the older archive would pin the app to a version the + // feed has already replaced until the next relaunch, which is the exact + // complaint this behavior exists to answer. if (!readyRefreshInProgress) { downloadedFilePath = null; cleanupUpdaterCacheDir({ @@ -1004,19 +1018,10 @@ export function createAutoUpdateService({ } ignoredDownloadVersion = null; if (staleHandoffRecoveryInProgress && isStaleHandoffError(err)) return; - if (readyMetadataRefreshInProgress) { - // The user asked what the newest version is and the feed did not answer. - // That is a reason to tell them nothing new, not a reason to throw away - // the update they already downloaded: setErrorSnapshot would replace the - // `ready` snapshot and, with currentPhase already moved to "download" by - // `checking-for-update`, take the finished download with it. - logger.warn("autoUpdate.metadata_refresh_failed", { - message, - kind: classified.kind, - readyVersion: snapshot.version, - }); - return; - } + // The pre-install refresh is tested first. It can piggyback on a periodic + // check that is already in flight, which leaves `readyCheckInProgress` set + // too. Recording the failure is what aborts the install; swallowing it here + // would let the install proceed on an unverified staged version. if (readyRefreshInProgress) { readyRefreshFailure.current = { error: err, @@ -1024,6 +1029,7 @@ export function createAutoUpdateService({ }; return; } + if (preserveStagedUpdateOnCheckFailure(err)) return; if (snapshot.status === "installing") { if ( isStaleHandoffError(err) @@ -1054,13 +1060,21 @@ export function createAutoUpdateService({ updater.on("update-cancelled", onUpdateCancelled); updater.on("error", onError); - async function runUpdateCheck( - args: { allowReady?: boolean; metadataOnlyWhenReady?: boolean } = {}, - ): Promise { + async function runUpdateCheck(): Promise { if (checkPromise) { await checkPromise; return; } + const isReadyCheck = snapshot.status === "ready" && !readyRefreshInProgress; + // An install transaction holds the staged archive until it flips the + // snapshot to `installing`, and the status stays `ready` across the whole + // `beforeQuitAndInstall` service uninstall. A check started in that window + // would let a strictly newer feed answer supersede and wipe the archive the + // install is about to hand to Squirrel/NSIS. The `!readyRefreshInProgress` + // term is required: when `restorePromise` is non-null, `run()` suspends at + // `await restorePromise` before the pre-install refresh, so + // `quitAndInstallPromise` is already assigned when that refresh runs. + if (isReadyCheck && (quitAndInstallPromise != null || installQuitArmed)) return; if ( snapshot.status === "checking" || snapshot.status === "downloading" @@ -1068,11 +1082,16 @@ export function createAutoUpdateService({ ) { return; } - if (!args.allowReady && snapshot.status === "ready") { + if (isReadyCheck) { + // A restore already owns the staged archive and drives the updater + // itself. Starting a feed check on top of it would race its download. + if (archiveRestoreInProgress) return; + // The archive is the thing the restart depends on. Put it back first and + // ask the feed on the next cycle, so one cycle never does both. if (!stagedArchiveStillPresent()) { void restoreStagedArchiveIfMissing("periodic_ready_check"); + return; } - return; } const reusableDownloadedVersion = snapshot.status === "error" && snapshot.errorDetails?.preservesDownload @@ -1081,8 +1100,7 @@ export function createAutoUpdateService({ preservedDownloadRetry = reusableDownloadedVersion ? { version: reusableDownloadedVersion, releaseNotesUrl: snapshot.releaseNotesUrl } : null; - readyMetadataRefreshInProgress = args.metadataOnlyWhenReady === true - && snapshot.status === "ready"; + readyCheckInProgress = isReadyCheck; checkPromise = updater.checkForUpdates() .then(async (result) => { const updateInfo = isUpdateCheckResultLike(result) ? result.updateInfo : undefined; @@ -1123,15 +1141,8 @@ export function createAutoUpdateService({ } }) .catch((error) => { - // Same reasoning as onError: a metadata-only refresh must leave the - // staged update exactly as it found it, however it fails. - if (readyMetadataRefreshInProgress) { - logger.warn("autoUpdate.metadata_refresh_failed", { - message: formatErrorMessage(error), - readyVersion: snapshot.version, - }); - return; - } + // Same ordering as onError: the pre-install refresh owns the failure + // first, then the guard that keeps a staged update intact. if (readyRefreshInProgress) { readyRefreshFailure.current = { error, @@ -1139,6 +1150,7 @@ export function createAutoUpdateService({ }; return; } + if (preserveStagedUpdateOnCheckFailure(error)) return; // electron-updater normally emits `error` as well as rejecting. Keep // this fallback so synchronous filesystem failures cannot disappear. if (snapshot.status !== "error") { @@ -1154,25 +1166,25 @@ export function createAutoUpdateService({ .finally(() => { checkPromise = null; preservedDownloadRetry = null; - readyMetadataRefreshInProgress = false; + readyCheckInProgress = false; }); await checkPromise; } /** - * `userInitiated` is what separates the Settings button from the startup and - * periodic timers. The automatic checks stay out of the way of an update that - * is already downloaded and waiting for a restart, but a person pressing - * "Check for updates" is asking a question, and answering it with an early - * return is indistinguishable from the button being broken — the version on - * screen just stays at whatever the last real check found. - * - * The `ready` branch of the check still returns before downloading, so this - * refreshes the newest known version without disturbing the staged download. + * Every entry point behaves the same way, whether it is the Settings button, + * the startup and periodic timers, the `ade update` CLI, or an ADE action. + * An update that is already staged does not stop the check: a strictly newer + * release supersedes the staged one and downloads in its place, a same or + * older release is ignored, and a check that fails leaves the staged update + * untouched. `userInitiated` is recorded for the logs only. */ function checkForUpdates(options: { userInitiated?: boolean } = {}): void { - const userInitiated = options.userInitiated === true; - void runUpdateCheck({ allowReady: userInitiated, metadataOnlyWhenReady: userInitiated }); + logger.info("autoUpdate.check_requested", { + userInitiated: options.userInitiated === true, + status: snapshot.status, + }); + void runUpdateCheck(); } async function refreshReadyUpdateBeforeInstall(): Promise { @@ -1182,7 +1194,7 @@ export function createAutoUpdateService({ readyRefreshInProgress = true; readyRefreshFailure.current = null; try { - await runUpdateCheck({ allowReady: true }); + await runUpdateCheck(); } finally { readyRefreshInProgress = false; } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d1f18ab3a..5aed3ee9f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1040,7 +1040,7 @@ Most services described here live under `apps/desktop/src/main/services/ | `state/` | `kvDb.ts`, `crsqliteExtension.ts`, `dbMaintenanceApi.ts`, `globalState.ts`, `projectState.ts`, `onConflictAudit.ts` | SQLite schema + open (WAL + `synchronous = NORMAL`), CRR extension loader, global state file, per-project state init. The desktop's Electron user-data `ade-state.json` holds machine-local shell state, including `AutoUpdatePreferences`; missing or malformed preference fields normalize to automatic installation off and idle-only safety on. `kvDb` also attaches the optional `maintenance` (`DbMaintenanceApi`) handle — retention prunes, zero-peers-only cr-sqlite compaction, and fragmentation-gated vacuum — whose interface and shared retention constants live in `dbMaintenanceApi.ts` and are invoked by the storage doctor. `globalState.upsertRecentProject` accepts `preserveRecentOrder` so reactivating an already-known project (by app focus, deep link, etc.) refreshes its `lastOpenedAt` in place instead of jumping it to the front of the recents list. Recent projects use stable keys: local rows are keyed by absolute root path, remote rows by `remote::`, so a remote path string never collides with a local project. Pinned rows are retained above normal recency ordering and survive beyond the cap. `model_picker_favorites` and `model_picker_recents` are per-project CRR tables shared by desktop, TUI, and iOS; they are primary-key-only so CRR can convert them, with the recents cap enforced in `modelPickerStore.ts`. `AdeDb.sync.discardUnpublishedChangesForTables(tableNames)` lets a service clear local CRR state for specific tables without leaking those clears to sync peers — it records the cleared tables and `through_db_version` in the local-only `local_crr_change_suppressions` table, and `exportChangesSince` filters local-site rows for those tables at or below that version on the way out. The local-only excluded set (still kept out of replication) includes that suppression table itself, the snapshot caches, `local_worktree_residual_cleanups`, `pr_auto_link_ignores`, `pull_request_ai_summaries`, and `runtime_processes`. `crsql_changes` DELETE statements run through a helper that swallows the read-only-table error the cr-sqlite extension raises when a CRR-managed table is wiped, with a `db.crr_changes_cleanup_skipped` warn log instead of failing the migration. | | `sync/` | `syncService.ts`, `syncHostService.ts`, `syncPeerService.ts`, `syncRemoteCommandService.ts`, `syncProtocol.ts`, `deviceRegistryService.ts`, `syncPairingStore.ts` | **Thin delegation to the ADE runtime's sync service.** The authoritative sync service now lives in `apps/ade-cli/src/services/sync/`; the desktop main-process instances default to a non-host viewer role for legacy state and tests. The old in-process host is disabled unless `ADE_ENABLE_DESKTOP_SYNC_HOST=1` (diagnostics only). Wire formats — WebSocket envelope, remote command routing, device registry, pairing secrets — are the same across both implementations. Viewer joins clear the local `devices` + `sync_cluster_state` rows and then call `db.sync.discardUnpublishedChangesForTables(["devices", "sync_cluster_state"])` so the resulting DELETE rows do not leak back to other peers; the peer client follows up with `syncPeerService.acknowledgeLocalDbVersion()` to advance the outbound cursor past the suppressed range. | | `tests/` | `testService.ts` | Test-suite execution + run history. | -| `updates/` | `autoUpdateService.ts`, `autoUpdateVersions.ts` | Electron auto-update wrapper around `electron-updater`. Owns the renderer-visible `AutoUpdateSnapshot` (`idle \| checking \| downloading \| ready \| installing \| error`, plus `currentVersion` / `latestKnownVersion` for the truthful-version surfaces), uses `compareUpdateVersions` (the SemVer-aware comparator in `autoUpdateVersions.ts`) to dedupe / supersede staged installers and to reconcile `pendingInstallUpdate` against the running version on next boot. Packaged builds schedule startup/periodic checks and downloads; source/dev launches construct the service without auto-check timers so missing `app-update.yml` never surfaces as a renderer error. ADE manually starts downloads after a cache-volume capacity preflight, checks the installed-app volume again before staging, classifies disk/quota/network/verification/permission/installer failures in the shared snapshot, preserves verified downloads when safe, and bounds the native installer handoff with a watchdog. A `ready` snapshot is not treated as proof the ZIP/EXE still exists: if the updater cache or `downloadedFile` is gone, the service re-downloads before uninstalling the runtime or calling native `quitAndInstall`, and it retries once when Squirrel reports a vanished-archive `network connection was lost`. The install is transactional: `quitAndInstall()` re-checks the staged version, and a consent that aborts before the native updater takes over lands in `snapshot.parked` (a typed `AutoUpdateInstallAbortReason`) so the exceptional shell banner offers a retry instead of silently losing the update; ordinary ready state remains in the top-right control. Restarting automatically is a separate machine-local `AutoUpdatePreferences` policy and defaults off. When enabled, its default-on idle safety waits for no active agent turns or work sessions (`RuntimeActivitySummary.idle`) before the grace period and renderer-visible countdown (`autoApplyPending`); users can opt into starting the countdown immediately instead. Cancel suppresses the next countdown (`autoApplySuppressedUntil`), disabling the preference clears it, and `ADE_DISABLE_AUTO_UPDATE_APPLY=1` is the process-level kill switch. `autoUpdateVersions.ts` also builds the changelog (`buildReleaseNotesUrl`) and GitHub release (`buildGithubReleaseUrl`) links. See [desktop auto-update disk-space behavior](./features/onboarding-and-settings/desktop-auto-update.md). | +| `updates/` | `autoUpdateService.ts`, `autoUpdateVersions.ts` | Electron auto-update wrapper around `electron-updater`. Owns the renderer-visible `AutoUpdateSnapshot` (`idle \| checking \| downloading \| ready \| installing \| error`, plus `currentVersion` / `latestKnownVersion` for the truthful-version surfaces), uses `compareUpdateVersions` (the SemVer-aware comparator in `autoUpdateVersions.ts`) to dedupe / supersede staged installers and to reconcile `pendingInstallUpdate` against the running version on next boot. A staged (`ready`) update does not pause those checks (an in-flight `quitAndInstall()` is the exception, because status stays `ready` until the native handoff): a strictly newer feed answer wipes the cached installer and downloads in its place, a same-or-older answer is ignored, and a failed check leaves the staged update untouched. Packaged builds schedule startup/periodic checks and downloads; source/dev launches construct the service without auto-check timers so missing `app-update.yml` never surfaces as a renderer error. ADE manually starts downloads after a cache-volume capacity preflight, checks the installed-app volume again before staging, classifies disk/quota/network/verification/permission/installer failures in the shared snapshot, preserves verified downloads when safe, and bounds the native installer handoff with a watchdog. A `ready` snapshot is not treated as proof the ZIP/EXE still exists: if the updater cache or `downloadedFile` is gone, the service re-downloads before uninstalling the runtime or calling native `quitAndInstall`, and it retries once when Squirrel reports a vanished-archive `network connection was lost`. The install is transactional: `quitAndInstall()` re-checks the staged version, and a consent that aborts before the native updater takes over lands in `snapshot.parked` (a typed `AutoUpdateInstallAbortReason`) so the exceptional shell banner offers a retry instead of silently losing the update; ordinary ready state remains in the top-right control. Restarting automatically is a separate machine-local `AutoUpdatePreferences` policy and defaults off. When enabled, its default-on idle safety waits for no active agent turns or work sessions (`RuntimeActivitySummary.idle`) before the grace period and renderer-visible countdown (`autoApplyPending`); users can opt into starting the countdown immediately instead. Cancel suppresses the next countdown (`autoApplySuppressedUntil`), disabling the preference clears it, and `ADE_DISABLE_AUTO_UPDATE_APPLY=1` is the process-level kill switch. `autoUpdateVersions.ts` also builds the changelog (`buildReleaseNotesUrl`) and GitHub release (`buildGithubReleaseUrl`) links. See [desktop auto-update disk-space behavior](./features/onboarding-and-settings/desktop-auto-update.md). | | `storage/` | `diskPressure.ts`, `volume.ts`, `storageInsightsService.ts`, `historyCompression.ts`, `storageLedger.ts`, `storageDbBreakdown.ts`, `storageMaintenanceJournal.ts` | Disk-full/recovery hardening + the storage doctor. `diskPressure` samples all ADE storage roots, classifies pressure with recovery hysteresis, and gates write-producing operation classes via `canPerform(kind)` (enforced at each start boundary in `agentChatService` / `ptyService` and the compressor). `storageInsightsService` builds the categorized Settings > Storage snapshot and preview-confirmed, link-safe cleanup, and runs the scheduled **storage doctor** maintenance sweep (`runMaintenanceNow` + post-boot/daily timers) that compresses history, reaps safe staging/backups/iOS build data, and invokes the kvDb DB-maintenance hooks. `storageLedger` is the declared bounding policy for every table/directory (with a CI coverage cross-check against `ADE_LAYOUT_DEFINITIONS`); `storageDbBreakdown` maps `dbstat` rows into the project-database breakdown; `storageMaintenanceJournal` reads/writes the 30-run doctor journal. `historyCompression` losslessly gzip-compresses inactive old transcripts/logs after byte-identity verification and exposes the transparent `.gz` read/reinflate helpers. Constructed in both `main.ts` and the `ade` runtime `bootstrap.ts`. See [features/storage-and-recovery/README.md](./features/storage-and-recovery/README.md). | | `usage/` | `usageTrackingService.ts`, `providerQuotaParsers.ts`, `usageStatsStore.ts`, `usageLedgerWorkerClient.ts`, `budgetCapService.ts`, `ledgers/localUsageLedgers.ts`, `usagePricing.ts`, `githubActivityStats.ts`, `accountUsageRollup.ts`, `accountUsageRollupStore.ts`, `accountUsageSource.ts`, `accountUsageLiveRefresh.ts` | Live provider quota/cost accounting, budget enforcement, and retrospective activity stats. `usageTrackingService.ts` owns polling, pacing, provider/GitHub cache orchestration, and `getAdeUsageStats`; `providerQuotaParsers.ts` normalizes Claude and Codex quota payload variants and classifies Codex windows by advertised duration rather than assuming the provider's primary/secondary positions. The stats read returns cached expensive sources plus live project-DB aggregates immediately, marks the result `refreshing` when stale, and revalidates provider ledgers / GitHub in the background. Expensive local-ledger aggregation runs through `usageLedgerWorkerClient.ts` in a separate process so it cannot block terminal input, project switching, or sync; packaged desktop/CLI builds ship a sidecar while the static runtime uses the equivalent embedded entrypoint. The worker streams NDJSON — a roster header then one line per provider as that provider finishes — so a timeout yields the providers that did land instead of discarding eight finished scans along with the ninth, and every budget in front of it (`USAGE_REFRESH_HISTORY_TIMEOUT_MS` for renderer IPC, `USAGE_REFRESH_HISTORY_REMOTE_TRANSPORT_TIMEOUT_MS` for the remote JSON-RPC leg) is derived from `LEDGER_WORKER_TIMEOUT_MS` so it outlives the worker rather than racing it. `usagePricing.ts` resolves per-model token rates, preferring the maintained public rate list over ADE's static table; `githubActivityStats.ts` owns the `gh` shell-outs behind the page's commit/PR/code-movement numbers and fails soft. `usageStatsStore.ts` aggregates AI calls, sessions, lanes, code movement, artifacts, automations, workers, streaks, and the local-only cross-client `usage_events` ledger. Local provider scanners live under `usage/ledgers/`. The `account*` modules add the account scope: each machine publishes day × provider × model aggregates (never a transcript record) into the CRR-replicated `usage_machine_rollups` / `usage_machine_rollup_meta`, `accountUsageLiveRefresh.ts` opportunistically pulls fresher rollups from reachable peers over `usage.getUsageRollup`, and `accountUsageSource.ts` counts two machines that read one shared transcript home only once, keyed on a `.ade-usage-source` marker id with digested roots as the fallback. Historical cost/tokens/code merge; live quota windows do not, because provider rate limits are per provider account rather than per machine. Budget caps can match a rule scope while `usd-per-run` evaluates usage records keyed to the active run id. For runtime-backed projects, the machine brain is the sole quota poller and the renderer consumes its pushed snapshot; `main.ts` does not start a competing project-context tracker. Threshold state remains shared at module level for the unbound/local contexts, and `main.ts` adds a final IPC-level dedup gate with a 10-minute TTL per `provider:threshold:resetCycle` key. | | `perf/` | `perfLog.ts`, `perfIpc.ts`, `metricsSampler.ts`, `chatTextProbe.ts`, `aggregator.ts` | Opt-in local performance harness. `ADE_PERF_RUN_ID` opens a JSONL event log, samples Electron process metrics (including main-process event-loop delay and V8 heap size), records IPC durations, accepts renderer perf marks/web-vitals/`streamSmoothness` windows, records `chatTextFlush` events from the assistant-text coalescer, and aggregates each run into `summary.json`. `perfLog.ts` holds the single `PERF_EVENT_KINDS` list that both the `PerfEventKind` type and the `isPerfEventKind` IPC guard derive from, so a new kind cannot be added to one and forgotten in the other. More than one process can append to the same log — Electron main and the `ade` runtime daemon both host chat sessions — so `appendEvent` must stay one `appendFileSync` of one already newline-terminated string (one `O_APPEND` write the kernel will not interleave). | diff --git a/docs/features/onboarding-and-settings/README.md b/docs/features/onboarding-and-settings/README.md index d0e52be70..3f7b62080 100644 --- a/docs/features/onboarding-and-settings/README.md +++ b/docs/features/onboarding-and-settings/README.md @@ -1099,15 +1099,20 @@ banner): periodic update checks, while dev/source launches leave those timers off to avoid surfacing missing-updater-config errors; if the new build is strictly newer, the cached installer dir is wiped and the snapshot - transitions back through `downloading`. `quitAndInstall()` is + transitions back through `downloading`. A staged update never stops a check; + see + [Checking while an update is staged](./desktop-auto-update.md#checking-while-an-update-is-staged) + for what each entry point does with a same, newer, or failed feed answer. + `userInitiated` only labels `autoUpdate.check_requested`; it does not + change whether the check runs. `quitAndInstall()` is transactional and asynchronous: it gates on the current snapshot being `ready`, - re-runs `updater.checkForUpdates()` with `allowReady: true` to + re-runs `updater.checkForUpdates()` to confirm the staged installer is still the latest, and only then flips the snapshot to `installing`, persists the `pendingInstallUpdate` global-state row, and calls - `updater.quitAndInstall(false, true)`. If the refresh check fails, - it surfaces the error, drops the cache, and clears the pending - install. A consent that aborts before the native updater takes over sets + `updater.quitAndInstall(false, true)`. If the refresh check fails, it parks as `refresh_failed`, returns + the snapshot to `ready` on the staged version, and keeps the + archive. A consent that aborts before the native updater takes over sets `snapshot.parked` with a typed `AutoUpdateInstallAbortReason` (`refresh_failed`, `install_preflight_failed`, `prepare_failed`, `prepare_timeout`, `handoff_failed`) so the shell banner can offer a retry. @@ -1730,19 +1735,23 @@ the previous two-way behaviour instead of reporting a state it cannot compute. exists — its settings moved into General + the top-bar Help menu. - **Auto-update install must refresh before quitting.** `quitAndInstall()` deliberately re-runs `updater.checkForUpdates()` - with `allowReady: true` before flipping to `installing`. Skipping + before flipping to `installing`. Skipping that step (e.g. a synchronous quitAndInstall) reintroduces the bug where ADE quits to install a stale download while a strictly newer build is available. Comparison goes through `compareUpdateVersions` — never `===` on the version string — because `v1.2.3` / `1.2.3-rc.1` / `1.2.3` all need consistent ordering on both the pending-install reconcile path and the supersede check. -- **`installing` is a sticky status.** While the snapshot is +- **`installing` is a sticky status; `ready` is not.** While the snapshot is `installing` the service ignores `update-not-available`, `checking-for-update`, and `error`, because the main process is in - the middle of quitAndInstall. New status checks should treat - `ready` and `installing` symmetrically when deciding whether to - cancel or override the staged update. + the middle of quitAndInstall. Checks keep running while status is `ready` + (same-or-older ignored, strictly newer supersedes, failed check leaves the + archive). Do not skip a check just because an update is staged. The one + window that *does* block a check is an already-running `quitAndInstall()`: + status stays `ready` until the native handoff, and a supersede in that + window would delete the archive about to be handed to Squirrel or NSIS. See + [Checking while an update is staged](./desktop-auto-update.md#checking-while-an-update-is-staged). - **A parked install is not a failure.** An aborted consent lands in `snapshot.parked`, not `error` — the download is still staged and the shell banner offers a **Restart now** retry. Keep parked distinct from the disk / diff --git a/docs/features/onboarding-and-settings/desktop-auto-update.md b/docs/features/onboarding-and-settings/desktop-auto-update.md index 4ec4f6b5b..d909febcf 100644 --- a/docs/features/onboarding-and-settings/desktop-auto-update.md +++ b/docs/features/onboarding-and-settings/desktop-auto-update.md @@ -34,7 +34,8 @@ differential cache copy — Squirrel pipes the pending ZIP recorded as service or calling native `quitAndInstall`, whenever the recorded `downloadedFile` or updater-cache ZIP/EXE is gone. - Re-downloads on the periodic ready check so a vanished cache does not sit - on the Install button until the user clicks it. + on the Install button until the user clicks it, and skips the feed check + for that cycle. - Treats install-phase `The network connection was lost` / `Cannot pipe` / `ENOENT` as a vanished local archive: restore the ZIP (which recreates the loopback server) and retry native handoff once. A second failure parks as @@ -213,6 +214,43 @@ bytes, and re-downloading the whole release on every retry is pure cost. A second consecutive failure on the same version stops trusting the archive and clears the updater cache. +## Checking while an update is staged + +A staged update does not pause update checks. The startup timer, the periodic +timer, the Settings **Check for updates** button, `ade update`, and the +`update.checkForUpdates` ADE action all reach the same code, and it behaves the +same way for all of them while the status is `ready`: + +| Feed answer | Result | +| --- | --- | +| Same or older than the staged version | Ignored (`autoUpdate.update_available_ignored`). The status, the staged version, and the cached archive do not change; `latestKnownVersion` still records what the feed reported. | +| Strictly newer | Supersedes. The recorded `downloadedFile` is dropped, the updater cache is wiped with reason `superseded_ready_update`, any auto-apply countdown for the old version is cancelled, and the snapshot runs `checking` → `downloading` → `ready` on the new version. The countdown re-arms on the new `ready`. | +| The check fails | Nothing changes. Both the `error` event and a rejected `checkForUpdates()` return early while the status is still `ready`, logging `autoUpdate.ready_check_failed`. | + +Every entry point first logs `autoUpdate.check_requested` with the current +status and a `userInitiated` label, so an operator can tell whether a check was +requested at all and what state it found. The flag does not change whether the +check runs. + +This is what stops the top-right pill from offering a release the feed has +already replaced. + +A failure *after* a supersede has started is an ordinary download failure: the +status is no longer `ready`, so it takes the normal error path. The old archive +is already gone at that point, which matches what a relaunch would have done. + +When the staged archive has vanished, that cycle restores the archive and skips +the feed check, so one cycle never runs a restore and a check at the same time. + +An install that is already running blocks the check entirely. The status stays +`ready` for the whole `quitAndInstall()` transaction, across the +`beforeQuitAndInstall` service uninstall, so a check started in that window +could supersede and delete the archive the install is about to hand to +Squirrel or NSIS. The pre-install refresh inside that transaction is the one +exception, and it keeps its own failure handling: a feed failure there aborts +the install with `parked.reason === "refresh_failed"` and returns the snapshot +to `ready` on the staged version. + ## Truthful version surfaces Every version surface reads from one shared snapshot so they can never disagree @@ -232,7 +270,7 @@ user sees the version they will get after the next restart rather than a stale ## Transactional install and exceptional recovery banners `quitAndInstall()` is transactional. Before flipping the snapshot to -`installing` it re-runs `updater.checkForUpdates({ allowReady: true })` to +`installing` it re-runs `updater.checkForUpdates()` to confirm the staged installer is still the latest, verifies the staged ZIP or NSIS installer is still on disk (and re-downloads it if macOS or Windows removed it), and only then uninstalls the background service, persists