refactor(updater): spawn the instance probe through the shared runner

The conflict detector reached for node:child_process directly, which the spawn
boundary guard refuses: those decisions belong to run-process, and an exception
would have to argue itself onto a shrink-only allowlist.

runProcess reports a non-zero exit or a timeout as data rather than throwing, so
the fail-open contract now has to be explicit — a probe that did not finish has
proved nothing about who is running, and its partial output must not name a
blocker and refuse an install the user could have had.
This commit is contained in:
Merge Sim
2026-09-07 02:22:00 -07:00
parent abae7783aa
commit 6eae6a6d9a
2 changed files with 71 additions and 31 deletions
@@ -4,7 +4,8 @@ import { describe, expect, it, vi } from 'vitest'
import {
describeConflictingAppInstances,
findConflictingAppInstancePids,
parseRunningApplicationPids
parseRunningApplicationPids,
runningApplicationQueryOutput
} from './updater-conflicting-app-instances'
const APP_EXECUTABLE = '/Applications/Orca.app/Contents/MacOS/Orca'
@@ -68,6 +69,30 @@ describe('findConflictingAppInstancePids', () => {
})
})
describe('runningApplicationQueryOutput', () => {
// Fail-open is the property that keeps a broken probe from blocking updates,
// and the runner reports these as data rather than throwing — so each one is a
// path that would otherwise look like a successful "no blockers" answer, or
// worse, like a partial list of them.
it('passes through the output of a query that exited cleanly', () => {
expect(runningApplicationQueryOutput({ timedOut: false, code: 0, stdout: '270\n' })).toBe(
'270\n'
)
})
it('discards partial output from a timed-out query', () => {
expect(runningApplicationQueryOutput({ timedOut: true, code: null, stdout: '270\n' })).toBe('')
})
it('discards output from a query that exited non-zero', () => {
expect(runningApplicationQueryOutput({ timedOut: false, code: 1, stdout: '270\n' })).toBe('')
})
it('discards output from a query killed by a signal', () => {
expect(runningApplicationQueryOutput({ timedOut: false, code: null, stdout: '270\n' })).toBe('')
})
})
describe('describeConflictingAppInstances', () => {
it('names a single blocking pid', () => {
expect(describeConflictingAppInstances([270])).toBe(
@@ -110,4 +135,11 @@ describe('conflicting-instance detection strategy', () => {
expect(source).not.toMatch(/['"`]\/bin\/ps['"`]/)
expect(source).not.toMatch(/\bpgrep\b/)
})
it('spawns through the shared runner, not node:child_process', () => {
// The tree-level guard in src/shared/child-process owns this rule; asserting
// it here too keeps the reason next to the code that has to obey it.
expect(source).not.toContain('node:child_process')
expect(source).toContain("from '../shared/child-process/run-process'")
})
})
+38 -30
View File
@@ -1,4 +1,4 @@
import { execFile } from 'node:child_process'
import { runProcess } from '../shared/child-process/run-process'
// Why: Squirrel.Mac's ShipIt waits for EVERY running instance of the target
// bundle to exit before it installs, and aborts with "App Still Running Error"
@@ -46,36 +46,44 @@ export type RunningApplicationPidReader = (
currentPid: number
) => Promise<string>
function readRunningApplicationPids(executablePath: string, currentPid: number): Promise<string> {
return new Promise((resolve, reject) => {
// Why: one bounded subprocess instead of a probe per candidate. Paths go
// through argv so nothing is interpolated into the script, and NSWorkspace
// metadata needs no Accessibility permission.
execFile(
'/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)
return
}
resolve(stdout)
}
)
async function readRunningApplicationPids(
executablePath: string,
currentPid: number
): Promise<string> {
// Why: one bounded subprocess instead of a probe per candidate. Paths go
// through argv so nothing is interpolated into the script, and NSWorkspace
// metadata needs no Accessibility permission.
const result = await runProcess({
program: '/usr/bin/osascript',
args: [
'-l',
'JavaScript',
'-e',
RUNNING_APPLICATION_QUERY,
'--',
executablePath,
String(currentPid)
],
timeoutMs: RUNNING_APPLICATION_QUERY_TIMEOUT_MS,
maxOutputBytes: RUNNING_APPLICATION_QUERY_MAX_BYTES
})
return runningApplicationQueryOutput(result)
}
/**
* Output only from a query that actually finished.
*
* Why it is a decision at all: `runProcess` reports a non-zero exit or a timeout
* as data rather than throwing, so partial stdout arrives looking like an answer.
* 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.
*/
export function runningApplicationQueryOutput(result: {
timedOut: boolean
code: number | null
stdout: string
}): string {
return result.timedOut || result.code !== 0 ? '' : result.stdout
}
export function parseRunningApplicationPids(output: string, currentPid: number): number[] {