From 1b3077382d1d77bb459ec6c937940d64f5fde812 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Fri, 4 Sep 2026 14:49:26 -0700 Subject: [PATCH] fix(updater): avoid deleting shared ShipIt state --- src/main/mac-update-install-marker.test.ts | 71 ++-------------------- src/main/mac-update-install-marker.ts | 9 +-- src/main/mac-update-shipit-state.ts | 56 ----------------- src/shared/shipit-liveness.ts | 10 --- 4 files changed, 6 insertions(+), 140 deletions(-) delete mode 100644 src/main/mac-update-shipit-state.ts diff --git a/src/main/mac-update-install-marker.test.ts b/src/main/mac-update-install-marker.test.ts index fcde575bead..f84c2e74856 100644 --- a/src/main/mac-update-install-marker.test.ts +++ b/src/main/mac-update-install-marker.test.ts @@ -1,12 +1,4 @@ -import { - existsSync, - mkdirSync, - mkdtempSync, - readFileSync, - readdirSync, - rmSync, - writeFileSync -} from 'node:fs' +import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -16,7 +8,6 @@ const { getVersionMock, recordUpdaterLifecycleMock, markerPathRef, - isShipItProvenExitedMock, getShipItLivenessMock, getProcessStartTimesMock, isProcessAliveMock @@ -24,7 +15,6 @@ const { getVersionMock: vi.fn(), recordUpdaterLifecycleMock: vi.fn(), markerPathRef: { dir: '', bundle: '/Applications/Orca.app' }, - isShipItProvenExitedMock: vi.fn(), getShipItLivenessMock: vi.fn(), getProcessStartTimesMock: vi.fn(), isProcessAliveMock: vi.fn() @@ -35,7 +25,6 @@ vi.mock('./updater-lifecycle-diagnostics', () => ({ recordUpdaterLifecycle: recordUpdaterLifecycleMock })) vi.mock('../shared/shipit-liveness', () => ({ - isShipItProvenExited: isShipItProvenExitedMock, getShipItLivenessForBundle: getShipItLivenessMock, getProcessStartTimes: getProcessStartTimesMock, isRecordedProcessAlive: isProcessAliveMock @@ -73,8 +62,6 @@ vi.mock('../shared/mac-update-install-marker', async (importOriginal) => { import type * as MarkerModule from '../shared/mac-update-install-marker' import { - _setShipItStatePathForTests, - canDeleteShipItState, isMacUpdateInstallInFlight, markMacUpdateInstallInFlight, reconcileMacUpdateInstallMarker, @@ -116,21 +103,16 @@ beforeEach(() => { markerPathRef.bundle = '/Applications/Orca.app' recordUpdaterLifecycleMock.mockReset() getVersionMock.mockReset() - isShipItProvenExitedMock.mockReset() - isShipItProvenExitedMock.mockReturnValue(false) getShipItLivenessMock.mockReset() getShipItLivenessMock.mockReturnValue('live') getProcessStartTimesMock.mockReset() getProcessStartTimesMock.mockReturnValue(new Map()) isProcessAliveMock.mockReset() isProcessAliveMock.mockReturnValue(false) - // Never let a test resolve this to the real Squirrel cache. - _setShipItStatePathForTests(join(dir, 'ShipItState.plist')) }) afterEach(() => { Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }) - _setShipItStatePathForTests(null) rmSync(dir, { recursive: true, force: true }) }) @@ -234,38 +216,6 @@ describe('shouldExitForInFlightMacUpdateInstall', () => { }) }) -describe('reconcile safety', () => { - it('leaves the installer state alone while a swap is still running', () => { - // A Dock or alternate-profile launch can reconcile while ShipIt is legitimately mid-swap; - // deleting its state there would cancel a working install. - const shipItState = join(dir, 'ShipItState.plist') - writeFileSync(shipItState, 'live installer state', 'utf8') - writeMarker() - getVersionMock.mockReturnValue('1.4.194') - isShipItProvenExitedMock.mockReturnValue(false) - - reconcileMacUpdateInstallMarker() - - expect(lifecycleNames()).toContain('install_did_not_apply') - expect(existsSync(shipItState)).toBe(true) - }) - - it('clears state left by an installer that is gone, which otherwise blocks updates forever', () => { - // #14732 needed this deleted by hand; a resumed stale state pins the machine to an update - // that can never complete. - const shipItState = join(dir, 'ShipItState.plist') - writeFileSync(shipItState, 'stale installer state', 'utf8') - writeMarker() - getVersionMock.mockReturnValue('1.4.194') - isShipItProvenExitedMock.mockReturnValue(true) - - reconcileMacUpdateInstallMarker() - - expect(existsSync(shipItState)).toBe(false) - expect(lifecycleNames()).toContain('shipit_state_cleared') - }) -}) - describe('startup gate liveness', () => { it('exits during the pre-spawn window, before the installer exists', () => { // Pins the writerAlive wiring in the startup gate. Without it, reverting to a bare liveness @@ -320,25 +270,14 @@ describe('startup gate liveness', () => { expect(shouldExitForInFlightMacUpdateInstall()).toBe(false) }) - it('refuses to resolve a real home path under test, and allows it in production', () => { - // Asserting the decision directly. An absence check cannot tell "the guard worked" from "the - // real file happened not to exist" — which is how this test passed while still being able to - // delete a developer's own Squirrel state. - expect(canDeleteShipItState({ hasPathOverride: false, isUnderTest: true })).toBe(false) - expect(canDeleteShipItState({ hasPathOverride: true, isUnderTest: true })).toBe(true) - expect(canDeleteShipItState({ hasPathOverride: false, isUnderTest: false })).toBe(true) - }) - - it('keeps a concurrent install whose owner is still running', () => { - // Reconcile reads then clears; an install committed in between must survive, or the next - // launch is free to cancel it. + it('keeps an install committed while an older attempt is being settled', () => { + // Reconcile reads then renames one exact attempt; a concurrently written one must survive. writeMarker({ targetVersion: '1.4.195', createdAtMs: Date.now() - 30_000 }) getVersionMock.mockReturnValue('1.4.194') const original = reconcileMacUpdateInstallMarker - // Simulate the race by rewriting the marker between read and clear. + // Simulate the race by writing another marker between read and settle. recordUpdaterLifecycleMock.mockImplementation(() => { - // A live owner (this process) means the attempt is still going; only a dead owner's marker - // is spent. A wall-clock cutoff would have wrongly cleared this. + // A live owner (this process) means the new attempt is still going. writeMarker({ targetVersion: '1.4.196', createdAtMs: Date.now(), diff --git a/src/main/mac-update-install-marker.ts b/src/main/mac-update-install-marker.ts index a70a9204cc4..558ea26f968 100644 --- a/src/main/mac-update-install-marker.ts +++ b/src/main/mac-update-install-marker.ts @@ -26,11 +26,8 @@ import { getShipItLivenessForBundle, isRecordedProcessAlive } from '../shared/shipit-liveness' -import { clearWedgedShipItState } from './mac-update-shipit-state' import { recordUpdaterLifecycle } from './updater-lifecycle-diagnostics' -export { _setShipItStatePathForTests, canDeleteShipItState } from './mac-update-shipit-state' - /** * Publish "an install is in flight for this bundle" where a launching process can see it. * @@ -271,12 +268,8 @@ export function reconcileMacUpdateInstallMarker(): void { // // Only the reported attempt is settled. An unexpired older sibling can therefore report its own // outcome on a later startup — telemetry noise, bounded by expiry, and preferable to settling - // attempts this pass never examined. The ShipIt cleanup below stays guarded by proven-exited, so - // a duplicate report cannot act on a swap that has since become real. + // attempts this pass never examined. settleMacUpdateInstallMarker(marker) - if (!applied) { - clearWedgedShipItState(bundlePath) - } } /** diff --git a/src/main/mac-update-shipit-state.ts b/src/main/mac-update-shipit-state.ts deleted file mode 100644 index 4f20812c670..00000000000 --- a/src/main/mac-update-shipit-state.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { unlinkSync } from 'node:fs' -import { homedir } from 'node:os' -import { join } from 'node:path' -import { isShipItProvenExited } from '../shared/shipit-liveness' -import { recordUpdaterLifecycle } from './updater-lifecycle-diagnostics' - -// Why injectable: this is destructive, and tests must never resolve it to the developer's cache. -let shipItStatePathOverride: string | null = null - -function getShipItStatePath(): string { - return ( - shipItStatePathOverride ?? - join(homedir(), 'Library', 'Caches', 'com.stablyai.orca.ShipIt', 'ShipItState.plist') - ) -} - -export function _setShipItStatePathForTests(path: string | null): void { - shipItStatePathOverride = path -} - -/** May this process delete the installer's state file? */ -export function canDeleteShipItState(input: { - hasPathOverride: boolean - isUnderTest: boolean -}): boolean { - return input.hasPathOverride || !input.isUnderTest -} - -/** Delete Squirrel's staged state only after its installer is proven gone. */ -export function clearWedgedShipItState(bundlePath: string): void { - if (!isShipItProvenExited(bundlePath)) { - return - } - // A forgotten override must not let a test delete the developer's real Squirrel state. - if ( - !canDeleteShipItState({ - hasPathOverride: shipItStatePathOverride !== null, - isUnderTest: Boolean(process.env.VITEST) - }) - ) { - return - } - try { - unlinkSync(getShipItStatePath()) - recordUpdaterLifecycle( - 'shipit_state_cleared', - {}, - { - level: 'warn', - message: 'Cleared a stale Squirrel install state left behind by a failed update' - } - ) - } catch { - // Absent is normal; stale state exists only after an aborted install. - } -} diff --git a/src/shared/shipit-liveness.ts b/src/shared/shipit-liveness.ts index 26cb29a94ed..4332d7ca891 100644 --- a/src/shared/shipit-liveness.ts +++ b/src/shared/shipit-liveness.ts @@ -75,16 +75,6 @@ export function getShipItLivenessForBundle(bundlePath: string): ShipItLiveness { } } -/** Destructive cleanup requires positive evidence of absence; uncertainty never qualifies. */ -export function isShipItProvenExited(bundlePath: string): boolean { - return getShipItLivenessForBundle(bundlePath) === 'exited' -} - -/** True while the installer may still be working — live, or we could not tell. */ -export function mayShipItBeRunning(bundlePath: string): boolean { - return getShipItLivenessForBundle(bundlePath) !== 'exited' -} - /** Read process birth times in one bounded probe so marker count cannot multiply subprocesses. */ export function getProcessStartTimes(pids: readonly number[]): ReadonlyMap | null { const uniquePids = [...new Set(pids.filter((pid) => Number.isInteger(pid) && pid > 0))]