From d6d2e4c48f474bcabfd8e0ffaf2e3dcfea6f5953 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:28:30 -0700 Subject: [PATCH] fix(updater): harden install guard recovery --- .../update-install-launch-gate.test.ts | 34 ++++++++++-- .../startup/update-install-launch-gate.ts | 9 +++- src/main/updater.mac-install.test.ts | 3 +- src/main/updater.test.ts | 4 ++ src/main/updater.ts | 15 ++++-- .../src/components/UpdateCard.test.ts | 33 +++++++++++- src/renderer/src/components/UpdateCard.tsx | 53 +++++++++++++------ src/shared/types.ts | 10 +++- 8 files changed, 132 insertions(+), 29 deletions(-) diff --git a/src/main/startup/update-install-launch-gate.test.ts b/src/main/startup/update-install-launch-gate.test.ts index 8e9f280c8a8..fe5b1b4e17c 100644 --- a/src/main/startup/update-install-launch-gate.test.ts +++ b/src/main/startup/update-install-launch-gate.test.ts @@ -1,7 +1,7 @@ import { mkdtempSync, existsSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import path from 'node:path' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { UPDATE_INSTALL_HANDOFF_MARKER_FILE, UPDATE_INSTALL_HANDOFF_MAX_AGE_MS, @@ -11,9 +11,12 @@ import { writeUpdateInstallHandoffMarker } from './update-install-launch-gate' +const execFileSyncMock = vi.hoisted(() => vi.fn()) +vi.mock('node:child_process', () => ({ execFileSync: execFileSyncMock })) + const APP_BIN = '/Applications/Orca.app/Contents/MacOS/Orca' -const SHIPIT_ROW = - '/Applications/Orca.app/Contents/Frameworks/Squirrel.framework/Resources/ShipIt com.stablyai.orca.ShipIt' +const SHIPIT_PATH = '/Applications/Orca.app/Contents/Frameworks/Squirrel.framework/Resources/ShipIt' +const SHIPIT_ROW = `${SHIPIT_PATH} com.stablyai.orca.ShipIt` const tempDirs: string[] = [] @@ -28,6 +31,7 @@ function markerFile(userData: string): string { } afterEach(() => { + execFileSyncMock.mockReset() for (const dir of tempDirs.splice(0)) { rmSync(dir, { recursive: true, force: true }) } @@ -45,9 +49,33 @@ describe('isBundleShipItRunning', () => { const grepRow = `/bin/zsh -c grep ${SHIPIT_ROW}` expect(isBundleShipItRunning(APP_BIN, grepRow)).toBe(false) }) + + it('does not accept another executable whose path only shares the ShipIt prefix', () => { + expect(isBundleShipItRunning(APP_BIN, `${SHIPIT_PATH}-backup update`)).toBe(false) + }) }) describe('shouldDeferLaunchForUpdateInstall', () => { + it('reads all users so a privileged ShipIt install is visible', () => { + const userData = makeUserData() + writeUpdateInstallHandoffMarker(userData, '1.0.51') + execFileSyncMock.mockReturnValue(SHIPIT_ROW) + + const defer = shouldDeferLaunchForUpdateInstall({ + isPackaged: true, + userDataPath: userData, + appVersion: '1.0.51', + deps: { platform: 'darwin', executablePath: APP_BIN } + }) + + expect(defer).toBe(true) + expect(execFileSyncMock).toHaveBeenCalledWith( + 'ps', + ['-axo', 'command='], + expect.objectContaining({ encoding: 'utf8' }) + ) + }) + it('defers a same-version launch while this bundle’s installer is alive', () => { const userData = makeUserData() writeUpdateInstallHandoffMarker(userData, '1.0.51') diff --git a/src/main/startup/update-install-launch-gate.ts b/src/main/startup/update-install-launch-gate.ts index 801c1f79984..cff239f32dc 100644 --- a/src/main/startup/update-install-launch-gate.ts +++ b/src/main/startup/update-install-launch-gate.ts @@ -68,7 +68,9 @@ function readUpdateInstallHandoffMarker(userDataPath: string): UpdateInstallHand } function readProcessCommandList(): string { - return execFileSync('ps', ['-xo', 'command='], { + // Why: ShipIt can run as root when the app bundle is not user-writable. + // The launch gate must see that privileged installer too. + return execFileSync('ps', ['-axo', 'command='], { encoding: 'utf8', timeout: PROCESS_LIST_TIMEOUT_MS, maxBuffer: PROCESS_LIST_MAX_BYTES @@ -91,7 +93,10 @@ export function isBundleShipItRunning(executablePath: string, processCommandList ) // Why: anchor on argv[0]. A shell or grep merely mentioning the ShipIt path // in its arguments must not read as a live installer. - return processCommandList.split('\n').some((line) => line.trimStart().startsWith(shipItPath)) + return processCommandList.split('\n').some((line) => { + const command = line.trimStart() + return command === shipItPath || command.startsWith(`${shipItPath} `) + }) } export type UpdateInstallLaunchGateDeps = { diff --git a/src/main/updater.mac-install.test.ts b/src/main/updater.mac-install.test.ts index edc34b4102b..ee829904674 100644 --- a/src/main/updater.mac-install.test.ts +++ b/src/main/updater.mac-install.test.ts @@ -183,7 +183,8 @@ describe('updater mac install handoff', () => { 'updater:status', expect.objectContaining({ state: 'error', - message: expect.stringContaining('PID 4242') + message: expect.stringContaining('PID 4242'), + retryAction: 'install' }) ) }) diff --git a/src/main/updater.test.ts b/src/main/updater.test.ts index ee74fa3ad63..77424cfc05b 100644 --- a/src/main/updater.test.ts +++ b/src/main/updater.test.ts @@ -1294,6 +1294,10 @@ describe('updater', () => { expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledTimes(1) expect(isQuittingForUpdate()).toBe(false) + if (process.platform === 'darwin') { + expect(writeHandoffMarkerMock).toHaveBeenCalledWith('/tmp/orca-test-user-data', '1.0.51') + expect(clearHandoffMarkerMock).toHaveBeenCalledWith('/tmp/orca-test-user-data') + } // Why: destructive prep runs only after quitAndInstall returns still in // progress; sync recovery clears flags first so PTYs stay alive. expect(killAllPtyMock).not.toHaveBeenCalled() diff --git a/src/main/updater.ts b/src/main/updater.ts index 6269e164f22..4da7b3fc04c 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -520,15 +520,20 @@ function shouldHandleUpdaterErrorEvent(): boolean { ) } -function sendErrorStatus(message: string, userInitiated?: boolean): void { +function sendErrorStatus(message: string, userInitiated?: boolean, retryAction?: 'install'): void { if ( currentStatus.state === 'error' && currentStatus.message === message && - currentStatus.userInitiated === userInitiated + currentStatus.userInitiated === userInitiated && + currentStatus.retryAction === retryAction ) { return } - sendStatus({ state: 'error', message, userInitiated }) + sendStatus( + retryAction + ? { state: 'error', message, userInitiated, retryAction } + : { state: 'error', message, userInitiated } + ) } function getKnownReleaseUrl(): string | undefined { @@ -594,7 +599,9 @@ async function performQuitAndInstall(): Promise { } ) sendErrorStatus( - `Another running copy of Orca is blocking the update (PID ${conflictingPids.join(', ')}) — possibly one launched by an agent or test run. Quit it, then install the update again.` + `Another running copy of Orca is blocking the update (PID ${conflictingPids.join(', ')}) — possibly one launched by an agent or test run. Quit it, then install the update again.`, + undefined, + 'install' ) return } diff --git a/src/renderer/src/components/UpdateCard.test.ts b/src/renderer/src/components/UpdateCard.test.ts index 9d4b75094d2..2972543c5eb 100644 --- a/src/renderer/src/components/UpdateCard.test.ts +++ b/src/renderer/src/components/UpdateCard.test.ts @@ -4,7 +4,24 @@ import { getDefaultUIState } from '../../../shared/constants' import type { ChangelogData, UpdateStatus } from '../../../shared/types' import { createUISlice } from '../store/slices/ui' import type { AppState } from '../store/types' -import { isHttp2ProtocolError } from './UpdateCard' +import { getUpdateErrorRetryTarget, isHttp2ProtocolError } from './UpdateCard' + +describe('getUpdateErrorRetryTarget', () => { + it('retries a blocked install without downloading the staged release again', () => { + expect( + getUpdateErrorRetryTarget( + { state: 'error', message: 'another instance is running', retryAction: 'install' }, + '1.2.0' + ) + ).toBe('install') + }) + + it('keeps existing download and check recovery for unclassified errors', () => { + const status = { state: 'error', message: 'failed' } as const + expect(getUpdateErrorRetryTarget(status, '1.2.0')).toBe('download') + expect(getUpdateErrorRetryTarget(status, null)).toBe('check') + }) +}) // ── Helpers ────────────────────────────────────────────────────────── @@ -324,7 +341,8 @@ function computeVisibility(input: VisibilityInput): VisibilityResult { const isUserInitiated = 'userInitiated' in status && status.userInitiated const updateUserInitiatedCycle = input.updateUserInitiatedCycle ?? false const shouldShowDetailedErrorCard = - status.state === 'error' && (hasStartedDownload || cachedVersion !== null) + status.state === 'error' && + (hasStartedDownload || cachedVersion !== null || status.retryAction === 'install') if (status.state === 'checking' && !isUserInitiated) { return 'hidden' @@ -383,6 +401,17 @@ describe('UpdateCard visibility gates', () => { ).toBe('visible') }) + it('shows an install-retry error even after the card remounts without a cached version', () => { + expect( + computeVisibility({ + status: { state: 'error', message: 'another instance is running', retryAction: 'install' }, + dismissedVersion: null, + cachedVersion: null, + hasStartedDownload: false + }) + ).toBe('visible') + }) + it('hides background not-available', () => { expect( computeVisibility({ diff --git a/src/renderer/src/components/UpdateCard.tsx b/src/renderer/src/components/UpdateCard.tsx index 806540832a3..c9ce6dd8df6 100644 --- a/src/renderer/src/components/UpdateCard.tsx +++ b/src/renderer/src/components/UpdateCard.tsx @@ -8,7 +8,7 @@ import { Card } from './ui/card' import { Button } from './ui/button' import { Progress } from './ui/progress' import { AlertCircle, Check, Loader2, Minus, Network, RotateCw, X } from 'lucide-react' -import type { ChangelogData } from '../../../shared/types' +import type { ChangelogData, UpdateStatus } from '../../../shared/types' import { translate } from '@/i18n/i18n' // ── Helpers ────────────────────────────────────────────────────────── @@ -50,6 +50,16 @@ type ErrorCardModel = { } } +export function getUpdateErrorRetryTarget( + status: Extract, + cachedVersion: string | null +): 'check' | 'download' | 'install' { + if (status.retryAction === 'install') { + return 'install' + } + return cachedVersion ? 'download' : 'check' +} + // ── Compact card (transient check feedback) ───────────────────────── function CompactCardContent({ @@ -236,8 +246,10 @@ export function UpdateCard() { const isUserInitiated = 'userInitiated' in status && status.userInitiated const cachedVersion = versionRef.current + const isInstallRetryError = status.state === 'error' && status.retryAction === 'install' const shouldShowDetailedErrorCard = - status.state === 'error' && (hasStartedDownload.current || cachedVersion !== null) + status.state === 'error' && + (hasStartedDownload.current || cachedVersion !== null || isInstallRetryError) // Compact transient states: only show for user-initiated checks. if (status.state === 'checking' && !isUserInitiated) { @@ -338,6 +350,8 @@ export function UpdateCard() { } const isHttp2UpdateError = status.state === 'error' && isHttp2ProtocolError(status.message) + const errorRetryTarget = + status.state === 'error' ? getUpdateErrorRetryTarget(status, cachedVersion) : null const errorCard: ErrorCardModel | null = status.state === 'error' ? isHttp2UpdateError @@ -357,26 +371,33 @@ export function UpdateCard() { : { // Why: title is scoped to the operation that failed so check-time // failures (commonly GitHub-side) don't read as a bug in Orca. - title: cachedVersion ? 'Update Error' : 'Update Check Failed', - summary: cachedVersion - ? 'Could not complete the update.' - : 'Could not check for updates.', + title: cachedVersion || isInstallRetryError ? 'Update Error' : 'Update Check Failed', + summary: + cachedVersion || isInstallRetryError + ? 'Could not complete the update.' + : 'Could not check for updates.', message: status.message, releaseUrl: releaseUrlForVersion(cachedVersion), // Why: check-time failures are often transient (offline, GitHub // hiccup), so offer a Re-check next to "Download Manually" instead // of forcing the user into the manual fallback. - primaryAction: cachedVersion - ? { - label: translate('auto.components.UpdateCard.48565a32bc', 'Retry Download'), - onClick: handleUpdate - } - : { - label: translate('auto.components.UpdateCard.6b0085010d', 'Re-check'), - onClick: () => { - void window.api.updater.check({ includePrerelease: false }) + primaryAction: + errorRetryTarget === 'install' + ? { + label: translate('auto.components.UpdateCard.2c2d3e03ca', 'Try Again'), + onClick: handleInstallRetry } - } + : errorRetryTarget === 'download' + ? { + label: translate('auto.components.UpdateCard.48565a32bc', 'Retry Download'), + onClick: handleUpdate + } + : { + label: translate('auto.components.UpdateCard.6b0085010d', 'Re-check'), + onClick: () => { + void window.api.updater.check({ includePrerelease: false }) + } + } } : installError ? { diff --git a/src/shared/types.ts b/src/shared/types.ts index c805b9a0131..5d6159f49a1 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -2263,7 +2263,15 @@ export type UpdateStatus = | { state: 'not-available'; userInitiated?: boolean } | { state: 'downloading'; percent: number; version: string; activeNudgeId?: string } | { state: 'downloaded'; version: string; releaseUrl?: string; activeNudgeId?: string } - | { state: 'error'; message: string; userInitiated?: boolean; activeNudgeId?: string } + | { + state: 'error' + message: string + userInitiated?: boolean + activeNudgeId?: string + // Why: a blocked handoff leaves the staged update intact, so retrying + // installation must not wastefully download the same release again. + retryAction?: 'install' + } // ─── Settings ──────────────────────────────────────────────────────── export type NotificationSettings = {