From 2107a748e2572873d3077baa841cfccf607a8650 Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Thu, 28 May 2026 18:10:26 -0700 Subject: [PATCH 1/4] refactor(updates): Introduce state machine and guard staged updates --- .../src/main/services/updates/service.test.ts | 90 ++++------ .../code/src/main/services/updates/service.ts | 161 +++++++++++------- 2 files changed, 129 insertions(+), 122 deletions(-) diff --git a/apps/code/src/main/services/updates/service.test.ts b/apps/code/src/main/services/updates/service.test.ts index a91bae898..2d5601d81 100644 --- a/apps/code/src/main/services/updates/service.test.ts +++ b/apps/code/src/main/services/updates/service.test.ts @@ -428,24 +428,21 @@ describe("UpdatesService", () => { // Verify setQuittingForUpdate is called first expect(mockLifecycleService.setQuittingForUpdate).toHaveBeenCalled(); - // Verify shutdownWithoutContainer is called (not full shutdown) - expect(mockLifecycleService.shutdownWithoutContainer).toHaveBeenCalled(); + // Hand off to Electron immediately so shutdown cleanup cannot block install. + expect( + mockLifecycleService.shutdownWithoutContainer, + ).not.toHaveBeenCalled(); expect(mockLifecycleService.shutdown).not.toHaveBeenCalled(); - // Verify quitAndInstall is called after cleanup expect(mockUpdater.quitAndInstall).toHaveBeenCalled(); - // Verify order: setQuittingForUpdate -> shutdownWithoutContainer -> quitAndInstall + // Verify order: setQuittingForUpdate -> quitAndInstall const setQuittingOrder = mockLifecycleService.setQuittingForUpdate.mock.invocationCallOrder[0]; - const cleanupOrder = - mockLifecycleService.shutdownWithoutContainer.mock - .invocationCallOrder[0]; const quitAndInstallOrder = mockUpdater.quitAndInstall.mock.invocationCallOrder[0]; - expect(setQuittingOrder).toBeLessThan(cleanupOrder); - expect(cleanupOrder).toBeLessThan(quitAndInstallOrder); + expect(setQuittingOrder).toBeLessThan(quitAndInstallOrder); }); it("returns false if quitAndInstall throws", async () => { @@ -515,7 +512,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 +524,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", () => { @@ -730,8 +725,8 @@ describe("UpdatesService", () => { }); }); - 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 +738,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 +766,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 +775,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 +789,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 +802,6 @@ describe("UpdatesService", () => { } expect(readyHandler).toHaveBeenCalledTimes(1); - // Periodic check resets and re-downloads same version - service.checkForUpdates("periodic"); readyHandler.mockClear(); if (downloadedHandler) { @@ -818,53 +812,31 @@ describe("UpdatesService", () => { expect(readyHandler).not.toHaveBeenCalled(); }); - it("returns already_checking when periodic check fires during in-flight check", async () => { - await initializeService(service); - - // Simulate update downloaded - const downloadedHandler = updaterHandlers.updateDownloaded; - if (downloadedHandler) { - downloadedHandler("v2.0.0"); - } - - // First periodic check starts (sets checkingForUpdates = true) - service.checkForUpdates("periodic"); - - // 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", - }); - - // 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 () => { + 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); - // First download of v2.0.0 + // Simulate update downloaded const downloadedHandler = updaterHandlers.updateDownloaded; if (downloadedHandler) { downloadedHandler("v2.0.0"); } - expect(readyHandler).toHaveBeenCalledTimes(1); + expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); - // Periodic check resets and downloads newer v3.0.0 - service.checkForUpdates("periodic"); readyHandler.mockClear(); if (downloadedHandler) { downloadedHandler("v3.0.0"); } - // Should notify since different version - expect(readyHandler).toHaveBeenCalledWith({ version: "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); }); }); diff --git a/apps/code/src/main/services/updates/service.ts b/apps/code/src/main/services/updates/service.ts index fdbec5d1d..b5b94afdc 100644 --- a/apps/code/src/main/services/updates/service.ts +++ b/apps/code/src/main/services/updates/service.ts @@ -17,6 +17,13 @@ import { } from "./schemas"; type CheckSource = "user" | "periodic"; +type UpdateState = + | "idle" + | "checking" + | "downloading" + | "ready" + | "installing" + | "error"; const log = logger.scope("updates"); @@ -45,9 +52,8 @@ 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; @@ -56,7 +62,7 @@ export class UpdatesService extends TypedEventEmitter { private unsubscribes: Array<() => void> = []; get hasUpdateReady(): boolean { - return this.updateReady; + return this.state === "ready" || this.state === "installing"; } get isEnabled(): boolean { @@ -103,7 +109,27 @@ export class UpdatesService extends TypedEventEmitter { return { success: false, errorMessage: reason, errorCode: "disabled" }; } - if (this.checkingForUpdates) { + if (this.isUpdateStaged()) { + log.info("Skipping update check because an update is already staged", { + source, + state: this.state, + downloadedVersion: this.downloadedVersion, + }); + + if (source === "user") { + this.pendingNotification = true; + this.flushPendingNotification(); + this.emitStatus({ + checking: false, + updateReady: true, + version: this.downloadedVersion ?? undefined, + }); + } + + return { success: true }; + } + + if (this.state === "checking" || this.state === "downloading") { return { success: false, errorMessage: "Already checking for updates", @@ -111,22 +137,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"); this.emitStatus({ checking: true }); this.performCheck(); @@ -134,8 +145,10 @@ export class UpdatesService extends TypedEventEmitter { } async installUpdate(): Promise { - if (!this.updateReady) { - log.warn("installUpdate called but no update is ready"); + if (this.state !== "ready") { + log.warn("installUpdate called but no update is ready", { + state: this.state, + }); return { installed: false }; } @@ -144,17 +157,18 @@ export class UpdatesService extends TypedEventEmitter { }); try { - // Set the flag FIRST so before-quit handler won't prevent quit + this.transitionTo("installing"); + this.emitStatus({ + checking: false, + updateReady: true, + version: this.downloadedVersion ?? undefined, + }); 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(); - this.updater.quitAndInstall(); return { installed: true }; } catch (error) { log.error("Failed to quit and install update", error); + this.transitionTo("ready"); return { installed: false }; } } @@ -191,27 +205,33 @@ 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 isUpdateStaged(): boolean { + return this.state === "ready" || this.state === "installing"; + } + 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()) { + return; + } + + if (this.state === "checking" || this.state === "downloading") { + this.transitionTo("error"); this.emitStatus({ checking: false, error: error.message, @@ -224,62 +244,75 @@ export class UpdatesService extends TypedEventEmitter { } 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"); 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"); + 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"); + this.emitStatus({ checking: false }); + 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,14 +327,12 @@ 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) { + if (this.state === "checking" || this.state === "downloading") { log.warn("Update check timed out after 60 seconds"); - this.checkingForUpdates = false; + this.transitionTo("error"); this.emitStatus({ checking: false, error: "Update check timed out. Please try again.", @@ -314,7 +345,7 @@ export class UpdatesService extends TypedEventEmitter { } catch (error) { this.clearCheckTimeout(); log.error("Failed to check for updates", error); - this.checkingForUpdates = false; + this.transitionTo("error"); this.emitStatus({ checking: false, error: "Failed to check for updates. Please try again.", @@ -322,6 +353,10 @@ export class UpdatesService extends TypedEventEmitter { } } + private transitionTo(state: UpdateState): void { + this.state = state; + } + private clearCheckTimeout(): void { if (this.checkTimeoutId) { clearTimeout(this.checkTimeoutId); From 32694068cd9b78086e691f77037ff46077de050c Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Thu, 28 May 2026 18:17:18 -0700 Subject: [PATCH 2/4] Harden staged update handling --- .../code/src/main/services/updates/schemas.ts | 18 ++- .../src/main/services/updates/service.test.ts | 96 +++++++++++- .../code/src/main/services/updates/service.ts | 113 ++++++++++++-- .../src/main/trpc/routers/updates.test.ts | 43 ++++++ apps/code/src/main/trpc/routers/updates.ts | 6 + .../src/renderer/stores/updateStore.test.ts | 138 ++++++++++++++++++ apps/code/src/renderer/stores/updateStore.ts | 59 ++++++-- 7 files changed, 433 insertions(+), 40 deletions(-) create mode 100644 apps/code/src/main/trpc/routers/updates.test.ts create mode 100644 apps/code/src/renderer/stores/updateStore.test.ts diff --git a/apps/code/src/main/services/updates/schemas.ts b/apps/code/src/main/services/updates/schemas.ts index dbdef957a..a7542fce0 100644 --- a/apps/code/src/main/services/updates/schemas.ts +++ b/apps/code/src/main/services/updates/schemas.ts @@ -13,6 +13,15 @@ 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(), + version: z.string().optional(), + error: z.string().optional(), +}); + export const installUpdateOutput = z.object({ installed: z.boolean(), }); @@ -28,14 +37,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 2d5601d81..3bebc8b6e 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: { @@ -80,17 +81,18 @@ const { shutdownWithoutContainer: vi.fn(() => Promise.resolve()), setQuittingForUpdate: 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 +136,7 @@ describe("UpdatesService", () => { mockAppMeta.isProduction = true; mockAppMeta.version = "1.0.0"; mockUpdater.isSupported.mockReturnValue(true); + mockUpdater.quitAndInstall.mockImplementation(() => undefined); mockAppLifecycle.whenReady.mockResolvedValue(undefined); // Set default platform to darwin (macOS) @@ -464,6 +467,26 @@ describe("UpdatesService", () => { const result = await resultPromise; expect(result).toEqual({ installed: false }); }); + + 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", () => { @@ -601,6 +624,31 @@ 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, + 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); @@ -840,6 +888,42 @@ describe("UpdatesService", () => { }); }); + 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, + }), + ); + }); + + it("logs skipped checks after an update is staged", async () => { + await initializeService(service); + updaterHandlers.updateDownloaded?.("v2.0.0"); + + mockLog.info.mockClear(); + service.checkForUpdates("periodic"); + + expect(mockLog.info).toHaveBeenCalledWith( + "Update state transition", + expect.objectContaining({ + source: "periodic", + fromState: "ready", + toState: "ready", + downloadedVersion: "v2.0.0", + skippedBecauseUpdateStaged: true, + }), + ); + }); + }); + describe("error handling", () => { it("catches errors during checkForUpdates", async () => { await initializeService(service); diff --git a/apps/code/src/main/services/updates/service.ts b/apps/code/src/main/services/updates/service.ts index b5b94afdc..6229148ce 100644 --- a/apps/code/src/main/services/updates/service.ts +++ b/apps/code/src/main/services/updates/service.ts @@ -24,6 +24,13 @@ type UpdateState = | "ready" | "installing" | "error"; +type TransitionContext = { + source?: CheckSource; + skippedBecauseUpdateStaged?: boolean; + reason?: string; + incomingVersion?: string | null; + error?: string; +}; const log = logger.scope("updates"); @@ -58,6 +65,7 @@ export class UpdatesService extends TypedEventEmitter { 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> = []; @@ -101,6 +109,33 @@ 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 { + checking: false, + updateReady: true, + version: this.downloadedVersion ?? undefined, + }; + } + + 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() @@ -110,10 +145,10 @@ export class UpdatesService extends TypedEventEmitter { } if (this.isUpdateStaged()) { - log.info("Skipping update check because an update is already staged", { + this.logStateTransition(this.state, { source, - state: this.state, - downloadedVersion: this.downloadedVersion, + skippedBecauseUpdateStaged: true, + reason: "check skipped because update is already staged", }); if (source === "user") { @@ -137,7 +172,7 @@ export class UpdatesService extends TypedEventEmitter { }; } - this.transitionTo("checking"); + this.transitionTo("checking", { source }); this.emitStatus({ checking: true }); this.performCheck(); @@ -145,6 +180,17 @@ export class UpdatesService extends TypedEventEmitter { } async installUpdate(): Promise { + if (this.state === "installing") { + log.info("Update install already in progress", { + fromState: this.state, + toState: this.state, + downloadedVersion: this.downloadedVersion, + 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, @@ -157,7 +203,7 @@ export class UpdatesService extends TypedEventEmitter { }); try { - this.transitionTo("installing"); + this.transitionTo("installing", { reason: "install requested" }); this.emitStatus({ checking: false, updateReady: true, @@ -168,7 +214,10 @@ export class UpdatesService extends TypedEventEmitter { return { installed: true }; } catch (error) { log.error("Failed to quit and install update", error); - this.transitionTo("ready"); + this.transitionTo("ready", { + reason: "install handoff failed", + error: error instanceof Error ? error.message : String(error), + }); return { installed: false }; } } @@ -227,11 +276,17 @@ export class UpdatesService extends TypedEventEmitter { }); 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.transitionTo("error"); + this.lastError = error.message; + this.transitionTo("error", { error: error.message }); this.emitStatus({ checking: false, error: error.message, @@ -255,7 +310,7 @@ export class UpdatesService extends TypedEventEmitter { } this.clearCheckTimeout(); - this.transitionTo("downloading"); + this.transitionTo("downloading", { reason: "update available" }); log.info("Update available, downloading..."); this.emitStatus({ checking: true, downloading: true }); } @@ -272,7 +327,7 @@ export class UpdatesService extends TypedEventEmitter { log.info("No updates available", { currentVersion: this.appMeta.version }); if (this.state === "checking" || this.state === "downloading") { - this.transitionTo("idle"); + this.transitionTo("idle", { reason: "no update available" }); this.emitStatus({ checking: false, upToDate: true, @@ -293,7 +348,10 @@ export class UpdatesService extends TypedEventEmitter { } this.downloadedVersion = releaseName ?? null; - this.transitionTo("ready"); + this.transitionTo("ready", { + reason: "update downloaded", + incomingVersion: releaseName ?? null, + }); this.emitStatus({ checking: false }); log.info("Update downloaded, awaiting user confirmation", { @@ -332,7 +390,10 @@ export class UpdatesService extends TypedEventEmitter { this.checkTimeoutId = setTimeout(() => { if (this.state === "checking" || this.state === "downloading") { log.warn("Update check timed out after 60 seconds"); - this.transitionTo("error"); + this.lastError = "Update check timed out. Please try again."; + this.transitionTo("error", { + error: "Update check timed out. Please try again.", + }); this.emitStatus({ checking: false, error: "Update check timed out. Please try again.", @@ -345,7 +406,10 @@ export class UpdatesService extends TypedEventEmitter { } catch (error) { this.clearCheckTimeout(); log.error("Failed to check for updates", error); - this.transitionTo("error"); + 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.", @@ -353,8 +417,31 @@ export class UpdatesService extends TypedEventEmitter { } } - private transitionTo(state: UpdateState): void { + 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 { 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..1726035c9 --- /dev/null +++ b/apps/code/src/main/trpc/routers/updates.test.ts @@ -0,0 +1,43 @@ +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(); + }); +}); 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..f8e6ae6af --- /dev/null +++ b/apps/code/src/renderer/stores/updateStore.test.ts @@ -0,0 +1,138 @@ +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, + }); + }); + + 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(); + }); +}); diff --git a/apps/code/src/renderer/stores/updateStore.ts b/apps/code/src/renderer/stores/updateStore.ts index 23b589bcf..d402212f9 100644 --- a/apps/code/src/renderer/stores/updateStore.ts +++ b/apps/code/src/renderer/stores/updateStore.ts @@ -62,27 +62,26 @@ 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" }); - } + applyStatus(status); + + if (status.upToDate) { if (menuCheckPending) { 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; toast.error("Failed to check for updates", { @@ -148,3 +147,37 @@ export function initializeUpdateStore() { menuCheckSub.unsubscribe(); }; } + +function applyStatus(status: { + checking: boolean; + downloading?: boolean; + upToDate?: boolean; + updateReady?: boolean; + version?: string; + error?: string; +}): void { + 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 === "checking" || current === "downloading") { + useUpdateStore.setState({ status: "idle" }); + } + } +} From 3a36d99c13e7d5f358e7bb2b4c768f691026d22b Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Thu, 28 May 2026 18:28:35 -0700 Subject: [PATCH 3/4] Bound update install cleanup --- .../src/main/services/updates/service.test.ts | 39 ++++++++++++++++--- .../code/src/main/services/updates/service.ts | 12 ++++++ 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/apps/code/src/main/services/updates/service.test.ts b/apps/code/src/main/services/updates/service.test.ts index 3bebc8b6e..a56e58be6 100644 --- a/apps/code/src/main/services/updates/service.test.ts +++ b/apps/code/src/main/services/updates/service.test.ts @@ -137,6 +137,9 @@ describe("UpdatesService", () => { 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) @@ -431,21 +434,45 @@ describe("UpdatesService", () => { // Verify setQuittingForUpdate is called first expect(mockLifecycleService.setQuittingForUpdate).toHaveBeenCalled(); - // Hand off to Electron immediately so shutdown cleanup cannot block install. - expect( - mockLifecycleService.shutdownWithoutContainer, - ).not.toHaveBeenCalled(); + expect(mockLifecycleService.shutdownWithoutContainer).toHaveBeenCalled(); expect(mockLifecycleService.shutdown).not.toHaveBeenCalled(); expect(mockUpdater.quitAndInstall).toHaveBeenCalled(); - // Verify order: setQuittingForUpdate -> quitAndInstall + // Verify order: setQuittingForUpdate -> shutdownWithoutContainer -> quitAndInstall const setQuittingOrder = mockLifecycleService.setQuittingForUpdate.mock.invocationCallOrder[0]; + const cleanupOrder = + mockLifecycleService.shutdownWithoutContainer.mock + .invocationCallOrder[0]; const quitAndInstallOrder = mockUpdater.quitAndInstall.mock.invocationCallOrder[0]; - expect(setQuittingOrder).toBeLessThan(quitAndInstallOrder); + expect(setQuittingOrder).toBeLessThan(cleanupOrder); + 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 () => { diff --git a/apps/code/src/main/services/updates/service.ts b/apps/code/src/main/services/updates/service.ts index 6229148ce..99101b321 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"; @@ -41,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"]; @@ -210,6 +212,16 @@ export class UpdatesService extends TypedEventEmitter { version: this.downloadedVersion ?? undefined, }); this.lifecycleService.setQuittingForUpdate(); + 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) { From fb78347adb2993a13b00dde98cbb5374a2bda4ae Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Thu, 28 May 2026 19:44:29 -0700 Subject: [PATCH 4/4] Address review feedback on staged update handling --- .../services/app-lifecycle/service.test.ts | 6 ++ .../main/services/app-lifecycle/service.ts | 4 + .../code/src/main/services/updates/schemas.ts | 1 + .../src/main/services/updates/service.test.ts | 94 +++++++++++++++++++ .../code/src/main/services/updates/service.ts | 72 +++++++------- .../src/main/trpc/routers/updates.test.ts | 13 +++ .../src/renderer/stores/updateStore.test.ts | 87 +++++++++++++++++ apps/code/src/renderer/stores/updateStore.ts | 62 +++++++----- 8 files changed, 276 insertions(+), 63 deletions(-) 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 a7542fce0..b4dd97394 100644 --- a/apps/code/src/main/services/updates/schemas.ts +++ b/apps/code/src/main/services/updates/schemas.ts @@ -18,6 +18,7 @@ export const updatesStatusOutput = z.object({ downloading: z.boolean().optional(), upToDate: z.boolean().optional(), updateReady: z.boolean().optional(), + installing: z.boolean().optional(), version: z.string().optional(), error: z.string().optional(), }); diff --git a/apps/code/src/main/services/updates/service.test.ts b/apps/code/src/main/services/updates/service.test.ts index a56e58be6..f21cbd874 100644 --- a/apps/code/src/main/services/updates/service.test.ts +++ b/apps/code/src/main/services/updates/service.test.ts @@ -80,6 +80,7 @@ const { shutdown: vi.fn(() => Promise.resolve()), shutdownWithoutContainer: vi.fn(() => Promise.resolve()), setQuittingForUpdate: vi.fn(), + clearQuittingForUpdate: vi.fn(), }, mockLog: { info: vi.fn(), @@ -495,6 +496,50 @@ describe("UpdatesService", () => { 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); @@ -606,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); @@ -660,6 +719,27 @@ describe("UpdatesService", () => { 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", }); }); @@ -798,6 +878,20 @@ 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("staged update guards", () => { diff --git a/apps/code/src/main/services/updates/service.ts b/apps/code/src/main/services/updates/service.ts index 99101b321..76d1c4c50 100644 --- a/apps/code/src/main/services/updates/service.ts +++ b/apps/code/src/main/services/updates/service.ts @@ -72,6 +72,10 @@ export class UpdatesService extends TypedEventEmitter { private unsubscribes: Array<() => void> = []; get hasUpdateReady(): boolean { + return this.isUpdateStaged(); + } + + private isUpdateStaged(): boolean { return this.state === "ready" || this.state === "installing"; } @@ -121,11 +125,7 @@ export class UpdatesService extends TypedEventEmitter { } if (this.isUpdateStaged()) { - return { - checking: false, - updateReady: true, - version: this.downloadedVersion ?? undefined, - }; + return this.stagedStatusPayload(); } if (this.state === "error") { @@ -156,11 +156,7 @@ export class UpdatesService extends TypedEventEmitter { if (source === "user") { this.pendingNotification = true; this.flushPendingNotification(); - this.emitStatus({ - checking: false, - updateReady: true, - version: this.downloadedVersion ?? undefined, - }); + this.emitStatus(this.stagedStatusPayload()); } return { success: true }; @@ -183,10 +179,7 @@ export class UpdatesService extends TypedEventEmitter { async installUpdate(): Promise { if (this.state === "installing") { - log.info("Update install already in progress", { - fromState: this.state, - toState: this.state, - downloadedVersion: this.downloadedVersion, + this.logStateTransition("installing", { skippedBecauseUpdateStaged: true, reason: "install already in progress", }); @@ -206,11 +199,7 @@ export class UpdatesService extends TypedEventEmitter { try { this.transitionTo("installing", { reason: "install requested" }); - this.emitStatus({ - checking: false, - updateReady: true, - version: this.downloadedVersion ?? undefined, - }); + this.emitStatus(this.stagedStatusPayload()); this.lifecycleService.setQuittingForUpdate(); const cleanupResult = await withTimeout( this.lifecycleService.shutdownWithoutContainer(), @@ -226,10 +215,12 @@ export class UpdatesService extends TypedEventEmitter { 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 }; } } @@ -258,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) => @@ -274,8 +265,13 @@ export class UpdatesService extends TypedEventEmitter { ); } - private isUpdateStaged(): boolean { - return this.state === "ready" || this.state === "installing"; + private stagedStatusPayload(): UpdatesStatusPayload { + return { + checking: false, + updateReady: true, + installing: this.state === "installing", + version: this.downloadedVersion ?? undefined, + }; } private handleError(error: Error): void { @@ -306,10 +302,6 @@ export class UpdatesService extends TypedEventEmitter { } } - private handleCheckingForUpdate(): void { - log.info("Checking for updates..."); - } - private handleUpdateAvailable(): void { if (this.isUpdateStaged()) { log.info( @@ -364,7 +356,8 @@ export class UpdatesService extends TypedEventEmitter { reason: "update downloaded", incomingVersion: releaseName ?? null, }); - this.emitStatus({ checking: false }); + this.clearCheckInterval(); + this.emitStatus(this.stagedStatusPayload()); log.info("Update downloaded, awaiting user confirmation", { currentVersion: this.appMeta.version, @@ -401,15 +394,12 @@ export class UpdatesService extends TypedEventEmitter { this.checkTimeoutId = setTimeout(() => { if (this.state === "checking" || this.state === "downloading") { - log.warn("Update check timed out after 60 seconds"); - this.lastError = "Update check timed out. Please try again."; - this.transitionTo("error", { - error: "Update check timed out. Please try again.", - }); - this.emitStatus({ - checking: false, - error: "Update check timed out. Please try again.", - }); + 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); @@ -463,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 index 1726035c9..b36e223d1 100644 --- a/apps/code/src/main/trpc/routers/updates.test.ts +++ b/apps/code/src/main/trpc/routers/updates.test.ts @@ -40,4 +40,17 @@ describe("updatesRouter", () => { 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/renderer/stores/updateStore.test.ts b/apps/code/src/renderer/stores/updateStore.test.ts index f8e6ae6af..f556a86c1 100644 --- a/apps/code/src/renderer/stores/updateStore.test.ts +++ b/apps/code/src/renderer/stores/updateStore.test.ts @@ -94,6 +94,7 @@ describe("updateStore", () => { status: "idle", version: null, isEnabled: false, + menuCheckPending: false, }); }); @@ -135,4 +136,90 @@ describe("updateStore", () => { 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 d402212f9..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; @@ -76,27 +86,30 @@ export function initializeUpdateStore() { applyStatus(status); if (status.upToDate) { - if (menuCheckPending) { - menuCheckPending = false; + 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 }); - 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 }); }, }); @@ -114,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,14 +161,15 @@ export function initializeUpdateStore() { }; } -function applyStatus(status: { - checking: boolean; - downloading?: boolean; - upToDate?: boolean; - updateReady?: boolean; - version?: string; - error?: string; -}): void { +function applyStatus(status: StatusPayload): void { + if (status.installing) { + useUpdateStore.setState({ + status: "installing", + version: status.version ?? null, + }); + return; + } + if (status.updateReady) { useUpdateStore.setState({ status: "ready", @@ -176,7 +190,7 @@ function applyStatus(status: { if (status.upToDate || status.error) { const current = useUpdateStore.getState().status; - if (current === "checking" || current === "downloading") { + if (current !== "ready" && current !== "installing") { useUpdateStore.setState({ status: "idle" }); } }