diff --git a/apps/code/src/main/services/app-lifecycle/service.test.ts b/apps/code/src/main/services/app-lifecycle/service.test.ts index 9d90e2870..ff200c023 100644 --- a/apps/code/src/main/services/app-lifecycle/service.test.ts +++ b/apps/code/src/main/services/app-lifecycle/service.test.ts @@ -91,6 +91,12 @@ describe("AppLifecycleService", () => { service.setQuittingForUpdate(); expect(service.isQuittingForUpdate).toBe(true); }); + + it("returns false after clearQuittingForUpdate is called", () => { + service.setQuittingForUpdate(); + service.clearQuittingForUpdate(); + expect(service.isQuittingForUpdate).toBe(false); + }); }); describe("isShuttingDown", () => { diff --git a/apps/code/src/main/services/app-lifecycle/service.ts b/apps/code/src/main/services/app-lifecycle/service.ts index 53f9c4f1d..18dcc9f9c 100644 --- a/apps/code/src/main/services/app-lifecycle/service.ts +++ b/apps/code/src/main/services/app-lifecycle/service.ts @@ -38,6 +38,10 @@ export class AppLifecycleService { this._isQuittingForUpdate = true; } + clearQuittingForUpdate(): void { + this._isQuittingForUpdate = false; + } + /** * Immediately kills the process. Used when shutdown is stuck or re-entrant. */ diff --git a/apps/code/src/main/services/updates/schemas.ts b/apps/code/src/main/services/updates/schemas.ts index dbdef957a..b4dd97394 100644 --- a/apps/code/src/main/services/updates/schemas.ts +++ b/apps/code/src/main/services/updates/schemas.ts @@ -13,6 +13,16 @@ export const checkForUpdatesOutput = z.object({ errorCode: checkErrorCode.optional(), }); +export const updatesStatusOutput = z.object({ + checking: z.boolean(), + downloading: z.boolean().optional(), + upToDate: z.boolean().optional(), + updateReady: z.boolean().optional(), + installing: z.boolean().optional(), + version: z.string().optional(), + error: z.string().optional(), +}); + export const installUpdateOutput = z.object({ installed: z.boolean(), }); @@ -28,14 +38,7 @@ export const UpdatesEvent = { CheckFromMenu: "check-from-menu", } as const; -export type UpdatesStatusPayload = { - checking: boolean; - downloading?: boolean; - upToDate?: boolean; - updateReady?: boolean; - version?: string; - error?: string; -}; +export type UpdatesStatusPayload = z.infer; export type UpdateReadyPayload = { version: string | null; diff --git a/apps/code/src/main/services/updates/service.test.ts b/apps/code/src/main/services/updates/service.test.ts index a91bae898..f21cbd874 100644 --- a/apps/code/src/main/services/updates/service.test.ts +++ b/apps/code/src/main/services/updates/service.test.ts @@ -8,6 +8,7 @@ const { mockAppMeta, mockMainWindow, mockLifecycleService, + mockLog, updaterHandlers, } = vi.hoisted(() => { const updaterHandlers: { @@ -79,18 +80,20 @@ const { shutdown: vi.fn(() => Promise.resolve()), shutdownWithoutContainer: vi.fn(() => Promise.resolve()), setQuittingForUpdate: vi.fn(), + clearQuittingForUpdate: vi.fn(), + }, + mockLog: { + info: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), }, }; }); vi.mock("../../utils/logger.js", () => ({ logger: { - scope: () => ({ - info: vi.fn(), - error: vi.fn(), - warn: vi.fn(), - debug: vi.fn(), - }), + scope: () => mockLog, }, })); @@ -134,6 +137,10 @@ describe("UpdatesService", () => { mockAppMeta.isProduction = true; mockAppMeta.version = "1.0.0"; mockUpdater.isSupported.mockReturnValue(true); + mockUpdater.quitAndInstall.mockImplementation(() => undefined); + mockLifecycleService.shutdownWithoutContainer.mockImplementation(() => + Promise.resolve(), + ); mockAppLifecycle.whenReady.mockResolvedValue(undefined); // Set default platform to darwin (macOS) @@ -428,11 +435,9 @@ describe("UpdatesService", () => { // Verify setQuittingForUpdate is called first expect(mockLifecycleService.setQuittingForUpdate).toHaveBeenCalled(); - // Verify shutdownWithoutContainer is called (not full shutdown) expect(mockLifecycleService.shutdownWithoutContainer).toHaveBeenCalled(); expect(mockLifecycleService.shutdown).not.toHaveBeenCalled(); - // Verify quitAndInstall is called after cleanup expect(mockUpdater.quitAndInstall).toHaveBeenCalled(); // Verify order: setQuittingForUpdate -> shutdownWithoutContainer -> quitAndInstall @@ -448,6 +453,29 @@ describe("UpdatesService", () => { expect(cleanupOrder).toBeLessThan(quitAndInstallOrder); }); + it("continues to quitAndInstall if partial shutdown times out", async () => { + await initializeService(service); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + + mockLifecycleService.shutdownWithoutContainer.mockReturnValue( + new Promise(() => {}), + ); + + const resultPromise = service.installUpdate(); + await vi.advanceTimersByTimeAsync(3000); + + await expect(resultPromise).resolves.toEqual({ installed: true }); + expect(mockUpdater.quitAndInstall).toHaveBeenCalled(); + expect(mockLog.warn).toHaveBeenCalledWith( + "Partial shutdown timed out before update install", + expect.objectContaining({ + timeoutMs: 3000, + downloadedVersion: "v2.0.0", + }), + ); + }); + it("returns false if quitAndInstall throws", async () => { await initializeService(service); @@ -467,6 +495,70 @@ describe("UpdatesService", () => { const result = await resultPromise; expect(result).toEqual({ installed: false }); }); + + it("clears the quitting-for-update lifecycle flag when install handoff fails", async () => { + await initializeService(service); + updaterHandlers.updateDownloaded?.("v2.0.0"); + + mockUpdater.quitAndInstall.mockImplementation(() => { + throw new Error("Failed to install"); + }); + + await service.installUpdate(); + + expect(mockLifecycleService.clearQuittingForUpdate).toHaveBeenCalled(); + const setOrder = + mockLifecycleService.setQuittingForUpdate.mock.invocationCallOrder[0]; + const clearOrder = + mockLifecycleService.clearQuittingForUpdate.mock.invocationCallOrder[0]; + expect(setOrder).toBeLessThan(clearOrder); + }); + + it("rolls back to a re-installable ready state when install handoff fails", async () => { + await initializeService(service); + updaterHandlers.updateDownloaded?.("v2.0.0"); + + mockUpdater.quitAndInstall.mockImplementation(() => { + throw new Error("Failed to install"); + }); + + const statusHandler = vi.fn(); + service.on(UpdatesEvent.Status, statusHandler); + + const first = await service.installUpdate(); + expect(first).toEqual({ installed: false }); + expect(service.hasUpdateReady).toBe(true); + expect(statusHandler).toHaveBeenLastCalledWith({ + checking: false, + updateReady: true, + installing: false, + version: "v2.0.0", + }); + + mockUpdater.quitAndInstall.mockImplementationOnce(() => undefined); + const second = await service.installUpdate(); + expect(second).toEqual({ installed: true }); + }); + + it("is idempotent when install is already in progress", async () => { + await initializeService(service); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + + await expect(service.installUpdate()).resolves.toEqual({ + installed: true, + }); + expect(mockUpdater.quitAndInstall).toHaveBeenCalledTimes(1); + + await expect(service.installUpdate()).resolves.toEqual({ + installed: true, + }); + expect(mockUpdater.quitAndInstall).toHaveBeenCalledTimes(1); + expect(mockLog.warn).not.toHaveBeenCalledWith( + "installUpdate called but no update is ready", + expect.anything(), + ); + }); }); describe("triggerMenuCheck", () => { @@ -515,7 +607,7 @@ describe("UpdatesService", () => { }); }); - it("shows update-ready notification instead of up-to-date when update is already downloaded", () => { + it("ignores later update events once an update is already downloaded", () => { // Simulate update already downloaded const downloadedHandler = updaterHandlers.updateDownloaded; if (downloadedHandler) { @@ -527,24 +619,22 @@ describe("UpdatesService", () => { service.on(UpdatesEvent.Status, statusHandler); service.on(UpdatesEvent.Ready, readyHandler); - // Start a periodic re-check + mockUpdater.check.mockClear(); + + // Periodic checks should be suppressed once an update is staged. service.checkForUpdates("periodic"); - statusHandler.mockClear(); + expect(mockUpdater.check).not.toHaveBeenCalled(); - // Server says no new update available const notAvailableHandler = updaterHandlers.noUpdate; if (notAvailableHandler) { notAvailableHandler(); } - // Should emit checking: false (not upToDate) - expect(statusHandler).toHaveBeenCalledWith({ checking: false }); + expect(statusHandler).not.toHaveBeenCalledWith({ checking: false }); expect(statusHandler).not.toHaveBeenCalledWith( expect.objectContaining({ upToDate: true }), ); - - // Should re-surface the downloaded update notification - expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); + expect(readyHandler).not.toHaveBeenCalled(); }); it("handles update-downloaded event with version info", () => { @@ -561,6 +651,20 @@ describe("UpdatesService", () => { expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); }); + it("emits a complete staged payload when an update is downloaded", () => { + const statusHandler = vi.fn(); + service.on(UpdatesEvent.Status, statusHandler); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + + expect(statusHandler).toHaveBeenCalledWith({ + checking: false, + updateReady: true, + installing: false, + version: "v2.0.0", + }); + }); + it("handles error event and emits status with error", () => { const statusHandler = vi.fn(); service.on(UpdatesEvent.Status, statusHandler); @@ -606,6 +710,52 @@ describe("UpdatesService", () => { }); }); + describe("status snapshots", () => { + it("returns update-ready status for a staged update", async () => { + await initializeService(service); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + + expect(service.getStatus()).toEqual({ + checking: false, + updateReady: true, + installing: false, + version: "v2.0.0", + }); + }); + + it("flags installing in the staged status payload while install is in flight", async () => { + await initializeService(service); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + mockLifecycleService.shutdownWithoutContainer.mockReturnValue( + new Promise(() => {}), + ); + + void service.installUpdate(); + // Allow the synchronous part of installUpdate to run. + await Promise.resolve(); + + expect(service.getStatus()).toEqual({ + checking: false, + updateReady: true, + installing: true, + version: "v2.0.0", + }); + }); + + it("returns downloading status while an update is downloading", async () => { + await initializeService(service); + + updaterHandlers.updateAvailable?.(); + + expect(service.getStatus()).toEqual({ + checking: true, + downloading: true, + }); + }); + }); + describe("check timeout", () => { beforeEach(async () => { await initializeService(service); @@ -728,10 +878,24 @@ describe("UpdatesService", () => { expect(mockUpdater.check.mock.calls.length).toBe(initialCallCount + 2); }); + + it("stops the periodic interval once an update is staged", async () => { + await initializeService(service); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + + const baselineCallCount = mockUpdater.check.mock.calls.length; + + // The interval would normally fire every hour; with the update staged it + // should be cleared so no further wake-ups occur. + await vi.advanceTimersByTimeAsync(60 * 60 * 1000 * 3); + + expect(mockUpdater.check.mock.calls.length).toBe(baselineCallCount); + }); }); - describe("periodic check re-checks when update already downloaded", () => { - it("re-checks for newer versions on periodic check when update is ready", async () => { + describe("staged update guards", () => { + it("does not re-check on periodic checks when update is ready", async () => { await initializeService(service); // Simulate update downloaded @@ -743,10 +907,10 @@ describe("UpdatesService", () => { // Clear the checkForUpdates calls from initialization mockUpdater.check.mockClear(); - // Periodic check should re-check without resetting existing update state + // Periodic check should not overwrite or refresh the staged update. const result = service.checkForUpdates("periodic"); expect(result).toEqual({ success: true }); - expect(mockUpdater.check).toHaveBeenCalled(); + expect(mockUpdater.check).not.toHaveBeenCalled(); // Update should still be ready (state not reset) expect(service.hasUpdateReady).toBe(true); }); @@ -771,7 +935,7 @@ describe("UpdatesService", () => { expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); }); - it("preserves downloaded update when periodic re-check errors", async () => { + it("preserves downloaded update when later updater errors fire", async () => { await initializeService(service); // Simulate update downloaded @@ -780,10 +944,11 @@ describe("UpdatesService", () => { downloadedHandler("v2.0.0"); } - // Periodic check proceeds + mockUpdater.check.mockClear(); service.checkForUpdates("periodic"); + expect(mockUpdater.check).not.toHaveBeenCalled(); - // Simulate error during re-check + // Simulate a stale updater error after staging. const errorHandler = updaterHandlers.error; if (errorHandler) { errorHandler(new Error("Network error")); @@ -793,7 +958,7 @@ describe("UpdatesService", () => { expect(service.hasUpdateReady).toBe(true); }); - it("does not re-notify when same version is re-downloaded after periodic check", async () => { + it("does not re-notify when same version is re-downloaded after staging", async () => { await initializeService(service); const readyHandler = vi.fn(); @@ -806,8 +971,6 @@ describe("UpdatesService", () => { } expect(readyHandler).toHaveBeenCalledTimes(1); - // Periodic check resets and re-downloads same version - service.checkForUpdates("periodic"); readyHandler.mockClear(); if (downloadedHandler) { @@ -818,53 +981,67 @@ describe("UpdatesService", () => { expect(readyHandler).not.toHaveBeenCalled(); }); - it("returns already_checking when periodic check fires during in-flight check", async () => { + it("does not overwrite staged version when a later download event arrives", async () => { await initializeService(service); + const readyHandler = vi.fn(); + service.on(UpdatesEvent.Ready, readyHandler); + // Simulate update downloaded const downloadedHandler = updaterHandlers.updateDownloaded; if (downloadedHandler) { downloadedHandler("v2.0.0"); } + expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); - // First periodic check starts (sets checkingForUpdates = true) - service.checkForUpdates("periodic"); + readyHandler.mockClear(); - // Second periodic check while first is still in-flight - const result = service.checkForUpdates("periodic"); - expect(result).toEqual({ - success: false, - errorMessage: "Already checking for updates", - errorCode: "already_checking", - }); + if (downloadedHandler) { + downloadedHandler("v3.0.0"); + } + + // User checks should still surface the originally staged update. + service.checkForUpdates("user"); + expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); // Update should still be ready (state not corrupted) expect(service.hasUpdateReady).toBe(true); }); + }); - it("notifies when a newer version is downloaded after periodic check", async () => { - await initializeService(service); - - const readyHandler = vi.fn(); - service.on(UpdatesEvent.Ready, readyHandler); + describe("transition logging", () => { + it("logs state transitions with source and state metadata", () => { + service.checkForUpdates("user"); + + expect(mockLog.info).toHaveBeenCalledWith( + "Update state transition", + expect.objectContaining({ + source: "user", + fromState: "idle", + toState: "checking", + downloadedVersion: null, + skippedBecauseUpdateStaged: false, + }), + ); + }); - // First download of v2.0.0 - const downloadedHandler = updaterHandlers.updateDownloaded; - if (downloadedHandler) { - downloadedHandler("v2.0.0"); - } - expect(readyHandler).toHaveBeenCalledTimes(1); + it("logs skipped checks after an update is staged", async () => { + await initializeService(service); + updaterHandlers.updateDownloaded?.("v2.0.0"); - // Periodic check resets and downloads newer v3.0.0 + mockLog.info.mockClear(); service.checkForUpdates("periodic"); - readyHandler.mockClear(); - - if (downloadedHandler) { - downloadedHandler("v3.0.0"); - } - // Should notify since different version - expect(readyHandler).toHaveBeenCalledWith({ version: "v3.0.0" }); + expect(mockLog.info).toHaveBeenCalledWith( + "Update state transition", + expect.objectContaining({ + source: "periodic", + fromState: "ready", + toState: "ready", + downloadedVersion: "v2.0.0", + skippedBecauseUpdateStaged: true, + }), + ); }); }); diff --git a/apps/code/src/main/services/updates/service.ts b/apps/code/src/main/services/updates/service.ts index fdbec5d1d..76d1c4c50 100644 --- a/apps/code/src/main/services/updates/service.ts +++ b/apps/code/src/main/services/updates/service.ts @@ -4,6 +4,7 @@ import type { IMainWindow } from "@posthog/platform/main-window"; import type { IUpdater } from "@posthog/platform/updater"; import { inject, injectable, postConstruct, preDestroy } from "inversify"; import { MAIN_TOKENS } from "../../di/tokens"; +import { withTimeout } from "../../utils/async"; import { isDevBuild } from "../../utils/env"; import { logger } from "../../utils/logger"; import { TypedEventEmitter } from "../../utils/typed-event-emitter"; @@ -17,6 +18,20 @@ import { } from "./schemas"; type CheckSource = "user" | "periodic"; +type UpdateState = + | "idle" + | "checking" + | "downloading" + | "ready" + | "installing" + | "error"; +type TransitionContext = { + source?: CheckSource; + skippedBecauseUpdateStaged?: boolean; + reason?: string; + incomingVersion?: string | null; + error?: string; +}; const log = logger.scope("updates"); @@ -27,6 +42,7 @@ export class UpdatesService extends TypedEventEmitter { private static readonly REPO_NAME = "code"; private static readonly CHECK_INTERVAL_MS = 60 * 60 * 1000; // 1 hour private static readonly CHECK_TIMEOUT_MS = 60 * 1000; // 1 minute timeout for checks + private static readonly INSTALL_SHUTDOWN_TIMEOUT_MS = 3000; private static readonly DISABLE_ENV_FLAG = "ELECTRON_DISABLE_AUTO_UPDATE"; private static readonly SUPPORTED_PLATFORMS = ["darwin", "win32"]; @@ -45,18 +61,22 @@ export class UpdatesService extends TypedEventEmitter { @inject(MAIN_TOKENS.MainWindow) private mainWindow!: IMainWindow; - private updateReady = false; + private state: UpdateState = "idle"; private pendingNotification = false; - private checkingForUpdates = false; private checkTimeoutId: ReturnType | null = null; private checkIntervalId: ReturnType | null = null; private downloadedVersion: string | null = null; private notifiedVersion: string | null = null; + private lastError: string | null = null; private initialized = false; private unsubscribes: Array<() => void> = []; get hasUpdateReady(): boolean { - return this.updateReady; + return this.isUpdateStaged(); + } + + private isUpdateStaged(): boolean { + return this.state === "ready" || this.state === "installing"; } get isEnabled(): boolean { @@ -95,6 +115,29 @@ export class UpdatesService extends TypedEventEmitter { this.emit(UpdatesEvent.CheckFromMenu, true); } + getStatus(): UpdatesStatusPayload { + if (this.state === "checking") { + return { checking: true }; + } + + if (this.state === "downloading") { + return { checking: true, downloading: true }; + } + + if (this.isUpdateStaged()) { + return this.stagedStatusPayload(); + } + + if (this.state === "error") { + return { + checking: false, + error: this.lastError ?? "Update check failed. Please try again.", + }; + } + + return { checking: false }; + } + checkForUpdates(source: CheckSource = "user"): CheckForUpdatesOutput { if (!this.isEnabled) { const reason = isDevBuild() @@ -103,7 +146,23 @@ export class UpdatesService extends TypedEventEmitter { return { success: false, errorMessage: reason, errorCode: "disabled" }; } - if (this.checkingForUpdates) { + if (this.isUpdateStaged()) { + this.logStateTransition(this.state, { + source, + skippedBecauseUpdateStaged: true, + reason: "check skipped because update is already staged", + }); + + if (source === "user") { + this.pendingNotification = true; + this.flushPendingNotification(); + this.emitStatus(this.stagedStatusPayload()); + } + + return { success: true }; + } + + if (this.state === "checking" || this.state === "downloading") { return { success: false, errorMessage: "Already checking for updates", @@ -111,22 +170,7 @@ export class UpdatesService extends TypedEventEmitter { }; } - if (this.updateReady && source !== "periodic") { - // User check: show the existing downloaded update notification - log.info("Update already downloaded, showing prompt again", { - downloadedVersion: this.downloadedVersion, - }); - this.pendingNotification = true; - this.flushPendingNotification(); - this.emitStatus({ - checking: false, - updateReady: true, - version: this.downloadedVersion ?? undefined, - }); - return { success: true }; - } - - this.checkingForUpdates = true; + this.transitionTo("checking", { source }); this.emitStatus({ checking: true }); this.performCheck(); @@ -134,8 +178,18 @@ export class UpdatesService extends TypedEventEmitter { } async installUpdate(): Promise { - if (!this.updateReady) { - log.warn("installUpdate called but no update is ready"); + if (this.state === "installing") { + this.logStateTransition("installing", { + skippedBecauseUpdateStaged: true, + reason: "install already in progress", + }); + return { installed: true }; + } + + if (this.state !== "ready") { + log.warn("installUpdate called but no update is ready", { + state: this.state, + }); return { installed: false }; } @@ -144,17 +198,29 @@ export class UpdatesService extends TypedEventEmitter { }); try { - // Set the flag FIRST so before-quit handler won't prevent quit + this.transitionTo("installing", { reason: "install requested" }); + this.emitStatus(this.stagedStatusPayload()); this.lifecycleService.setQuittingForUpdate(); - - // Do lightweight cleanup: kill processes, shut down watchers - // Skip container teardown so before-quit handler can still access services - await this.lifecycleService.shutdownWithoutContainer(); - + const cleanupResult = await withTimeout( + this.lifecycleService.shutdownWithoutContainer(), + UpdatesService.INSTALL_SHUTDOWN_TIMEOUT_MS, + ); + if (cleanupResult.result === "timeout") { + log.warn("Partial shutdown timed out before update install", { + timeoutMs: UpdatesService.INSTALL_SHUTDOWN_TIMEOUT_MS, + downloadedVersion: this.downloadedVersion, + }); + } this.updater.quitAndInstall(); return { installed: true }; } catch (error) { log.error("Failed to quit and install update", error); + this.lifecycleService.clearQuittingForUpdate(); + this.transitionTo("ready", { + reason: "install handoff failed", + error: error instanceof Error ? error.message : String(error), + }); + this.emitStatus(this.stagedStatusPayload()); return { installed: false }; } } @@ -183,7 +249,7 @@ export class UpdatesService extends TypedEventEmitter { this.unsubscribes.push( this.updater.onError((error) => this.handleError(error)), - this.updater.onCheckStart(() => this.handleCheckingForUpdate()), + this.updater.onCheckStart(() => log.info("Checking for updates...")), this.updater.onUpdateAvailable(() => this.handleUpdateAvailable()), this.updater.onNoUpdate(() => this.handleNoUpdate()), this.updater.onUpdateDownloaded((releaseName) => @@ -191,27 +257,44 @@ export class UpdatesService extends TypedEventEmitter { ), ); - // Perform initial check (periodic source — not user-initiated) this.checkForUpdates("periodic"); - // Set up periodic checks this.checkIntervalId = setInterval( () => this.checkForUpdates("periodic"), UpdatesService.CHECK_INTERVAL_MS, ); } + private stagedStatusPayload(): UpdatesStatusPayload { + return { + checking: false, + updateReady: true, + installing: this.state === "installing", + version: this.downloadedVersion ?? undefined, + }; + } + private handleError(error: Error): void { this.clearCheckTimeout(); log.error("Auto update error", { message: error.message, stack: error.stack, feedUrl: this.feedUrl, + state: this.state, }); - // Reset checking state on error so user can retry - if (this.checkingForUpdates) { - this.checkingForUpdates = false; + if (this.isUpdateStaged()) { + this.logStateTransition(this.state, { + skippedBecauseUpdateStaged: true, + reason: "updater error ignored because update is staged", + error: error.message, + }); + return; + } + + if (this.state === "checking" || this.state === "downloading") { + this.lastError = error.message; + this.transitionTo("error", { error: error.message }); this.emitStatus({ checking: false, error: error.message, @@ -219,67 +302,80 @@ export class UpdatesService extends TypedEventEmitter { } } - private handleCheckingForUpdate(): void { - log.info("Checking for updates..."); - } - private handleUpdateAvailable(): void { + if (this.isUpdateStaged()) { + log.info( + "Ignoring update-available because an update is already staged", + { + downloadedVersion: this.downloadedVersion, + }, + ); + return; + } + this.clearCheckTimeout(); + this.transitionTo("downloading", { reason: "update available" }); log.info("Update available, downloading..."); - // Keep checkingForUpdates true while downloading this.emitStatus({ checking: true, downloading: true }); } private handleNoUpdate(): void { this.clearCheckTimeout(); - log.info("No updates available", { currentVersion: this.appMeta.version }); - if (this.checkingForUpdates) { - this.checkingForUpdates = false; - if (this.updateReady) { - this.emitStatus({ checking: false }); - this.pendingNotification = true; - this.flushPendingNotification(); - } else { - this.emitStatus({ - checking: false, - upToDate: true, - version: this.appMeta.version, - }); - } + if (this.isUpdateStaged()) { + log.info("Ignoring update-not-available because update is staged", { + downloadedVersion: this.downloadedVersion, + }); + return; + } + + log.info("No updates available", { currentVersion: this.appMeta.version }); + if (this.state === "checking" || this.state === "downloading") { + this.transitionTo("idle", { reason: "no update available" }); + this.emitStatus({ + checking: false, + upToDate: true, + version: this.appMeta.version, + }); } } private handleUpdateDownloaded(releaseName?: string): void { this.clearCheckTimeout(); - const wasChecking = this.checkingForUpdates; - this.checkingForUpdates = false; - this.downloadedVersion = releaseName ?? null; - if (wasChecking) { - this.emitStatus({ checking: false }); + if (this.isUpdateStaged()) { + log.info("Ignoring duplicate update-downloaded event", { + existingVersion: this.downloadedVersion, + incomingVersion: releaseName, + }); + return; } + this.downloadedVersion = releaseName ?? null; + this.transitionTo("ready", { + reason: "update downloaded", + incomingVersion: releaseName ?? null, + }); + this.clearCheckInterval(); + this.emitStatus(this.stagedStatusPayload()); + log.info("Update downloaded, awaiting user confirmation", { currentVersion: this.appMeta.version, downloadedVersion: this.downloadedVersion, }); - this.updateReady = true; - - // Only show notification if this is a different version than already notified if (this.notifiedVersion !== this.downloadedVersion) { this.pendingNotification = true; this.flushPendingNotification(); } else { - log.info("Skipping notification — same version already notified", { + log.info("Skipping notification - same version already notified", { version: this.downloadedVersion, }); } } private flushPendingNotification(): void { - if (this.updateReady && this.pendingNotification) { + if (this.state === "ready" && this.pendingNotification) { log.info("Notifying user that update is ready", { downloadedVersion: this.downloadedVersion, }); @@ -294,18 +390,16 @@ export class UpdatesService extends TypedEventEmitter { } private performCheck(): void { - // Clear any existing timeout this.clearCheckTimeout(); - // Set a timeout to reset the checking state if the check takes too long this.checkTimeoutId = setTimeout(() => { - if (this.checkingForUpdates) { - log.warn("Update check timed out after 60 seconds"); - this.checkingForUpdates = false; - this.emitStatus({ - checking: false, - error: "Update check timed out. Please try again.", - }); + if (this.state === "checking" || this.state === "downloading") { + const timeoutSeconds = UpdatesService.CHECK_TIMEOUT_MS / 1000; + const message = "Update check timed out. Please try again."; + log.warn(`Update check timed out after ${timeoutSeconds} seconds`); + this.lastError = message; + this.transitionTo("error", { error: message }); + this.emitStatus({ checking: false, error: message }); } }, UpdatesService.CHECK_TIMEOUT_MS); @@ -314,7 +408,10 @@ export class UpdatesService extends TypedEventEmitter { } catch (error) { this.clearCheckTimeout(); log.error("Failed to check for updates", error); - this.checkingForUpdates = false; + this.lastError = "Failed to check for updates. Please try again."; + this.transitionTo("error", { + error: error instanceof Error ? error.message : String(error), + }); this.emitStatus({ checking: false, error: "Failed to check for updates. Please try again.", @@ -322,6 +419,33 @@ export class UpdatesService extends TypedEventEmitter { } } + private transitionTo( + state: UpdateState, + context: TransitionContext = {}, + ): void { + this.logStateTransition(state, context); + this.state = state; + if (state !== "error") { + this.lastError = null; + } + } + + private logStateTransition( + toState: UpdateState, + context: TransitionContext = {}, + ): void { + log.info("Update state transition", { + source: context.source, + fromState: this.state, + toState, + downloadedVersion: this.downloadedVersion, + skippedBecauseUpdateStaged: context.skippedBecauseUpdateStaged ?? false, + reason: context.reason, + incomingVersion: context.incomingVersion, + error: context.error, + }); + } + private clearCheckTimeout(): void { if (this.checkTimeoutId) { clearTimeout(this.checkTimeoutId); @@ -329,13 +453,17 @@ export class UpdatesService extends TypedEventEmitter { } } - @preDestroy() - shutdown(): void { - this.clearCheckTimeout(); + private clearCheckInterval(): void { if (this.checkIntervalId) { clearInterval(this.checkIntervalId); this.checkIntervalId = null; } + } + + @preDestroy() + shutdown(): void { + this.clearCheckTimeout(); + this.clearCheckInterval(); for (const unsub of this.unsubscribes) unsub(); this.unsubscribes = []; } diff --git a/apps/code/src/main/trpc/routers/updates.test.ts b/apps/code/src/main/trpc/routers/updates.test.ts new file mode 100644 index 000000000..b36e223d1 --- /dev/null +++ b/apps/code/src/main/trpc/routers/updates.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it, vi } from "vitest"; + +const { mockUpdatesService } = vi.hoisted(() => ({ + mockUpdatesService: { + isEnabled: true, + checkForUpdates: vi.fn(() => ({ success: true })), + getStatus: vi.fn(() => ({ + checking: false, + updateReady: true, + version: "v2.0.0", + })), + installUpdate: vi.fn(() => Promise.resolve({ installed: true })), + toIterable: vi.fn(), + }, +})); + +vi.mock("../../di/container", () => ({ + container: { + get: vi.fn(() => mockUpdatesService), + }, +})); + +import { updatesRouter } from "./updates"; + +describe("updatesRouter", () => { + it("returns the current update status snapshot", async () => { + const caller = updatesRouter.createCaller({}); + + await expect(caller.getStatus()).resolves.toEqual({ + checking: false, + updateReady: true, + version: "v2.0.0", + }); + expect(mockUpdatesService.getStatus).toHaveBeenCalled(); + }); + + it("delegates menu/user checks to the updates service", async () => { + const caller = updatesRouter.createCaller({}); + + await expect(caller.check()).resolves.toEqual({ success: true }); + expect(mockUpdatesService.checkForUpdates).toHaveBeenCalled(); + }); + + it("reports whether updates are enabled", async () => { + const caller = updatesRouter.createCaller({}); + + await expect(caller.isEnabled()).resolves.toEqual({ enabled: true }); + }); + + it("delegates install to the updates service", async () => { + const caller = updatesRouter.createCaller({}); + + await expect(caller.install()).resolves.toEqual({ installed: true }); + expect(mockUpdatesService.installUpdate).toHaveBeenCalled(); + }); +}); diff --git a/apps/code/src/main/trpc/routers/updates.ts b/apps/code/src/main/trpc/routers/updates.ts index 7cabefb5e..6931e3e21 100644 --- a/apps/code/src/main/trpc/routers/updates.ts +++ b/apps/code/src/main/trpc/routers/updates.ts @@ -6,6 +6,7 @@ import { isEnabledOutput, UpdatesEvent, type UpdatesEvents, + updatesStatusOutput, } from "../../services/updates/schemas"; import type { UpdatesService } from "../../services/updates/service"; import { publicProcedure, router } from "../trpc"; @@ -34,6 +35,11 @@ export const updatesRouter = router({ return service.checkForUpdates(); }), + getStatus: publicProcedure.output(updatesStatusOutput).query(() => { + const service = getService(); + return service.getStatus(); + }), + install: publicProcedure.output(installUpdateOutput).mutation(() => { const service = getService(); return service.installUpdate(); diff --git a/apps/code/src/renderer/stores/updateStore.test.ts b/apps/code/src/renderer/stores/updateStore.test.ts new file mode 100644 index 000000000..f556a86c1 --- /dev/null +++ b/apps/code/src/renderer/stores/updateStore.test.ts @@ -0,0 +1,225 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const { + checkMutate, + getStatusQuery, + installMutate, + isEnabledQuery, + subscriptions, + toast, +} = vi.hoisted(() => ({ + checkMutate: vi.fn(), + getStatusQuery: vi.fn(), + installMutate: vi.fn(), + isEnabledQuery: vi.fn(), + subscriptions: { + onStatus: null as + | null + | ((status: { + checking: boolean; + downloading?: boolean; + upToDate?: boolean; + updateReady?: boolean; + version?: string; + error?: string; + }) => void), + onReady: null as null | ((data: { version: string | null }) => void), + onCheckFromMenu: null as null | (() => void), + }, + toast: { + error: vi.fn(), + success: vi.fn(), + }, +})); + +vi.mock("@renderer/trpc/client", () => ({ + trpcClient: { + updates: { + isEnabled: { query: isEnabledQuery }, + getStatus: { query: getStatusQuery }, + check: { mutate: checkMutate }, + install: { mutate: installMutate }, + onStatus: { + subscribe: vi.fn((_input, handlers) => { + subscriptions.onStatus = handlers.onData; + return { unsubscribe: vi.fn() }; + }), + }, + onReady: { + subscribe: vi.fn((_input, handlers) => { + subscriptions.onReady = handlers.onData; + return { unsubscribe: vi.fn() }; + }), + }, + onCheckFromMenu: { + subscribe: vi.fn((_input, handlers) => { + subscriptions.onCheckFromMenu = handlers.onData; + return { unsubscribe: vi.fn() }; + }), + }, + }, + }, +})); + +vi.mock("@utils/logger", () => ({ + logger: { + scope: () => ({ + error: vi.fn(), + }), + }, +})); + +vi.mock("@utils/toast", () => ({ + toast, +})); + +import { initializeUpdateStore, useUpdateStore } from "./updateStore"; + +async function flushPromises(): Promise { + await Promise.resolve(); + await Promise.resolve(); +} + +describe("updateStore", () => { + beforeEach(() => { + vi.clearAllMocks(); + subscriptions.onStatus = null; + subscriptions.onReady = null; + subscriptions.onCheckFromMenu = null; + isEnabledQuery.mockResolvedValue({ enabled: true }); + getStatusQuery.mockResolvedValue({ checking: false }); + checkMutate.mockResolvedValue({ success: true }); + installMutate.mockResolvedValue({ installed: true }); + useUpdateStore.setState({ + status: "idle", + version: null, + isEnabled: false, + menuCheckPending: false, + }); + }); + + it("hydrates an already-ready update from the main status snapshot", async () => { + getStatusQuery.mockResolvedValue({ + checking: false, + updateReady: true, + version: "v2.0.0", + }); + + const dispose = initializeUpdateStore(); + await flushPromises(); + + expect(getStatusQuery).toHaveBeenCalled(); + expect(useUpdateStore.getState()).toMatchObject({ + isEnabled: true, + status: "ready", + version: "v2.0.0", + }); + + dispose(); + }); + + it("surfaces an already-staged update from a menu check replay", async () => { + const dispose = initializeUpdateStore(); + await flushPromises(); + + subscriptions.onCheckFromMenu?.(); + await flushPromises(); + + expect(checkMutate).toHaveBeenCalled(); + + subscriptions.onReady?.({ version: "v2.0.0" }); + expect(useUpdateStore.getState()).toMatchObject({ + status: "ready", + version: "v2.0.0", + }); + + subscriptions.onStatus?.({ checking: false }); + dispose(); + }); + + it("hydrates an installing update so the renderer keeps the restart spinner", async () => { + getStatusQuery.mockResolvedValue({ + checking: false, + updateReady: true, + installing: true, + version: "v2.0.0", + }); + + const dispose = initializeUpdateStore(); + await flushPromises(); + + expect(useUpdateStore.getState()).toMatchObject({ + status: "installing", + version: "v2.0.0", + }); + + dispose(); + }); + + it("does not reset a ready update when a stale upToDate status arrives", async () => { + getStatusQuery.mockResolvedValue({ + checking: false, + updateReady: true, + version: "v2.0.0", + }); + + const dispose = initializeUpdateStore(); + await flushPromises(); + + subscriptions.onStatus?.({ checking: false, upToDate: true }); + + expect(useUpdateStore.getState().status).toBe("ready"); + dispose(); + }); + + it("shows the success toast when a menu check resolves with upToDate", async () => { + const dispose = initializeUpdateStore(); + await flushPromises(); + + subscriptions.onCheckFromMenu?.(); + await flushPromises(); + expect(useUpdateStore.getState().menuCheckPending).toBe(true); + + subscriptions.onStatus?.({ checking: false, upToDate: true }); + + expect(toast.success).toHaveBeenCalledWith("You're on the latest version"); + expect(useUpdateStore.getState().menuCheckPending).toBe(false); + dispose(); + }); + + it("clears the menu-check flag on disabled errors and shows the error toast", async () => { + checkMutate.mockResolvedValue({ + success: false, + errorCode: "disabled", + errorMessage: "Updates only available in packaged builds", + }); + + const dispose = initializeUpdateStore(); + await flushPromises(); + + subscriptions.onCheckFromMenu?.(); + await flushPromises(); + + expect(useUpdateStore.getState().menuCheckPending).toBe(false); + expect(toast.error).toHaveBeenCalledWith( + "Updates only available in packaged builds", + ); + dispose(); + }); + + it("keeps the menu-check flag when an in-flight check is already running", async () => { + checkMutate.mockResolvedValue({ + success: false, + errorCode: "already_checking", + }); + + const dispose = initializeUpdateStore(); + await flushPromises(); + + subscriptions.onCheckFromMenu?.(); + await flushPromises(); + + expect(useUpdateStore.getState().menuCheckPending).toBe(true); + dispose(); + }); +}); diff --git a/apps/code/src/renderer/stores/updateStore.ts b/apps/code/src/renderer/stores/updateStore.ts index 23b589bcf..145b49544 100644 --- a/apps/code/src/renderer/stores/updateStore.ts +++ b/apps/code/src/renderer/stores/updateStore.ts @@ -5,8 +5,6 @@ import { create } from "zustand"; const log = logger.scope("update-store"); -let menuCheckPending = false; - type UpdateStatus = | "idle" | "checking" @@ -14,10 +12,21 @@ type UpdateStatus = | "ready" | "installing"; +interface StatusPayload { + checking: boolean; + downloading?: boolean; + upToDate?: boolean; + updateReady?: boolean; + installing?: boolean; + version?: string; + error?: string; +} + interface UpdateState { status: UpdateStatus; version: string | null; isEnabled: boolean; + menuCheckPending: boolean; installUpdate: () => Promise; checkForUpdates: () => void; @@ -27,6 +36,7 @@ export const useUpdateStore = create()((set, get) => ({ status: "idle", version: null, isEnabled: false, + menuCheckPending: false, installUpdate: async () => { if (get().status === "installing") return; @@ -62,42 +72,44 @@ export function initializeUpdateStore() { log.error("Failed to get update enabled status", { error }); }); + trpcClient.updates.getStatus + .query() + .then((status) => { + applyStatus(status); + }) + .catch((error: unknown) => { + log.error("Failed to get update status", { error }); + }); + const statusSub = trpcClient.updates.onStatus.subscribe(undefined, { onData: (status) => { - if (status.checking && status.downloading) { - useUpdateStore.setState({ status: "downloading" }); - } else if (status.checking) { - useUpdateStore.setState({ status: "checking" }); - } else if (status.upToDate) { - const current = useUpdateStore.getState().status; - if (current === "checking" || current === "downloading") { - useUpdateStore.setState({ status: "idle" }); - } - if (menuCheckPending) { - menuCheckPending = false; + applyStatus(status); + + if (status.upToDate) { + if (useUpdateStore.getState().menuCheckPending) { + useUpdateStore.setState({ menuCheckPending: false }); toast.success("You're on the latest version"); } } else if (status.error) { log.error("Update check failed", { error: status.error }); - const current = useUpdateStore.getState().status; - if (current === "checking" || current === "downloading") { - useUpdateStore.setState({ status: "idle" }); - } - if (menuCheckPending) { - menuCheckPending = false; + if (useUpdateStore.getState().menuCheckPending) { + useUpdateStore.setState({ menuCheckPending: false }); toast.error("Failed to check for updates", { description: status.error, }); } - } else if (status.checking === false && menuCheckPending) { - // Check finished and an update was found (download in progress / ready) — - // the UpdateBanner will surface it, so suppress the menu-check toast. - menuCheckPending = false; + } else if ( + status.checking === false && + useUpdateStore.getState().menuCheckPending + ) { + // Check finished and an update was found (download in progress / ready) + // — the UpdateBanner will surface it, so suppress the menu-check toast. + useUpdateStore.setState({ menuCheckPending: false }); } }, onError: (error) => { log.error("Update status subscription error", { error }); - menuCheckPending = false; + useUpdateStore.setState({ menuCheckPending: false }); }, }); @@ -115,24 +127,24 @@ export function initializeUpdateStore() { const menuCheckSub = trpcClient.updates.onCheckFromMenu.subscribe(undefined, { onData: () => { - menuCheckPending = true; + useUpdateStore.setState({ menuCheckPending: true }); trpcClient.updates.check .mutate() .then((result) => { if (!result.success) { if (result.errorCode === "disabled") { - menuCheckPending = false; + useUpdateStore.setState({ menuCheckPending: false }); toast.error(result.errorMessage ?? "Updates not available"); } else if (result.errorCode !== "already_checking") { // Unknown/future error code — reset the flag so it never gets stuck. - menuCheckPending = false; + useUpdateStore.setState({ menuCheckPending: false }); } // For "already_checking", keep the flag so the in-flight check // surfaces the toast when it resolves. } }) .catch((error: unknown) => { - menuCheckPending = false; + useUpdateStore.setState({ menuCheckPending: false }); log.error("Failed to check for updates", { error }); toast.error("Failed to check for updates"); }); @@ -148,3 +160,38 @@ export function initializeUpdateStore() { menuCheckSub.unsubscribe(); }; } + +function applyStatus(status: StatusPayload): void { + if (status.installing) { + useUpdateStore.setState({ + status: "installing", + version: status.version ?? null, + }); + return; + } + + if (status.updateReady) { + useUpdateStore.setState({ + status: "ready", + version: status.version ?? null, + }); + return; + } + + if (status.checking && status.downloading) { + useUpdateStore.setState({ status: "downloading" }); + return; + } + + if (status.checking) { + useUpdateStore.setState({ status: "checking" }); + return; + } + + if (status.upToDate || status.error) { + const current = useUpdateStore.getState().status; + if (current !== "ready" && current !== "installing") { + useUpdateStore.setState({ status: "idle" }); + } + } +}