fix(updater): harden install guard recovery

This commit is contained in:
Brennan Benson
2026-07-16 17:28:31 -07:00
parent 17870106a4
commit d6d2e4c48f
8 changed files with 132 additions and 29 deletions
@@ -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')
@@ -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 = {
+2 -1
View File
@@ -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'
})
)
})
+4
View File
@@ -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()
+11 -4
View File
@@ -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<void> {
}
)
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
}
+31 -2
View File
@@ -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({
+37 -16
View File
@@ -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<UpdateStatus, { state: 'error' }>,
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
? {
+9 -1
View File
@@ -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 = {