From 9c1df125491bfab688dd2a399e8e07279201604c Mon Sep 17 00:00:00 2001 From: Thiago Salvatore Date: Wed, 22 Jul 2026 16:05:33 -0300 Subject: [PATCH 1/3] fix(updates): supersede pending updates instead of stepping one per restart The update state machine latched onto the first update it found: once a release was available or downloaded it stopped re-checking the feed (and cleared the periodic interval entirely on download) until the app was installed and relaunched. When several releases were behind, each restart only advanced one version, so users had to restart repeatedly to reach the latest build. Keep the periodic check running while an update is pending and supersede it in place when a strictly-newer release appears: - periodic checks now perform a silent background re-check while an update is available or downloaded (no banner flicker, no status emitted) - a strictly-newer available/downloaded version replaces the pending one (seamlessly re-downloading when auto-download is on) - an install handoff and duplicate/older events are still never disturbed Skipping intermediate versions is safe here: SQLite and zustand-persist migrations both run from current on-disk state to expected state, never keyed on the previous app version, and differential downloads are already disabled. Adds a small semver-ish comparator (versionCompare.ts) with tests. Generated-By: PostHog Code Task-Id: d03c3d60-e63e-4075-89dc-2f0af6c01b01 --- packages/core/src/updates/updates.test.ts | 156 ++++++++++++++---- packages/core/src/updates/updates.ts | 127 +++++++++++--- .../core/src/updates/versionCompare.test.ts | 48 ++++++ packages/core/src/updates/versionCompare.ts | 66 ++++++++ 4 files changed, 348 insertions(+), 49 deletions(-) create mode 100644 packages/core/src/updates/versionCompare.test.ts create mode 100644 packages/core/src/updates/versionCompare.ts diff --git a/packages/core/src/updates/updates.test.ts b/packages/core/src/updates/updates.test.ts index 34fd9f21b6..63bc5c4169 100644 --- a/packages/core/src/updates/updates.test.ts +++ b/packages/core/src/updates/updates.test.ts @@ -533,7 +533,7 @@ describe("UpdatesService", () => { }); }); - it("ignores later update events once an update is already downloaded", () => { + it("re-checks silently once an update is downloaded without disturbing it", () => { // Simulate update already downloaded const downloadedHandler = updaterHandlers.updateDownloaded; if (downloadedHandler) { @@ -547,20 +547,24 @@ describe("UpdatesService", () => { mockUpdater.check.mockClear(); - // Periodic checks should be suppressed once an update is staged. + // A periodic check now re-checks in the background so a newer release can + // supersede the staged one, but it must do so silently. service.checkForUpdates("periodic"); - expect(mockUpdater.check).not.toHaveBeenCalled(); + expect(mockUpdater.check).toHaveBeenCalled(); const notAvailableHandler = updaterHandlers.noUpdate; if (notAvailableHandler) { notAvailableHandler(); } + // Nothing newer shipped, so the staged update is untouched and the UI + // sees no "checking"/"upToDate" flicker. expect(statusHandler).not.toHaveBeenCalledWith({ checking: false }); expect(statusHandler).not.toHaveBeenCalledWith( expect.objectContaining({ upToDate: true }), ); expect(readyHandler).not.toHaveBeenCalled(); + expect(service.hasUpdateReady).toBe(true); }); it("handles update-downloaded event with version info", () => { @@ -723,7 +727,7 @@ describe("UpdatesService", () => { }); describe("available update guards", () => { - it("does not re-check or clear the banner on periodic checks while available", async () => { + it("silently re-checks on periodic checks while available without clearing the banner", async () => { await initializeService(service); updaterHandlers.updateAvailable?.({ @@ -741,8 +745,10 @@ describe("UpdatesService", () => { const result = service.checkForUpdates("periodic"); + // The re-check runs (so a newer version can supersede) but emits no + // status, leaving the existing "available" banner in place. expect(result).toEqual({ success: true }); - expect(mockUpdater.check).not.toHaveBeenCalled(); + expect(mockUpdater.check).toHaveBeenCalled(); expect(statusHandler).not.toHaveBeenCalled(); expect(service.getStatus()).toMatchObject({ available: true, @@ -750,6 +756,45 @@ describe("UpdatesService", () => { }); }); + it("supersedes an available update when a periodic re-check finds a newer one", async () => { + await initializeService(service); + + updaterHandlers.updateAvailable?.({ + version: "v2.0.0", + releaseNotes: "Old notes", + }); + expect(service.getStatus()).toMatchObject({ availableVersion: "v2.0.0" }); + + service.checkForUpdates("periodic"); + updaterHandlers.updateAvailable?.({ + version: "v3.0.0", + releaseNotes: "New notes", + }); + + expect(service.getStatus()).toMatchObject({ + available: true, + availableVersion: "v3.0.0", + releaseNotes: "New notes", + }); + }); + + it("keeps the available version when a re-check reports the same or an older release", async () => { + await initializeService(service); + + updaterHandlers.updateAvailable?.({ + version: "v2.0.0", + releaseNotes: "Notes", + }); + + service.checkForUpdates("periodic"); + updaterHandlers.updateAvailable?.({ + version: "v1.5.0", + releaseNotes: "Older", + }); + + expect(service.getStatus()).toMatchObject({ availableVersion: "v2.0.0" }); + }); + it("starts the download when auto-download is enabled while available", async () => { await initializeService(service); @@ -893,23 +938,46 @@ describe("UpdatesService", () => { expect(mockUpdater.check.mock.calls.length).toBe(initialCallCount + 2); }); - it("stops the periodic interval once an update is staged", async () => { + it("keeps the periodic interval running 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. + // The interval keeps firing so a newer release can supersede the staged + // one in the background instead of forcing a restart-per-version. await vi.advanceTimersByTimeAsync(60 * 60 * 1000 * 3); - expect(mockUpdater.check.mock.calls.length).toBe(baselineCallCount); + expect(mockUpdater.check.mock.calls.length).toBe(baselineCallCount + 3); + }); + + it("supersedes a staged update when a later re-check downloads a newer one", async () => { + await initializeService(service); + service.setAutoDownloadEnabled(true); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + expect(service.getStatus()).toMatchObject({ version: "v2.0.0" }); + + // A background re-check finds a newer release; with auto-download on it + // is fetched and replaces the staged one seamlessly. + service.checkForUpdates("periodic"); + updaterHandlers.updateAvailable?.({ + version: "v3.0.0", + releaseNotes: null, + }); + expect(mockUpdater.download).toHaveBeenCalled(); + + updaterHandlers.updateDownloaded?.("v3.0.0"); + expect(service.getStatus()).toMatchObject({ + updateReady: true, + version: "v3.0.0", + }); }); }); describe("staged update guards", () => { - it("does not re-check on periodic checks when update is ready", async () => { + it("re-checks silently on periodic checks when an update is ready", async () => { await initializeService(service); // Simulate update downloaded @@ -921,10 +989,11 @@ describe("UpdatesService", () => { // Clear the checkForUpdates calls from initialization mockUpdater.check.mockClear(); - // Periodic check should not overwrite or refresh the staged update. + // Periodic check re-checks in the background but leaves the staged update + // untouched unless a newer release comes back. const result = service.checkForUpdates("periodic"); expect(result).toEqual({ success: true }); - expect(mockUpdater.check).not.toHaveBeenCalled(); + expect(mockUpdater.check).toHaveBeenCalled(); // Update should still be ready (state not reset) expect(service.hasUpdateReady).toBe(true); }); @@ -960,7 +1029,7 @@ describe("UpdatesService", () => { mockUpdater.check.mockClear(); service.checkForUpdates("periodic"); - expect(mockUpdater.check).not.toHaveBeenCalled(); + expect(mockUpdater.check).toHaveBeenCalled(); // Simulate a stale updater error after staging. const errorHandler = updaterHandlers.error; @@ -968,7 +1037,7 @@ describe("UpdatesService", () => { errorHandler(new Error("Network error")); } - // Update should still be ready + // A failed background re-check must not discard the staged update. expect(service.hasUpdateReady).toBe(true); }); @@ -995,31 +1064,40 @@ describe("UpdatesService", () => { expect(readyHandler).not.toHaveBeenCalled(); }); - it("does not overwrite staged version when a later download event arrives", async () => { + it("supersedes the staged version when a newer 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"); - } + downloadedHandler?.("v2.0.0"); expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); readyHandler.mockClear(); - if (downloadedHandler) { - downloadedHandler("v3.0.0"); - } + // A strictly newer downloaded build replaces the staged one and re-notifies. + downloadedHandler?.("v3.0.0"); + expect(readyHandler).toHaveBeenCalledWith({ version: "v3.0.0" }); + expect(service.getStatus()).toMatchObject({ + updateReady: true, + version: "v3.0.0", + }); + }); - // User checks should still surface the originally staged update. - service.checkForUpdates("user"); - expect(readyHandler).toHaveBeenCalledWith({ version: "v2.0.0" }); + it("ignores a stale download event that is not newer than the staged version", async () => { + await initializeService(service); - // Update should still be ready (state not corrupted) - expect(service.hasUpdateReady).toBe(true); + const downloadedHandler = updaterHandlers.updateDownloaded; + downloadedHandler?.("v3.0.0"); + + const readyHandler = vi.fn(); + service.on(UpdatesEvent.Ready, readyHandler); + + // An older/equal download arriving late must not downgrade the staged update. + downloadedHandler?.("v2.0.0"); + expect(readyHandler).not.toHaveBeenCalled(); + expect(service.getStatus()).toMatchObject({ version: "v3.0.0" }); }); }); @@ -1039,7 +1117,7 @@ describe("UpdatesService", () => { ); }); - it("logs skipped checks after an update is staged", async () => { + it("logs the background re-check performed after an update is staged", async () => { await initializeService(service); updaterHandlers.updateDownloaded?.("v2.0.0"); @@ -1053,6 +1131,28 @@ describe("UpdatesService", () => { fromState: "ready", toState: "ready", downloadedVersion: "v2.0.0", + reason: "periodic re-check for a newer update while one is pending", + }), + ); + }); + + it("logs skipped checks while an install is in progress", async () => { + await initializeService(service); + updaterHandlers.updateDownloaded?.("v2.0.0"); + mockLifecycleService.shutdownWithoutContainer.mockReturnValue( + new Promise(() => {}), + ); + void service.installUpdate(); + await Promise.resolve(); + + mockLog.info.mockClear(); + service.checkForUpdates("periodic"); + + expect(mockLog.info).toHaveBeenCalledWith( + "Update state transition", + expect.objectContaining({ + source: "periodic", + toState: "installing", skippedBecauseUpdateStaged: true, }), ); diff --git a/packages/core/src/updates/updates.ts b/packages/core/src/updates/updates.ts index 10ea50f090..2f185af9b9 100644 --- a/packages/core/src/updates/updates.ts +++ b/packages/core/src/updates/updates.ts @@ -28,6 +28,7 @@ import { type UpdatesEvents, type UpdatesStatusPayload, } from "./schemas"; +import { isStrictlyNewer } from "./versionCompare"; type CheckSource = "user" | "periodic"; type UpdateState = @@ -191,11 +192,12 @@ export class UpdatesService extends TypedEventEmitter { return { success: false, errorMessage: reason, errorCode: "disabled" }; } - if (this.isUpdateStaged()) { + // An install handoff is already underway — never disturb it. + if (this.state === "installing") { this.logStateTransition(this.state, { source, skippedBecauseUpdateStaged: true, - reason: "check skipped because update is already staged", + reason: "check skipped because an install is in progress", }); if (source === "user") { @@ -207,14 +209,6 @@ export class UpdatesService extends TypedEventEmitter { return { success: true }; } - if (source === "periodic" && this.state === "available") { - this.logStateTransition(this.state, { - source, - reason: "periodic check skipped because an update is already available", - }); - return { success: true }; - } - if (this.state === "checking" || this.state === "downloading") { return { success: false, @@ -223,6 +217,31 @@ export class UpdatesService extends TypedEventEmitter { }; } + // We already have an update available or downloaded. A user-initiated check + // just re-surfaces what we already have so we don't yank the banner away. A + // periodic check quietly looks for a newer release, so the app can supersede + // toward the latest version without the user having to restart once per + // intermediate release. + if (this.state === "available" || this.state === "ready") { + if (source === "user") { + if (this.state === "ready") { + this.pendingNotification = true; + this.flushPendingNotification(); + this.emitStatus(this.stagedStatusPayload()); + } else { + this.emitStatus(this.availableStatusPayload()); + } + return { success: true }; + } + + this.logStateTransition(this.state, { + source, + reason: "periodic re-check for a newer update while one is pending", + }); + this.performSilentCheck(); + return { success: true }; + } + this.transitionTo("checking", { source }); this.emitStatus({ checking: true }); this.performCheck(); @@ -375,17 +394,44 @@ export class UpdatesService extends TypedEventEmitter { } private handleUpdateAvailable(info: UpdateAvailableInfo): void { - if (this.isUpdateStaged()) { - this.log.info( - "Ignoring update-available because an update is already staged", - { - downloadedVersion: this.downloadedVersion, - }, - ); + this.clearCheckTimeout(); + + // Never disturb an install handoff. + if (this.state === "installing") { return; } - this.clearCheckTimeout(); + // If an update is already downloaded, only a strictly newer release should + // displace it; otherwise a background re-check is just re-reporting the + // version we already have staged. + if (this.state === "ready") { + if (!isStrictlyNewer(info.version, this.downloadedVersion)) { + this.log.info( + "Ignoring update-available; not newer than the staged update", + { + stagedVersion: this.downloadedVersion, + incomingVersion: info.version, + }, + ); + return; + } + this.log.info("Newer update available; superseding staged update", { + stagedVersion: this.downloadedVersion, + incomingVersion: info.version, + }); + this.downloadedVersion = null; + } else if (this.state === "available") { + if (!isStrictlyNewer(info.version, this.availableInfo?.version)) { + // Same (or older) version re-reported by a background re-check — leave + // the existing banner untouched. + return; + } + this.log.info("Newer update available; replacing the pending update", { + previousVersion: this.availableInfo?.version, + incomingVersion: info.version, + }); + } + this.availableInfo = info; this.downloadProgress = null; @@ -450,8 +496,22 @@ export class UpdatesService extends TypedEventEmitter { private handleUpdateDownloaded(version?: string): void { this.clearCheckTimeout(); - if (this.isUpdateStaged()) { - this.log.info("Ignoring duplicate update-downloaded event", { + // Never disturb an install handoff. + if (this.state === "installing") { + this.log.info("Ignoring update-downloaded event during install", { + existingVersion: this.downloadedVersion, + incomingVersion: version, + }); + return; + } + + // If we already have a downloaded update, only accept a strictly newer one + // so a duplicate or stale event can't reset the staged version. + if ( + this.state === "ready" && + !isStrictlyNewer(version, this.downloadedVersion) + ) { + this.log.info("Ignoring duplicate or older update-downloaded event", { existingVersion: this.downloadedVersion, incomingVersion: version, }); @@ -463,7 +523,9 @@ export class UpdatesService extends TypedEventEmitter { reason: "update downloaded", incomingVersion: version ?? null, }); - this.clearCheckInterval(); + // The periodic interval intentionally keeps running: if a newer release + // ships while this one is staged, the next re-check supersedes it in place + // instead of forcing a restart-per-version. this.emitStatus(this.stagedStatusPayload()); this.log.info("Update downloaded, awaiting user confirmation", { @@ -526,6 +588,29 @@ export class UpdatesService extends TypedEventEmitter { } } + // A background re-check that leaves the visible state untouched. The update + // event handlers only act on a strictly newer version, so if nothing newer + // has shipped the pending update is left exactly as it was — no banner + // flicker, no "checking" status emitted to the UI. + private performSilentCheck(): void { + this.clearCheckTimeout(); + + this.checkTimeoutId = setTimeout(() => { + // A stalled silent re-check has nothing to surface; just clear itself and + // leave the pending update in place. + this.clearCheckTimeout(); + }, UpdatesService.CHECK_TIMEOUT_MS); + + try { + this.updater.check(); + } catch (error) { + this.clearCheckTimeout(); + this.log.warn("Silent update re-check failed", { + error: error instanceof Error ? error.message : String(error), + }); + } + } + private transitionTo( state: UpdateState, context: TransitionContext = {}, diff --git a/packages/core/src/updates/versionCompare.test.ts b/packages/core/src/updates/versionCompare.test.ts new file mode 100644 index 0000000000..357574933b --- /dev/null +++ b/packages/core/src/updates/versionCompare.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { compareVersions, isStrictlyNewer } from "./versionCompare"; + +describe("compareVersions", () => { + it.each([ + ["1.2.4", "1.2.3"], + ["1.3.0", "1.2.9"], + ["2.0.0", "1.9.9"], + ["1.2.10", "1.2.9"], + ["v2.0.0", "v1.0.0"], + ["1.0.0", "1.0.0-beta.1"], + ])("reports %s as newer than %s", (a, b) => { + expect(compareVersions(a, b)).toBeGreaterThan(0); + expect(compareVersions(b, a)).toBeLessThan(0); + }); + + it.each([ + ["1.2.3", "1.2.3"], + ["v1.2.3", "1.2.3"], + ["1.2.3+build.5", "1.2.3+build.9"], + ])("reports %s and %s as equal", (a, b) => { + expect(compareVersions(a, b)).toBe(0); + }); + + it("orders prereleases below their final release consistently", () => { + expect(compareVersions("1.2.0-alpha", "1.2.0-beta")).toBeLessThan(0); + expect(compareVersions("1.2.0", "1.2.0-beta")).toBeGreaterThan(0); + }); + + it("treats unparseable input as incomparable (equal)", () => { + expect(compareVersions("not-a-version", "1.2.3")).toBe(0); + expect(compareVersions("1.2.3", "")).toBe(0); + }); +}); + +describe("isStrictlyNewer", () => { + it("is true only when the candidate is strictly newer", () => { + expect(isStrictlyNewer("1.2.4", "1.2.3")).toBe(true); + expect(isStrictlyNewer("1.2.3", "1.2.3")).toBe(false); + expect(isStrictlyNewer("1.2.2", "1.2.3")).toBe(false); + }); + + it("is false when either version is missing", () => { + expect(isStrictlyNewer(null, "1.2.3")).toBe(false); + expect(isStrictlyNewer("1.2.3", null)).toBe(false); + expect(isStrictlyNewer(undefined, undefined)).toBe(false); + }); +}); diff --git a/packages/core/src/updates/versionCompare.ts b/packages/core/src/updates/versionCompare.ts new file mode 100644 index 0000000000..67738a8828 --- /dev/null +++ b/packages/core/src/updates/versionCompare.ts @@ -0,0 +1,66 @@ +type ParsedVersion = { + release: [number, number, number]; + prerelease: string | null; +}; + +function parseVersion(input: string): ParsedVersion | null { + const trimmed = input.trim().replace(/^v/i, ""); + if (trimmed.length === 0) return null; + + const [coreWithBuild, ...prereleaseParts] = trimmed.split("-"); + const core = coreWithBuild.split("+")[0]; + const segments = core.split("."); + + const release: [number, number, number] = [0, 0, 0]; + for (let i = 0; i < 3; i++) { + const segment = segments[i] ?? "0"; + const value = Number(segment); + if (!Number.isInteger(value) || value < 0) return null; + release[i] = value; + } + + return { + release, + prerelease: prereleaseParts.length > 0 ? prereleaseParts.join("-") : null, + }; +} + +/** + * Compare two semver-like version strings. + * + * Returns a positive number if `a` is newer than `b`, a negative number if it + * is older, and 0 if they are equal or cannot be compared. Tolerates a leading + * "v", ignores build metadata, and treats a prerelease (e.g. "1.2.0-beta.1") as + * older than its final release ("1.2.0"). Anything that fails to parse compares + * as equal so callers never act on garbage input. + */ +export function compareVersions(a: string, b: string): number { + const parsedA = parseVersion(a); + const parsedB = parseVersion(b); + if (!parsedA || !parsedB) return 0; + + for (let i = 0; i < 3; i++) { + if (parsedA.release[i] !== parsedB.release[i]) { + return parsedA.release[i] - parsedB.release[i]; + } + } + + if (parsedA.prerelease === parsedB.prerelease) return 0; + // A final release outranks any prerelease of the same core version. + if (parsedA.prerelease === null) return 1; + if (parsedB.prerelease === null) return -1; + return parsedA.prerelease < parsedB.prerelease ? -1 : 1; +} + +/** + * True when `candidate` is a strictly newer version than `current`. Missing + * values (either side null/undefined) are treated as "not newer" so we never + * supersede a known update on the basis of an unknown version. + */ +export function isStrictlyNewer( + candidate: string | null | undefined, + current: string | null | undefined, +): boolean { + if (!candidate || !current) return false; + return compareVersions(candidate, current) > 0; +} From 2a68d7c669efd0f7de82ea4639253f657742690b Mon Sep 17 00:00:00 2001 From: Thiago Salvatore Date: Wed, 22 Jul 2026 16:41:43 -0300 Subject: [PATCH 2/3] fix(updates): preserve staged build on failed supersede; proper prerelease ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 of PR review found two issues: - A failed superseding auto-download discarded an already-downloaded, ready-to-install update: superseding nulled downloadedVersion before the newer build existed, so a download error dropped to `error` with nothing installable. Keep the prior build in `supersededReadyVersion` and roll back to it in handleError instead of losing it. - compareVersions compared prerelease identifiers lexically, so numeric identifiers ordered wrong (e.g. alpha.10 < alpha.9). Compare per semver §11: numeric identifiers numerically, numeric below alphanumeric, longer sets above shorter prefixes. Also folds in a simplify pass: performCheck/performSilentCheck share a runUpdaterCheck scaffold, and handleUpdateAvailable's ready/available guards are unified into one isStrictlyNewer check. Generated-By: PostHog Code Task-Id: d03c3d60-e63e-4075-89dc-2f0af6c01b01 --- packages/core/src/updates/updates.test.ts | 29 ++++ packages/core/src/updates/updates.ts | 145 +++++++++++------- .../core/src/updates/versionCompare.test.ts | 14 ++ packages/core/src/updates/versionCompare.ts | 32 +++- 4 files changed, 165 insertions(+), 55 deletions(-) diff --git a/packages/core/src/updates/updates.test.ts b/packages/core/src/updates/updates.test.ts index 63bc5c4169..09ee9151b5 100644 --- a/packages/core/src/updates/updates.test.ts +++ b/packages/core/src/updates/updates.test.ts @@ -974,6 +974,35 @@ describe("UpdatesService", () => { version: "v3.0.0", }); }); + + it("restores the previously staged update when a superseding download fails", async () => { + await initializeService(service); + service.setAutoDownloadEnabled(true); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + expect(service.getStatus()).toMatchObject({ version: "v2.0.0" }); + + // A newer release supersedes and starts downloading, dropping the staged + // v2.0.0 from view while v3.0.0 is fetched. + service.checkForUpdates("periodic"); + updaterHandlers.updateAvailable?.({ + version: "v3.0.0", + releaseNotes: null, + }); + expect(service.getStatus()).toMatchObject({ downloading: true }); + + // The v3.0.0 download fails — we must fall back to the still-installable + // v2.0.0 rather than leaving the user with nothing. + updaterHandlers.error?.(new Error("network error")); + + expect(service.hasUpdateReady).toBe(true); + expect(service.getStatus()).toEqual({ + checking: false, + updateReady: true, + installing: false, + version: "v2.0.0", + }); + }); }); describe("staged update guards", () => { diff --git a/packages/core/src/updates/updates.ts b/packages/core/src/updates/updates.ts index 2f185af9b9..5d16b39ba1 100644 --- a/packages/core/src/updates/updates.ts +++ b/packages/core/src/updates/updates.ts @@ -89,6 +89,10 @@ export class UpdatesService extends TypedEventEmitter { private checkTimeoutId: ReturnType | null = null; private checkIntervalId: ReturnType | null = null; private downloadedVersion: string | null = null; + // When a strictly-newer release supersedes an already-downloaded update, this + // holds the version we had staged so a failed supersede download can roll back + // to it instead of dropping to an error with nothing installable. + private supersededReadyVersion: string | null = null; private notifiedVersion: string | null = null; private lastError: string | null = null; private initialized = false; @@ -383,6 +387,23 @@ export class UpdatesService extends TypedEventEmitter { return; } + // A superseding download that fails must not discard the update we already + // had staged. Roll back to it instead of dropping to an error with nothing + // installable. + if (this.state === "downloading" && this.supersededReadyVersion !== null) { + this.downloadedVersion = this.supersededReadyVersion; + this.supersededReadyVersion = null; + this.availableInfo = null; + this.downloadProgress = null; + this.transitionTo("ready", { + reason: + "superseding download failed; restored previously staged update", + error: error.message, + }); + this.emitStatus(this.stagedStatusPayload()); + return; + } + if (this.state === "checking" || this.state === "downloading") { this.lastError = error.message; this.transitionTo("error", { error: error.message }); @@ -401,35 +422,35 @@ export class UpdatesService extends TypedEventEmitter { return; } - // If an update is already downloaded, only a strictly newer release should - // displace it; otherwise a background re-check is just re-reporting the - // version we already have staged. - if (this.state === "ready") { - if (!isStrictlyNewer(info.version, this.downloadedVersion)) { + // If an update is already available or downloaded, only a strictly newer + // release should displace it; otherwise a background re-check is just + // re-reporting the version we already have pending. + if (this.state === "ready" || this.state === "available") { + const currentVersion = + this.state === "ready" + ? this.downloadedVersion + : this.availableInfo?.version; + + if (!isStrictlyNewer(info.version, currentVersion)) { this.log.info( - "Ignoring update-available; not newer than the staged update", - { - stagedVersion: this.downloadedVersion, - incomingVersion: info.version, - }, + "Ignoring update-available; not newer than the pending update", + { currentVersion, incomingVersion: info.version }, ); return; } - this.log.info("Newer update available; superseding staged update", { - stagedVersion: this.downloadedVersion, + + this.log.info("Newer update available; superseding pending update", { + currentVersion, incomingVersion: info.version, }); - this.downloadedVersion = null; - } else if (this.state === "available") { - if (!isStrictlyNewer(info.version, this.availableInfo?.version)) { - // Same (or older) version re-reported by a background re-check — leave - // the existing banner untouched. - return; + + if (this.state === "ready") { + // Keep the already-downloaded build recorded until the newer one is + // in hand, so a failed supersede download can restore it + // (handleError). + this.supersededReadyVersion = this.downloadedVersion; + this.downloadedVersion = null; } - this.log.info("Newer update available; replacing the pending update", { - previousVersion: this.availableInfo?.version, - incomingVersion: info.version, - }); } this.availableInfo = info; @@ -519,6 +540,8 @@ export class UpdatesService extends TypedEventEmitter { } this.downloadedVersion = version ?? null; + // The newer build is in hand; we no longer need the superseded fallback. + this.supersededReadyVersion = null; this.transitionTo("ready", { reason: "update downloaded", incomingVersion: version ?? null, @@ -559,33 +582,31 @@ export class UpdatesService extends TypedEventEmitter { } private performCheck(): void { - this.clearCheckTimeout(); - - this.checkTimeoutId = setTimeout(() => { - if (this.state === "checking" || this.state === "downloading") { - const timeoutSeconds = UpdatesService.CHECK_TIMEOUT_MS / 1000; - const message = "Update check timed out. Please try again."; - this.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); - - try { - this.updater.check(); - } catch (error) { - this.clearCheckTimeout(); - this.log.error("Failed to check for updates", { 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.", - }); - } + this.runUpdaterCheck({ + onTimeout: () => { + if (this.state === "checking" || this.state === "downloading") { + const timeoutSeconds = UpdatesService.CHECK_TIMEOUT_MS / 1000; + const message = "Update check timed out. Please try again."; + this.log.warn( + `Update check timed out after ${timeoutSeconds} seconds`, + ); + this.lastError = message; + this.transitionTo("error", { error: message }); + this.emitStatus({ checking: false, error: message }); + } + }, + onError: (error) => { + this.log.error("Failed to check for updates", { 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.", + }); + }, + }); } // A background re-check that leaves the visible state untouched. The update @@ -593,21 +614,37 @@ export class UpdatesService extends TypedEventEmitter { // has shipped the pending update is left exactly as it was — no banner // flicker, no "checking" status emitted to the UI. private performSilentCheck(): void { + this.runUpdaterCheck({ + // A stalled silent re-check has nothing to surface; just clear itself + // and leave the pending update in place. + onTimeout: () => {}, + onError: (error) => { + this.log.warn("Silent update re-check failed", { + error: error instanceof Error ? error.message : String(error), + }); + }, + }); + } + + // Shared scaffold for `performCheck`/`performSilentCheck`: arms the check + // timeout, invokes the updater, and routes timeout/error handling to the + // caller so each can decide whether to surface it to the UI. + private runUpdaterCheck(handlers: { + onTimeout: () => void; + onError: (error: unknown) => void; + }): void { this.clearCheckTimeout(); this.checkTimeoutId = setTimeout(() => { - // A stalled silent re-check has nothing to surface; just clear itself and - // leave the pending update in place. this.clearCheckTimeout(); + handlers.onTimeout(); }, UpdatesService.CHECK_TIMEOUT_MS); try { this.updater.check(); } catch (error) { this.clearCheckTimeout(); - this.log.warn("Silent update re-check failed", { - error: error instanceof Error ? error.message : String(error), - }); + handlers.onError(error); } } diff --git a/packages/core/src/updates/versionCompare.test.ts b/packages/core/src/updates/versionCompare.test.ts index 357574933b..5342be6b15 100644 --- a/packages/core/src/updates/versionCompare.test.ts +++ b/packages/core/src/updates/versionCompare.test.ts @@ -27,6 +27,20 @@ describe("compareVersions", () => { expect(compareVersions("1.2.0", "1.2.0-beta")).toBeGreaterThan(0); }); + it("compares numeric prerelease identifiers numerically, not lexically", () => { + expect(compareVersions("1.2.0-alpha.10", "1.2.0-alpha.9")).toBeGreaterThan( + 0, + ); + expect(compareVersions("1.2.0-beta.2", "1.2.0-beta.11")).toBeLessThan(0); + }); + + it("ranks numeric identifiers below alphanumeric and longer sets above shorter", () => { + // Numeric identifier has lower precedence than an alphanumeric one. + expect(compareVersions("1.0.0-1", "1.0.0-alpha")).toBeLessThan(0); + // A longer set of identifiers outranks its shorter prefix. + expect(compareVersions("1.0.0-alpha.1", "1.0.0-alpha")).toBeGreaterThan(0); + }); + it("treats unparseable input as incomparable (equal)", () => { expect(compareVersions("not-a-version", "1.2.3")).toBe(0); expect(compareVersions("1.2.3", "")).toBe(0); diff --git a/packages/core/src/updates/versionCompare.ts b/packages/core/src/updates/versionCompare.ts index 67738a8828..2481538b00 100644 --- a/packages/core/src/updates/versionCompare.ts +++ b/packages/core/src/updates/versionCompare.ts @@ -49,7 +49,37 @@ export function compareVersions(a: string, b: string): number { // A final release outranks any prerelease of the same core version. if (parsedA.prerelease === null) return 1; if (parsedB.prerelease === null) return -1; - return parsedA.prerelease < parsedB.prerelease ? -1 : 1; + return comparePrerelease(parsedA.prerelease, parsedB.prerelease); +} + +// Compare two prerelease strings per semver §11: dot-separated identifiers, +// numeric ones compared numerically, numeric ranked below alphanumeric, and a +// longer set of identifiers ranked above a shorter prefix of it. +function comparePrerelease(a: string, b: string): number { + const aIds = a.split("."); + const bIds = b.split("."); + const length = Math.max(aIds.length, bIds.length); + + for (let i = 0; i < length; i++) { + if (i >= aIds.length) return -1; + if (i >= bIds.length) return 1; + + const aId = aIds[i]; + const bId = bIds[i]; + const aNumeric = /^\d+$/.test(aId); + const bNumeric = /^\d+$/.test(bId); + + if (aNumeric && bNumeric) { + const diff = Number(aId) - Number(bId); + if (diff !== 0) return diff < 0 ? -1 : 1; + } else if (aNumeric !== bNumeric) { + return aNumeric ? -1 : 1; + } else if (aId !== bId) { + return aId < bId ? -1 : 1; + } + } + + return 0; } /** From 005ec7653000b86eba84220e38364c04c3241172 Mon Sep 17 00:00:00 2001 From: Thiago Salvatore Date: Wed, 22 Jul 2026 17:09:15 -0300 Subject: [PATCH 3/3] fix(updates): guard against stale download event during a supersede MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stamphog and Greptile flagged that handleUpdateDownloaded only rejected stale/older download events when state === "ready". While a supersede is in flight (state "downloading"/"available", downloadedVersion momentarily null), a delayed update-downloaded event for the old artifact bypassed the guard and overwrote the pending newer update — stranding the app on the old version, the exact bug this PR fixes. Make the guard state-independent: reject any download no newer than the version we already have staged or are superseding away from (downloadedVersion ?? supersededReadyVersion). Adds a test for the stale-event-mid-supersede case. Generated-By: PostHog Code Task-Id: d03c3d60-e63e-4075-89dc-2f0af6c01b01 --- packages/core/src/updates/updates.test.ts | 32 +++++++++++++++++++++++ packages/core/src/updates/updates.ts | 19 +++++++++----- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/packages/core/src/updates/updates.test.ts b/packages/core/src/updates/updates.test.ts index 09ee9151b5..ecafd230c8 100644 --- a/packages/core/src/updates/updates.test.ts +++ b/packages/core/src/updates/updates.test.ts @@ -975,6 +975,38 @@ describe("UpdatesService", () => { }); }); + it("ignores a stale download for the old artifact while a supersede is in flight", async () => { + await initializeService(service); + service.setAutoDownloadEnabled(true); + + updaterHandlers.updateDownloaded?.("v2.0.0"); + expect(service.getStatus()).toMatchObject({ version: "v2.0.0" }); + + // Supersede to v3.0.0 kicks off a download; v2.0.0 is no longer staged. + service.checkForUpdates("periodic"); + updaterHandlers.updateAvailable?.({ + version: "v3.0.0", + releaseNotes: null, + }); + expect(service.getStatus()).toMatchObject({ downloading: true }); + + // A delayed update-downloaded for the OLD v2.0.0 arrives mid-download; it + // must not strand us back on v2.0.0. + updaterHandlers.updateDownloaded?.("v2.0.0"); + expect(service.hasUpdateReady).toBe(false); + expect(service.getStatus()).toMatchObject({ + downloading: true, + availableVersion: "v3.0.0", + }); + + // The real v3.0.0 download still completes and stages cleanly. + updaterHandlers.updateDownloaded?.("v3.0.0"); + expect(service.getStatus()).toMatchObject({ + updateReady: true, + version: "v3.0.0", + }); + }); + it("restores the previously staged update when a superseding download fails", async () => { await initializeService(service); service.setAutoDownloadEnabled(true); diff --git a/packages/core/src/updates/updates.ts b/packages/core/src/updates/updates.ts index 5d16b39ba1..8d82d79b35 100644 --- a/packages/core/src/updates/updates.ts +++ b/packages/core/src/updates/updates.ts @@ -526,14 +526,19 @@ export class UpdatesService extends TypedEventEmitter { return; } - // If we already have a downloaded update, only accept a strictly newer one - // so a duplicate or stale event can't reset the staged version. - if ( - this.state === "ready" && - !isStrictlyNewer(version, this.downloadedVersion) - ) { + // Reject a stale or duplicate download for a version no newer than one we + // already have in hand — or are actively superseding away from — + // regardless of state. A delayed update-downloaded event for the old + // artifact can arrive while a newer supersede download is still in flight + // (state "downloading"/"available", so downloadedVersion is momentarily + // null); without this guard it would overwrite the pending newer update + // with the stale one and strand us on the old version. The two fields are + // mutually exclusive — a supersede nulls downloadedVersion and records + // supersededReadyVersion — so `??` picks whichever floor applies. + const floorVersion = this.downloadedVersion ?? this.supersededReadyVersion; + if (floorVersion !== null && !isStrictlyNewer(version, floorVersion)) { this.log.info("Ignoring duplicate or older update-downloaded event", { - existingVersion: this.downloadedVersion, + floorVersion, incomingVersion: version, }); return;