mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 16:02:56 +00:00
fix(updater): address second-round review findings on the install fence
From an independent adversarial review of the branch: 1. (medium) The installer-appearance window now starts at the first verified source-dead observation, not arm time. A wedged teardown holds the source until the 20s exit watchdog, which previously left launchd only 10s to expose ShipIt before the monitor declared installer-never-started and relaunched the old app into the install window. The monitor also takes a confirm probe before any failure: a live ShipIt cancels the failure and a late completion wins outright, so recovery can never race a live installer. 2. (low-medium) A fence armed on disk but not in memory (throw after the record reached disk, e.g. directory-fsync failure) could block every relaunch until the age cap. The arm catch now clears the attempt path unconditionally, which also cancels the just-spawned monitor. 3. (low) Version equality is now canonical (leading v, build metadata) via compareAppVersions in both the startup gate and the monitor, so a feed formatting variant cannot turn a successful install into a failure card. 4. (low) An orphaned heartbeat file with no attempt record is reclaimed at startup instead of living forever. Each fix carries a regression test.
This commit is contained in:
@@ -1,10 +1,13 @@
|
||||
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { afterEach, describe, expect, it } from 'vitest'
|
||||
import {
|
||||
areMacUpdateVersionsEqual,
|
||||
armMacUpdateInstallAttempt,
|
||||
decideMacUpdateInstallLaunch,
|
||||
getMacUpdateInstallAttemptPath,
|
||||
getMacUpdateInstallHeartbeatPath,
|
||||
getMacUpdateProcessIdentityState,
|
||||
MAC_UPDATE_INSTALL_ATTEMPT_SCHEMA_VERSION,
|
||||
MAC_UPDATE_INSTALL_ATTEMPT_STALE_MS,
|
||||
@@ -164,6 +167,52 @@ describe('macOS update install startup probes', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('macOS update version equality', () => {
|
||||
it('treats formatting variants of the same version as installed', () => {
|
||||
expect(areMacUpdateVersionsEqual('v1.4.192', '1.4.192')).toBe(true)
|
||||
expect(areMacUpdateVersionsEqual('1.4.192+build.7', '1.4.192')).toBe(true)
|
||||
expect(areMacUpdateVersionsEqual('1.4.192', '1.4.193')).toBe(false)
|
||||
expect(areMacUpdateVersionsEqual('not-a-version', 'also-not')).toBe(false)
|
||||
|
||||
expect(
|
||||
decideMacUpdateInstallLaunch({
|
||||
attempt: createAttempt({ targetVersion: '1.4.192' }),
|
||||
currentBundlePath: BUNDLE_PATH,
|
||||
currentVersion: 'v1.4.192',
|
||||
nowMs: 3_000,
|
||||
monitorAlive: true,
|
||||
shipItAlive: true
|
||||
})
|
||||
).toEqual({ action: 'allow-and-clear', reason: 'target-installed' })
|
||||
})
|
||||
})
|
||||
|
||||
describe('macOS update orphan heartbeat', () => {
|
||||
it('reclaims a heartbeat file that has no attempt record', () => {
|
||||
const appDataPath = createTempDir()
|
||||
const attemptPath = getMacUpdateInstallAttemptPath(appDataPath)
|
||||
writeMacUpdateInstallHeartbeat(attemptPath, {
|
||||
schemaVersion: MAC_UPDATE_INSTALL_ATTEMPT_SCHEMA_VERSION,
|
||||
attemptId: 'orphaned-attempt',
|
||||
heartbeatAtMs: 5_000
|
||||
})
|
||||
|
||||
expect(
|
||||
resolveMacUpdateInstallStartup({
|
||||
appDataPath,
|
||||
appVersion: '1.4.191',
|
||||
executablePath: EXECUTABLE_PATH,
|
||||
isPackaged: true,
|
||||
platform: 'darwin',
|
||||
nowMs: 6_000,
|
||||
readProcessStartedAtMs: () => null,
|
||||
readProcessList: () => ''
|
||||
})
|
||||
).toEqual({ action: 'allow', reason: 'no-attempt' })
|
||||
expect(existsSync(getMacUpdateInstallHeartbeatPath(attemptPath))).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('macOS update install arming identity', () => {
|
||||
it.runIf(process.platform === 'darwin')(
|
||||
'refuses to fence on a spawned pid whose recorded start time is implausible',
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { randomUUID } from 'node:crypto'
|
||||
import { existsSync } from 'node:fs'
|
||||
import { join, resolve } from 'node:path'
|
||||
import { isValidAppVersion } from '../shared/app-version'
|
||||
import { compareAppVersions, isValidAppVersion } from '../shared/app-version'
|
||||
import { spawnProcess } from '../shared/child-process/run-process'
|
||||
import { getProcessStartedAtMs } from './daemon/daemon-process-start-time'
|
||||
import {
|
||||
@@ -17,6 +17,7 @@ export {
|
||||
} from './mac-update-install-process-probes'
|
||||
import {
|
||||
clearMacUpdateInstallAttempt,
|
||||
clearMacUpdateInstallHeartbeat,
|
||||
getMacUpdateInstallAttemptPath,
|
||||
MAC_UPDATE_INSTALL_ATTEMPT_SCHEMA_VERSION,
|
||||
readMacUpdateInstallAttempt,
|
||||
@@ -58,6 +59,19 @@ export type MacUpdateInstallLaunchDecision =
|
||||
}
|
||||
| { action: 'block'; reason: 'active-install' | 'shipit-alive' }
|
||||
|
||||
/**
|
||||
* Canonical version equality: a feed may carry a leading `v` or build metadata while the
|
||||
* bundle plist is bare. A successful install must never read as a failure over formatting.
|
||||
*/
|
||||
export function areMacUpdateVersionsEqual(left: string, right: string): boolean {
|
||||
if (left === right) {
|
||||
return true
|
||||
}
|
||||
return (
|
||||
isValidAppVersion(left) && isValidAppVersion(right) && compareAppVersions(left, right) === 0
|
||||
)
|
||||
}
|
||||
|
||||
export function resolveMacUpdateBundlePath(executablePath: string): string {
|
||||
const bundlePath = resolve(executablePath, '..', '..', '..')
|
||||
if (!bundlePath.toLowerCase().endsWith('.app')) {
|
||||
@@ -83,7 +97,7 @@ export function decideMacUpdateInstallLaunch(options: {
|
||||
if (!macPathsEqual(attempt.targetBundlePath, options.currentBundlePath)) {
|
||||
return { action: 'allow', reason: 'different-bundle' }
|
||||
}
|
||||
if (options.currentVersion === attempt.targetVersion) {
|
||||
if (areMacUpdateVersionsEqual(options.currentVersion, attempt.targetVersion)) {
|
||||
return { action: 'allow-and-clear', reason: 'target-installed' }
|
||||
}
|
||||
const ageMs = options.nowMs - attempt.createdAtMs
|
||||
@@ -233,6 +247,8 @@ function resolveMacUpdateInstallStartupUnsafe(options: {
|
||||
const attemptPath = getMacUpdateInstallAttemptPath(options.appDataPath)
|
||||
const attempt = readMacUpdateInstallAttempt(attemptPath)
|
||||
if (!attempt) {
|
||||
// Why: a heartbeat with no attempt record is an orphan from a lost race; reclaim it.
|
||||
clearMacUpdateInstallHeartbeat(attemptPath)
|
||||
return { action: 'allow', reason: 'no-attempt' }
|
||||
}
|
||||
const monitorAlive =
|
||||
|
||||
@@ -121,6 +121,86 @@ describe('macOS update monitor probe safety', () => {
|
||||
).toEqual({ action: 'fail', reason: 'install-timed-out' })
|
||||
})
|
||||
|
||||
it('measures the installer-appearance window from first verified source death', () => {
|
||||
const attempt = createAttempt()
|
||||
const wedgedQuitReleaseMs = attempt.createdAtMs + 25_000
|
||||
// 29s after arm but only 4s after the source verifiably died: not a failure yet.
|
||||
expect(
|
||||
decideMacUpdateMonitorStep({
|
||||
attempt,
|
||||
observation: { bundleVersion: attempt.sourceVersion, shipIt: 'absent', source: 'dead' },
|
||||
nowMs: attempt.createdAtMs + 29_000,
|
||||
shipItSeen: false,
|
||||
shipItMissingSinceMs: null,
|
||||
sourceDeadSinceMs: wedgedQuitReleaseMs
|
||||
}).action
|
||||
).toBe('continue')
|
||||
// The full window after source death has elapsed: now it is a failure.
|
||||
expect(
|
||||
decideMacUpdateMonitorStep({
|
||||
attempt,
|
||||
observation: { bundleVersion: attempt.sourceVersion, shipIt: 'absent', source: 'dead' },
|
||||
nowMs: wedgedQuitReleaseMs + MAC_UPDATE_MONITOR_SHIPIT_APPEARANCE_MS,
|
||||
shipItSeen: false,
|
||||
shipItMissingSinceMs: null,
|
||||
sourceDeadSinceMs: wedgedQuitReleaseMs
|
||||
})
|
||||
).toEqual({ action: 'fail', reason: 'installer-never-started' })
|
||||
})
|
||||
|
||||
it('cancels a failure when the confirm probe finds ShipIt alive again', async () => {
|
||||
const { attempt, path } = createAttemptFile()
|
||||
const launchRecovery = vi.fn().mockResolvedValue(true)
|
||||
let calls = 0
|
||||
|
||||
await expect(
|
||||
runMacUpdateInstallMonitor({
|
||||
attemptPath: path,
|
||||
attemptId: attempt.attemptId,
|
||||
now: () => attempt.createdAtMs + MAC_UPDATE_MONITOR_SHIPIT_APPEARANCE_MS + 1,
|
||||
wait: async () => {},
|
||||
observe: async () => {
|
||||
calls += 1
|
||||
// 1: verified absent, source dead -> fail decision. 2: confirm probe sees ShipIt
|
||||
// alive -> failure cancelled. 3: install completes.
|
||||
if (calls === 1) {
|
||||
return { bundleVersion: attempt.sourceVersion, shipIt: 'absent', source: 'dead' }
|
||||
}
|
||||
if (calls === 2) {
|
||||
return { bundleVersion: attempt.sourceVersion, shipIt: 'alive', source: 'dead' }
|
||||
}
|
||||
return { bundleVersion: attempt.targetVersion, shipIt: 'absent', source: 'dead' }
|
||||
},
|
||||
launchRecovery
|
||||
})
|
||||
).resolves.toBe('completed')
|
||||
expect(launchRecovery).not.toHaveBeenCalled()
|
||||
expect(readMacUpdateInstallAttempt(path)).toBeNull()
|
||||
})
|
||||
|
||||
it('completes instead of failing when the confirm probe sees the target version', async () => {
|
||||
const { attempt, path } = createAttemptFile()
|
||||
const launchRecovery = vi.fn().mockResolvedValue(true)
|
||||
let calls = 0
|
||||
|
||||
await expect(
|
||||
runMacUpdateInstallMonitor({
|
||||
attemptPath: path,
|
||||
attemptId: attempt.attemptId,
|
||||
now: () => attempt.createdAtMs + MAC_UPDATE_MONITOR_TIMEOUT_MS,
|
||||
wait: async () => {},
|
||||
observe: async () => {
|
||||
calls += 1
|
||||
return calls === 1
|
||||
? { bundleVersion: attempt.sourceVersion, shipIt: 'absent', source: 'dead' }
|
||||
: { bundleVersion: attempt.targetVersion, shipIt: 'absent', source: 'dead' }
|
||||
},
|
||||
launchRecovery
|
||||
})
|
||||
).resolves.toBe('completed')
|
||||
expect(launchRecovery).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not resurrect an attempt cleared mid-observation via the heartbeat write', async () => {
|
||||
const { attempt, path } = createAttemptFile()
|
||||
let observations = 0
|
||||
|
||||
@@ -105,6 +105,8 @@ describe('macOS update install monitor', () => {
|
||||
const observations = [
|
||||
{ bundleVersion: attempt.sourceVersion, shipIt: 'alive' as const, source: 'dead' as const },
|
||||
{ bundleVersion: attempt.sourceVersion, shipIt: 'absent' as const, source: 'dead' as const },
|
||||
{ bundleVersion: attempt.sourceVersion, shipIt: 'absent' as const, source: 'dead' as const },
|
||||
// Consumed by the pre-recovery confirm probe: still gone, still the old version.
|
||||
{ bundleVersion: attempt.sourceVersion, shipIt: 'absent' as const, source: 'dead' as const }
|
||||
]
|
||||
const times = [2_000, 3_000, 9_000]
|
||||
@@ -153,7 +155,9 @@ describe('macOS update install monitor', () => {
|
||||
sourcePid: process.pid,
|
||||
sourceStartedAtMs: 1
|
||||
})
|
||||
const times = [31_001, 15 * 60_000 + 1_001]
|
||||
// First verified source-dead observation starts the appearance window; the second
|
||||
// poll lands past that window without reaching the overall timeout.
|
||||
const times = [31_001, 61_002]
|
||||
const launchRecovery = vi.fn().mockResolvedValue(true)
|
||||
|
||||
await expect(
|
||||
|
||||
@@ -2,6 +2,7 @@ import { readFile } from 'node:fs/promises'
|
||||
import { join } from 'node:path'
|
||||
import { runProcess, spawnProcess } from '../shared/child-process/run-process'
|
||||
import {
|
||||
areMacUpdateVersionsEqual,
|
||||
clearMacUpdateInstallAttempt,
|
||||
clearMacUpdateInstallHeartbeat,
|
||||
failMacUpdateInstallAttemptIfCurrent,
|
||||
@@ -40,9 +41,14 @@ export function decideMacUpdateMonitorStep(options: {
|
||||
nowMs: number
|
||||
shipItSeen: boolean
|
||||
shipItMissingSinceMs: number | null
|
||||
/** First verified source-dead observation; the appearance window must not start earlier. */
|
||||
sourceDeadSinceMs?: number | null
|
||||
}): MacUpdateMonitorDecision {
|
||||
const { attempt, observation, nowMs } = options
|
||||
if (observation.bundleVersion === attempt.targetVersion) {
|
||||
if (
|
||||
observation.bundleVersion !== null &&
|
||||
areMacUpdateVersionsEqual(observation.bundleVersion, attempt.targetVersion)
|
||||
) {
|
||||
return { action: 'complete' }
|
||||
}
|
||||
if (nowMs - attempt.createdAtMs >= MAC_UPDATE_MONITOR_TIMEOUT_MS) {
|
||||
@@ -62,7 +68,14 @@ export function decideMacUpdateMonitorStep(options: {
|
||||
return { action: 'continue', shipItSeen, shipItMissingSinceMs: options.shipItMissingSinceMs }
|
||||
}
|
||||
if (!shipItSeen) {
|
||||
if (nowMs - attempt.createdAtMs >= MAC_UPDATE_MONITOR_SHIPIT_APPEARANCE_MS) {
|
||||
// Why the max: a wedged teardown can hold the source alive until the 20s exit watchdog,
|
||||
// eating most of the appearance window. Launchd deserves the full window measured from
|
||||
// when the source verifiably died, or a slow quit turns into a relaunch into ShipIt.
|
||||
const appearanceBaselineMs = Math.max(
|
||||
attempt.createdAtMs,
|
||||
options.sourceDeadSinceMs ?? attempt.createdAtMs
|
||||
)
|
||||
if (nowMs - appearanceBaselineMs >= MAC_UPDATE_MONITOR_SHIPIT_APPEARANCE_MS) {
|
||||
return { action: 'fail', reason: 'installer-never-started' }
|
||||
}
|
||||
return { action: 'continue', shipItSeen: false, shipItMissingSinceMs: null }
|
||||
@@ -90,6 +103,7 @@ export async function runMacUpdateInstallMonitor(options: {
|
||||
}
|
||||
let shipItSeen = false
|
||||
let shipItMissingSinceMs: number | null = null
|
||||
let sourceDeadSinceMs: number | null = null
|
||||
|
||||
for (;;) {
|
||||
const current = readMacUpdateInstallAttempt(options.attemptPath)
|
||||
@@ -100,18 +114,46 @@ export async function runMacUpdateInstallMonitor(options: {
|
||||
attempt = current
|
||||
const nowMs = now()
|
||||
const observation = await (options.observe ?? observeInstall)(attempt)
|
||||
if (observation.source === 'dead') {
|
||||
sourceDeadSinceMs ??= nowMs
|
||||
} else if (observation.source === 'alive') {
|
||||
sourceDeadSinceMs = null
|
||||
}
|
||||
const decision = decideMacUpdateMonitorStep({
|
||||
attempt,
|
||||
observation,
|
||||
nowMs,
|
||||
shipItSeen,
|
||||
shipItMissingSinceMs
|
||||
shipItMissingSinceMs,
|
||||
sourceDeadSinceMs
|
||||
})
|
||||
if (decision.action === 'complete') {
|
||||
clearMacUpdateInstallAttempt(options.attemptPath, attempt.attemptId)
|
||||
return 'completed'
|
||||
}
|
||||
if (decision.action === 'fail') {
|
||||
// Why a confirm probe: the failing observation is up to a poll old, and launching
|
||||
// recovery against a live ShipIt would relaunch the old app into the install window —
|
||||
// the exact race this monitor exists to prevent. A late completion wins outright.
|
||||
const confirm = await (options.observe ?? observeInstall)(attempt)
|
||||
if (
|
||||
confirm.bundleVersion !== null &&
|
||||
areMacUpdateVersionsEqual(confirm.bundleVersion, attempt.targetVersion)
|
||||
) {
|
||||
clearMacUpdateInstallAttempt(options.attemptPath, attempt.attemptId)
|
||||
return 'completed'
|
||||
}
|
||||
if (decision.reason !== 'install-timed-out' && confirm.shipIt === 'alive') {
|
||||
shipItSeen = true
|
||||
shipItMissingSinceMs = null
|
||||
writeMacUpdateInstallHeartbeat(options.attemptPath, {
|
||||
schemaVersion: MAC_UPDATE_INSTALL_ATTEMPT_SCHEMA_VERSION,
|
||||
attemptId: attempt.attemptId,
|
||||
heartbeatAtMs: nowMs
|
||||
})
|
||||
await wait(MAC_UPDATE_MONITOR_POLL_MS)
|
||||
continue
|
||||
}
|
||||
// Why guarded: cleanup may have cleared or replaced the attempt while this step observed;
|
||||
// writing an unconditional failure would resurrect a finished attempt and launch a
|
||||
// recovery app nobody asked for.
|
||||
|
||||
@@ -443,7 +443,11 @@ describe('updater mac install handoff', () => {
|
||||
// abort an update that would otherwise succeed.
|
||||
await vi.waitFor(() => expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledOnce())
|
||||
expect(armMacUpdateInstallAttemptMock).toHaveBeenCalledOnce()
|
||||
expect(clearMacUpdateInstallAttemptMock).not.toHaveBeenCalled()
|
||||
// Why: arming can throw after the record reached disk; the catch must clear the path
|
||||
// unguarded or a live fence would block every relaunch until the age cap.
|
||||
expect(clearMacUpdateInstallAttemptMock).toHaveBeenCalledWith(
|
||||
'/tmp/orca-update-install-attempt.json'
|
||||
)
|
||||
} finally {
|
||||
if (resourcesPathDescriptor) {
|
||||
Object.defineProperty(process, 'resourcesPath', resourcesPathDescriptor)
|
||||
|
||||
@@ -104,6 +104,15 @@ export abstract class UpdaterInstallSupport extends UpdaterCheckState {
|
||||
{ message: error instanceof Error ? error.message : String(error) },
|
||||
{ level: 'warn', message: 'Proceeding with the macOS install without the relaunch fence' }
|
||||
)
|
||||
// Why unconditional: arming can throw after the record reached disk (e.g. a directory
|
||||
// fsync failure), leaving a live fence nothing in this process would ever clear — which
|
||||
// would silently block every relaunch until the age cap. Clearing also cancels the
|
||||
// just-spawned monitor within one poll.
|
||||
try {
|
||||
clearMacUpdateInstallAttempt(getMacUpdateInstallAttemptPath(app.getPath('appData')))
|
||||
} catch {
|
||||
// The stale attempt self-expires via the age cap; never let cleanup mask the install.
|
||||
}
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user