From 8051cfdd9ef8a4a3820aecd749c4e1c119a8f1a7 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:41:28 -0700 Subject: [PATCH] fix(updater): close remaining install guard races --- src/main/index.ts | 2 +- .../update-install-launch-gate.test.ts | 181 ++++++++++++++---- .../startup/update-install-launch-gate.ts | 105 +++++++--- .../updater-conflicting-app-instances.test.ts | 78 +++++--- src/main/updater-conflicting-app-instances.ts | 107 +++++++---- src/main/updater.mac-install.test.ts | 6 +- src/main/updater.test.ts | 11 +- src/main/updater.ts | 4 +- 8 files changed, 356 insertions(+), 138 deletions(-) diff --git a/src/main/index.ts b/src/main/index.ts index 6d2febb792e..376cdd999dd 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -452,7 +452,7 @@ configureOrcaUserDataPathEnv() if ( shouldDeferLaunchForUpdateInstall({ isPackaged: app.isPackaged, - userDataPath: app.getPath('userData'), + appDataPath: app.getPath('appData'), appVersion: app.getVersion() }) ) { diff --git a/src/main/startup/update-install-launch-gate.test.ts b/src/main/startup/update-install-launch-gate.test.ts index ff31792af5b..96ba678695b 100644 --- a/src/main/startup/update-install-launch-gate.test.ts +++ b/src/main/startup/update-install-launch-gate.test.ts @@ -1,11 +1,12 @@ -import { mkdtempSync, existsSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import path from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import { - UPDATE_INSTALL_HANDOFF_MARKER_FILE, + UPDATE_INSTALL_HANDOFF_ARMING_MS, UPDATE_INSTALL_HANDOFF_MAX_AGE_MS, clearUpdateInstallHandoffMarker, + getUpdateInstallHandoffMarkerPath, isBundleShipItRunning, shouldDeferLaunchForUpdateInstall, writeUpdateInstallHandoffMarker @@ -20,14 +21,14 @@ const SHIPIT_ROW = `${SHIPIT_PATH} com.stablyai.orca.ShipIt` const tempDirs: string[] = [] -function makeUserData(): string { +function makeAppData(): string { const dir = mkdtempSync(path.join(tmpdir(), 'orca-launch-gate-')) tempDirs.push(dir) return dir } -function markerFile(userData: string): string { - return path.join(userData, UPDATE_INSTALL_HANDOFF_MARKER_FILE) +function markerFile(appData: string, executablePath = APP_BIN): string { + return getUpdateInstallHandoffMarkerPath(appData, executablePath) } afterEach(() => { @@ -57,15 +58,20 @@ describe('isBundleShipItRunning', () => { describe('shouldDeferLaunchForUpdateInstall', () => { it('reads all users so a privileged ShipIt install is visible', () => { - const userData = makeUserData() - writeUpdateInstallHandoffMarker(userData, '1.0.51') + const appData = makeAppData() + const createdAt = 1_000_000 + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', createdAt) execFileSyncMock.mockReturnValue(SHIPIT_ROW) const defer = shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', - deps: { platform: 'darwin', executablePath: APP_BIN } + deps: { + platform: 'darwin', + executablePath: APP_BIN, + now: createdAt + UPDATE_INSTALL_HANDOFF_ARMING_MS + 1 + } }) expect(defer).toBe(true) @@ -76,59 +82,134 @@ describe('shouldDeferLaunchForUpdateInstall', () => { ) }) - it('defers a same-version launch while this bundle’s installer is alive', () => { - const userData = makeUserData() - writeUpdateInstallHandoffMarker(userData, '1.0.51') + it('defers during marker arming without waiting for a process-table read', () => { + const appData = makeAppData() + const createdAt = 1_000_000 + const readProcessList = vi.fn(() => '/usr/bin/ps') + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', createdAt) const defer = shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', - deps: { platform: 'darwin', executablePath: APP_BIN, readProcessList: () => SHIPIT_ROW } + deps: { + platform: 'darwin', + executablePath: APP_BIN, + now: createdAt + UPDATE_INSTALL_HANDOFF_ARMING_MS, + readProcessList + } }) expect(defer).toBe(true) - expect(existsSync(markerFile(userData))).toBe(true) + expect(readProcessList).not.toHaveBeenCalled() }) - it('lets the post-update relaunch through and clears the marker', () => { - const userData = makeUserData() - writeUpdateInstallHandoffMarker(userData, '1.0.51') + it('defers a same-version launch while this bundle’s installer is alive', () => { + const appData = makeAppData() + const createdAt = 1_000_000 + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', createdAt) const defer = shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, + appVersion: '1.0.51', + deps: { + platform: 'darwin', + executablePath: APP_BIN, + now: createdAt + UPDATE_INSTALL_HANDOFF_ARMING_MS + 1, + readProcessList: () => SHIPIT_ROW + } + }) + + expect(defer).toBe(true) + expect(existsSync(markerFile(appData))).toBe(true) + }) + + it('shares the bundle-scoped marker across isolated userData profiles', () => { + const appData = makeAppData() + const createdAt = 1_000_000 + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', createdAt) + + const deferFromIsolatedProfile = shouldDeferLaunchForUpdateInstall({ + isPackaged: true, + appDataPath: appData, + appVersion: '1.0.51', + deps: { + platform: 'darwin', + executablePath: APP_BIN, + now: createdAt + 1, + readProcessList: () => '/usr/bin/ps' + } + }) + + expect(deferFromIsolatedProfile).toBe(true) + }) + + it('does not share markers between side-by-side app bundles', () => { + const appData = makeAppData() + const otherAppBin = '/Applications/Orca Preview.app/Contents/MacOS/Orca' + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', 1_000_000) + + expect( + shouldDeferLaunchForUpdateInstall({ + isPackaged: true, + appDataPath: appData, + appVersion: '1.0.51', + deps: { + platform: 'darwin', + executablePath: otherAppBin, + now: 1_000_001, + readProcessList: () => SHIPIT_ROW + } + }) + ).toBe(false) + expect(existsSync(markerFile(appData))).toBe(true) + }) + + it('lets the post-update relaunch through and clears the marker', () => { + const appData = makeAppData() + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51') + + const defer = shouldDeferLaunchForUpdateInstall({ + isPackaged: true, + appDataPath: appData, appVersion: '1.0.61', deps: { platform: 'darwin', executablePath: APP_BIN, readProcessList: () => SHIPIT_ROW } }) expect(defer).toBe(false) - expect(existsSync(markerFile(userData))).toBe(false) + expect(existsSync(markerFile(appData))).toBe(false) }) it('launches normally once the installer has exited (aborted/failed install)', () => { - const userData = makeUserData() - writeUpdateInstallHandoffMarker(userData, '1.0.51') + const appData = makeAppData() + const createdAt = 1_000_000 + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', createdAt) const defer = shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', - deps: { platform: 'darwin', executablePath: APP_BIN, readProcessList: () => '/usr/bin/ps' } + deps: { + platform: 'darwin', + executablePath: APP_BIN, + now: createdAt + UPDATE_INSTALL_HANDOFF_ARMING_MS + 1, + readProcessList: () => '/usr/bin/ps' + } }) expect(defer).toBe(false) - expect(existsSync(markerFile(userData))).toBe(false) + expect(existsSync(markerFile(appData))).toBe(false) }) it('expires stale markers instead of gating launches', () => { - const userData = makeUserData() + const appData = makeAppData() const staleCreatedAt = 1_000_000 - writeUpdateInstallHandoffMarker(userData, '1.0.51', staleCreatedAt) + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', staleCreatedAt) const defer = shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', deps: { platform: 'darwin', @@ -139,30 +220,33 @@ describe('shouldDeferLaunchForUpdateInstall', () => { }) expect(defer).toBe(false) - expect(existsSync(markerFile(userData))).toBe(false) + expect(existsSync(markerFile(appData))).toBe(false) }) it('fails open on corrupt markers, unreadable process tables, and other platforms', () => { - const userData = makeUserData() - writeFileSync(markerFile(userData), 'not json') + const appData = makeAppData() + mkdirSync(path.dirname(markerFile(appData)), { recursive: true }) + writeFileSync(markerFile(appData), 'not json') expect( shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', deps: { platform: 'darwin', executablePath: APP_BIN, readProcessList: () => SHIPIT_ROW } }) ).toBe(false) - writeUpdateInstallHandoffMarker(userData, '1.0.51') + const createdAt = 1_000_000 + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', createdAt) expect( shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', deps: { platform: 'darwin', executablePath: APP_BIN, + now: createdAt + UPDATE_INSTALL_HANDOFF_ARMING_MS + 1, readProcessList: () => { throw new Error('ps timed out') } @@ -170,11 +254,11 @@ describe('shouldDeferLaunchForUpdateInstall', () => { }) ).toBe(false) - writeUpdateInstallHandoffMarker(userData, '1.0.51') + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51') expect( shouldDeferLaunchForUpdateInstall({ isPackaged: true, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', deps: { platform: 'win32', executablePath: APP_BIN, readProcessList: () => SHIPIT_ROW } }) @@ -183,17 +267,32 @@ describe('shouldDeferLaunchForUpdateInstall', () => { expect( shouldDeferLaunchForUpdateInstall({ isPackaged: false, - userDataPath: userData, + appDataPath: appData, appVersion: '1.0.51', deps: { platform: 'darwin', executablePath: APP_BIN, readProcessList: () => SHIPIT_ROW } }) ).toBe(false) }) + it('fails open and clears a marker timestamped in the future', () => { + const appData = makeAppData() + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51', 2_000_000) + + expect( + shouldDeferLaunchForUpdateInstall({ + isPackaged: true, + appDataPath: appData, + appVersion: '1.0.51', + deps: { platform: 'darwin', executablePath: APP_BIN, now: 1_000_000 } + }) + ).toBe(false) + expect(existsSync(markerFile(appData))).toBe(false) + }) + it('clearUpdateInstallHandoffMarker removes the marker', () => { - const userData = makeUserData() - writeUpdateInstallHandoffMarker(userData, '1.0.51') - clearUpdateInstallHandoffMarker(userData) - expect(existsSync(markerFile(userData))).toBe(false) + const appData = makeAppData() + writeUpdateInstallHandoffMarker(appData, APP_BIN, '1.0.51') + clearUpdateInstallHandoffMarker(appData, APP_BIN) + expect(existsSync(markerFile(appData))).toBe(false) }) }) diff --git a/src/main/startup/update-install-launch-gate.ts b/src/main/startup/update-install-launch-gate.ts index b5f5555dff8..80ca3fb816a 100644 --- a/src/main/startup/update-install-launch-gate.ts +++ b/src/main/startup/update-install-launch-gate.ts @@ -1,5 +1,6 @@ import { execFileSync } from 'node:child_process' -import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { createHash } from 'node:crypto' +import { existsSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs' import path from 'node:path' // Why: Squirrel.Mac installs by swapping the bundle AFTER the app exits, and @@ -11,7 +12,11 @@ import path from 'node:path' // launch that finds the marker fresh while this bundle's ShipIt installer is // still alive exits instead — ShipIt then finishes and relaunches the updated // app itself, so the user's click still ends with Orca open. -export const UPDATE_INSTALL_HANDOFF_MARKER_FILE = 'update-install-handoff.json' +export const UPDATE_INSTALL_HANDOFF_MARKER_DIRECTORY = 'orca-update-install-handoffs' +// Why: quitAndInstall writes the marker immediately before spawning ShipIt. +// During this bounded arming window, the marker is authoritative because the +// installer may not be visible in the process table yet. +export const UPDATE_INSTALL_HANDOFF_ARMING_MS = 5_000 // Why: the handoff normally resolves in seconds; the ceiling only bounds how // long a crashed install could keep gating launches. The live-ShipIt check is // the real gate, so a stale marker never locks the app out. @@ -25,44 +30,84 @@ export type UpdateInstallHandoffMarker = { createdAtMs: number } -function markerPath(userDataPath: string): string { - return path.join(userDataPath, UPDATE_INSTALL_HANDOFF_MARKER_FILE) +function removeFileBestEffort(filePath: string): void { + try { + rmSync(filePath, { force: true }) + } catch { + // Marker cleanup must never prevent an install or a normal app launch. + } +} + +function bundleRootForExecutable(executablePath: string): string { + // Why: these paths always describe a macOS bundle, even in cross-platform + // unit tests, so host-native separators would change the identity. + return path.posix.resolve(executablePath, '..', '..', '..') +} + +export function getUpdateInstallHandoffMarkerPath( + appDataPath: string, + executablePath: string +): string { + // Why: appData is shared by isolated Orca profiles, while the executable + // hash keeps side-by-side app bundles from gating each other's launches. + const bundleId = createHash('sha256') + .update(bundleRootForExecutable(executablePath)) + .digest('hex') + .slice(0, 16) + return path.join(appDataPath, UPDATE_INSTALL_HANDOFF_MARKER_DIRECTORY, `${bundleId}.json`) } /** Written by the updater immediately before the native install handoff. */ export function writeUpdateInstallHandoffMarker( - userDataPath: string, + appDataPath: string, + executablePath: string, appVersion: string, now = Date.now() ): void { + const finalPath = getUpdateInstallHandoffMarkerPath(appDataPath, executablePath) + const temporaryPath = `${finalPath}.${process.pid}.${now}.tmp` try { const marker: UpdateInstallHandoffMarker = { appVersion, createdAtMs: now } - writeFileSync(markerPath(userDataPath), JSON.stringify(marker)) + mkdirSync(path.dirname(finalPath), { recursive: true }) + writeFileSync(temporaryPath, JSON.stringify(marker)) + // Why: a direct overwrite can expose partial JSON to the launch racing + // this write. Same-directory rename publishes the complete marker at once. + renameSync(temporaryPath, finalPath) } catch { // Why: the marker only powers the relaunch-race guard; failing to write // it must never block the install itself. + } finally { + removeFileBestEffort(temporaryPath) } } -export function clearUpdateInstallHandoffMarker(userDataPath: string): void { - try { - rmSync(markerPath(userDataPath), { force: true }) - } catch { - /* Removal is best-effort; a stale marker self-expires. */ - } +export function clearUpdateInstallHandoffMarker(appDataPath: string, executablePath: string): void { + removeFileBestEffort(getUpdateInstallHandoffMarkerPath(appDataPath, executablePath)) } -function readUpdateInstallHandoffMarker(userDataPath: string): UpdateInstallHandoffMarker | null { +function readUpdateInstallHandoffMarker( + appDataPath: string, + executablePath: string +): UpdateInstallHandoffMarker | null { + const filePath = getUpdateInstallHandoffMarkerPath(appDataPath, executablePath) try { - if (!existsSync(markerPath(userDataPath))) { + if (!existsSync(filePath)) { return null } - const parsed = JSON.parse(readFileSync(markerPath(userDataPath), 'utf8')) - if (typeof parsed?.appVersion !== 'string' || typeof parsed?.createdAtMs !== 'number') { + const parsed = JSON.parse(readFileSync(filePath, 'utf8')) + if ( + typeof parsed?.appVersion !== 'string' || + typeof parsed?.createdAtMs !== 'number' || + !Number.isFinite(parsed.createdAtMs) + ) { + removeFileBestEffort(filePath) return null } return { appVersion: parsed.appVersion, createdAtMs: parsed.createdAtMs } } catch { + // Why: a corrupt marker cannot ever become actionable; removing it avoids + // repeated startup reads while preserving the fail-open contract. + removeFileBestEffort(filePath) return null } } @@ -82,9 +127,7 @@ function readProcessCommandList(): string { * this exact install is considered. */ export function isBundleShipItRunning(executablePath: string, processCommandList: string): boolean { // /Applications/Orca.app/Contents/MacOS/Orca → /Applications/Orca.app - // Why: this matcher always receives a macOS path, even when its unit tests - // run on Windows, so host-native separators would corrupt the comparison. - const bundleRoot = path.posix.resolve(executablePath, '..', '..', '..') + const bundleRoot = bundleRootForExecutable(executablePath) const shipItPath = path.posix.join( bundleRoot, 'Contents', @@ -112,11 +155,12 @@ export type UpdateInstallLaunchGateDeps = { * Decide whether this launch should exit to let an in-flight update install * finish. Sync by design — it must resolve before any startup side effects, * and the process-table read only happens on the rare fresh-marker path. - * Fails open everywhere: an unreadable marker or process table means launch. + * An unreadable marker fails open; after the bounded arming phase, an + * unreadable process table does too. */ export function shouldDeferLaunchForUpdateInstall(options: { isPackaged: boolean - userDataPath: string + appDataPath: string appVersion: string deps?: UpdateInstallLaunchGateDeps }): boolean { @@ -124,33 +168,38 @@ export function shouldDeferLaunchForUpdateInstall(options: { if (platform !== 'darwin' || !options.isPackaged) { return false } - const marker = readUpdateInstallHandoffMarker(options.userDataPath) + const executablePath = options.deps?.executablePath ?? process.execPath + const marker = readUpdateInstallHandoffMarker(options.appDataPath, executablePath) if (!marker) { return false } // A different running version means the install already happened (or was // rolled elsewhere) — this launch is the post-update relaunch. if (marker.appVersion !== options.appVersion) { - clearUpdateInstallHandoffMarker(options.userDataPath) + clearUpdateInstallHandoffMarker(options.appDataPath, executablePath) return false } const now = options.deps?.now ?? Date.now() - if (now - marker.createdAtMs > UPDATE_INSTALL_HANDOFF_MAX_AGE_MS) { - clearUpdateInstallHandoffMarker(options.userDataPath) + const markerAgeMs = now - marker.createdAtMs + if (markerAgeMs < 0 || markerAgeMs > UPDATE_INSTALL_HANDOFF_MAX_AGE_MS) { + clearUpdateInstallHandoffMarker(options.appDataPath, executablePath) return false } + if (markerAgeMs <= UPDATE_INSTALL_HANDOFF_ARMING_MS) { + return true + } let processList: string try { processList = (options.deps?.readProcessList ?? readProcessCommandList)() } catch { - clearUpdateInstallHandoffMarker(options.userDataPath) + clearUpdateInstallHandoffMarker(options.appDataPath, executablePath) return false } - if (isBundleShipItRunning(options.deps?.executablePath ?? process.execPath, processList)) { + if (isBundleShipItRunning(executablePath, processList)) { return true } // Same version and no installer left: the install died (aborted/failed). // Clear the marker so this and future launches proceed normally. - clearUpdateInstallHandoffMarker(options.userDataPath) + clearUpdateInstallHandoffMarker(options.appDataPath, executablePath) return false } diff --git a/src/main/updater-conflicting-app-instances.test.ts b/src/main/updater-conflicting-app-instances.test.ts index c1e46148ff1..5edebd376dd 100644 --- a/src/main/updater-conflicting-app-instances.test.ts +++ b/src/main/updater-conflicting-app-instances.test.ts @@ -1,47 +1,71 @@ -import { describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { findConflictingAppInstancePids, - parseSameExecutablePids + parseRunningApplicationPids } from './updater-conflicting-app-instances' const ORCA_BIN = '/Applications/Orca.app/Contents/MacOS/Orca' +const execFileMock = vi.hoisted(() => vi.fn()) -describe('parseSameExecutablePids', () => { - it('matches other pids running the exact executable path', () => { - const output = [ - ` 100 ${ORCA_BIN}`, - ' 200 /Applications/Orca.app/Contents/Frameworks/Orca Helper.app/Contents/MacOS/Orca Helper', - ` 300 ${ORCA_BIN}`, - ' 400 /usr/sbin/cfprefsd' - ].join('\n') +vi.mock('node:child_process', () => ({ execFile: execFileMock })) - expect(parseSameExecutablePids(output, ORCA_BIN, 100)).toEqual([300]) +beforeEach(() => { + execFileMock.mockReset() +}) + +describe('parseRunningApplicationPids', () => { + it('parses positive pids and excludes the current process', () => { + expect(parseRunningApplicationPids('100\n 300\n', 100)).toEqual([300]) }) - it('keeps executable paths containing spaces intact', () => { - const binWithSpaces = '/Users/dev/My Apps/Orca.app/Contents/MacOS/Orca' - const output = ` 512 ${binWithSpaces}\n 513 ${binWithSpaces} Helper` - - expect(parseSameExecutablePids(output, binWithSpaces, 1)).toEqual([512]) - }) - - it('ignores dev-shell noise and malformed rows', () => { - const output = ['not-a-row', ' -5 /bin/zsh', ` 42 ${ORCA_BIN}extra`].join('\n') - - expect(parseSameExecutablePids(output, ORCA_BIN, 1)).toEqual([]) + it('ignores malformed, negative, and zero rows', () => { + expect(parseRunningApplicationPids('not-a-row\n-5\n0\n42extra', 1)).toEqual([]) }) }) describe('findConflictingAppInstancePids', () => { it('reports other same-binary instances on darwin', async () => { + let readerArgs: [string, number] | null = null const pids = await findConflictingAppInstancePids({ platform: 'darwin', executablePath: ORCA_BIN, currentPid: 100, - readProcessList: async () => ` 100 ${ORCA_BIN}\n 777 ${ORCA_BIN}` + readRunningApplicationPids: async (executablePath, currentPid) => { + readerArgs = [executablePath, currentPid] + return '777' + } }) expect(pids).toEqual([777]) + expect(readerArgs).toEqual([ORCA_BIN, 100]) + }) + + it('uses one bounded AppKit query with path arguments kept out of the script', async () => { + execFileMock.mockImplementation((...args: unknown[]) => { + const callback = args[3] as (error: Error | null, stdout: string) => void + callback(null, '777\n') + }) + + const pids = await findConflictingAppInstancePids({ + platform: 'darwin', + executablePath: ORCA_BIN, + currentPid: 100 + }) + + expect(pids).toEqual([777]) + expect(execFileMock).toHaveBeenCalledTimes(1) + const [file, args, options] = execFileMock.mock.calls[0] + expect(file).toBe('/usr/bin/osascript') + expect(args).toEqual([ + '-l', + 'JavaScript', + '-e', + expect.stringContaining('NSWorkspace.sharedWorkspace.runningApplications'), + '--', + ORCA_BIN, + '100' + ]) + expect(options).toEqual(expect.objectContaining({ timeout: 2_000, maxBuffer: 64 * 1024 })) }) it('skips the check off darwin — Win/Linux installers own this', async () => { @@ -49,19 +73,19 @@ describe('findConflictingAppInstancePids', () => { platform: 'win32', executablePath: ORCA_BIN, currentPid: 100, - readProcessList: async () => ` 777 ${ORCA_BIN}` + readRunningApplicationPids: async () => '777' }) expect(pids).toEqual([]) }) - it('fails open when the process table is unreadable', async () => { + it('fails open when the application query is unavailable', async () => { const pids = await findConflictingAppInstancePids({ platform: 'darwin', executablePath: ORCA_BIN, currentPid: 100, - readProcessList: async () => { - throw new Error('ps timed out') + readRunningApplicationPids: async () => { + throw new Error('osascript timed out') } }) diff --git a/src/main/updater-conflicting-app-instances.ts b/src/main/updater-conflicting-app-instances.ts index 8fcacd39400..3618493d067 100644 --- a/src/main/updater-conflicting-app-instances.ts +++ b/src/main/updater-conflicting-app-instances.ts @@ -9,25 +9,66 @@ import { execFile } from 'node:child_process' // user sees "app quit but never relaunched, still on the old version". // Detecting those instances before the install handoff turns that silent // wedge into an actionable message. -const PROCESS_LIST_TIMEOUT_MS = 2_000 -// Why: a busy dev machine's per-user process table can exceed execFile's 1MB -// default; truncation would silently drop a conflicting instance. -const PROCESS_LIST_MAX_BYTES = 16 * 1024 * 1024 +const RUNNING_APPLICATION_QUERY_TIMEOUT_MS = 2_000 +const RUNNING_APPLICATION_QUERY_MAX_BYTES = 64 * 1024 -export type ProcessListReader = () => Promise +// Why: Squirrel identifies blockers as NSRunningApplication instances by +// bundle identity. An exact-executable `ps` scan also catches Orca CLI +// processes running through ELECTRON_RUN_AS_NODE, even though AppKit gives +// those processes no bundle id or URL and Squirrel does not wait for them. +const RUNNING_APPLICATION_QUERY = String.raw` +function run(argv) { + ObjC.import('AppKit') + const executablePath = argv[0] + const currentPid = Number(argv[1]) + const applications = $.NSWorkspace.sharedWorkspace.runningApplications + const pids = [] + for (let index = 0; index < applications.count; index += 1) { + const application = applications.objectAtIndex(index) + const executableUrl = application.executableURL + const bundleIdentifier = application.bundleIdentifier + const pid = Number(application.processIdentifier) + if ( + executableUrl && + bundleIdentifier && + String(ObjC.unwrap(executableUrl.path)) === executablePath && + pid !== currentPid + ) { + pids.push(String(pid)) + } + } + return pids.join('\n') +}` -function readCurrentUserProcessList(): Promise { +export type RunningApplicationPidReader = ( + executablePath: string, + currentPid: number +) => Promise + +function readCurrentUserRunningApplicationPids( + executablePath: string, + currentPid: number +): Promise { return new Promise((resolve, reject) => { - // Why: `-x` (without `-a`) scopes to the current user's processes, which - // mirrors NSRunningApplication's login-session scope — another macOS - // user's Orca does not block ShipIt and must not block the update here. - // On macOS `comm` is the full executable path, so instances launched from - // the same bundle match exactly whatever their argv or profile is. - // Pin the system binary because Orca hydrates PATH from the user's shell. + // Why: query AppKit in one bounded subprocess instead of launching a probe + // per candidate. Passing paths as argv avoids script interpolation, and + // osascript needs no Accessibility permission for NSWorkspace metadata. execFile( - '/bin/ps', - ['-xo', 'pid=,comm='], - { encoding: 'utf8', timeout: PROCESS_LIST_TIMEOUT_MS, maxBuffer: PROCESS_LIST_MAX_BYTES }, + '/usr/bin/osascript', + [ + '-l', + 'JavaScript', + '-e', + RUNNING_APPLICATION_QUERY, + '--', + executablePath, + String(currentPid) + ], + { + encoding: 'utf8', + timeout: RUNNING_APPLICATION_QUERY_TIMEOUT_MS, + maxBuffer: RUNNING_APPLICATION_QUERY_MAX_BYTES + }, (error, stdout) => { if (error) { reject(error) @@ -39,22 +80,15 @@ function readCurrentUserProcessList(): Promise { }) } -/** Parse `ps -xo pid=,comm=` output into pids whose executable path equals - * `executablePath`, excluding `currentPid`. Executable paths may contain - * spaces, so only the leading pid column is positional. */ -export function parseSameExecutablePids( - psOutput: string, - executablePath: string, - currentPid: number -): number[] { +export function parseRunningApplicationPids(output: string, currentPid: number): number[] { const pids: number[] = [] - for (const line of psOutput.split('\n')) { - const match = /^\s*(\d+)\s+(.+?)\s*$/.exec(line) - if (!match) { + for (const line of output.split('\n')) { + const normalized = line.trim() + if (!/^\d+$/.test(normalized)) { continue } - const pid = Number(match[1]) - if (pid !== currentPid && match[2] === executablePath) { + const pid = Number(normalized) + if (pid > 0 && pid !== currentPid) { pids.push(pid) } } @@ -65,7 +99,7 @@ export type ConflictingInstanceDeps = { platform?: NodeJS.Platform executablePath?: string currentPid?: number - readProcessList?: ProcessListReader + readRunningApplicationPids?: RunningApplicationPidReader } /** @@ -73,8 +107,8 @@ export type ConflictingInstanceDeps = { * * macOS-only by design: this mirrors Squirrel.Mac's pre-install wait/abort * semantics. The Windows and Linux installers manage running instances - * themselves. Fails open — an undetectable process table must never block an - * update install. + * themselves. Fails open — an unavailable application query must never block + * an update install. */ export async function findConflictingAppInstancePids( deps: ConflictingInstanceDeps = {} @@ -82,13 +116,14 @@ export async function findConflictingAppInstancePids( if ((deps.platform ?? process.platform) !== 'darwin') { return [] } + const executablePath = deps.executablePath ?? process.execPath + const currentPid = deps.currentPid ?? process.pid try { - const output = await (deps.readProcessList ?? readCurrentUserProcessList)() - return parseSameExecutablePids( - output, - deps.executablePath ?? process.execPath, - deps.currentPid ?? process.pid + const output = await (deps.readRunningApplicationPids ?? readCurrentUserRunningApplicationPids)( + executablePath, + currentPid ) + return parseRunningApplicationPids(output, currentPid) } catch { return [] } diff --git a/src/main/updater.mac-install.test.ts b/src/main/updater.mac-install.test.ts index 481e7c9c8a6..1f25815e1f3 100644 --- a/src/main/updater.mac-install.test.ts +++ b/src/main/updater.mac-install.test.ts @@ -198,7 +198,11 @@ describe('updater mac install handoff', () => { expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledWith(false, true) }) // The relaunch-race handoff marker must be down before the native call. - expect(writeHandoffMarkerMock).toHaveBeenCalledWith('/tmp/orca-test-user-data', '1.0.51') + expect(writeHandoffMarkerMock).toHaveBeenCalledWith( + '/tmp/orca-test-user-data', + process.execPath, + '1.0.51' + ) expect(writeHandoffMarkerMock.mock.invocationCallOrder[0]).toBeLessThan( autoUpdaterMock.quitAndInstall.mock.invocationCallOrder[0] ) diff --git a/src/main/updater.test.ts b/src/main/updater.test.ts index 77424cfc05b..2726038a4e6 100644 --- a/src/main/updater.test.ts +++ b/src/main/updater.test.ts @@ -1295,8 +1295,15 @@ 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') + expect(writeHandoffMarkerMock).toHaveBeenCalledWith( + '/tmp/orca-test-user-data', + process.execPath, + '1.0.51' + ) + expect(clearHandoffMarkerMock).toHaveBeenCalledWith( + '/tmp/orca-test-user-data', + process.execPath + ) } // Why: destructive prep runs only after quitAndInstall returns still in // progress; sync recovery clears flags first so PTYs stay alive. diff --git a/src/main/updater.ts b/src/main/updater.ts index 518bc616978..4db052edc0c 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -645,7 +645,7 @@ async function performQuitAndInstall(): Promise { if (process.platform === 'darwin') { // Why: lets a launch that races Squirrel's install back off instead of // aborting it (see update-install-launch-gate). Cleared on recovery. - writeUpdateInstallHandoffMarker(app.getPath('userData'), app.getVersion()) + writeUpdateInstallHandoffMarker(app.getPath('appData'), process.execPath, app.getVersion()) } // Why: invoke quitAndInstall before killAllPty/remove close listeners so a // sync 'error' (common "no filepath" path) recovers while windows and @@ -707,7 +707,7 @@ function resetQuitForUpdateState(): void { resetMacInstallState() // Why: install recovery keeps this process alive, so the handoff marker // would otherwise gate the user's next manual relaunch for no reason. - clearUpdateInstallHandoffMarker(app.getPath('userData')) + clearUpdateInstallHandoffMarker(app.getPath('appData'), process.execPath) } // Why: electron-updater often reports quitAndInstall failures via the 'error'