diff --git a/src/main/updater-conflicting-app-instances.test.ts b/src/main/updater-conflicting-app-instances.test.ts index 985d3c9aa98..de47af2d980 100644 --- a/src/main/updater-conflicting-app-instances.test.ts +++ b/src/main/updater-conflicting-app-instances.test.ts @@ -119,15 +119,22 @@ describe('runningApplicationQueryOutput', () => { }) describe('describeConflictingAppInstances', () => { + it('does not promise a retry the card has no button for', () => { + // A non-retryable error leaves no primary action, and Settings shows + // "Restart to Update" only for a downloaded state — so there is nothing to + // try again from once the other copy is quit. + expect(describeConflictingAppInstances([270])).not.toContain('try again') + }) + it('names a single blocking pid, and reads as singular throughout', () => { expect(describeConflictingAppInstances([270])).toBe( - 'Another copy of Orca is running (PID 270). macOS cannot replace the app while it is open — quit it, then try again.' + 'Another copy of Orca is running (PID 270). macOS cannot replace the app while it is open — quit it.' ) }) it('reads as plural for more than one', () => { expect(describeConflictingAppInstances([270, 811])).toBe( - '2 other copies of Orca are running (PIDs 270, 811). macOS cannot replace the app while they are open — quit them, then try again.' + '2 other copies of Orca are running (PIDs 270, 811). macOS cannot replace the app while they are open — quit them.' ) }) @@ -154,31 +161,39 @@ describe('conflicting-instance detection strategy', () => { /** The condition deciding which running applications count as blockers. */ const blockerCondition = query.match(/if\s*\(([\s\S]*?)\)\s*\{/)?.[1] ?? '' - it('reads its blocker set from AppKit, not the process table', () => { + it('enumerates candidates through LaunchServices, which is what omits the CLI', () => { + // THIS is the assertion that keeps Orca CLI processes out of the blocker set. + // The CLI runs from the same bundle executable under ELECTRON_RUN_AS_NODE, so + // a `ps`-style scan on the executable path would report every CLI invocation + // as a blocker and refuse updates on any machine that uses the CLI. Such a + // process never registers with LaunchServices, so `runningApplications` does + // not list it at all — the omission is in the enumeration, not in any filter. + // Measured 2026-09-07: `ps` found 4 processes on this bundle executable and + // this query returned 1. expect(query).not.toBe('') expect(query).toContain('NSWorkspace.sharedWorkspace.runningApplications') }) - it('identifies blockers by bundle identity, so Orca CLI processes are not counted', () => { - // The Orca CLI runs from the SAME bundle executable under - // ELECTRON_RUN_AS_NODE, so `ps`-style matching on the executable path - // reports every CLI invocation as a blocking app instance and refuses the - // update outright on any machine that uses the CLI. AppKit gives those - // processes no bundle identity and Squirrel does not wait for them, so the - // non-null bundleIdentifier requirement is what makes this set match - // Squirrel's. Measured on a live machine: three processes shared the bundle - // executable path (the app plus two `ELECTRON_RUN_AS_NODE` CLI processes) - // and this query returned only the app. + it('still requires a bundle identity, as defence in depth rather than the filter', () => { + // Deliberately NOT described as the CLI exclusion: the same measurement found + // 0 of 93 running applications with a null bundleIdentifier, and a variant + // with this clause removed returned the identical single pid. It filters + // nothing observed today. It stays because NSRunningApplication.bundleIdentifier + // is documented nil-able and an unbundled process is not one Squirrel waits + // for — so removing it would widen the set on a machine we have not measured. expect(blockerCondition).not.toBe('') - // Order- and whitespace-independent, so reformatting cannot redden this. + // Order- and whitespace-independent, so reformatting cannot redden these. expect(blockerCondition).toContain('bundleIdentifier') expect(blockerCondition).toContain('executableUrl') expect(blockerCondition).toContain('executablePath') }) it('never enumerates blockers from the process table', () => { - expect(source).not.toMatch(/['"`]\/bin\/ps['"`]/) - expect(source).not.toMatch(/\bpgrep\b/) + // Why the query and not the file: an earlier version matched only a quoted + // `/bin/ps`, so a bare `/bin/ps -A` at line start sailed through it. + expect(query).not.toMatch(/\/bin\/ps\b/) + expect(query).not.toMatch(/\bpgrep\b/) + expect(query).not.toMatch(/\bNSProcessInfo\b/) }) it('spawns through the shared runner, not node:child_process', () => { diff --git a/src/main/updater-conflicting-app-instances.ts b/src/main/updater-conflicting-app-instances.ts index 62c38b27e19..0fcc13c3fb3 100644 --- a/src/main/updater-conflicting-app-instances.ts +++ b/src/main/updater-conflicting-app-instances.ts @@ -11,12 +11,23 @@ import { runProcess } from '../shared/child-process/run-process' const RUNNING_APPLICATION_QUERY_TIMEOUT_MS = 2_000 const RUNNING_APPLICATION_QUERY_MAX_BYTES = 64 * 1024 -// Why AppKit and not `ps`: the Orca CLI runs from this same executable under -// ELECTRON_RUN_AS_NODE, so an exact-executable process scan reports every CLI -// invocation as a blocker and refuses updates on any machine that uses the CLI. -// AppKit gives those processes no bundle identity, and Squirrel does not wait -// for them either — so requiring a non-null bundleIdentifier matches Squirrel's -// own blocking set. See the regression in the sibling test file. +// Why AppKit and not `ps`: the Orca CLI runs from this same bundle executable +// under ELECTRON_RUN_AS_NODE, so an exact-executable process scan reports every +// CLI invocation as a blocker and refuses updates on any machine that uses the +// CLI. Squirrel does not wait for those processes, and neither does this. +// +// The exclusion happens at the enumeration, not in the filter below: a process +// under ELECTRON_RUN_AS_NODE never registers with LaunchServices, so +// `runningApplications` does not list it at all. Measured 2026-09-07 — `ps` +// found 4 processes on this bundle executable, this query returned 1, and a +// variant with the `bundleIdentifier` requirement removed also returned 1. +// So `NSWorkspace.sharedWorkspace.runningApplications` is the load-bearing +// choice; that is what the sibling test protects. +// +// `bundleIdentifier` is kept as defence in depth, not as the mechanism: the same +// measurement found 0 of 93 running applications with a null identifier, so it +// filtered nothing here — but `NSRunningApplication.bundleIdentifier` is +// documented nil-able, and an unbundled process is not one Squirrel waits for. const RUNNING_APPLICATION_QUERY = String.raw` function run(argv) { ObjC.import('AppKit') @@ -78,11 +89,16 @@ async function readRunningApplicationPids( * A probe that could not finish has proved nothing about who is running, and * naming a blocker on that basis would refuse an install the user could have had. * - * `outputTruncated` is the third way to get a partial list and the only one that - * arrives with a clean exit: the bounded sink clips at `maxOutputBytes` and says - * so precisely because callers that parse the output have to tell a short answer + * `outputTruncated` is a third way to get a partial list, and one that arrives + * with a clean exit: the bounded sink clips at `maxOutputBytes` and says so + * precisely because callers that parse the output have to tell a short answer * from a clipped one. Unreachable in practice — the cap holds thousands of pids — * but this is the fail-open path, where the contract is the safety property. + * + * It is not the only clean-exit partial: a swallowed stdout stream `error` also + * shortens the list without failing the run. Every such case can only DROP pids, + * which degrades toward the pre-fix behaviour of not naming a blocker — never + * toward refusing an install that would have worked. */ export function runningApplicationQueryOutput(result: { timedOut: boolean @@ -146,7 +162,16 @@ export async function findConflictingAppInstancePids( const MAX_REPORTED_CONFLICTING_PIDS = 5 -/** Actionable copy for the update card: what is blocking, and what to do. */ +/** + * Card copy: what is blocking, and the one step the user can actually take. + * + * Why it stops at "quit it" and does not say "then try again": a non-retryable + * error leaves the card with no primary action, and Settings renders its own + * "Restart to Update" only for a `downloaded` state, so after quitting the other + * copy there is no surface to try again from. Instructing an action that exists + * nowhere is worse than instructing one fewer step. Restoring the retry sentence + * needs a real recovery action first (STA follow-up). + */ export function describeConflictingAppInstances(pids: readonly number[]): string { const reported = pids.slice(0, MAX_REPORTED_CONFLICTING_PIDS) const suffix = pids.length > reported.length ? ', …' : '' @@ -157,5 +182,5 @@ export function describeConflictingAppInstances(pids: readonly number[]): string `${pids.length} other copies of Orca are running (PIDs ${reported.join(', ')}${suffix})`, 'they are open — quit them' ] - return `${subject}. macOS cannot replace the app while ${closing}, then try again.` + return `${subject}. macOS cannot replace the app while ${closing}.` } diff --git a/src/main/updater.conflicting-instance-install.test.ts b/src/main/updater.conflicting-instance-install.test.ts index 871abdd871d..a5aa5b49d48 100644 --- a/src/main/updater.conflicting-instance-install.test.ts +++ b/src/main/updater.conflicting-instance-install.test.ts @@ -119,6 +119,23 @@ describe('macOS install blocked by other running app instances', () => { expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledTimes(1) }) + it('installs anyway when the conflict scan throws, without latching or escaping', async () => { + // The scan runs outside the install span's catch and its claim is held across + // the await, so a rejection would both escape unhandled and latch every later + // install into `quit_and_install_ignored` — no card, no recovery short of a + // relaunch. Failing open is the probe's own contract: an unavailable scan + // must never block an install. The probe cannot throw today; this pins that + // a future one changing that cannot strand the user. + findConflictingAppInstancePidsMock.mockRejectedValueOnce(new Error('probe exploded')) + const { setupAutoUpdater, quitAndInstall } = await loadUpdaterModule() + setupAutoUpdater({ webContents: { send: vi.fn() } } as never) + + quitAndInstall() + await vi.advanceTimersByTimeAsync(100) + + expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledTimes(1) + }) + it('ignores a duplicate install request that arrives during the conflict scan', async () => { let releaseScan: (pids: number[]) => void = () => {} findConflictingAppInstancePidsMock.mockReturnValue( diff --git a/src/main/updater/updater-install-execution.ts b/src/main/updater/updater-install-execution.ts index 1554c66b492..271cd84021b 100644 --- a/src/main/updater/updater-install-execution.ts +++ b/src/main/updater/updater-install-execution.ts @@ -65,7 +65,25 @@ export abstract class UpdaterInstallExecution extends UpdaterPackageRecovery { // every running instance of this bundle to exit and aborts if one appears // mid-install, so quitting into a doomed handoff strands the user on the // old version with no window and no explanation. - const conflictingInstancePids = await findConflictingAppInstancePids() + // + // Why this is caught even though the probe fails open internally and cannot + // currently throw: it runs OUTSIDE the span's catch below, and the claim + // above is held across its await. A rejection would both escape as an + // unhandled rejection and latch the claim, so every later install would + // return `quit_and_install_ignored` with no card and no recovery short of + // relaunching — the same silent wedge this guard exists to remove, reached + // from the other side. Proceeding is the probe's own contract: an + // unavailable scan must never block an install. + let conflictingInstancePids: number[] = [] + try { + conflictingInstancePids = await findConflictingAppInstancePids() + } catch (error) { + recordUpdaterLifecycle( + 'quit_and_install_conflict_scan_failed', + { errorType: error instanceof Error ? error.name : typeof error }, + { level: 'warn', message: 'Could not check for other running app instances' } + ) + } if (conflictingInstancePids.length > 0) { // Nothing else has been armed yet, so releasing the claim is the whole rollback. this.quitAndInstallInProgress = false