mirror of
https://github.com/stablyai/orca.git
synced 2026-10-08 08:02:32 +00:00
fix(updater): correct the exclusion mechanism, the copy, and the scan's failure path
Three corrections from review, one of them to a claim this PR made about itself. The CLI processes are not excluded by the bundleIdentifier clause. Measured: ps found 4 processes on the bundle executable, the query returned 1, and a variant without that clause also returned 1 — 0 of 93 running applications had a null identifier. They are absent because a process under ELECTRON_RUN_AS_NODE never registers with LaunchServices, so runningApplications never lists it. The clause stays as defence in depth (the field is documented nil-able); the comments and test names now say which line does the work. The card promised a retry with nowhere to perform it: a non-retryable error leaves no primary action, and Settings shows Restart to Update only for a downloaded state. Drop 'then try again' rather than instruct a missing surface. The scan runs outside the install span's catch while holding the in-progress claim, so a rejection would escape unhandled and latch every later install into quit_and_install_ignored. Catch and proceed — failing open is the probe's own contract.
This commit is contained in:
@@ -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', () => {
|
||||
|
||||
@@ -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}.`
|
||||
}
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user